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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions internal/locale/locales/en-us.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1150,6 +1150,8 @@ bundle_version_updated:
other: "Bundle updated: [NOTICE]{{.V0}}@{{.V1}}[/RESET]"
err_package_removed:
other: Failed to remove package
err_remove_requirement_not_found:
other: Could not remove requirement '[ACTIONABLE]{{.V0}}[/RESET]', because it does not exist.
err_bundle_removed:
other: Failed to remove bundle
err_packages_removed:
Expand Down Expand Up @@ -1747,6 +1749,8 @@ operation_success_local:
Run [ACTIONABLE]state push[/RESET] to save changes to the platform.
buildplan_err:
other: "Could not plan build, platform responded with:\n{{.V0}}\n{{.V1}}"
err_buildplanner_head_on_branch_moved:
other: The branch you're trying to update has changed remotely, please run '[ACTIONABLE]state pull[/RESET]'.
transient_solver_tip:
other: You may want to retry this command if the failure was related to a resourcing or networking issue.
warning_git_project_mismatch:
Expand Down
3 changes: 2 additions & 1 deletion internal/runners/packages/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ func NewInstall(prime primeable) *Install {
}

// Run executes the install behavior.
func (a *Install) Run(params InstallRunParams, nsType model.NamespaceType) error {
func (a *Install) Run(params InstallRunParams, nsType model.NamespaceType) (rerr error) {
defer rationalizeError(a.prime.Auth(), &rerr)
logging.Debug("ExecuteInstall")
return requirements.NewRequirementOperation(a.prime).ExecuteRequirementOperation(
params.Package.Name(),
Expand Down
68 changes: 68 additions & 0 deletions internal/runners/packages/rationalize.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package packages

import (
"errors"

"github.com/ActiveState/cli/internal/errs"
"github.com/ActiveState/cli/internal/locale"
"github.com/ActiveState/cli/internal/runbits/rationalize"
bpModel "github.com/ActiveState/cli/pkg/platform/api/buildplanner/model"
"github.com/ActiveState/cli/pkg/platform/authentication"
"github.com/ActiveState/cli/pkg/platform/runtime/buildexpression"
)

func rationalizeError(auth *authentication.Auth, err *error) {
var commitError *bpModel.CommitError
var requirementNotFoundErr *buildexpression.RequirementNotFoundError

switch {
case err == nil:
return

// No activestate.yaml.
case errors.Is(*err, rationalize.ErrNoProject):
*err = errs.WrapUserFacing(*err,
locale.T("err_no_project"),
errs.SetInput(),
)

// 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.

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.

*err = errs.WrapUserFacing(*err,
locale.Tl("err_packages_not_found", "Could not make runtime changes because your project was not found."),
errs.SetInput(),
errs.SetTips(locale.T("tip_private_project_auth")),
)
case bpModel.ForbiddenErrorType:
*err = errs.WrapUserFacing(*err,
locale.Tl("err_packages_forbidden", "Could not make runtime changes because you do not have permission to do so."),
errs.SetInput(),
errs.SetTips(locale.T("tip_private_project_auth")),
)
case bpModel.HeadOnBranchMovedErrorType:
*err = errs.WrapUserFacing(*err,
locale.T("err_buildplanner_head_on_branch_moved"),
errs.SetInput(),
)
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.

errs.SetInput(),
)
default:
*err = errs.WrapUserFacing(*err,
locale.Tl("err_packages_buildplanner_error", "Could not make runtime changes due to the following error: {{.V0}}", commitError.Message),
errs.SetInput(),
)
}

// Requirement not found for uninstall.
case errors.As(*err, &requirementNotFoundErr):
*err = errs.WrapUserFacing(*err,
locale.Tr("err_remove_requirement_not_found", requirementNotFoundErr.Name),
errs.SetInput(),
)
}
}
7 changes: 4 additions & 3 deletions internal/runners/packages/uninstall.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package packages

import (
"github.com/ActiveState/cli/internal/locale"
"github.com/ActiveState/cli/internal/logging"
"github.com/ActiveState/cli/internal/runbits/rationalize"
"github.com/ActiveState/cli/internal/runbits/requirements"
bpModel "github.com/ActiveState/cli/pkg/platform/api/buildplanner/model"
"github.com/ActiveState/cli/pkg/platform/model"
Expand All @@ -24,10 +24,11 @@ func NewUninstall(prime primeable) *Uninstall {
}

// Run executes the uninstall behavior.
func (u *Uninstall) Run(params UninstallRunParams, nsType model.NamespaceType) error {
func (u *Uninstall) Run(params UninstallRunParams, nsType model.NamespaceType) (rerr error) {
defer rationalizeError(u.prime.Auth(), &rerr)
logging.Debug("ExecuteUninstall")
if u.prime.Project() == nil {
return locale.NewInputError("err_no_project")
return rationalize.ErrNoProject
}

return requirements.NewRequirementOperation(u.prime).ExecuteRequirementOperation(
Expand Down
31 changes: 26 additions & 5 deletions pkg/platform/api/buildplanner/model/buildplan.go
Original file line number Diff line number Diff line change
Expand Up @@ -302,22 +302,43 @@ func IsErrorResponse(errorType string) bool {
errorType == ComitHasNoParentErrorType
}

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!

}

func ProcessCommitError(commit *Commit, fallbackMessage string) error {
if commit.Error == nil {
return errs.New(fallbackMessage)
}

switch commit.Type {
case NotFoundErrorType:
return locale.NewInputError("err_buildplanner_commit_not_found", "Could not find commit, received message: {{.V0}}", commit.Message)
return &CommitError{
commit.Type, commit.Message,
locale.NewInputError("err_buildplanner_commit_not_found", "Could not find commit, received message: {{.V0}}", commit.Message),
}
case ParseErrorType:
return locale.NewInputError("err_buildplanner_parse_error", "The platform failed to parse the build expression, received message: {{.V0}}. Path: {{.V1}}", commit.Message, commit.ParseError.Path)
return &CommitError{
commit.Type, commit.Message,
locale.NewInputError("err_buildplanner_parse_error", "The platform failed to parse the build expression, received message: {{.V0}}. Path: {{.V1}}", commit.Message, commit.ParseError.Path),
}
case ForbiddenErrorType:
return locale.NewInputError("err_buildplanner_forbidden", "Operation forbidden: {{.V0}}, received message: {{.V1}}", commit.Operation, commit.Message)
return &CommitError{
commit.Type, commit.Message,
locale.NewInputError("err_buildplanner_forbidden", "Operation forbidden: {{.V0}}, received message: {{.V1}}", commit.Operation, commit.Message),
}
case HeadOnBranchMovedErrorType:
return errs.Wrap(locale.NewInputError("err_buildplanner_head_on_branch_moved", "The branch you're trying to update has changed remotely, please run '[ACTIONABLE]state pull[/RESET]'."), "received message: "+commit.Error.Message)
return errs.Wrap(&CommitError{
commit.Type, commit.Error.Message,
locale.NewInputError("err_buildplanner_head_on_branch_moved"),
}, "received message: "+commit.Error.Message)
case NoChangeSinceLastCommitErrorType:
return errs.Wrap(locale.NewInputError("err_buildplanner_no_change_since_last_commit", "No new changes to commit."), commit.Error.Message)
return errs.Wrap(&CommitError{
commit.Type, commit.Error.Message,
locale.NewInputError("err_buildplanner_no_change_since_last_commit", "No new changes to commit."),
}, commit.Error.Message)
default:
return errs.New(fallbackMessage)
}
Expand Down
10 changes: 9 additions & 1 deletion pkg/platform/runtime/buildexpression/buildexpression.go
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,11 @@ func (e *BuildExpression) addRequirement(requirement model.Requirement) error {
return nil
}

type RequirementNotFoundError struct {
Name string
*locale.LocalizedError // for legacy non-user-facing error usages
}

func (e *BuildExpression) removeRequirement(requirement model.Requirement) error {
requirementsNode, err := e.getRequirementsNode()
if err != nil {
Expand All @@ -789,7 +794,10 @@ func (e *BuildExpression) removeRequirement(requirement model.Requirement) error
}

if !found {
return locale.NewInputError("err_remove_requirement_not_found", "Could not remove requirement '[ACTIONABLE]{{.V0}}[/RESET]', because it does not exist.", requirement.Name)
return &RequirementNotFoundError{
requirement.Name,
locale.NewInputError("err_remove_requirement_not_found", "", requirement.Name),
}
}

solveNode, err := e.getSolveNode()
Expand Down
13 changes: 11 additions & 2 deletions test/integration/package_int_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"path/filepath"
"runtime"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -398,9 +399,13 @@ func (suite *PackageIntegrationTestSuite) TestPackage_Duplicate() {
cp.ExpectExitCode(0)

cp = ts.Spawn("install", "requests") // install again
cp.Expect("No new changes to commit")
cp.Expect("already installed")
cp.ExpectNotExitCode(0)
ts.IgnoreLogErrors()

if strings.Count(cp.Snapshot(), " x ") != 2 { // 2 because "Creating commit x Failed" is also printed
suite.Fail("Expected exactly ONE error message, got: ", cp.Snapshot())
}
}

func (suite *PackageIntegrationTestSuite) PrepareActiveStateYAML(ts *e2e.Session) {
Expand All @@ -425,9 +430,13 @@ func (suite *PackageIntegrationTestSuite) TestPackage_UninstallDoesNotExist() {
suite.PrepareActiveStateYAML(ts)

cp := ts.Spawn("uninstall", "doesNotExist")
cp.Expect("Error occurred while trying to create a commit")
cp.Expect("does not exist")
cp.ExpectExitCode(1)
ts.IgnoreLogErrors()

if strings.Count(cp.Snapshot(), " x ") != 2 { // 2 because "Creating commit x Failed" is also printed
suite.Fail("Expected exactly ONE error message, got: ", cp.Snapshot())
}
}

func (suite *PackageIntegrationTestSuite) TestJSON() {
Expand Down
Loading