BuildImage sync to 1.20 API by KostyaSha · Pull Request #299 · docker-java/docker-java
attempt to sync...
@marcuslinke please kick me to the right way as it really annoying work
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for what this required at all? Created by analogue
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The toString() implementations differ between various commands. I would prefer commons-lang ToStringBuilder.reflectionsToString() as default implementation for all commands. WDYT?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it for CLI analog, because .append(tag != null ? "-t " + tag + " " : "") can't be reflected
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt that CLI analogy is really needed in the string representations. What should it be for?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho toString should be standard toString, not sure what reflection will do with tarstream.
imho if it URL composer then some toApi*
AFAIR interface was supposed for CLI implementation, so such command must be implemented separately.
I can try wipe this CLI analogue from toString if you want
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep toString() returning useful debug information. This is why i prefer ToStringBuilder.reflectionsToString(). It reflects all members of the class and produces a name/value map like description calling toString() on each, so InputStream.toString() is printed for tarstream for example. Thats very useful when tracking down problems/bugs.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i mean that IDE may generate it also, but we can skip internal non-representative info
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, let's try, will replace with reflection
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
This was referenced
Aug 19, 2015Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use ToStringStyle.SIMLE_STYLE. Thanks!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Reordered fields according to 1.20 API for easier tracking
rebased.
And as usual headache with tests?
Yes, I'm thinking of dropping CircleCI for now as it has many limitations. Half of the tests do not run...
I mean that i don't know what/how implement :( Also i changed primitives to objects, that may affect people, but 2.1.0 maybe enough for bump as X.Y may contain api breakage...
Changing primitives to objects for one single command (BuildCmd) isn't a good idea. IMHO this needs to be done in a single release for all commands. Following SemVer (http://semver.org/) this is a major release then (v3.0.0). Maybe we should collect all this (1.2.0 API sync and primitives to objects) in a separate branch? The problem is we mix two different things/issues then and I doubt this is a good idea...
@marcuslinke library already has mix of primitives and objects, so doing it at once doesn't make any sense. Docker is breaking api for 1.19 vs 1.20 and etc. I can only revert primitives and keep new fields as i already have.
@marcuslinke i think i restored original fields behaviour. But we need test somehow difference of using primitives vs objects, i.e. for will be deserialised and how docker API will react on different calls. If null wouldn't produce any fields in deserialised JSON, then it probably fine and good because it what expected on API side.
And btw, what about removing this 'withXXX()` zero argument methods? They are only complicate library code while it not so difficult for library consumer to set explicitly true/false.
If you are ok with this changes, then i can squash commits
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a primitive here?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because somebody can set it to null with setter to exclude this parameter from remote call at all.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you switched from objects back to primitives again. As I see all other boolean properties use primitives.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched only existed to keep compatibility, other values are new and i decided to make them as Objects to have "not defined" state (i.e. if remote API wouldn't return some values). I think it should also help keeping backward compatibility with constantly changing remote API versions.
You've added FindBugs. Could you explain why please?
@marcuslinke This annotations should help any library used code to detect potential NPEs. Ideally for this class CheckForNull should be set on package level, but i never used such defaults before (will simplify when will have known version).
In theory should avoid such errors:
jenkinsci/docker-plugin@bf893a8
@marcuslinke i didn't add FB plugin for analysing library code itself because i don't know it design and how you expect using it. As example see hub4j/github-api#210
mm.. i need check whether annotations also required on implementations, please wait with merge...
i need check whether annotations also required on implementations, please wait with merge...
In the case of static analysis with edu.umd.cs.findbugs.annotations. There is no such need. Annotations are being inherited from the super implementations and interface definitions. AFAIK you can only explicitly change CheckForNull to NonNull if you want to do it for a child class.
In the case of other annotation libs and cases (e.g. fail in the compile time or in the runtime)... It depends on the annotation processor you use.
Thanks, i don't plan adding FB (atm) want start with clean-uping critical for docker-plugin parts.
Then i'm done with this PR :)
@marcuslinke will be very glad to get it in release so i can release new docker-plugin for jenkins with normal build logging and options for build.
@KostyaSha I would like to include this and some other PRs in the next release (v2.1.0):
Hopefully I'm able to merge these 3 PRs today evening.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters