-
Notifications
You must be signed in to change notification settings - Fork 173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pass: return correct error, and ignore empty stores on list #321
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #321 +/- ##
==========================================
- Coverage 54.10% 51.46% -2.65%
==========================================
Files 9 8 -1
Lines 621 513 -108
==========================================
- Hits 336 264 -72
+ Misses 238 212 -26
+ Partials 47 37 -10 ☔ View full report in Codecov by Sentry. |
85a678b
to
bed185c
Compare
commit 2fc2313 changed the errors returned by the pass credentials-helper to use a errCredentialsNotFound. This error string is used in the client to distinguish a "not found" error from other errors. (see [client.Get][1]). However, there were additional second code-paths that returned a custom error, which would not be detected as a "not found" error, resulting in an error when logging out; Removing login credentials for https://index.docker.io/v1/ WARNING: could not erase credentials: https://index.docker.io/v1/: error erasing credentials - err: exit status 1, out: `error getting credentials - err: exit status 1, out: `no usernames for https://index.docker.io/v1/`` This patch: - updates Pass.Get() to return a errCredentialsNotFound if no credentials were found - updates Pass.List() to not return an error if any of the domains had no credentials stored. [1]: https://github.com/docker/docker-credential-helpers/blob/73b9e5d51f8dc9f598e08a0f2171c5d5a828e76b/client/client.go#L51-L55 Signed-off-by: Sebastiaan van Stijn <[email protected]>
FWIW, we should probably consider using the But this also requires the pass-store to be initialised in that directory, so that required more work; docker-credential-helpers/pass/pass.go Lines 107 to 110 in 73b9e5d
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! thanks from all linux users 🐧 !
Let me bring this one in; thanks! I saw some small bits that could use a cleanup after this; will also discuss doing a new patch release 👍 |
commit 2fc2313 changed the errors returned by the pass credentials-helper to use a errCredentialsNotFound. This error string is used in the client to distinguish a "not found" error from other errors. (see client.Get).
However, there were additional second code-paths that returned a custom error, which would not be detected as a "not found" error, resulting in an error when logging out;
This patch:
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)