Deprecate "network" and enable "networks" stats (remote Docker API 1.21) (branch master) by bonigarcia · Pull Request #361 · docker-java/docker-java
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add java doc with @since to indicate when it appeared
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object under question, doesn't it have some better structure?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus add @CheckForNull
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like over naming. Class itself about stats and naming vars with stats tail under question.
@marcuslinke imho better name it as is to be closer to docker API docs. Hopefully for this variable docker devs placed normal name.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables are normally named like JSON fields in docker-java. But what about getter? getNetwork()/getNetworks() should return an dedicated object instead of a map, right?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no specific structure and wrappers for it. Until it just a map iface:stats Map looks good. But i don't like <Object> how it expected for usage?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do you want create the same analogue as PortBindings?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then @marcuslinke should create 2.x or 2.1 branch
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are API breaking changes already in master, so this will not work. We should really consider another branching model...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcuslinke just create new branch based on previous to cc5b11a (hope it has no bad changes before) then @bonigarcia will PR to this branch and you will do 2.1.3 in parallel.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Done. Created branch 2.x
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR to branch 2.x in #362
bonigarcia
changed the title
Deprecate "network" and enable "networks" stats (remote Docker API 1.21)
Deprecate "network" and enable "networks" stats (remote Docker API 1.21) (branch master)
Closing because of duplicate PR (#362). Merged into master also.
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