Deprecate "network" and enable "networks" stats (remote Docker API 1.21) (branch master) by bonigarcia · Pull Request #361 · docker-java/docker-java

@bonigarcia

@bonigarcia

KostyaSha

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?

KostyaSha

Choose a reason for hiding this comment

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

Plus add @CheckForNull

@bonigarcia

KostyaSha

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 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)

Nov 11, 2015

@marcuslinke

Closing because of duplicate PR (#362). Merged into master also.