cli icon indicating copy to clipboard operation
cli copied to clipboard

Fix:CLI returns "unknown flag" error when you misspell a command and pass a flag

Open Billy-North opened this issue 2 years ago • 6 comments

Set the DisableFlagParsing value to true in cmd/docker to pass commands as arguements.

- What I did Reproduced the bug #4550 and identified a solution.

- How I did it Set DisableFlagParsing to true in cmd/docker/docker.go.

cobra documentation DisableFlagParsing disables the flag parsing. If this is true all flags will be passed to the command as arguments.

- How to verify it Ran the following commands to verify the issue had been resolved and to check for any introduced issues.

docker iamges --filter

returns

docker: 'iamges' is not a docker command.
See 'docker --help'

I also ran a few other commands to check the output.

An invalid command with an invalid flag.

docker iamges --filterr

Returns

docker: 'iamges' is not a docker command.
See 'docker --help'

Check an existing command with a flag and value in the flag still functions as expected.

docker ps --filter status="running"

Returns

CONTAINER ID   IMAGE            COMMAND             CREATED       STATUS       PORTS     NAMES
9440fe53b574   docker-cli-dev   "/bin/sh -c bash"   2 hours ago   Up 2 hours             nifty_franklin

Check that valid command with an invalid flag returns the flag help.

docker images --filterrr

Returns

unknown flag: --filterrr
See 'docker images --help'.

Check valid command with valid flag but no arguement for the flag

docker ps --filter

Returns

flag needs an argument: --filter
See 'docker ps --help'.

- Description for the changelog

When TraverseChildren is set to true in cobra, flags will be parsed before looking for the appropriate command to run which was causing the unexpected flag errors for commands that do not exist. Setting TraverseChildren to false is not really an option - from what I can see it looks as though its intentionally been set to true to chain commands/flags.

The change I have made checks that the command exists and returns an invalid docker command error if it does not before the flag error is returned.

closes #4550

edit: Update with new PR changes.

Billy-North avatar Sep 16 '23 09:09 Billy-North

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 59.70%. Comparing base (421e3b5) to head (ce5992f).

:exclamation: Current head ce5992f differs from pull request most recent head 33845a4

Please upload reports for the commit 33845a4 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4568      +/-   ##
==========================================
- Coverage   61.34%   59.70%   -1.65%     
==========================================
  Files         298      288      -10     
  Lines       20706    24822    +4116     
==========================================
+ Hits        12703    14820    +2117     
- Misses       7103     9115    +2012     
+ Partials      900      887      -13     

codecov-commenter avatar Sep 16 '23 12:09 codecov-commenter

Thanks for working on this!

So (but I haven't dug into the exact logic) the complication may be that the commandline accepts flags in multiple places: "top-level" options, and "per command" options. For example (using a compose command as example, where compose is a CLI plugin);

docker --host=unix://var/run/docker.sock compose --file=...  up --dry-run my_compose_service_name

thaJeztah avatar Sep 18 '23 13:09 thaJeztah

It seems like there have not been any updates on Billy-North's change in a month. I will give it a try. if anyone opposes please let me know.

achneerov avatar Oct 23 '23 01:10 achneerov

It seems like there have not been any updates on Billy-North's change in a month. I will give it a try. if anyone opposes please let me know.

Go for it, I have been too busy to look at this for the past few weeks and I am doubtful I will get a chance again for at least another few weeks. Feel free to take this one :)

Billy-North avatar Oct 23 '23 12:10 Billy-North

Hi, I have tested multiple parameters on args from Billy's code, and my command is working fine.

example: docker run -d -it -i -p 8000:8000 --env-file .env --name=my-container-apps my-container-apps

exec docker asdasd --version return docker: 'asdasd' is not a docker command. and also not returning docker version

so is there any other concern about going with that PR code above? @thaJeztah

arxngr avatar Nov 02 '23 04:11 arxngr

@thaJeztah would you be able to have a look at this PR when you get a chance.

Saw this bug the other day, found it a bit frustraiting as I knew I had looked into it earlier so decided to give it a revisit :)

I have dome some force pushes to remove old commits with incorrect fixes and get a "clean" history on this branch.

Billy-North avatar May 23 '24 13:05 Billy-North