From 00941a81e8498ac7dbd3b944475ab745a393b404 Mon Sep 17 00:00:00 2001 From: mitchell Date: Wed, 10 Jan 2024 13:42:00 -0500 Subject: [PATCH] Added new analytic for resolving merge conflicts on pull. --- internal/analytics/constants/constants.go | 9 +++++++++ internal/runners/pull/pull.go | 24 +++++++++++++++++++---- test/integration/pull_int_test.go | 15 ++++++++++++++ 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/internal/analytics/constants/constants.go b/internal/analytics/constants/constants.go index 3fde7bd722..3672b87f5b 100644 --- a/internal/analytics/constants/constants.go +++ b/internal/analytics/constants/constants.go @@ -228,3 +228,12 @@ const UpdateErrorNoInstaller = "Downloaded update does not have installer" // UpdateErrorInstallPath is sent if the install path could not be detected const UpdateErrorInstallPath = "Could not detect install path" + +// CatInteractions is the event category used for tracking user interactions. +const CatInteractions = "interactions" + +// ActVcsConflict is the event action sent when `state pull` results in a conflict. +const ActVcsConflict = "vcs-conflict" + +// LabelVcsConflictMergeStrategyFailed is the label to use when a merge fails. +const LabelVcsConflictMergeStrategyFailed = "Failed" diff --git a/internal/runners/pull/pull.go b/internal/runners/pull/pull.go index 7fe032b8c5..c00b3a8ac8 100644 --- a/internal/runners/pull/pull.go +++ b/internal/runners/pull/pull.go @@ -5,6 +5,8 @@ import ( "strings" "github.com/ActiveState/cli/internal/analytics" + anaConst "github.com/ActiveState/cli/internal/analytics/constants" + "github.com/ActiveState/cli/internal/analytics/dimensions" "github.com/ActiveState/cli/internal/config" "github.com/ActiveState/cli/internal/constants" "github.com/ActiveState/cli/internal/errs" @@ -13,6 +15,7 @@ import ( "github.com/ActiveState/cli/internal/output" "github.com/ActiveState/cli/internal/primer" "github.com/ActiveState/cli/internal/prompt" + "github.com/ActiveState/cli/internal/rtutils/ptr" "github.com/ActiveState/cli/internal/runbits" buildscriptRunbits "github.com/ActiveState/cli/internal/runbits/buildscript" "github.com/ActiveState/cli/internal/runbits/commit" @@ -117,20 +120,24 @@ func (p *Pull) Run(params *PullParams) (rerr error) { // 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. + strategy := bpModel.MergeCommitStrategyFastForward + bp := model.NewBuildPlannerModel(p.auth) params := &model.MergeCommitParams{ Owner: remoteProject.Owner, Project: remoteProject.Project, TargetRef: localCommit.String(), OtherRef: remoteCommit.String(), - Strategy: bpModel.MergeCommitStrategyFastForward, + Strategy: strategy, } 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()) + strategy = bpModel.MergeCommitStrategyRecursiveOverwriteOnConflict + c, err := p.performMerge(*remoteCommit, *localCommit, remoteProject, p.project.BranchName(), strategy) if err != nil { + p.notifyMergeStrategy(anaConst.LabelVcsConflictMergeStrategyFailed, *localCommit, remoteProject) return errs.Wrap(err, "performing merge commit failed") } resultingCommit = &c @@ -138,6 +145,8 @@ func (p *Pull) Run(params *PullParams) (rerr error) { logging.Debug("Fast-forward merge succeeded, setting commit ID to %s", resultCommit.String()) resultingCommit = &resultCommit } + + p.notifyMergeStrategy(string(strategy), *localCommit, remoteProject) } commitID, err := commitmediator.Get(p.project) @@ -170,7 +179,7 @@ func (p *Pull) Run(params *PullParams) (rerr error) { return nil } -func (p *Pull) performMerge(remoteCommit, localCommit strfmt.UUID, namespace *project.Namespaced, branchName string) (strfmt.UUID, error) { +func (p *Pull) performMerge(remoteCommit, localCommit strfmt.UUID, namespace *project.Namespaced, branchName string, strategy bpModel.MergeStrategy) (strfmt.UUID, error) { // Re-enable in DX-2307. //err := p.mergeBuildScript(strategies, remoteCommit) //if err != nil { @@ -189,7 +198,7 @@ func (p *Pull) performMerge(remoteCommit, localCommit strfmt.UUID, namespace *pr Project: namespace.Project, TargetRef: localCommit.String(), OtherRef: remoteCommit.String(), - Strategy: bpModel.MergeCommitStrategyRecursiveOverwriteOnConflict, + Strategy: strategy, } resultCommit, err := bp.MergeCommit(params) if err != nil { @@ -253,3 +262,10 @@ func resolveRemoteProject(prj *project.Project) (*project.Namespaced, error) { return ns, nil } + +func (p *Pull) notifyMergeStrategy(strategy string, commitID strfmt.UUID, namespace *project.Namespaced) { + p.analytics.EventWithLabel(anaConst.CatInteractions, anaConst.ActVcsConflict, strategy, &dimensions.Values{ + CommitID: ptr.To(commitID.String()), + ProjectNameSpace: ptr.To(namespace.String()), + }) +} diff --git a/test/integration/pull_int_test.go b/test/integration/pull_int_test.go index 05947aca57..fcada61fd6 100644 --- a/test/integration/pull_int_test.go +++ b/test/integration/pull_int_test.go @@ -6,10 +6,13 @@ import ( "runtime" "testing" + "github.com/ActiveState/cli/internal/analytics/client/sync/reporters" + anaConst "github.com/ActiveState/cli/internal/analytics/constants" "github.com/ActiveState/cli/internal/constants" "github.com/ActiveState/cli/internal/fileutils" "github.com/ActiveState/cli/internal/testhelpers/e2e" "github.com/ActiveState/cli/internal/testhelpers/tagsuite" + bpModel "github.com/ActiveState/cli/pkg/platform/api/buildplanner/model" "github.com/ActiveState/cli/pkg/platform/runtime/buildscript" "github.com/ActiveState/cli/pkg/project" "github.com/ActiveState/cli/pkg/projectfile" // remove in DX-2307 @@ -38,6 +41,8 @@ func (suite *PullIntegrationTestSuite) TestPull() { //suite.Require().True(fileutils.DirExists(projectConfigDir)) //suite.Assert().True(fileutils.FileExists(filepath.Join(projectConfigDir, constants.CommitIdFileName))) + suite.assertMergeStrategyNotification(ts, string(bpModel.MergeCommitStrategyFastForward)) + cp = ts.Spawn("pull") cp.Expect("already up to date") cp.ExpectExitCode(0) @@ -83,6 +88,8 @@ func (suite *PullIntegrationTestSuite) TestPull_Merge() { cp = ts.SpawnCmd("bash", "-c", fmt.Sprintf("cd %s && %s history | head -n 10", wd, exe)) cp.Expect("Merged") cp.ExpectExitCode(0) + + suite.assertMergeStrategyNotification(ts, string(bpModel.MergeCommitStrategyRecursiveOverwriteOnConflict)) } func (suite *PullIntegrationTestSuite) TestMergeBuildScript() { @@ -124,6 +131,14 @@ func (suite *PullIntegrationTestSuite) TestMergeBuildScript() { suite.Assert().Contains(string(bytes), ">>>>>>>", "No merge conflict markers are in build script") } +func (suite *PullIntegrationTestSuite) assertMergeStrategyNotification(ts *e2e.Session, strategy string) { + conflictEvents := filterEvents(parseAnalyticsEvents(suite, ts), func(e reporters.TestLogEntry) bool { + return e.Category == anaConst.CatInteractions && e.Action == anaConst.ActVcsConflict + }) + suite.Assert().Equal(1, len(conflictEvents), "Should have a single VCS Conflict event report") + suite.Assert().Equal(strategy, conflictEvents[0].Label) +} + func TestPullIntegrationTestSuite(t *testing.T) { suite.Run(t, new(PullIntegrationTestSuite)) }