From 18276ed75d7a32ce4a9cc6c95b2d7d86af8a67d0 Mon Sep 17 00:00:00 2001 From: mdrakos Date: Wed, 1 Nov 2023 13:13:55 -0700 Subject: [PATCH 1/2] 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 2/2] 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.