Skip to content
This repository has been archived by the owner on May 18, 2021. It is now read-only.

Helpful error when okta credentials not in keyring #238

Merged
merged 2 commits into from
Oct 29, 2019

Conversation

pelletier
Copy link
Contributor

No description provided.

Copy link
Member

@Fauzyy Fauzyy left a comment

Choose a reason for hiding this comment

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

@pelletier looks good!

@@ -598,6 +598,9 @@ type OktaProvider struct {
func (p *OktaProvider) Retrieve() (sts.Credentials, string, error) {
log.Debugf("Using okta provider (%s)", p.OktaAccountName)
item, err := p.Keyring.Get(p.OktaAccountName)
if err == keyring.ErrKeyNotFound {
Copy link
Member

Choose a reason for hiding this comment

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

@pelletier on second thought, could we move this error handling into the cmd package one level up where Retrieve is called. This feels like something the CLI client should be responsible for.

If you like, you could just add it to one of the commands to start and have a TODO there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put here because the same handling was performed a few lines below. Happy to move both if you think it’s better.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep the unmarshal error here for now, and move the ErrKeyNotFound outside. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took a look this morning. Provider.Retrieve is used 5 separate times in cmd:

Screen Shot 2019-10-29 at 10 04 06 AM

Pushing this error handling would probably need a bit of factoring to avoid copy pasting, which seems outside the scope of this PR. Happy to go either way!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's pretty minor, let's merge this in the meantime 👍

Copy link
Member

@Fauzyy Fauzyy left a comment

Choose a reason for hiding this comment

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

One more comment

@Fauzyy Fauzyy merged commit 3a97126 into master Oct 29, 2019
@Fauzyy Fauzyy deleted the pelletier/error-message-keyring branch October 29, 2019 16:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants