Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

[WIP] APP-214 Enable 'docker app validate' for experimental mode only #625

Merged
merged 2 commits into from
Nov 12, 2019

Conversation

aiordache
Copy link
Contributor

@aiordache aiordache commented Sep 20, 2019

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

$ DOCKER_CLI_EXPERIMENTAL="disabled" docker app --help
Usage:	docker app COMMAND
Commands:
  ...
  run         Run an App from an App image
  update      Update a running App
$ DOCKER_CLI_EXPERIMENTAL="enabled" docker app --help
Usage:	docker app COMMAND
Commands:
  ...
  run         Run an App from an App image
  update      Update a running App
  validate    Check that an App definition (.dockerapp) is syntactically correct
$ DOCKER_CLI_EXPERIMENTAL="enabled" docker app validate
Validated "/home/anca/go/src/github.com/docker/app/examples/test.dockerapp"
$ DOCKER_CLI_EXPERIMENTAL="disabled" docker app validate
"validate" is not a docker app command
See 'docker app --help'

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

@thaJeztah
Copy link
Member

looks like there's a merge-conflict, so might need a rebase.


isExperimentalMode := dockerCli.ClientInfo().HasExperimental
for _, ccmd := range listOfCommands {
value, ok := ccmd.Annotations["experimental"]
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Updated it.

@silvin-lubecki
Copy link
Contributor

The tests are failing.

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a 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?

@aiordache aiordache changed the title Enable 'docker app validate' for experimental mode only [WIP] Enable 'docker app validate' for experimental mode only Sep 24, 2019
@ndeloof
Copy link
Contributor

ndeloof commented Oct 3, 2019

What's the status of this one ? Shall we better close ?

@aiordache
Copy link
Contributor Author

aiordache commented Oct 3, 2019 via email

@aiordache aiordache changed the title [WIP] Enable 'docker app validate' for experimental mode only [WIP] APP-214 Enable 'docker app validate' for experimental mode only Oct 3, 2019
@aiordache aiordache force-pushed the app-214_experimental_validate branch from abd8a98 to 12bac15 Compare October 3, 2019 10:33
@aiordache aiordache force-pushed the app-214_experimental_validate branch 5 times, most recently from 0abfed5 to 807c3ca Compare November 8, 2019 16:55
@codecov
Copy link

codecov bot commented Nov 12, 2019

Codecov Report

Merging #625 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
internal/commands/validate.go 75% <100%> (+0.8%) ⬆️
internal/commands/root.go 74.69% <100%> (+2.33%) ⬆️
types/parameters/parameters.go 92.06% <0%> (-4.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e0989c...ed930a8. Read the comment docs.

[[override]]
name = "github.com/Microsoft/hcsshim"
revision = "2226e083fc390003ae5aa8325c3c92789afa0e7a"

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: empty line

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a 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>
@rumpl rumpl merged commit aeb12c0 into docker:master Nov 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants