Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dockerPackageMappings to be absolute paths #1305

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

joan38
Copy link
Contributor

@joan38 joan38 commented Feb 5, 2020

Fixes #1304

@lightbend-cla-validator
Copy link

At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user

Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the bug report and the pull request to fix this 🤗

There are a couple of things that make me hesitate to merge this

  • the issues seems to be in the MappingsHelper if the paths that are returned are not correct
  • using Paths seems like a good workaround, but I'm not sure if this is a maintainable solution as this relies heavily on the underlying OS (e.g. file separators, how path representation works )
  • a regression test that demonstrates the fix would always be nice

@joan38
Copy link
Contributor Author

joan38 commented Feb 11, 2020

I don't think MappingsHelper has any issue since it returns relative paths by design.

Your concerne about the underlying OS would be an issue if we are using a different OS in the docker image we are building (not the host machine that is running SBT).

I will add a test for it.

@muuki88
Copy link
Contributor

muuki88 commented Oct 21, 2020

Hi @joan38 thanks again for tackeling this 😃
Do you know if you have the time to add some tests for this?

@joan38
Copy link
Contributor Author

joan38 commented Oct 23, 2020

Hi @muuki88, I'm not sure I'll have much time to do this also since we moved away from SBT to Mill.

@muuki88
Copy link
Contributor

muuki88 commented Oct 23, 2020

Thanks for the update @joan38 😊

@muuki88 muuki88 added docker minor release drafter version labels Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker minor release drafter version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building Docker fails to chmod it .sh files
3 participants