Assorted improvements, and add "--version, -v", and "--help, -h" flags #284
Conversation
This file contains 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
Don't set Env if not set; the default is already handled if it's nil; from the documentation: https://pkg.go.dev/os/exec@go1.20.4#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/exec@go1.20.4#Cmd.Environ Also remove a redundant `fmt.Sprintf()`, as we're only concatenating strings. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- 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 <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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 <github@gone.nl>
Don't use an err if we can print the error immediately :) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- 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 <github@gone.nl>
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 <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #284 +/- ##
==========================================
- Coverage 55.23% 54.68% -0.56%
==========================================
Files 9 9
Lines 668 673 +5
==========================================
- Hits 369 368 -1
- Misses 256 262 +6
Partials 43 43
☔ View full report in Codecov by Sentry. |
crazy-max
reviewed
May 28, 2023
Comment on lines
+72
to
+74
| case "--version", "-v": | ||
| _ = PrintVersion(os.Stdout) | ||
| os.Exit(0) |
There was a problem hiding this comment.
Do we need an extra version flag as we already have the version action?
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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/exec@go1.20.4#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/exec@go1.20.4#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)