-
Notifications
You must be signed in to change notification settings - Fork 176
[WIP] APP-214 Enable 'docker app validate' for experimental mode only #625
Conversation
looks like there's a merge-conflict, so might need a rebase. |
2ab8657
to
1687455
Compare
internal/commands/root.go
Outdated
|
||
isExperimentalMode := dockerCli.ClientInfo().HasExperimental | ||
for _, ccmd := range listOfCommands { | ||
value, ok := ccmd.Annotations["experimental"] |
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.
What do you think of:
switch ccmd.Annotations["experimental"]{
case "true":
if isExperimentalMode {
cmd.AddCommand(ccmd)
}
default:
cmd.AddCommand(ccmd)
}
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.
Thanks! Updated it.
The tests are failing. |
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.
Shouldn't this PR use docker/cli PR-2095, targeting your own docker/cli fork, so we can review that everything is aligned? And mark this PR as Draft so we won't merge it until the docker/cli PR is merged?
1687455
to
abd8a98
Compare
What's the status of this one ? Shall we better close ? |
I am not sure about this :(
I'll rebase the PR on cli and wait for it to be accepted and merged, we are
waiting for some feedback from Sebastiaan on it. Then, probably we can
update the vendor in the PR for docker app....?
…On Thu, Oct 3, 2019 at 10:37 AM Nicolas De loof ***@***.***> wrote:
What's the status of this one ? Shall we better close ?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#625?email_source=notifications&email_token=AAGFXL3NBHBKDNTVWNCGCULQMW4MLA5CNFSM4IYZWFQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAHTW2Y#issuecomment-537869163>,
or mute the thread
</~https://github.com/notifications/unsubscribe-auth/AAGFXL3FIWZKJCADMHV4VDTQMW4MLANCNFSM4IYZWFQA>
.
|
abd8a98
to
12bac15
Compare
0abfed5
to
807c3ca
Compare
807c3ca
to
9635037
Compare
Codecov Report
@@ Coverage Diff @@
## master #625 +/- ##
==========================================
- Coverage 69.3% 69.28% -0.02%
==========================================
Files 63 63
Lines 3388 3396 +8
==========================================
+ Hits 2348 2353 +5
- Misses 731 733 +2
- Partials 309 310 +1
Continue to review full report at Codecov.
|
[[override]] | ||
name = "github.com/Microsoft/hcsshim" | ||
revision = "2226e083fc390003ae5aa8325c3c92789afa0e7a" | ||
|
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.
nit: empty line
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.
LGTM
Signed-off-by: Anca Iordache <anca.iordache@docker.com>
Signed-off-by: Anca Iordache <anca.iordache@docker.com>
9635037
to
ed930a8
Compare
Signed-off-by: Anca Iordache anca.iordache@docker.com
Fixes APP-214
Requires PR-2095
- What I did
Enabled validate command only for experimental mode
- How I did it
Added validate command to the list of commands only if experimental flag is on.
- How to verify it
- Description for the changelog
Add validate to the list of docker app commands only in experimental mode.
- A picture of a cute animal (not mandatory but encouraged)