Skip to content

Conversation

@cberes
Copy link
Contributor

@cberes cberes commented Aug 28, 2016

Fixes #681

This change is Reviewable

*/
@Override
public BuildImageCmd withLabel(String key, String value) {
if (this.labels == null) {
Copy link
Member

@KostyaSha KostyaSha Aug 28, 2016

Choose a reason for hiding this comment

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

this unneeded

@cberes
Copy link
Contributor Author

cberes commented Aug 28, 2016

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


src/main/java/com/github/dockerjava/core/command/BuildImageCmdImpl.java, line 318 [r1] (raw file):

Previously, KostyaSha (Kanstantsin Shautsou) wrote…

this unneeded

you prefer to create the map when the object is created? for example: private Map labels = new HashMap<>();

Comments from Reviewable

@KostyaSha
Copy link
Member

src/main/java/com/github/dockerjava/core/command/BuildImageCmdImpl.java, line 318 [r1] (raw file):

Previously, cberes (Corey Beres) wrote…

you prefer to create the map when the object is created? for example:
private Map<String, String> labels = new HashMap<>();

I mean don't use `this` where it not needed. In this case `labels` are not hidden by any input argument, so redundant.

Comments from Reviewable

/**
*@since {@link RemoteApiVersion#VERSION_1_23}
*/
BuildImageCmd withLabel(String key, String value);
Copy link
Member

Choose a reason for hiding this comment

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

Why only one label? withLabels(Map<String, String>)

@cberes
Copy link
Contributor Author

cberes commented Aug 28, 2016

I followed the same pattern as withBuildArg(String, String). So withLabel(String, String) can be called multiple times to add multiple labels. But I will change the method to accept a map if you prefer that. Or have both withLabels(Map) and withLabel(String, String)?

@KostyaSha
Copy link
Member

I prefer raw data type as docker-java atm is simple layer to API. Your method should be addLabel according to logic. But then it become additional logic not related to clean API...

@cberes
Copy link
Contributor Author

cberes commented Aug 29, 2016

ok, I changed withLabel(String, String) to withLabels(Map<String, String>)

@KostyaSha KostyaSha closed this Sep 28, 2016
@KostyaSha KostyaSha reopened this Sep 28, 2016
@KostyaSha
Copy link
Member

rebuilding PR as too much matrix configuration builds failed...

@KostyaSha
Copy link
Member

Looks fine.

@KostyaSha KostyaSha added this to the 3.0.7 milestone Sep 29, 2016
@KostyaSha KostyaSha merged commit 305b73a into docker-java:master Sep 29, 2016
panuse pushed a commit to TuKangTech/docker-java that referenced this pull request Aug 20, 2017
* Ability to set labels on image build (docker-java#681)

* Check API version in new tests (docker-java#681)

* remove unnecessary this (docker-java#681)

* change withLabel to withLabels (docker-java#681)
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.

2 participants