always add but hide experimental cmds and flags #28010
Conversation
cmd/docker/docker.go
Outdated
There was a problem hiding this comment.
Would it be possible to add a property to these commands / flags so that we can show/hide based on that property?
There was a problem hiding this comment.
Yes, we can do that in our fork of cobra (I suggested that for the other option as well #28008 (comment)).
So it sounds like either option would benefit from this.
|
Can you explain how it differs from the other solution? |
|
In this solution, the commands are added, parsed and then hidden. In the other solution, commands are always displayed. |
063db82 to
ee50783
Compare
Signed-off-by: Victor Vieux <vieux@docker.com>
db6cccb to
db82561
Compare
|
@dnephin @vdemeester @thaJeztah PTAL I believe it's now more clean and ready to be merged. |
5e088e7 to
77467c4
Compare
|
rebased with the vndr change (<3 @LK4D4) |
cli/command/container/start.go
Outdated
There was a problem hiding this comment.
nit: I'd rather have this just after the flag it affects
dnephin
left a comment
There was a problem hiding this comment.
One small nit, otherwise LGTM
cli/command/cli.go
Outdated
There was a problem hiding this comment.
This is now called from only one place, after initializing the client. Maybe we could remove HasExperimental() and instead just call Ping() directly?
I think it would still be only a single function call. The rest of this code is just for managing state, and we don't need the state if we're only looking at it once.
There was a problem hiding this comment.
Alternatively, we could keep HasExperimental() but make it only return the boolean, and not store anything. So we could still remove hasExperimental
Signed-off-by: Victor Vieux <vieux@docker.com>
77467c4 to
d34ca01
Compare
|
@dnephin @mlaventure I addressed both of your comments. |
|
LGTM |
always add but hide experimental cmds and flags
another solution to fix #28008
right now it uses names, any better idea ?
ping @dnephin @mlaventure @icecrime