cli
cli copied to clipboard
Fix:CLI returns "unknown flag" error when you misspell a command and pass a flag
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.
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
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
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.
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 :)
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
@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.