BuildImage sync to 1.20 API#299
Conversation
There was a problem hiding this comment.
for what this required at all? Created by analogue
There was a problem hiding this comment.
The toString() implementations differ between various commands. I would prefer commons-lang ToStringBuilder.reflectionsToString() as default implementation for all commands. WDYT?
There was a problem hiding this comment.
I thought it for CLI analog, because .append(tag != null ? "-t " + tag + " " : "") can't be reflected
There was a problem hiding this comment.
I doubt that CLI analogy is really needed in the string representations. What should it be for?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
i mean that IDE may generate it also, but we can skip internal non-representative info
There was a problem hiding this comment.
Well, let's try, will replace with reflection
There was a problem hiding this comment.
Please use ToStringStyle.SIMLE_STYLE. Thanks!
KostyaSha
force-pushed
the
buildImage
branch
2 times, most recently
from
d20111f to
18a0feb
Compare
Reordered fields according to 1.20 API for easier tracking
|
rebased. |
|
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... |
|
this might be interesting for you when thinking of dropping CircleCI: http://docs.travis-ci.com/user/docker/ |
|
@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 |
There was a problem hiding this comment.
Shouldn't this be a primitive here?
There was a problem hiding this comment.
No, because somebody can set it to null with setter to exclude this parameter from remote call at all.
There was a problem hiding this comment.
I thought you switched from objects back to primitives again. As I see all other boolean properties use primitives.
There was a problem hiding this comment.
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: |
|
@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... |
In the case of static analysis with 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. |
|
@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. |
|
cool! |
|
@marcuslinke kindly reminder about release! |
4 participants
attempt to sync...
@marcuslinke please kick me to the right way as it really annoying work