From ba33bce11ce4976aac01410bd3bff4c5bea55a9c Mon Sep 17 00:00:00 2001 From: mitchell Date: Thu, 26 Oct 2023 15:17:06 -0400 Subject: [PATCH 01/10] Add more logging to help debug random panic for buildplans that result in nil environment definitions. --- pkg/platform/runtime/store/store.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/platform/runtime/store/store.go b/pkg/platform/runtime/store/store.go index 3239f3e990..448e28970d 100644 --- a/pkg/platform/runtime/store/store.go +++ b/pkg/platform/runtime/store/store.go @@ -239,6 +239,14 @@ func (s *Store) updateEnviron(orderedArtifacts []artifact.ArtifactID, artifacts } } + if rtGlobal == nil { + // Returning nil will end up causing a nil-pointer-exception panic in setup.Update(). + // There is additional logging of the buildplan there that may help diagnose why this is happening. + logging.Error("There were artifacts returned, but none of them ended up being stored/installed.") + logging.Error("Artifacts returned: %v", orderedArtifacts) + logging.Error("Artifacts stored: %v", artifacts) + } + return rtGlobal, nil } From c0ad8fa665972d435c53db4ae3f89c61e31616f4 Mon Sep 17 00:00:00 2001 From: mdrakos Date: Mon, 30 Oct 2023 13:43:27 -0700 Subject: [PATCH 02/10] Update revert to use the build planner --- internal/locale/locales/en-us.yaml | 2 +- internal/runners/revert/revert.go | 107 +++++++++++------- .../api/buildplanner/model/buildplan.go | 19 ++++ .../api/buildplanner/request/revertcommit.go | 78 +++++++++++++ pkg/platform/model/buildplanner.go | 23 ++++ 5 files changed, 187 insertions(+), 42 deletions(-) create mode 100644 pkg/platform/api/buildplanner/request/revertcommit.go diff --git a/internal/locale/locales/en-us.yaml b/internal/locale/locales/en-us.yaml index 08b65df6fe..fa7f35c689 100644 --- a/internal/locale/locales/en-us.yaml +++ b/internal/locale/locales/en-us.yaml @@ -1715,7 +1715,7 @@ err_user_network_solution: err_revert_get_commit: other: "Could not fetch commit details for commit with ID: {{.V0}}" err_revert_commit: - other: "Could not revert{{.V0}} commit: {{.V1}}" + other: "Could not revert {{.V0}} commit: {{.V1}}" install_initial_success: other: "An activestate.yaml has been created at: {{.V0}}." err_package_info_no_packages: diff --git a/internal/runners/revert/revert.go b/internal/runners/revert/revert.go index f2e345a29a..2db7b9449f 100644 --- a/internal/runners/revert/revert.go +++ b/internal/runners/revert/revert.go @@ -11,7 +11,6 @@ import ( "github.com/ActiveState/cli/pkg/cmdlets/commit" "github.com/ActiveState/cli/pkg/localcommit" gqlmodel "github.com/ActiveState/cli/pkg/platform/api/graphql/model" - "github.com/ActiveState/cli/pkg/platform/api/mono/mono_models" "github.com/ActiveState/cli/pkg/platform/authentication" "github.com/ActiveState/cli/pkg/platform/model" "github.com/ActiveState/cli/pkg/platform/runtime/target" @@ -61,43 +60,37 @@ func (r *Revert) Run(params *Params) error { if !strfmt.IsUUID(params.CommitID) { return locale.NewInputError("err_invalid_commit_id", "Invalid commit ID") } + latestCommit, err := localcommit.Get(r.project.Dir()) if err != nil { return errs.Wrap(err, "Unable to get local commit") } + if params.CommitID == latestCommit.String() && params.To { return locale.NewInputError("err_revert_to_current_commit", "The commit to revert to cannot be the latest commit") } r.out.Notice(locale.Tl("operating_message", "", r.project.NamespaceString(), r.project.Dir())) - commitID := strfmt.UUID(params.CommitID) - var targetCommit *mono_models.Commit // the commit to revert the contents of, or the commit to revert to - var fromCommit, toCommit strfmt.UUID - if !params.To { - priorCommits, err := model.CommitHistoryPaged(commitID, 0, 2) - if err != nil { - return errs.AddTips( - locale.WrapError(err, "err_revert_get_commit", "", params.CommitID), - locale.T("tip_private_project_auth"), - ) - } - if priorCommits.TotalCommits < 2 { - return locale.NewInputError("err_revert_no_history", "Cannot revert commit {{.V0}}: no prior history", params.CommitID) - } - targetCommit = priorCommits.Commits[0] - fromCommit = commitID - toCommit = priorCommits.Commits[1].CommitID // parent commit - } else { - var err error - targetCommit, err = model.GetCommitWithinCommitHistory(latestCommit, commitID) - if err != nil { - return errs.AddTips( - locale.WrapError(err, "err_revert_get_commit", "", params.CommitID), - locale.T("tip_private_project_auth"), - ) - } - fromCommit = latestCommit - toCommit = targetCommit.CommitID + bp := model.NewBuildPlannerModel(r.auth) + targetCommitID := params.CommitID // the commit to revert the contents of, or the commit to revert to + revertParams := revertParams{ + organization: r.project.Owner(), + project: r.project.Name(), + branch: r.project.BranchName(), + parentCommitID: latestCommit.String(), + revertCommitID: params.CommitID, + } + revertFunc := r.revertCommit + if params.To { + revertFunc = r.revertToCommit + } + + targetCommit, err := model.GetCommitWithinCommitHistory(latestCommit, strfmt.UUID(targetCommitID)) + if err != nil { + return errs.AddTips( + locale.WrapError(err, "err_revert_get_commit", "", params.CommitID), + locale.T("tip_private_project_auth"), + ) } var orgs []gqlmodel.Organization @@ -126,7 +119,7 @@ func (r *Revert) Run(params *Params) error { return locale.NewInputError("err_revert_aborted", "Revert aborted by user") } - revertCommit, err := model.RevertCommitWithinHistory(fromCommit, toCommit, latestCommit) + revertCommit, err := revertFunc(revertParams, bp) if err != nil { return errs.AddTips( locale.WrapError(err, "err_revert_commit", "", preposition, params.CommitID), @@ -134,14 +127,14 @@ func (r *Revert) Run(params *Params) error { locale.T("tip_private_project_auth")) } - err = runbits.RefreshRuntime(r.auth, r.out, r.analytics, r.project, revertCommit.CommitID, true, target.TriggerRevert, r.svcModel) + err = localcommit.Set(r.project.Dir(), revertCommit.String()) if err != nil { - return locale.WrapError(err, "err_refresh_runtime") + return errs.Wrap(err, "Unable to set local commit") } - err = localcommit.Set(r.project.Dir(), revertCommit.CommitID.String()) + err = runbits.RefreshRuntime(r.auth, r.out, r.analytics, r.project, revertCommit, true, target.TriggerRevert, r.svcModel) if err != nil { - return errs.Wrap(err, "Unable to set local commit") + return locale.WrapError(err, "err_refresh_runtime") } r.out.Print(output.Prepare( @@ -149,18 +142,50 @@ func (r *Revert) Run(params *Params) error { &struct { CurrentCommitID string `json:"current_commit_id"` }{ - revertCommit.CommitID.String(), + revertCommit.String(), }, )) r.out.Notice(locale.T("operation_success_local")) return nil } -func containsCommitID(history []*mono_models.Commit, commitID strfmt.UUID) bool { - for _, c := range history { - if c.CommitID == commitID { - return true - } +type revertFunc func(params revertParams, bp *model.BuildPlanner) (strfmt.UUID, error) + +type revertParams struct { + organization string + project string + branch string + parentCommitID string + revertCommitID string +} + +func (r *Revert) revertCommit(params revertParams, bp *model.BuildPlanner) (strfmt.UUID, error) { + newCommitID, err := bp.RevertCommit(params.organization, params.project, params.branch, params.revertCommitID) + if err != nil { + return "", errs.Wrap(err, "could not revert commit") } - return false + + return newCommitID, nil +} + +func (r *Revert) revertToCommit(params revertParams, bp *model.BuildPlanner) (strfmt.UUID, error) { + buildExpression, err := bp.GetBuildExpression(params.organization, params.project, params.revertCommitID) + if err != nil { + return "", errs.Wrap(err, "could not get build expression") + } + + stageCommitParams := model.StageCommitParams{ + Owner: params.organization, + Project: params.project, + ParentCommit: params.parentCommitID, + Description: locale.Tl("revert_commit_description", "Revert to commit {{.V0}}", params.revertCommitID), + Expression: buildExpression, + } + + newCommitID, err := bp.StageCommit(stageCommitParams) + if err != nil { + return "", errs.Wrap(err, "could not stage commit") + } + + return newCommitID, nil } diff --git a/pkg/platform/api/buildplanner/model/buildplan.go b/pkg/platform/api/buildplanner/model/buildplan.go index 303e84298d..605faff0fb 100644 --- a/pkg/platform/api/buildplanner/model/buildplan.go +++ b/pkg/platform/api/buildplanner/model/buildplan.go @@ -44,6 +44,7 @@ const ( BuildLogRecipeID = "RECIPE_ID" BuildRequestID = "BUILD_REQUEST_ID" + // Version Comparators ComparatorEQ string = "eq" ComparatorGT = "gt" ComparatorGTE = "gte" @@ -51,15 +52,21 @@ const ( ComparatorLTE = "lte" ComparatorNE = "ne" + // Version Requirement keys VersionRequirementComparatorKey = "comparator" VersionRequirementVersionKey = "version" + // MIME types XArtifactMimeType = "application/x.artifact" XActiveStateArtifactMimeType = "application/x-activestate-artifacts" XCamelInstallerMimeType = "application/x-camel-installer" XGozipInstallerMimeType = "application/x-gozip-installer" XActiveStateBuilderMimeType = "application/x-activestate-builder" + // RevertCommit strategies + RevertCommitStrategyForce = "Force" + RevertCommitStrategyDefault = "Default" + // Error types ErrorType = "Error" NotFoundErrorType = "NotFound" @@ -380,6 +387,18 @@ type CreateProjectResult struct { ProjectCreated *projectCreated `json:"createProject"` } +type RevertedCommit struct { + Type string `json:"__typename"` + Commit *Commit `json:"commit"` + CommonAncestor strfmt.UUID `json:"commonAncestorID"` + ConflictPaths []string `json:"conflictPaths"` + *Error +} + +type RevertCommitResult struct { + RevertedCommit *RevertedCommit `json:"revertCommit"` +} + // Error contains an error message. type Error struct { Message string `json:"message"` diff --git a/pkg/platform/api/buildplanner/request/revertcommit.go b/pkg/platform/api/buildplanner/request/revertcommit.go new file mode 100644 index 0000000000..48df69a407 --- /dev/null +++ b/pkg/platform/api/buildplanner/request/revertcommit.go @@ -0,0 +1,78 @@ +package request + +import "github.com/ActiveState/cli/pkg/platform/api/buildplanner/model" + +func RevertCommit(organization, project, branch, commitID string) *revertCommit { + return &revertCommit{map[string]interface{}{ + "organization": organization, + "project": project, + "branch": branch, + "commitId": commitID, + // Currently, we use the force strategy for all revert commits. + // This is because we don't have a way to show the user the conflicts + // and let them resolve them yet. + // https://activestatef.atlassian.net/browse/AR-80?focusedCommentId=46998 + "strategy": model.RevertCommitStrategyForce, + }} +} + +type revertCommit struct { + vars map[string]interface{} +} + +func (r *revertCommit) Query() string { + return ` +mutation ($organization: String!, $project: String!, $commitId: String!, $branch: String!, $strategy: RevertStrategy) { + revertCommit( + input: {organization: $organization, project: $project, commitId: $commitId, branch: $branch, strategy: $strategy} + ) { + ... on RevertedCommit { + __typename + commit { + __typename + commitId + } + } + ... on RevertConflict { + __typename + message + branchName + conflictPaths + } + ... on CommitHasNoParent { + __typename + message + } + ... on NotFound { + __typename + message + mayNeedAuthentication + } + ... on ParseError { + __typename + message + path + } + ... on ValidationError { + __typename + message + } + ... on Forbidden { + __typename + message + } + ... on HeadOnBranchMoved { + __typename + message + } + ... on NoChangeSinceLastCommit { + message + commitId + } + } +}` +} + +func (r *revertCommit) Vars() map[string]interface{} { + return r.vars +} diff --git a/pkg/platform/model/buildplanner.go b/pkg/platform/model/buildplanner.go index 6237d5410a..34dd089a88 100644 --- a/pkg/platform/model/buildplanner.go +++ b/pkg/platform/model/buildplanner.go @@ -385,6 +385,29 @@ func (bp *BuildPlanner) CreateProject(params *CreateProjectParams) (strfmt.UUID, return resp.ProjectCreated.Commit.CommitID, nil } +func (bp *BuildPlanner) RevertCommit(organization, project, branch, commitID string) (strfmt.UUID, error) { + logging.Debug("RevertCommit, organization: %s, project: %s, commitID: %s", organization, project, commitID) + resp := &bpModel.RevertCommitResult{} + err := bp.client.Run(request.RevertCommit(organization, project, branch, commitID), resp) + if err != nil { + return "", processBuildPlannerError(err, "failed to revert commit") + } + + if resp.RevertedCommit == nil { + return "", errs.New("Commit is nil") + } + + if bpModel.IsErrorResponse(resp.RevertedCommit.Type) { + return "", bpModel.ProcessCommitError(resp.RevertedCommit.Commit, "Could not revert commit") + } + + if resp.RevertedCommit.Commit.CommitID == "" { + return "", errs.New("Commit does not contain commitID") + } + + return resp.RevertedCommit.Commit.CommitID, nil +} + // processBuildPlannerError will check for special error types that should be // handled differently. If no special error type is found, the fallback message // will be used. From 18276ed75d7a32ce4a9cc6c95b2d7d86af8a67d0 Mon Sep 17 00:00:00 2001 From: mdrakos Date: Wed, 1 Nov 2023 13:13:55 -0700 Subject: [PATCH 03/10] Use the buildplanner in the pull runner --- internal/runners/pull/pull.go | 60 +++++++++++-------- .../api/buildplanner/model/buildplan.go | 29 ++++++++- 2 files changed, 63 insertions(+), 26 deletions(-) diff --git a/internal/runners/pull/pull.go b/internal/runners/pull/pull.go index 2234aee863..25a366cd0b 100644 --- a/internal/runners/pull/pull.go +++ b/internal/runners/pull/pull.go @@ -1,7 +1,6 @@ package pull import ( - "errors" "path/filepath" "strings" @@ -10,6 +9,7 @@ import ( "github.com/ActiveState/cli/internal/constants" "github.com/ActiveState/cli/internal/errs" "github.com/ActiveState/cli/internal/locale" + "github.com/ActiveState/cli/internal/logging" "github.com/ActiveState/cli/internal/output" "github.com/ActiveState/cli/internal/primer" "github.com/ActiveState/cli/internal/prompt" @@ -17,6 +17,7 @@ import ( buildscriptRunbits "github.com/ActiveState/cli/internal/runbits/buildscript" "github.com/ActiveState/cli/pkg/cmdlets/commit" "github.com/ActiveState/cli/pkg/localcommit" + bpModel "github.com/ActiveState/cli/pkg/platform/api/buildplanner/model" "github.com/ActiveState/cli/pkg/platform/api/mono/mono_models" "github.com/ActiveState/cli/pkg/platform/authentication" "github.com/ActiveState/cli/pkg/platform/model" @@ -125,21 +126,31 @@ func (p *Pull) Run(params *PullParams) error { resultingCommit := remoteCommit // resultingCommit is the commit we want to update the local project file with if localCommit != nil { - strategies, err := model.MergeCommit(*remoteCommit, *localCommit) - if err != nil { - if errors.Is(err, model.ErrMergeFastForward) { - // No merge necessary - resultingCommit = localCommit - } else if !errors.Is(err, model.ErrMergeCommitInHistory) { - return locale.WrapError(err, "err_mergecommit", "Could not detect if merge is necessary.") - } + // Attempt to fast-forward merge. This will succeed if the commits are + // compatible, meaning that we can simply update the local commit ID to + // the remoteCommit ID. The commitID returned from MergeCommit with this + // strategy should just be the remote commit ID. + // If this call fails then we will try a recursive merge. + bp := model.NewBuildPlannerModel(p.auth) + params := &model.MergeCommitParams{ + Owner: remoteProject.Owner, + Project: remoteProject.Project, + TargetRef: localCommit.String(), + OtherRef: remoteCommit.String(), + Strategy: bpModel.MergeCommitStrategyFastForward, } - if err == nil && strategies != nil { - c, err := p.performMerge(strategies, *remoteCommit, *localCommit, remoteProject, p.project.BranchName()) + + resultCommit, mergeErr := bp.MergeCommit(params) + if mergeErr != nil { + logging.Debug("Merge with fast-forward failed with error: %s, trying recursive overwrite", mergeErr.Error()) + c, err := p.performMerge(*remoteCommit, *localCommit, remoteProject, p.project.BranchName()) if err != nil { return errs.Wrap(err, "performing merge commit failed") } resultingCommit = &c + } else { + logging.Debug("Fast-forward merge succeeded, setting commit ID to %s", resultCommit.String()) + resultingCommit = &resultCommit } } @@ -180,24 +191,22 @@ func (p *Pull) Run(params *PullParams) error { return nil } -func (p *Pull) performMerge(strategies *mono_models.MergeStrategies, remoteCommit strfmt.UUID, localCommit strfmt.UUID, namespace *project.Namespaced, branchName string) (strfmt.UUID, error) { - err := p.mergeBuildScript(strategies, remoteCommit) - if err != nil { - return "", errs.Wrap(err, "Could not merge local build script with remote changes") - } - +func (p *Pull) performMerge(remoteCommit, localCommit strfmt.UUID, namespace *project.Namespaced, branchName string) (strfmt.UUID, error) { p.out.Notice(output.Title(locale.Tl("pull_diverged", "Merging history"))) p.out.Notice(locale.Tr( "pull_diverged_message", - namespace.String(), branchName, localCommit.String(), remoteCommit.String())) + namespace.String(), branchName, localCommit.String(), remoteCommit.String()), + ) - commitID, err := localcommit.Get(p.project.Dir()) - if err != nil { - return "", errs.Wrap(err, "Unable to get local commit") + bp := model.NewBuildPlannerModel(p.auth) + params := &model.MergeCommitParams{ + Owner: namespace.Owner, + Project: namespace.Project, + TargetRef: localCommit.String(), + OtherRef: remoteCommit.String(), + Strategy: bpModel.MergeCommitStrategyRecursiveOverwriteOnConflict, } - - commitMessage := locale.Tr("pull_merge_commit", remoteCommit.String(), commitID.String()) - resultCommit, err := model.CommitChangeset(remoteCommit, commitMessage, strategies.OverwriteChanges) + resultCommit, err := bp.MergeCommit(params) if err != nil { return "", locale.WrapError(err, "err_pull_merge_commit", "Could not create merge commit.") } @@ -209,7 +218,8 @@ func (p *Pull) performMerge(strategies *mono_models.MergeStrategies, remoteCommi changes, _ := commit.FormatChanges(cmit) p.out.Notice(locale.Tl( "pull_diverged_changes", - "The following changes will be merged:\n{{.V0}}\n", strings.Join(changes, "\n"))) + "The following changes will be merged:\n{{.V0}}\n", strings.Join(changes, "\n")), + ) return resultCommit, nil } diff --git a/pkg/platform/api/buildplanner/model/buildplan.go b/pkg/platform/api/buildplanner/model/buildplan.go index c450bea089..d68f33e376 100644 --- a/pkg/platform/api/buildplanner/model/buildplan.go +++ b/pkg/platform/api/buildplanner/model/buildplan.go @@ -82,6 +82,8 @@ const ( MergeConflictType = "MergeConflict" FastForwardErrorType = "FastForwardError" NoCommonBaseFoundType = "NoCommonBaseFound" + ValidationErrorType = "ValidationError" + MergeConflictErrorType = "MergeConflict" ) func IsStateToolArtifact(mimeType string) bool { @@ -282,7 +284,8 @@ func IsErrorResponse(errorType string) bool { errorType == NotFoundErrorType || errorType == MergeConflictType || errorType == FastForwardErrorType || - errorType == NoCommonBaseFoundType + errorType == NoCommonBaseFoundType || + errorType == ValidationErrorType } func ProcessCommitError(commit *Commit, fallbackMessage string) error { @@ -402,6 +405,9 @@ type projectCreated struct { Type string `json:"__typename"` Commit *Commit `json:"commit"` *Error + *NotFoundError + *ParseError + *ForbiddenError } type CreateProjectResult struct { @@ -412,6 +418,13 @@ type mergedCommit struct { Type string `json:"__typename"` Commit *Commit `json:"commit"` *Error + *MergeConflictError + *MergeError + *NotFoundError + *ParseError + *ForbiddenError + *HeadOnBranchMovedError + *NoChangeSinceLastCommitError } // MergeCommitResult is the result of a merge commit mutation. @@ -622,6 +635,20 @@ type NoChangeSinceLastCommitError struct { NoChangeCommitID strfmt.UUID `json:"commitId"` } +// MergeConflictError represents an error that occurred because of a merge conflict. +type MergeConflictError struct { + CommonAncestorID strfmt.UUID `json:"commonAncestorId"` + ConflictPaths []string `json:"conflictPaths"` +} + +// MergeError represents two different errors in the BuildPlanner's graphQL +// schema with the same fields. Those errors being: FastForwardError and +// NoCommonBaseFound. Inspect the Type field to determine which error it is. +type MergeError struct { + TargetVCSRef strfmt.UUID `json:"targetVcsRef"` + OtherVCSRef strfmt.UUID `json:"otherVcsRef"` +} + // BuildExprLocation represents a location in the build script where an error occurred. type BuildExprLocation struct { Type string `json:"__typename"` From 5126fcd411c396e0a631118069917e136e2ff670 Mon Sep 17 00:00:00 2001 From: mdrakos Date: Wed, 1 Nov 2023 15:12:59 -0700 Subject: [PATCH 04/10] Update integration tests --- internal/runners/pull/pull.go | 4 +++- internal/runners/pull/rationalize.go | 31 ++++++++++++++++++++++++++++ test/integration/pull_int_test.go | 4 ++-- 3 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 internal/runners/pull/rationalize.go diff --git a/internal/runners/pull/pull.go b/internal/runners/pull/pull.go index 25a366cd0b..518b54961d 100644 --- a/internal/runners/pull/pull.go +++ b/internal/runners/pull/pull.go @@ -78,7 +78,9 @@ func (o *pullOutput) MarshalStructured(format output.Format) interface{} { return o } -func (p *Pull) Run(params *PullParams) error { +func (p *Pull) Run(params *PullParams) (rerr error) { + defer rationalizeError(&rerr) + if p.project == nil { return locale.NewInputError("err_no_project") } diff --git a/internal/runners/pull/rationalize.go b/internal/runners/pull/rationalize.go new file mode 100644 index 0000000000..4690016f48 --- /dev/null +++ b/internal/runners/pull/rationalize.go @@ -0,0 +1,31 @@ +package pull + +import ( + "errors" + + "github.com/ActiveState/cli/internal/errs" + "github.com/ActiveState/cli/internal/locale" + "github.com/ActiveState/cli/pkg/platform/api/buildplanner/model" +) + +func rationalizeError(err *error) { + if err == nil { + return + } + + var mergeCommitErr *model.MergedCommitError + + switch { + case errors.As(*err, &mergeCommitErr): + switch mergeCommitErr.Type { + // Custom target does not have a compatible history + case model.NoCommonBaseFoundType: + *err = errs.WrapUserFacing(*err, + locale.Tl("err_pull_no_common_base", + "Could not merge, no common base found between local and remote commits", + ), + errs.SetInput(), + ) + } + } +} diff --git a/test/integration/pull_int_test.go b/test/integration/pull_int_test.go index 2861d128c7..c35d75b334 100644 --- a/test/integration/pull_int_test.go +++ b/test/integration/pull_int_test.go @@ -74,7 +74,7 @@ func (suite *PullIntegrationTestSuite) TestPullSetProjectUnrelated() { cp.ExpectNotExitCode(0) cp = ts.Spawn("pull", "--non-interactive", "--set-project", "ActiveState-CLI/Python3") - cp.Expect("Could not detect common parent") + cp.Expect("no common base") cp.ExpectExitCode(1) } @@ -123,7 +123,7 @@ func (suite *PullIntegrationTestSuite) TestPull_RestoreNamespace() { // Attempt to update to unrelated project. cp := ts.Spawn("pull", "--non-interactive", "--set-project", "ActiveState-CLI/Python3") - cp.Expect("Could not detect common parent") + cp.Expect("no common base") cp.ExpectNotExitCode(0) // Verify namespace is unchanged. From 69aa7d1486961897736e9fa92c6bc97aeed0af14 Mon Sep 17 00:00:00 2001 From: mdrakos Date: Thu, 2 Nov 2023 12:32:55 -0700 Subject: [PATCH 05/10] Update revertCommit call --- internal/runners/revert/revert.go | 9 +-------- pkg/platform/api/buildplanner/request/revertcommit.go | 11 ++++++----- pkg/platform/model/buildplanner.go | 4 ++-- 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/internal/runners/revert/revert.go b/internal/runners/revert/revert.go index 9bf34fb230..329602f0d9 100644 --- a/internal/runners/revert/revert.go +++ b/internal/runners/revert/revert.go @@ -75,7 +75,6 @@ func (r *Revert) Run(params *Params) error { revertParams := revertParams{ organization: r.project.Owner(), project: r.project.Name(), - branch: r.project.BranchName(), parentCommitID: latestCommit.String(), revertCommitID: params.CommitID, } @@ -126,11 +125,6 @@ func (r *Revert) Run(params *Params) error { locale.T("tip_private_project_auth")) } - err = runbits.RefreshRuntime(r.auth, r.out, r.analytics, r.project, revertCommit, true, target.TriggerRevert, r.svcModel) - if err != nil { - return locale.WrapError(err, "err_refresh_runtime") - } - err = commitmediator.Set(r.project, revertCommit.String()) if err != nil { return errs.Wrap(err, "Unable to set local commit") @@ -158,13 +152,12 @@ type revertFunc func(params revertParams, bp *model.BuildPlanner) (strfmt.UUID, type revertParams struct { organization string project string - branch string parentCommitID string revertCommitID string } func (r *Revert) revertCommit(params revertParams, bp *model.BuildPlanner) (strfmt.UUID, error) { - newCommitID, err := bp.RevertCommit(params.organization, params.project, params.branch, params.revertCommitID) + newCommitID, err := bp.RevertCommit(params.organization, params.project, params.parentCommitID, params.revertCommitID) if err != nil { return "", errs.Wrap(err, "could not revert commit") } diff --git a/pkg/platform/api/buildplanner/request/revertcommit.go b/pkg/platform/api/buildplanner/request/revertcommit.go index 48df69a407..70c0829f7a 100644 --- a/pkg/platform/api/buildplanner/request/revertcommit.go +++ b/pkg/platform/api/buildplanner/request/revertcommit.go @@ -2,11 +2,11 @@ package request import "github.com/ActiveState/cli/pkg/platform/api/buildplanner/model" -func RevertCommit(organization, project, branch, commitID string) *revertCommit { +func RevertCommit(organization, project, targetVcsRef, commitID string) *revertCommit { return &revertCommit{map[string]interface{}{ "organization": organization, "project": project, - "branch": branch, + "targetVcsRef": targetVcsRef, "commitId": commitID, // Currently, we use the force strategy for all revert commits. // This is because we don't have a way to show the user the conflicts @@ -22,9 +22,9 @@ type revertCommit struct { func (r *revertCommit) Query() string { return ` -mutation ($organization: String!, $project: String!, $commitId: String!, $branch: String!, $strategy: RevertStrategy) { +mutation ($organization: String!, $project: String!, $commitId: String!, $targetVcsRef: String!, $strategy: RevertStrategy) { revertCommit( - input: {organization: $organization, project: $project, commitId: $commitId, branch: $branch, strategy: $strategy} + input: {organization: $organization, project: $project, commitId: $commitId, targetVcsRef: $targetVcsRef, strategy: $strategy} ) { ... on RevertedCommit { __typename @@ -36,7 +36,8 @@ mutation ($organization: String!, $project: String!, $commitId: String!, $branch ... on RevertConflict { __typename message - branchName + commitId + targetCommitId conflictPaths } ... on CommitHasNoParent { diff --git a/pkg/platform/model/buildplanner.go b/pkg/platform/model/buildplanner.go index bc3d863b57..8bcb341460 100644 --- a/pkg/platform/model/buildplanner.go +++ b/pkg/platform/model/buildplanner.go @@ -395,10 +395,10 @@ func (bp *BuildPlanner) CreateProject(params *CreateProjectParams) (strfmt.UUID, return resp.ProjectCreated.Commit.CommitID, nil } -func (bp *BuildPlanner) RevertCommit(organization, project, branch, commitID string) (strfmt.UUID, error) { +func (bp *BuildPlanner) RevertCommit(organization, project, parentCommitID, commitID string) (strfmt.UUID, error) { logging.Debug("RevertCommit, organization: %s, project: %s, commitID: %s", organization, project, commitID) resp := &bpModel.RevertCommitResult{} - err := bp.client.Run(request.RevertCommit(organization, project, branch, commitID), resp) + err := bp.client.Run(request.RevertCommit(organization, project, parentCommitID, commitID), resp) if err != nil { return "", processBuildPlannerError(err, "failed to revert commit") } From 824ecadf24e0b2396d55a185e9ee63550e36067b Mon Sep 17 00:00:00 2001 From: mdrakos Date: Thu, 2 Nov 2023 14:13:16 -0700 Subject: [PATCH 06/10] Attempt to fix integration test failures --- internal/runners/revert/revert.go | 7 +++---- test/integration/revert_int_test.go | 5 ++--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/internal/runners/revert/revert.go b/internal/runners/revert/revert.go index 329602f0d9..7eaa5d7633 100644 --- a/internal/runners/revert/revert.go +++ b/internal/runners/revert/revert.go @@ -79,8 +79,10 @@ func (r *Revert) Run(params *Params) error { revertCommitID: params.CommitID, } revertFunc := r.revertCommit + preposition := "" if params.To { revertFunc = r.revertToCommit + preposition = " to" // need leading whitespace } targetCommit, err := model.GetCommitWithinCommitHistory(latestCommit, strfmt.UUID(targetCommitID)) @@ -99,10 +101,7 @@ func (r *Revert) Run(params *Params) error { return locale.WrapError(err, "err_revert_get_organizations", "Could not get organizations for current user") } } - preposition := "" - if params.To { - preposition = " to" // need leading whitespace - } + if !r.out.Type().IsStructured() { r.out.Print(locale.Tl("revert_info", "You are about to revert{{.V0}} the following commit:", preposition)) commit.PrintCommit(r.out, targetCommit, orgs) diff --git a/test/integration/revert_int_test.go b/test/integration/revert_int_test.go index 9a59e4e2d8..3389fc3355 100644 --- a/test/integration/revert_int_test.go +++ b/test/integration/revert_int_test.go @@ -42,7 +42,7 @@ func (suite *RevertIntegrationTestSuite) TestRevert() { e2e.OptArgs("history"), e2e.OptWD(wd), ) - cp.Expect("Revert commit " + commitID) + cp.Expect("Reverted commit " + commitID) cp.Expect("- urllib3") cp.Expect("+ argparse") // parent commit cp.Expect("+ urllib3") // commit whose changes were just reverted @@ -83,7 +83,7 @@ func (suite *RevertIntegrationTestSuite) TestRevert_failsOnCommitNotInHistory() cp.Expect(fmt.Sprintf("Operating on project %s", namespace)) cp.SendLine("Y") cp.Expect(commitID) - cp.Expect("The commit being reverted is not within the current commit's history") + cp.Expect("The target commit is not within the current commit's history") cp.ExpectNotExitCode(0) } @@ -115,7 +115,6 @@ func (suite *RevertIntegrationTestSuite) TestRevertTo() { e2e.OptArgs("history"), e2e.OptWD(wd), ) - cp.Expect("Reverting to commit " + commitID) cp.Expect("- argparse") // effectively reverting previous commit cp.Expect("+ argparse") // commit being effectively reverted cp.Expect("+ urllib3") // commit reverted to From b251994dde852f96d393985c54202a95c375f586 Mon Sep 17 00:00:00 2001 From: mdrakos Date: Thu, 2 Nov 2023 14:28:47 -0700 Subject: [PATCH 07/10] Update expect --- test/integration/revert_int_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/revert_int_test.go b/test/integration/revert_int_test.go index 3389fc3355..de39e339e8 100644 --- a/test/integration/revert_int_test.go +++ b/test/integration/revert_int_test.go @@ -42,7 +42,7 @@ func (suite *RevertIntegrationTestSuite) TestRevert() { e2e.OptArgs("history"), e2e.OptWD(wd), ) - cp.Expect("Reverted commit " + commitID) + cp.Expect("Reverted commit for commit " + commitID) cp.Expect("- urllib3") cp.Expect("+ argparse") // parent commit cp.Expect("+ urllib3") // commit whose changes were just reverted From f6fb72d94de71ff244d52dce7c41257c7f6775cb Mon Sep 17 00:00:00 2001 From: mdrakos Date: Thu, 2 Nov 2023 15:49:26 -0700 Subject: [PATCH 08/10] Address PR review feedback Capitalize error messages Add back expect line --- internal/locale/locales/en-us.yaml | 2 +- internal/runners/revert/revert.go | 6 +++--- pkg/platform/api/buildplanner/model/buildplan.go | 4 ++-- pkg/platform/model/buildplanner.go | 2 +- test/integration/revert_int_test.go | 2 ++ 5 files changed, 9 insertions(+), 7 deletions(-) diff --git a/internal/locale/locales/en-us.yaml b/internal/locale/locales/en-us.yaml index b207da6be3..80cceaee0e 100644 --- a/internal/locale/locales/en-us.yaml +++ b/internal/locale/locales/en-us.yaml @@ -1703,7 +1703,7 @@ err_user_network_solution: err_revert_get_commit: other: "Could not fetch commit details for commit with ID: {{.V0}}" err_revert_commit: - other: "Could not revert {{.V0}} commit: {{.V1}}" + other: "Could not revert{{.V0}} commit: {{.V1}}" install_initial_success: other: "An activestate.yaml has been created at: {{.V0}}." err_package_info_no_packages: diff --git a/internal/runners/revert/revert.go b/internal/runners/revert/revert.go index 7eaa5d7633..de5bb3450c 100644 --- a/internal/runners/revert/revert.go +++ b/internal/runners/revert/revert.go @@ -158,7 +158,7 @@ type revertParams struct { func (r *Revert) revertCommit(params revertParams, bp *model.BuildPlanner) (strfmt.UUID, error) { newCommitID, err := bp.RevertCommit(params.organization, params.project, params.parentCommitID, params.revertCommitID) if err != nil { - return "", errs.Wrap(err, "could not revert commit") + return "", errs.Wrap(err, "Could not revert commit") } return newCommitID, nil @@ -167,7 +167,7 @@ func (r *Revert) revertCommit(params revertParams, bp *model.BuildPlanner) (strf func (r *Revert) revertToCommit(params revertParams, bp *model.BuildPlanner) (strfmt.UUID, error) { buildExpression, err := bp.GetBuildExpression(params.organization, params.project, params.revertCommitID) if err != nil { - return "", errs.Wrap(err, "could not get build expression") + return "", errs.Wrap(err, "Could not get build expression") } stageCommitParams := model.StageCommitParams{ @@ -180,7 +180,7 @@ func (r *Revert) revertToCommit(params revertParams, bp *model.BuildPlanner) (st newCommitID, err := bp.StageCommit(stageCommitParams) if err != nil { - return "", errs.Wrap(err, "could not stage commit") + return "", errs.Wrap(err, "Could not stage commit") } return newCommitID, nil diff --git a/pkg/platform/api/buildplanner/model/buildplan.go b/pkg/platform/api/buildplanner/model/buildplan.go index c480357973..7df0997423 100644 --- a/pkg/platform/api/buildplanner/model/buildplan.go +++ b/pkg/platform/api/buildplanner/model/buildplan.go @@ -415,7 +415,7 @@ type CreateProjectResult struct { ProjectCreated *projectCreated `json:"createProject"` } -type RevertedCommit struct { +type revertedCommit struct { Type string `json:"__typename"` Commit *Commit `json:"commit"` CommonAncestor strfmt.UUID `json:"commonAncestorID"` @@ -424,7 +424,7 @@ type RevertedCommit struct { } type RevertCommitResult struct { - RevertedCommit *RevertedCommit `json:"revertCommit"` + RevertedCommit *revertedCommit `json:"revertCommit"` } type mergedCommit struct { diff --git a/pkg/platform/model/buildplanner.go b/pkg/platform/model/buildplanner.go index 8bcb341460..392eb90a76 100644 --- a/pkg/platform/model/buildplanner.go +++ b/pkg/platform/model/buildplanner.go @@ -400,7 +400,7 @@ func (bp *BuildPlanner) RevertCommit(organization, project, parentCommitID, comm resp := &bpModel.RevertCommitResult{} err := bp.client.Run(request.RevertCommit(organization, project, parentCommitID, commitID), resp) if err != nil { - return "", processBuildPlannerError(err, "failed to revert commit") + return "", processBuildPlannerError(err, "Failed to revert commit") } if resp.RevertedCommit == nil { diff --git a/test/integration/revert_int_test.go b/test/integration/revert_int_test.go index de39e339e8..b864fe2c1a 100644 --- a/test/integration/revert_int_test.go +++ b/test/integration/revert_int_test.go @@ -115,10 +115,12 @@ func (suite *RevertIntegrationTestSuite) TestRevertTo() { e2e.OptArgs("history"), e2e.OptWD(wd), ) + cp.Expect("Revert to commit " + commitID) cp.Expect("- argparse") // effectively reverting previous commit cp.Expect("+ argparse") // commit being effectively reverted cp.Expect("+ urllib3") // commit reverted to cp.Expect("+ python") // initial commit + fmt.Println("Output: ", cp.Snapshot()) } func (suite *RevertIntegrationTestSuite) TestRevertTo_failsOnCommitNotInHistory() { From 1611afe785b005a2ebd34b5948078bc317b412d8 Mon Sep 17 00:00:00 2001 From: mdrakos Date: Fri, 3 Nov 2023 08:32:03 -0700 Subject: [PATCH 09/10] Remove debug line --- test/integration/revert_int_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/revert_int_test.go b/test/integration/revert_int_test.go index b864fe2c1a..a7230c411d 100644 --- a/test/integration/revert_int_test.go +++ b/test/integration/revert_int_test.go @@ -120,7 +120,6 @@ func (suite *RevertIntegrationTestSuite) TestRevertTo() { cp.Expect("+ argparse") // commit being effectively reverted cp.Expect("+ urllib3") // commit reverted to cp.Expect("+ python") // initial commit - fmt.Println("Output: ", cp.Snapshot()) } func (suite *RevertIntegrationTestSuite) TestRevertTo_failsOnCommitNotInHistory() { From dd57061d5657ea37c939efa3e3450249e18b2667 Mon Sep 17 00:00:00 2001 From: mitchell Date: Fri, 3 Nov 2023 15:11:43 -0400 Subject: [PATCH 10/10] `state` supports JSON output for `--verbose`. --- cmd/state/internal/cmdtree/cmdtree.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/state/internal/cmdtree/cmdtree.go b/cmd/state/internal/cmdtree/cmdtree.go index 7fc7f6c1fd..ec3cf4b5fd 100644 --- a/cmd/state/internal/cmdtree/cmdtree.go +++ b/cmd/state/internal/cmdtree/cmdtree.go @@ -313,6 +313,7 @@ func newStateCommand(globals *globalOptions, prime *primer.Values) *captain.Comm cmd.SetHasVariableArguments() cmd.OnExecStart(cmdCall.OnExecStart) cmd.OnExecStop(cmdCall.OnExecStop) + cmd.SetSupportsStructuredOutput() return cmd }