Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #34 - Windows based docker images for maven (jdk8 and jdk11 supported) #119
Fix #34 - Windows based docker images for maven (jdk8 and jdk11 supported) #119
Changes from 1 commit
f370e1c
edbfbb7
5f50f0c
0243415
7da4320
1a7112d
e483302
188e9f1
6d4f058
2d1ce89
87864fb
336b312
297328d
2ee9fff
929e16b
5f26db8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For lack of highlighting, see github-linguist/linguist#4566 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yow, a lot of duplicated content between images. Is there a straightforward way to share some of these steps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. I am not a super user with docker, so I am not aware of any tricks for sharing content. I based my implementation on the existing image setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, we should not have a dozen copies of the same script.
Perhaps it would be better to refactor the repository layout to use a flat structure, so that any
Dockerfile.*
could freelyCOPY
any resource file (perhaps just a temporary script toRUN
and delete).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the mvn-entrypoint.sh is copied across the various image directories as well, this is because Docker doesn't support COPY from outside the current directory (and children) into the image. I'm sure there could be some refactoring to take that into account, but I didn't want to make that part of this changset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaning up an existing situation is indeed something that should be deferred to a separate PR. I mentioned this here because this PR introduces a lot of duplication.
Perhaps we could start by introducing a single directory with all the Windows images, and leave the Linux images untouched for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely open to that method if that is desired. I'd also like to know if that would work for @carlossg to have that method.