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 load support. #645

Merged
merged 48 commits into from
Jan 2, 2017

Conversation

Katsuya-Tomioka
Copy link
Contributor

Signed-off-by: Katsuya Tomioka katsuya@basistech.com

chonton and others added 2 commits December 9, 2016 16:07
tamslinn and others added 2 commits December 15, 2016 10:11
The global option 'image' was renamed to 'filter' but the push section referenced the old name.
Signed-off-by: Tamsin Slinn <tam.slinn@gmail.com>
Corrected reference to global option 'filter'
@rhuss
Copy link
Collaborator

rhuss commented Dec 17, 2016

Thanks a lot ! I will have review next week.

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, except the place where the configuration should go (see comment).

Beside this (and beyond the scope of the PR) a docker:save goal would be nice, too, for exporting a tar archive (which then e.g. could be checked into git to be later refered to by this feature). So, if you are fancy feel, I would highly appreciate a PR for this feature, too ;-)

@@ -33,6 +33,9 @@
@Parameter
private String registry;

@Parameter
private String file;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I would move this to the <build> configuration as it is about creating images. There is should be probably called something like dockerArchive on the same footprint as dockerFile or dockerFileDir. Also there should be a check, that only one of dockerArchive and dockerFile is given.

chonton and others added 2 commits December 21, 2016 10:05
Signed-off-by: Chas Honton <chas@apache.org>
    Signed-off-by: Katsuya Tomioka <katsuya@basistech.com>
raystorm and others added 13 commits December 23, 2016 22:04
added ability to explicitly via configuration to create named volumes
via <volumes><volume> configuration.
Example config
```xml
<volumes>
  <volume>
    <name>Volume Name</name>
    <driver>local</driver>
    <driverOpts>
      <driverOpt-key-name>driverOpt-value</driverOpt-key-name>
    </driverOpts>
    <labels>
    	<label-key>someValue</label-key>
      <com-github-fabric8-dmp-sample-volume>
        voluminous
      </com-github-fabric8-dmp-sample-volume>
    </labels>
  </volume>
</volumes>
```
Added a new volume sample
Added a new goal volume-create
minor formatting / spelling correction for intro.md

Signed-off-by: Tom Burton <Tom.Burton@Outlook.com>
Signed-off-by: Tom Burton <Tom.Burton@Outlook.com>
fabric8io#664 (review)
corrected spelling
corrected ascii-doc + formatting
code formatting

Signed-off-by: Tom Burton <Tom.Burton@Outlook.com>
fabric8io#664 (review)
corrected spelling
corrected ascii-doc + formatting
code formatting

Signed-off-by: Tom Burton <Tom.Burton@Outlook.com>
This is a cleanup of PR fabric8io#664 with the following major points tackled:

* Cleaned formatting
* Removed unneeded classes and service method for listing and getting volumes. This is not needed now and probablu not in the future.
* Fixed some Copy&Pase errors (like the default phase for 'volume-remove' which was the same as for 'volume-create'
* Fixed tests. Please note, that *no service method* should be adapted to be "testable". Especially do not introduce a non-private method only for this purpose and test this method instead of the offiical API. Modern Mock frameworks can deal with the the problems which are tried to be solved with this. Otherwise the API gets dilulted, more complicated, more maintenance, more coupled.
* Removed assert that do nothing.

Tests can be still improved for the service but its fine for now,

Next step is to add support for the PropertyHandler to allow configuration of the volumes also via properties.
Signed-off-by: Katsuya Tomioka <katsuya@basistech.com>
        Signed-off-by: Katsuya Tomioka <katsuya@basistech.com>
Signed-off-by: Katsuya Tomioka <katsuya@basistech.com>
@Katsuya-Tomioka
Copy link
Contributor Author

I moved dockerFile part to the build side. Also added a bunch of tests to pass the coverage check. Hope you don't mind. If you could review/edit the doc, that'd be great.

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 a lot ! Looks good for me (with some minor comments). I tuned it a bit and added some extra docs for the property handling stuff.

@@ -468,4 +468,9 @@ private String withLatestIfNoTag(String name) {
ImageName imageName = new ImageName(name);
return imageName.getTag() == null ? imageName.getNameWithoutTag() + ":latest" : name;
}

protected void loadImage(ServiceHub hub, String image, String filename) throws DockerAccessException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method seems to be unused. If so, I would remove it.

@@ -357,6 +361,19 @@ public void removeContainer(String containerId, boolean removeVolumes)
}

@Override
public void loadImage(String image, String filepath)
throws DockerAccessException {
ImageName name = new ImageName(image);
Copy link
Collaborator

Choose a reason for hiding this comment

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

name is not used here

- docs for docker.dockerArchive external property
- removed unneed boolean tag
- removed unused method
@rhuss
Copy link
Collaborator

rhuss commented Jan 2, 2017

[merge]

@fusesource-ci fusesource-ci merged commit 729583e into fabric8io:master Jan 2, 2017
rgbj pushed a commit to rgbj/docker-maven-plugin that referenced this pull request Jun 21, 2017
- docs for docker.dockerArchive external property
- removed unneed boolean tag
- removed unused method
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.

8 participants