Assorted improvements, and add "--version, -v", and "--help, -h" flags#284
Merged
thaJeztah merged 9 commits intodocker:masterfrom May 28, 2023
Merged
Assorted improvements, and add "--version, -v", and "--help, -h" flags#284thaJeztah merged 9 commits intodocker:masterfrom
thaJeztah merged 9 commits intodocker:masterfrom
Conversation
Don't set Env if not set; the default is already handled if it's nil; from the documentation: https://pkg.go.dev/os/[email protected]#Cmd.Env // If Env is nil, the new process uses the current process's // environment. Use `os/exec/Cmd.Environ()` instead of `os.Environ()`, which was added in go1.19, and handles additional environment variables, such as `PWD` on POSIX systems, and `SYSTEMROOT` on Windows. https://pkg.go.dev/os/[email protected]#Cmd.Environ Also remove a redundant `fmt.Sprintf()`, as we're only concatenating strings. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
- Explicitly suppress some unhandled errors - Use "pass" credentials helper in examples, which is available on more platforms than "secretservice" (only supporte on Linux) - Update domain and username in examples. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
GNU guidelines describes; https://www.gnu.org/prep/standards/html_node/_002d_002dversion.html#g_t_002d_002dversion The program’s name should be a constant string; don’t compute it from argv[0]. The idea is to state the standard or canonical name for the program, not its file name. Although the above recommendation is for `--version` output, it probably makes sense to do the same for the "usage" output. Before this change: /usr/local/bin/docker-credential-osxkeychain invalid command Usage: /usr/local/bin/docker-credential-osxkeychain <store|get|erase|list|version> /Applications/Docker.app/Contents/Resources/bin/docker-credential-osxkeychain invalid command Usage: /Applications/Docker.app/Contents/Resources/bin/docker-credential-osxkeychain <store|get|erase|list|version> With this patch: /usr/local/bin/docker-credential-osxkeychain invalid command Usage: docker-credential-osxkeychain <store|get|erase|list|version> /Applications/Docker.app/Contents/Resources/bin/docker-credential-osxkeychain invalid command Usage: docker-credential-osxkeychain <store|get|erase|list|version> Signed-off-by: Sebastiaan van Stijn <[email protected]>
Don't use an err if we can print the error immediately :) Signed-off-by: Sebastiaan van Stijn <[email protected]>
- renamed the "key" variable, which was slightly confusing
- include the name of the binary in the error
Before this change:
docker-credential-osxkeychain nosuchaction
Unknown credential action `nosuchaction`
After this change:
docker-credential-osxkeychain nosuchaction
docker-credential-osxkeychain: unknown action: nosuchaction
Signed-off-by: Sebastiaan van Stijn <[email protected]>
As recommended in the GNU documentation; - https://www.gnu.org/prep/standards/standards.html#g_t_002d_002dversion - https://www.gnu.org/prep/standards/standards.html#g_t_002d_002dhelp With this patch: $ docker-credential-osxkeychain --version docker-credential-osxkeychain (github.com/docker/docker-credential-helpers) v0.7.0-51-g26c426e.m $ docker-credential-osxkeychain -v docker-credential-osxkeychain (github.com/docker/docker-credential-helpers) v0.7.0-51-g26c426e.m $ docker-credential-osxkeychain --help Usage: docker-credential-osxkeychain <store|get|erase|list|version> $ docker-credential-osxkeychain -h Usage: docker-credential-osxkeychain <store|get|erase|list|version> Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
crazy-max
reviewed
May 28, 2023
Comment on lines
+72
to
+74
| case "--version", "-v": | ||
| _ = PrintVersion(os.Stdout) | ||
| os.Exit(0) |
Member
There was a problem hiding this comment.
Do we need an extra version flag as we already have the version action?
Member
Author
There was a problem hiding this comment.
Mostly making them a bit more standard; GNU (and POSIX) define --help and --version as the minimum; https://www.gnu.org/prep/standards/standards.html#Command_002dLine-Interfaces
All programs should support two standard options: ‘--version’ and ‘--help’. CGI programs should accept these as command-line options, and also if given as the PATH_INFO; for instance, visiting ‘http://example.org/p.cgi/--help’ in a browser should output the same information as invoking ‘p.cgi --help’ from the command line.
I could omit the shorthand ones though
crazy-max
approved these changes
May 28, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See individual commits for details. Some outlined below:
client: use os/exec/Cmd.Environ() instead of os.Environ()
Don't set Env if not set; the default is already handled if it's nil; from
the documentation: https://pkg.go.dev/os/[email protected]#Cmd.Env
Use
os/exec/Cmd.Environ()instead ofos.Environ(), which was added ingo1.19, and handles additional environment variables, such as
PWDon POSIXsystems, and
SYSTEMROOTon Windows. https://pkg.go.dev/os/[email protected]#Cmd.EnvironAlso remove a redundant
fmt.Sprintf(), as we're only concatenating strings.credentials: Serve(): use "Name instead of "os.Args[0]" for usage output
GNU guidelines describes; https://www.gnu.org/prep/standards/html_node/_002d_002dversion.html#g_t_002d_002dversion
Although the above recommendation is for
--versionoutput, it probably makessense to do the same for the "usage" output.
Before this change:
With this patch:
credentials: Serve(): simplify error-handling logic
Don't use an err if we can print the error immediately :)
credentials: HandleCommand(): improve error for unknown command/action
Before this change:
After this change:
credentials: Serve(): implement "--version, -v", and "--help, -h" flags
As recommended in the GNU documentation;
With this patch:
credentials: define consts for supported actions (sub-commands)