Allow multiple tags in build image command #739

orzeh · 2016-11-15T12:57:44Z

Several tags for image can be defined by subsequent calling withTag() method of BuildImageCmd.

@marcuslinke please check my modifications of Netty WebTarget class to support several query parameters with the same name as this class is widely used.

This change is

Several tags for image can be defined by calling multiple time withTag() method of BuildImageCmd. In Netty implementation, the WebTarget class is modified to support several query parameteres with the same name. Fix #720

KostyaSha · 2016-11-15T13:20:21Z

src/main/java/com/github/dockerjava/core/command/BuildImageCmdImpl.java

    private InputStream tarInputStream;

-    private String tag;
+    private Set<String> tags;


NO don't remove existing fields either you will break all < 1.21 users.

This is a private filed - to preserve backward compatibility I've left getTag() method.

Ah, first of all it Cmd in cmd we can break compatibility, but better do .Y increment in version then.

I don't think this is a breaking change - previously written code will still work with all versions of Docker engine (unless you already called withTag() multiple times).

This change add some features so feel free to release it with incremented .Y in version.

KostyaSha · 2016-11-15T13:22:13Z

src/main/java/com/github/dockerjava/core/command/BuildImageCmdImpl.java

+        if (this.tags == null) {
+            this.tags = new HashSet<>(4);
+        }
+        this.tags.add(tag);


you can't be sure what target API is used to know what to put. Add version check to get conditional and default to latest behaviour.

@KostyaSha is such version check already implemented somewhere in the code base or I have to write one?

Check the place where Auth headers are added

I was unable to find it. However, IMO there is no need to do version check. In recently added labels support we do not check Docker engine version either. Client will use this features when targeting newer version of Docker.

codecov-io · 2016-11-19T15:47:45Z

Codecov Report

Merging #739 into master will increase coverage by 0.14%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #739      +/-   ##
==========================================
+ Coverage   71.61%   71.75%   +0.14%     
==========================================
  Files         306      306              
  Lines        6584     6597      +13     
  Branches      483      488       +5     
==========================================
+ Hits         4715     4734      +19     
+ Misses       1582     1577       -5     
+ Partials      287      286       -1

Impacted Files	Coverage Δ
...in/java/com/github/dockerjava/netty/WebTarget.java	`74.46% <100%> (+2.37%)`	⬆️
...hub/dockerjava/core/command/BuildImageCmdImpl.java	`59.57% <100%> (+1.33%)`	⬆️
...ithub/dockerjava/netty/exec/BuildImageCmdExec.java	`54.9% <33.33%> (-0.21%)`	⬇️
...com/github/dockerjava/jaxrs/BuildImageCmdExec.java	`56.36% <80%> (+1.46%)`	⬆️
...ava/netty/handler/FramedResponseStreamHandler.java	`89.65% <0%> (+3.44%)`	⬆️
...va/org/apache/http/impl/io/ChunkedInputStream.java	`61.16% <0%> (+5.82%)`	⬆️

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 f9fdc39...8e1f392. Read the comment docs.

orzeh · 2017-02-12T21:46:38Z

@KostyaSha Is there anything else I should do before it can be merged?

KostyaSha · 2017-03-28T21:41:43Z

src/main/java/com/github/dockerjava/api/command/BuildImageCmd.java

+     * @deprecated since docker API version 1.21 there can be multiple tags
+     * specified so use {@link #getTags()}
     */
+    @Deprecated()


Just @Deprecated

KostyaSha · 2017-03-28T21:43:30Z

src/main/java/com/github/dockerjava/core/command/BuildImageCmdImpl.java

+            return null;
+        }
+        // return first tag to be backward compatible
+        return tags.iterator().next();


So maybe keep both fields and pick List<> and if it null or empty return getTag()?

@KostyaSha ok, but then we end up with withTag(String tag) and new method withTags(Set<String> tags) - I don't think this is more elegant solution. But you decide.

if i right understand witTag(String) is already existing and we can Deprecate it and remove in future and add new Set?

KostyaSha · 2017-03-28T21:46:36Z

src/test/java/com/github/dockerjava/core/command/BuildImageCmdImplTest.java

+        File baseDir = fileFromBuildTestResource("labels");
+
+        String imageId = dockerClient.buildImageCmd(baseDir).withNoCache(true)
+                .withTag("docker-java-test:tag1").withTag("docker-java-test:tag2")


imho that looks not obvious. If user wants only one tag it should call .withTag if multiple then .withTags(Set<String>) Maybe keep getTag() and add getTags() and prefer getTags with fallback to getTag()?

ok, I'll do it this way 😄

KostyaSha · 2017-03-28T22:04:55Z

src/test/java/com/github/dockerjava/netty/exec/BuildImageCmdExecTest.java

+        LOG.info("Image Inspect: {}", inspectImageResponse.toString());
+
+        assertThat(inspectImageResponse.getRepoTags().size(), equalTo(2));
+        assertThat(inspectImageResponse.getRepoTags().contains("docker-java-test:tag1"), equalTo(true));


if inspectImageResponse.getRepoTags() it return List or collection then you need search for hamcrest matcher (probably see example at com/github/dockerjava/core/command/CreateContainerCmdImplTest.java:165 or com/github/dockerjava/core/command/CreateContainerCmdImplTest.java:191 remember that some matchers are strict ordered and docker may return in different order items (even depends on java collection types afair))

MarkRx · 2017-05-05T14:28:39Z

~~Was this supposed to be merged instead of closed?~~

Looks like it was moved to #835

Reworked PR #739 (multiple tags)

Reworked PR docker-java#739 (multiple tags)

Allow multiple tags in build image command

81a33c6

Several tags for image can be defined by calling multiple time withTag() method of BuildImageCmd. In Netty implementation, the WebTarget class is modified to support several query parameteres with the same name. Fix #720

KostyaSha reviewed Nov 15, 2016

View reviewed changes

orzeh mentioned this pull request Dec 28, 2016

Pushing multiple tags bmuschko/gradle-docker-plugin#318

Closed

orzeh mentioned this pull request Feb 16, 2017

Docker build with multiple tags #720

Closed

KostyaSha reviewed Mar 28, 2017

View reviewed changes

Preserve withTag and add withTags in build image command

8e1f392

This was referenced Apr 23, 2017

Compare with #739 #834

Closed

Reworked PR #739 (multiple tags) #835

Merged

KostyaSha added this to the 3.0.10 milestone May 5, 2017

KostyaSha closed this May 5, 2017

KostyaSha added a commit that referenced this pull request May 5, 2017

Merge pull request #835 from KostyaSha/pr/739

ec30edc

Reworked PR #739 (multiple tags)

panuse pushed a commit to TuKangTech/docker-java that referenced this pull request Aug 20, 2017

Merge pull request docker-java#835 from KostyaSha/pr/739

afec3b0

Reworked PR docker-java#739 (multiple tags)

Allow multiple tags in build image command #739

Allow multiple tags in build image command #739

Uh oh!

Conversation

orzeh commented Nov 15, 2016 • edited by marcuslinke Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orzeh Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orzeh commented Feb 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KostyaSha Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarkRx commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

orzeh commented Nov 15, 2016 •

edited by marcuslinke

Loading

orzeh Nov 15, 2016 •

edited

Loading

codecov-io commented Nov 19, 2016 •

edited

Loading

KostyaSha Mar 28, 2017 •

edited

Loading

MarkRx commented May 5, 2017 •

edited

Loading