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

Add ExternalErrors error type #3280

Merged
merged 7 commits into from
May 3, 2024
Merged

Add ExternalErrors error type #3280

merged 7 commits into from
May 3, 2024

Conversation

MDrakos
Copy link
Member

@MDrakos MDrakos commented May 2, 2024

TaskDX-2735 We can mark errors as 'ExternalError' and avoid it being reported to rollbar

The major changes are in internal/errs/errs.go and internal/locale/errors.go. The rest of the changeset it updating user input errors to external errors. I tried to catch as many as I could. The vast majority of NewInputError calls are correct so this mostly updates the wrapped errors.

@github-actions github-actions bot changed the base branch from master to version/0-45-0-RC1 May 2, 2024 22:25
@MDrakos MDrakos requested a review from mitchell-as May 3, 2024 18:40
mitchell-as
mitchell-as previously approved these changes May 3, 2024
Copy link
Contributor

@mitchell-as mitchell-as left a comment

Choose a reason for hiding this comment

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

This looks fine to me. I've left a couple of comments with suggestions for improvements if we want to keep an eye towards the future. I'll leave it up to you if you think they are worth it or not.

Comment on lines +24 to +25
inputErr bool
externalErr bool
Copy link
Contributor

Choose a reason for hiding this comment

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

There may be a case for changing these into a single enum field if they are mutually exclusive. I'll leave it up to you though.

@@ -189,10 +189,10 @@ func (r *Initialize) Run(params *RunParams) (rerr error) {

version, err := deriveVersion(lang, languageVersion, r.auth)
if err != nil {
if inferred || !locale.IsInputError(err) {
if inferred || (!locale.IsInputError(err) && !errs.IsExternalError(err)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is at least the second time I've seen this. Would it make sense to add a function that would wrap this so if we were to add more types of errors, we'd only have to change that function instead of all of these instances?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good idea. Thanks for the suggestion.

@@ -143,10 +143,10 @@ func confirmLock(prom prompt.Prompter) error {
return nil
}

func fetchExactVersion(an analytics.Dispatcher, svc *model.SvcModel, channel, version string) (string, error) {
func fetchExactVersion(svc *model.SvcModel, channel, version string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks for catching this unused parameter and removing it!

@MDrakos MDrakos marked this pull request as ready for review May 3, 2024 20:51
@MDrakos MDrakos requested a review from mitchell-as May 3, 2024 20:51
@MDrakos MDrakos merged commit 98ed532 into version/0-45-0-RC1 May 3, 2024
6 of 7 checks passed
@MDrakos MDrakos deleted the DX-2735 branch May 3, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants