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

Issue #1185 Prevent docker:save failing when no images are defined. #1194

Closed
wants to merge 0 commits into from

Conversation

wrosenuance
Copy link
Contributor

Address issue #1185 by having docker:save handle POMs with no images with a build configuration.

Signed-off-by: William Rose william.rose@nuance.com

BEFORE

$ mvn io.fabric8:docker-maven-plugin:0.28.0:save
[INFO] Scanning for projects...
[INFO]
[INFO] -------------------< io.fabric8:docker-maven-plugin >-------------------
[INFO] Building docker-maven-plugin 0.28-SNAPSHOT
[INFO] ----------------------------[ maven-plugin ]----------------------------
[INFO]
[INFO] --- docker-maven-plugin:0.28.0:save (default-cli) @ docker-maven-plugin ---
[ERROR] DOCKER> More than one image with build configuration is defined. Please specify the image with 'docker.name' or 'docker.alias'.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 2.227 s
[INFO] Finished at: 2019-04-09T00:07:12-04:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal io.fabric8:docker-maven-plugin:0.28.0:save (default-cli) on project docker-maven-plugin: More than one image with build configuration is defined. Please specify the image with 'docker.name' or 'docker.alias'. -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

AFTER

$ mvn io.fabric8:docker-maven-plugin:0.28-SNAPSHOT:save
[INFO] Scanning for projects...
[INFO]
[INFO] -------------------< io.fabric8:docker-maven-plugin >-------------------
[INFO] Building docker-maven-plugin 0.28-SNAPSHOT
[INFO] ----------------------------[ maven-plugin ]----------------------------
[INFO]
[INFO] --- docker-maven-plugin:0.28-SNAPSHOT:save (default-cli) @ docker-maven-plugin ---
[INFO] DOCKER> No images have a build configuration defined: save skipped
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1.978 s
[INFO] Finished at: 2019-04-09T00:07:24-04:00
[INFO] ------------------------------------------------------------------------

@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #1194 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #1194      +/-   ##
============================================
- Coverage     52.82%   52.78%   -0.04%     
  Complexity     1511     1511              
============================================
  Files           150      150              
  Lines          7974     7979       +5     
  Branches       1200     1202       +2     
============================================
  Hits           4212     4212              
- Misses         3348     3353       +5     
  Partials        414      414
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/io/fabric8/maven/docker/SaveMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️

2 similar comments
@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #1194 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #1194      +/-   ##
============================================
- Coverage     52.82%   52.78%   -0.04%     
  Complexity     1511     1511              
============================================
  Files           150      150              
  Lines          7974     7979       +5     
  Branches       1200     1202       +2     
============================================
  Hits           4212     4212              
- Misses         3348     3353       +5     
  Partials        414      414
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/io/fabric8/maven/docker/SaveMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️

@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #1194 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #1194      +/-   ##
============================================
- Coverage     52.82%   52.78%   -0.04%     
  Complexity     1511     1511              
============================================
  Files           150      150              
  Lines          7974     7979       +5     
  Branches       1200     1202       +2     
============================================
  Hits           4212     4212              
- Misses         3348     3353       +5     
  Partials        414      414
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/io/fabric8/maven/docker/SaveMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️

@@ -101,6 +104,10 @@ private String getImageName() throws MojoExecutionException {
// specify image by name or alias
if (saveName == null && saveAlias == null) {
List<ImageConfiguration> buildImages = getImagesWithBuildConfig(images);
if (buildImages.isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move that check up to the start of executeInternal() ? E.g. we can obtain the configuration there and add it as an argument to this method.

Its much clearer to skip execution early if needed.

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.

Looks good to me except that I would move the check to a poistion as early as the check for skipSave. (see comment below).

Please add an entry to changelog.md, too.

@wrosenuance
Copy link
Contributor Author

I made this change, but I am not sure it's clearer -- is this what you meant?

@rhuss
Copy link
Collaborator

rhuss commented Apr 9, 2019

Thanks ! No, not fully what I meant. Is ok when I push to your branch with the changes I thought about ? (it's easier to explain in code ;-). It's only a small change.

Thanks for the test btw !

@wrosenuance
Copy link
Contributor Author

Please do! I agree it will be faster 😁

@rhuss
Copy link
Collaborator

rhuss commented Apr 9, 2019

Coolio. However, you would need to give me access to this as your master branch seems to be protected.
See https://help.github.com/en/articles/committing-changes-to-a-pull-request-branch-created-from-a-fork for more info, but in essence, the easiest way is to create a PR on a separate branch as this is by default allowed to push it.

If this is too much of a hassle for you, I can create a new pull request (and we could close this one).

@rhuss
Copy link
Collaborator

rhuss commented Apr 9, 2019

I created a new PR #1196 with my suggestion.

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.

2 participants