Skip to content

[RCCA-9133] Clear error msg when using api key with no secret saved locally#1465

Merged
MuweiHe merged 6 commits intomainfrom
RCCA-9133
Oct 13, 2022
Merged

[RCCA-9133] Clear error msg when using api key with no secret saved locally#1465
MuweiHe merged 6 commits intomainfrom
RCCA-9133

Conversation

@MuweiHe
Copy link
Contributor

@MuweiHe MuweiHe commented Oct 12, 2022

Checklist

  1. [CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?
    • yes: ok

What

User caught the error very vague when using an api-key with no locally saved secret: the key id being the error msg itself. This is because we used a wrapper in UseAPIKey that changed the error type, so it's no longer a TypedError and can't print out the UserFacingError.
Changed the content in Error() for this pr. Will do a follow up to discuss about all other typedErrors

References

Test & Review

@MuweiHe MuweiHe marked this pull request as ready for review October 12, 2022 20:35
@MuweiHe MuweiHe requested a review from a team as a code owner October 12, 2022 20:35
// this means the requested api-key exists, but we just don't have the secret saved locally
return &errors.UnconfiguredAPISecretError{APIKey: apiKey, ClusterID: clusterID}
// the requested api-key exists, but the secret is not saved locally
return errors.Wrap(&errors.UnconfiguredAPISecretError{APIKey: apiKey, ClusterID: clusterID}, errors.APISecretNotSavedLocallyErrorMsg)

Choose a reason for hiding this comment

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

Did you figure out why Error() is being called but UserFacingError() isn't?

Choose a reason for hiding this comment

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

We shouldn't need to change this line if the user-facing error can be displayed properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because in

return errors.NewWrapErrorWithSuggestions(err, errors.APIKeyUseFailedErrorMsg, fmt.Sprintf(errors.APIKeyUseFailedSuggestions, apiKey))
we changed the error type to errors.ErrorWithSuggestionsImpl and it's no longer a cli typed error.

Copy link
Contributor Author

@MuweiHe MuweiHe Oct 12, 2022

Choose a reason for hiding this comment

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

Or we can don't wrap any suggestion in command_use.go, and wrap it in UseApiKey and FetchApiKeyError.

Choose a reason for hiding this comment

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

  1. I think the Error() and UserFacingError() functions are a bad pattern (due to this bug that started this incident). We should move the UserFacingError() into Error() and only show Error().
  2. Sounds like we're trying to add suggestions twice in this codepath? Do we have support for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2 yes, the UserFacingError is already an error with a suggestion. it'll look like:

mhe@C02DV0KVMD6T cli % confluent api-key use 123 --resource lkc-1x00v
Error: no API secret for API key "123" of resource "lkc-1x00v" stored in local CLI state

Suggestions:
    Store the API secret with `confluent api-key store 123 --resource lkc-1x00v`.

So using those typed errors you're not expected to add more wrappers after returning them to the command RunE. I can look further into all use cases of those typed errors and we'll discuss if we want to 1: remove typed errors and just keep the suggestion wrappers; 2: remove extra wrappers to ensure the error is being returned as a CLITypedError. Let's fix this one for this week's release first

Copy link

Choose a reason for hiding this comment

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

This fixes the bug, but we should likely do the same for the other custom errors, so we don't encounter a similar bug in the future. Can you open a ticket for that?

@MuweiHe MuweiHe merged commit e3dcebc into main Oct 13, 2022
@MuweiHe MuweiHe deleted the RCCA-9133 branch October 13, 2022 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants