-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Deprecate "network" and enable "networks" stats (remote Docker API 1.21) (branch master) #361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus add @CheckForNull
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like design question. Up to @marcuslinke
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
Closing because of duplicate PR (#362). Merged into master also. |
Superseed #360