Skip to content
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

cmd: propagate keyring reading errors #5360

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

johanbrandhorst
Copy link
Collaborator

Reading a token from the keyring can sometimes fail for reasons other than the token not being present in the keyring. This PR introduces a new error used to indicate that the token requested could not be found in the keyring and propagates errors from the keyring up to the caller, so that the caller can act on them.

Additionally, this fixes a bug in the client cache where it would incorrectly assume that a token had been removed from the keyring, when in fact there was an error reading from the keyring.

Copy link
Collaborator

@moduli moduli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've ran the e2e tests a number of times and it seems to have resolved the failure.

@johanbrandhorst johanbrandhorst added this to the 0.19.x milestone Dec 13, 2024
moduli
moduli previously approved these changes Dec 18, 2024
jimlambrt
jimlambrt previously approved these changes Dec 19, 2024
Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a few minor suggestions for unit tests. Ty!

internal/cmd/base/keyring.go Show resolved Hide resolved
@johanbrandhorst johanbrandhorst marked this pull request as ready for review December 20, 2024 20:02
@johanbrandhorst johanbrandhorst requested a review from a team as a code owner December 20, 2024 20:02
Reading a token from the keyring can sometimes fail for reasons
other than the token not being present in the keyring. This PR
introduces a new error used to indicate that the token requested
could not be found in the keyring and propagates errors from the
keyring up to the caller, so that the caller can act on them.

Additionally, this fixes a bug in the client cache where it would
incorrectly assume that a token had been removed from the
keyring, when in fact there was an error reading from the keyring.
@johanbrandhorst johanbrandhorst dismissed stale reviews from jimlambrt and moduli via da1c4de December 20, 2024 23:08
@johanbrandhorst johanbrandhorst force-pushed the jbrandhorst-add-error-check-to-keyring-load branch from cff33ab to da1c4de Compare December 20, 2024 23:08
@johanbrandhorst johanbrandhorst merged commit 1a63207 into main Dec 23, 2024
61 of 62 checks passed
@johanbrandhorst johanbrandhorst deleted the jbrandhorst-add-error-check-to-keyring-load branch December 23, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants