secretservice: replace pkg/errors for Go stdlib errors, and fix unhandled error #111
Conversation
This code was always returning a nil error instead of the error produced by the org.freedesktop.Secret.Prompt.Prompt call. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
pkg/errors' errors.Wrap function implicitly discards nil-errors. While this is convenient, it also can be err-prone, as this behavior differs from go stdlib, making it easy to miss conditions where the reader assumes an error is returned (but in reality no error). This patch updates the code to explicitly handle non-nil errors to prevent accidental regressions if this code would be rewritten using go stdlib. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The pkg/errors dependency was introduced with the secretservice implementation in 7f2ef9f in March 2019. go1.13 (September 2019) introduced native support for unwrapping errors, no longer requiring this dependency to be used, and the pkg/errors module has been archived (as feature complete). While pkg/errors does have some advantages (for example, it can provide a stack trace), this functionality doesn't appear to be used in this module, and the pkg/errors package is not used in other implementations (for macOS). This patch removes the dependency, replacing its use for the equivalent in Go stdlib. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
cc @joshblum @mmaxim @heronhaye PTAL We are considering switching Docker's credential helpers to use your module; docker/docker-credential-helpers#282, and are wondering if it's an option to start tagging releases for this module (v0.x.x would be fine if the API should not be considered stable yet) |
|
@thaJeztah thanks! and yes we can tag releases |
|
Thanks for reviewing, and thanks for considering tagging. For a slight bit of context; we had an implementation of the osx-keychain code in our helper that was done quite some time ago (same for secret service); still functional, but needed some updates. Your module was achieving similar goals, so why not share the effort 😄 We may still have to re-design some of our own module structure to better separate some of the "library" code from binary code (so we didn't do a v1.0.0 for that reason), but we're aware some Linux distributions also include our credentials helpers in their packages, and generally prefer seeing a tag. I definitely don't want to force your hands here, and definitely don't "do a v1.0.0" (and now being locked into "no breaking changes allowed!"), just trying to see if it's an option! I'll also look around in our team(s) to see if we can contribute changes; while working on this PR, I noticed a bunch of placeholder GoDoc comments (as things go), to lift the code a bit. Thanks again in either case for considering! |
|
@thaJeztah we made a release v0.0.1 |
|
Spotted it! Thanks! |
3 participants
secretservice: fix SecretService.PromptAndWait discarding error
This code was always returning a nil error instead of the error produced
by the org.freedesktop.Secret.Prompt.Prompt call.
secretservice: SecretService.openSessionRaw explicitly handle error
pkg/errors' errors.Wrap function implicitly discards nil-errors. While
this is convenient, it also can be err-prone, as this behavior differs
from go stdlib, making it easy to miss conditions where the reader assumes
an error is returned (but in reality no error).
This patch updates the code to explicitly handle non-nil errors to prevent
accidental regressions if this code would be rewritten using go stdlib.
secretservice: rename var that shadowed type
secretservice: fix GoDoc comment
secretservice: replace pkg/errors for Go stdlib errors
The pkg/errors dependency was introduced with the secretservice implementation
in 7f2ef9f in March 2019.
go1.13 (September 2019) introduced native support for unwrapping errors, no
longer requiring this dependency to be used, and the pkg/errors module has
been archived (as feature complete).
While pkg/errors does have some advantages (for example, it can provide a
stack trace), this functionality doesn't appear to be used in this module,
and the pkg/errors package is not used in other implementations (for macOS).
This patch removes the dependency, replacing its use for the equivalent
in Go stdlib.