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

Initial implementation of user-facing errors for state install and state uninstall. #2877

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

mitchell-as
Copy link
Contributor

@mitchell-as mitchell-as commented Nov 10, 2023

StoryDX-2232 As a user I can expect user facing errors when I install/uninstall packages

I don't really know what I'm doing. I tried to identify what could go wrong during install/uninstall, and tried some various install/uninstall scenarios to see what happened:

  • Not authenticated install/uninstall
  • Install with non-existent package name
  • Install package that already exists
  • Uninstall with non-existent package

They should all return user-facing errors now.

@mitchell-as mitchell-as force-pushed the mitchell/dx-2232 branch 3 times, most recently from 0e31cd6 to 0786182 Compare November 10, 2023 22:53
type CommitError struct {
Type string
Message string
*locale.LocalizedError // for legacy, non-user-facing error usages
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since not all consumers are using user-facing errors, keep using the localized errors from the buildplanner for those consumers. The user-facing error rationalizer in this PR is making use of Type.

Copy link
Member

Choose a reason for hiding this comment

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

I believe that for this to be used this type needs an Error() function that returns the localized error message. Unless there is another way to surface this that I am missing.

Copy link
Contributor Author

@mitchell-as mitchell-as Nov 14, 2023

Choose a reason for hiding this comment

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

I think it inherits locale.LocalizedError's Error() function. Otherwise there would be a compiler error that you cannot use a CommitError struct as an error type. For example, https://github.com/ActiveState/cli/pull/2877/files#diff-14612946db10f18744a68890b9e353cf87d4da9104592d9fe4346264ed83ef77R320 is the localized error used by non-user-facing errors applications.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh yes, you're right!

@mitchell-as
Copy link
Contributor Author

Test failures are not due to this PR.

@mitchell-as mitchell-as requested a review from MDrakos November 13, 2023 19:07
@mitchell-as mitchell-as marked this pull request as ready for review November 13, 2023 19:07
Copy link
Member

@MDrakos MDrakos left a comment

Choose a reason for hiding this comment

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

This is a great start and my comments are relatively minor. I think the user-facing error initiative is one that we will add to and evolve. This is great groundwork for us to add more errors to the package operation flow over time.

// Error staging a commit during install.
case errors.As(*err, &commitError):
switch commitError.Type {
case bpModel.NotFoundErrorType:
Copy link
Member

Choose a reason for hiding this comment

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

I believe the Forbidden error type can also be covered by this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite because of the message, but I've added another case with a slightly different message.


// Error staging a commit during install.
case errors.As(*err, &commitError):
switch commitError.Type {
Copy link
Member

Choose a reason for hiding this comment

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

We should at least log the error message returned from the buildplanner for error types we are not handling here. We could also have a default case that just forwards the error message from the API. I know that for the ParseError we definitely want to do that this gives us the ability to have updated error messages in the case that the build expression format changes.

type CommitError struct {
Type string
Message string
*locale.LocalizedError // for legacy, non-user-facing error usages
Copy link
Member

Choose a reason for hiding this comment

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

I believe that for this to be used this type needs an Error() function that returns the localized error message. Unless there is another way to surface this that I am missing.

@mitchell-as mitchell-as requested a review from MDrakos November 14, 2023 20:16
Copy link
Member

@MDrakos MDrakos left a comment

Choose a reason for hiding this comment

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

Looking good, just one minor change request.

)
case bpModel.NoChangeSinceLastCommitErrorType:
*err = errs.WrapUserFacing(*err,
locale.Tl("err_packages_exists", "That package is already installed."),
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 this message will have to be more generic as it could also arise when running state uninstall.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May I ask how? If the user specifies a requirement that is not already installed, they'll get a requirementNotFoundErr, which is handled below.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh I see, I missed that part. Thanks for pointing that out.

type CommitError struct {
Type string
Message string
*locale.LocalizedError // for legacy, non-user-facing error usages
Copy link
Member

Choose a reason for hiding this comment

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

Ahh yes, you're right!

@mitchell-as mitchell-as requested a review from MDrakos November 14, 2023 23:00
@mitchell-as
Copy link
Contributor Author

#2877 (comment)

)
case bpModel.NoChangeSinceLastCommitErrorType:
*err = errs.WrapUserFacing(*err,
locale.Tl("err_packages_exists", "That package is already installed."),
Copy link
Member

Choose a reason for hiding this comment

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

Ahh I see, I missed that part. Thanks for pointing that out.

@mitchell-as mitchell-as reopened this Nov 15, 2023
@mitchell-as mitchell-as merged commit 94b8811 into version/0-43-0-RC1 Nov 15, 2023
10 of 13 checks passed
@mitchell-as mitchell-as deleted the mitchell/dx-2232 branch November 15, 2023 14:43
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