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

docker-save support #687

Merged
merged 24 commits into from
Mar 20, 2017

Conversation

Katsuya-Tomioka
Copy link
Contributor

I wrote to specify which one (and where) to save rather than go over build configs as in 'source'. Also not sure about where to autopull. Seemed too much in this context. Let me know.

@codecov-io
Copy link

codecov-io commented Jan 8, 2017

Codecov Report

Merging #687 into master will increase coverage by 0.03%.
The diff coverage is 40%.

@@             Coverage Diff              @@
##             master     #687      +/-   ##
============================================
+ Coverage     47.48%   47.52%   +0.03%     
- Complexity     1002     1023      +21     
============================================
  Files           125      126       +1     
  Lines          6596     6694      +98     
  Branches        847      870      +23     
============================================
+ Hits           3132     3181      +49     
- Misses         3196     3241      +45     
- Partials        268      272       +4
Impacted Files Coverage Δ Complexity Δ
...8/maven/docker/assembly/DockerAssemblyManager.java 14.14% <ø> (ø) 6 <0> (ø) ⬇️
.../main/java/io/fabric8/maven/docker/SourceMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...c/main/java/io/fabric8/maven/docker/StartMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ic8/maven/docker/access/DockerAccessException.java 36.36% <0%> (-13.64%) 2 <0> (ø)
...va/io/fabric8/maven/docker/AbstractDockerMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...rc/main/java/io/fabric8/maven/docker/SaveMojo.java 0% <0%> (ø) 0 <0> (?)
...ric8/maven/docker/config/BuildImageSelectMode.java 0% <0%> (ø) 0 <0> (?)
...8/maven/docker/config/BuildImageConfiguration.java 81.81% <100%> (+3.24%) 45 <0> (+1) ⬆️
...n/java/io/fabric8/maven/docker/util/ImageName.java 91.35% <100%> (-0.11%) 34 <0> (ø)
...abric8/maven/docker/config/ImageConfiguration.java 44.92% <100%> (ø) 11 <2> (ø) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6d2cbe...fbf2fd0. Read the comment docs.

@rhuss
Copy link
Collaborator

rhuss commented Jan 23, 2017

Thanks for the PR and sorry for the delay ! I hope I will have a chance to look at it soon.

@rhuss
Copy link
Collaborator

rhuss commented Feb 9, 2017

Sorry again for the delay, but should be added soon ;-)

Your PR looks good, I tuned it a bit. Especially its allowed now to run mvn docker:save with the need to provide any additional arguments as the plugin select some sane defaults:

  • If no name or alias is given, then the configuration is examined if there is only a single image with a build configuration. If this is the case, this image's name is used.
  • If no file is given, then the filename is calculated based on the alias or name. Currently the alias has prio, but maybe its better to use always the name. wdyt ?

What is still missing is documentation. As soon as we have this, we can merge it. When you feel fancy, feel free to submit some documentation. But I can update it, too.

Also some extra unit tests would be awesome (e.g. for ArchiveCompression which already was there but which I enhanced to cover the use cases introduced by SaveMojo).

Katsuya-Tomioka and others added 2 commits February 9, 2017 22:55
* Allow an no-argument call to docker:save by chosing sane defaults
* Reused ArchiveCompression for selecting the compression mode

and a bit more
@Katsuya-Tomioka
Copy link
Contributor Author

I'd probably prefer name over alias as default. $name-$alias.tar.gz if alias is available?

@rhuss
Copy link
Collaborator

rhuss commented Mar 15, 2017

Thanks a lot, I will have a look at the weekend and will integrate the PR.

@rhuss
Copy link
Collaborator

rhuss commented Mar 20, 2017

I'd probably prefer name over alias as default. $name-$alias.tar.gz if alias is available?

In any other usage scenarios alias typically takes precedence, so we should probably use this here, too.

Also, we need to sanitise the name as it can contain /, too (names with more than two segments are allowed, too).

if (classifier != null) {
projectHelper.attachArtifact(project, type, classifier, fileObj);
} else {
projectHelper.attachArtifact(project, type, fileObj);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not do this here, as it will replace the main artefact of the build when no classifier is used.

From the JavaDoc:

Add or replace an artifact to the current project.

and with classifier == null it will replace the main artefact.

I suggest to remove this feature for now and, if needed for a specific use case, introduce it again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also I don't think attaching an image like this make sense, as it's probably really huge and your Maven repo (either local or remote) won't cope with suche multi hundred mega byte tars for every version of your artefact.

@@ -65,6 +65,11 @@ public String deleteImage(String name, boolean force) {
.build();
}

public String getImage(ImageName name) {
return u("images/%s/get", name.getNameWithoutTag())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should use the name without tag because according to the Docker reference manual in that case all image of all tags are returned. Actually we should use the full image name here.

* Removed attaching of artifacts as they are too huge anyway
* Harmonized properties name to be more specific
* Added a bit to documentation
* Fixed fetching of all images by using the full name
@rhuss
Copy link
Collaborator

rhuss commented Mar 20, 2017

I updated the PR according to my comments, if you don't mind.

Going to merge this into master now, we can still tune it before doing the next release.

Thanks a lot for you contribution ! 'hope my updates are fine for you.

@rhuss
Copy link
Collaborator

rhuss commented Mar 20, 2017

[merge]

@fusesource-ci fusesource-ci merged commit 2d23669 into fabric8io:master Mar 20, 2017
rgbj pushed a commit to rgbj/docker-maven-plugin that referenced this pull request Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants