Fix help message flags on docker stack commands and sub-commands by silvin-lubecki · Pull Request #1251 · docker/cli

silvin-lubecki · 2018-07-31T23:54:47Z

- What I did
Fix issue #1243
Add an e2e test as regression test

- How I did it
PersistentPreRunE needs to be called within the help function to initialize all the flags (notably the orchestrator flag)

- How to verify it

$ docker stack deploy --help --orchestrator=kubernetes

Usage:	docker stack deploy [OPTIONS] STACK

Deploy a new stack or update an existing stack

Aliases:
  deploy, up

Options:
  -c, --compose-file strings   Path to a Compose file, or "-" to read from stdin
      --kubeconfig string      Kubernetes config file
      --namespace string       Kubernetes namespace to use
      --orchestrator string    Orchestrator to use (swarm|kubernetes|all)

$ docker stack deploy --help --orchestrator=swarm

Usage:	docker stack deploy [OPTIONS] STACK

Deploy a new stack or update an existing stack

Aliases:
  deploy, up

Options:
      --bundle-file string     Path to a Distributed Application Bundle file
  -c, --compose-file strings   Path to a Compose file, or "-" to read from stdin
      --orchestrator string    Orchestrator to use (swarm|kubernetes|all)
      --prune                  Prune services that are no longer referenced
      --resolve-image string   Query the registry to resolve image digest and supported platforms ("always"|"changed"|"never") (default "always")
      --with-registry-auth     Send registry authentication details to Swarm agents

- Description for the changelog

Fix docker stack deploy help is missing orchestration specific flags #1243 help message flags on docker stack commands and sub-commands

- A picture of a cute animal (not mandatory but encouraged)

Lots of kudos for @albers !!

PersistentPreRunE needs to be called within the help function to initialize all the flags (notably the orchestrator flag) Add an e2e test as regression test Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

codecov-io · 2018-08-01T00:00:27Z

Codecov Report

Merging #1251 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1251      +/-   ##
==========================================
- Coverage   54.23%   54.21%   -0.02%     
==========================================
  Files         268      268              
  Lines       17809    17814       +5     
==========================================
  Hits         9658     9658              
- Misses       7543     7548       +5     
  Partials      608      608

albers · 2018-08-01T06:40:39Z

+func testStackDeployHelp(t *testing.T, orchestrator string) {
+	result := icmd.RunCommand("docker", "stack", "deploy", "--orchestrator", orchestrator, "--help")
+	result.Assert(t, icmd.Success)
+	golden.Assert(t, result.Stdout(), fmt.Sprintf("stack-deploy-help-%s.golden", orchestrator))


Is this the commonly used way to verify command outputs in this project? This will break every time an option or help message is added/modified, this producing unnecessary maintenance work.

Alternatively, we could check for the presence of specific flags via substring checks.

@albers I think it's safe to "break every time" on that command as we don't often change those, and if we do well, the test is easily fixable (go test -update or sthg like that). I do agree it's not ideal though, but 😅

albers

Works perfectly, thanks a lot.

albers · 2018-08-01T07:49:08Z

There is also a problem in docker version: --kubeconfig is annotated as kubernetes but visible for the swarm orchestrator.

# DOCKER_STACK_ORCHESTRATOR=swarm docker version --help | grep kubeconfig
      --kubeconfig string   Kubernetes config file
# DOCKER_STACK_ORCHESTRATOR=kubernetes docker version --help | grep kubeconfig
      --kubeconfig string   Kubernetes config file
# docker --version
Docker version 18.09.0-dev, build 21cce52b

vdemeester

LGTM 🦁

vdemeester · 2018-08-01T08:20:33Z

+func testStackDeployHelp(t *testing.T, orchestrator string) {
+	result := icmd.RunCommand("docker", "stack", "deploy", "--orchestrator", orchestrator, "--help")
+	result.Assert(t, icmd.Success)
+	golden.Assert(t, result.Stdout(), fmt.Sprintf("stack-deploy-help-%s.golden", orchestrator))


@albers I think it's safe to "break every time" on that command as we don't often change those, and if we do well, the test is easily fixable (go test -update or sthg like that). I do agree it's not ideal though, but 😅

silvin-lubecki · 2018-08-01T11:13:07Z

@albers I'll fix the docker version flags in a follow-up.

thaJeztah

LGTM

was wondering if this should be a unit-test, but looks like that requires more rigging if we need the full output

silvin-lubecki · 2018-08-02T13:06:39Z

Well the problem is that the root docker command does a lot of stuffs with the help command (formatting included), so testing at the stack command level won't really help testing the final feature.

albers · 2018-08-03T11:20:58Z

I'll fix the docker version flags in a follow-up.

@silvin-lubecki I created #1265 as a reminder.

Fix help message flags on docker stack commands and sub-commands

21cce52

PersistentPreRunE needs to be called within the help function to initialize all the flags (notably the orchestrator flag) Add an e2e test as regression test Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

silvin-lubecki requested review from albers, thaJeztah and vdemeester July 31, 2018 23:54

GordonTheTurtle added the status/0-triage label Jul 31, 2018

silvin-lubecki added the kind/bug label Jul 31, 2018

albers reviewed Aug 1, 2018

View reviewed changes

albers approved these changes Aug 1, 2018

View reviewed changes

vdemeester approved these changes Aug 1, 2018

View reviewed changes

albers mentioned this pull request Aug 1, 2018

Add bash completion for kubernetes orchestrator #1254

Merged

vdemeester added status/2-code-review and removed status/0-triage labels Aug 1, 2018

thaJeztah approved these changes Aug 2, 2018

View reviewed changes

thaJeztah merged commit 4fbb009 into docker:master Aug 2, 2018

GordonTheTurtle added this to the 18.09.0 milestone Aug 2, 2018

silvin-lubecki deleted the fix-stack-help-command branch August 2, 2018 13:06

albers mentioned this pull request Aug 3, 2018

Kubernetes specific option is always shown in help message of docker version #1265

Open

thaJeztah mentioned this pull request Aug 3, 2018

[18.06] backport "Fix help message flags on docker stack commands and sub-commands" #1267

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix help message flags on docker stack commands and sub-commands#1251

Fix help message flags on docker stack commands and sub-commands#1251
thaJeztah merged 1 commit into
docker:masterfrom
silvin-lubecki:fix-stack-help-command

silvin-lubecki commented Jul 31, 2018

Uh oh!

codecov-io commented Aug 1, 2018

Uh oh!

albers Aug 1, 2018

Uh oh!

vdemeester Aug 1, 2018

Uh oh!

albers left a comment

Uh oh!

albers commented Aug 1, 2018

Uh oh!

vdemeester left a comment

Uh oh!

vdemeester Aug 1, 2018

Uh oh!

silvin-lubecki commented Aug 1, 2018

Uh oh!

thaJeztah left a comment

Uh oh!

silvin-lubecki commented Aug 2, 2018

Uh oh!

albers commented Aug 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

silvin-lubecki commented Jul 31, 2018

Uh oh!

codecov-io commented Aug 1, 2018

Codecov Report

Uh oh!

albers Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

vdemeester Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

albers left a comment

Choose a reason for hiding this comment

Uh oh!

albers commented Aug 1, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki commented Aug 1, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki commented Aug 2, 2018

Uh oh!

albers commented Aug 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants