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

Handle properly when dockerFile and dockerFileDir also set #624

Closed
wants to merge 1 commit into from

Conversation

lmesz
Copy link
Contributor

@lmesz lmesz commented Nov 25, 2016

When both parameter set the dockerFileDir was ignored because of the logic how to find the given dockerfile.
After the pull request when both parameter set docker-maven-plugin determine properly the location of the Dockerfile.

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Actually it makes sense to help people if they want to provide both parameters, but in general it is easier to use a dockerFile only with an absolute path.

Its not really clear what's the advantage of providing both, but since this doesn't harm either, its perfectly fine for me to merge this in.

Thanks !

@@ -457,7 +457,10 @@ public String initAndValidate(Logger log) throws IllegalArgumentException {

// Initialize the dockerfile location and the build mode
private void initDockerFileFile(Logger log) {
if (dockerFile != null) {
if (dockerFile != null && dockerFileDir != null) {
dockerFileFile = new File(dockerFileDir, dockerFile);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add a check, that dockerFile is not an absolute path and throw an error if it is ?

@lmesz lmesz force-pushed the fix_dockerFile_dockerFileDir branch from 5ea81cd to ebf131b Compare November 25, 2016 13:48
@lmesz
Copy link
Contributor Author

lmesz commented Nov 25, 2016

Added check for absolutepath that you asked :)

@lmesz
Copy link
Contributor Author

lmesz commented Dec 2, 2016

May I ask if it will be merged or not?

@rhuss
Copy link
Collaborator

rhuss commented Dec 2, 2016

@lmesz sorry, I'm bit work overflowed, but I will add it asap when I have the next free cycles. I've one conference next week, but after this things should calm down considerably.

@rhuss
Copy link
Collaborator

rhuss commented Dec 17, 2016

@lmesz next week I have dedicated some cycles to docker-maven-plugin and will integrate you PR then. Thanks for your patience ...

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me ! Sorry for the delay. This week I'm planning a release, and would add this there.

Could you do me a favor and add this behaviour to the documentation , too ? thanks ...

…kerFileDir also set

Signed-off-by: Laszlo Meszaros <lacienator@gmail.com>
@lmesz lmesz force-pushed the fix_dockerFile_dockerFileDir branch from ebf131b to 5de287a Compare December 22, 2016 09:31
@lmesz
Copy link
Contributor Author

lmesz commented Dec 22, 2016

Sure, you mean changelog or the documentation itself? Where can I find the documentation from which the user guide created?

@codecov-io
Copy link

codecov-io commented Dec 22, 2016

Current coverage is 47.42% (diff: 100%)

Merging #624 into master will increase coverage by 0.05%

@@             master       #624   diff @@
==========================================
  Files           113        113          
  Lines          5881       5887     +6   
  Methods           0          0          
  Messages          0          0          
  Branches       1020       1022     +2   
==========================================
+ Hits           2786       2792     +6   
  Misses         2850       2850          
  Partials        245        245          
Diff Coverage File Path
•••••••••• 100% ...ic8/maven/docker/config/BuildImageConfiguration.java

Sunburst

Powered by Codecov. Last update 67fa15b...761448b

@lmesz lmesz force-pushed the fix_dockerFile_dockerFileDir branch from 5de287a to 761448b Compare December 22, 2016 09:50
@rhuss
Copy link
Collaborator

rhuss commented Jan 2, 2017

@lmesz I just integrated PR #645 which conflicts with this PR, so that I will resolve the conflict (and will update the documentation on the go.

fyi, the documentation can be found in src/main/asciidoc and uses Asciidoc to generate the user manual.

rgbj pushed a commit to rgbj/docker-maven-plugin that referenced this pull request Jun 21, 2017
Also streamlined the load image functionality. This includes
the code from fabric8io#624 and fixes fabric8io#624
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants