Nolantestsquash by emirot · Pull Request #1119 · argoproj-labs/gitops-promoter

emirot · 2026-02-27T23:19:13Z

No description provided.

Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>

Signed-off-by: emirot <emirot.nolan@gmail.com>

codecov-commenter · 2026-02-27T23:21:07Z

Bundle Report

Bundle size has no change ✅

Signed-off-by: emirot <emirot.nolan@gmail.com>

codecov-commenter · 2026-02-27T23:46:20Z

Codecov Report

❌ Patch coverage is 64.17910% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.37%. Comparing base (3e90f58) to head (7764a13).

Files with missing lines	Patch %	Lines
internal/git/git.go	59.37%	11 Missing and 2 partials ⚠️
...rnal/controller/changetransferpolicy_controller.go	68.57%	6 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1119      +/-   ##
==========================================
- Coverage   50.53%   49.37%   -1.16%     
==========================================
  Files          51       51              
  Lines        5501     5551      +50     
==========================================
- Hits         2780     2741      -39     
- Misses       2418     2501      +83     
- Partials      303      309       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: emirot <emirot.nolan@gmail.com>

Copilot

Pull request overview

Adds handling for externally squash-merged active branches by detecting branch ancestry divergence and reconciling proposed branch history so future promotion PRs don’t include stale commits.

Changes:

Introduces a git ancestry check and updates the “ours” merge strategy implementation to rewrite proposed branch history after divergence.
Extends the ChangeTransferPolicy reconciler to detect divergence and emit a new event reason/message.
Adds a controller test that simulates an external squash-merge scenario and validates PR behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`internal/types/constants/events.go`	Adds new event reason/message constants for divergence resolution.
`internal/git/git.go`	Adds `IsAncestor` and changes the “ours merge” operation to a commit-tree + force-push rewrite flow.
`internal/controller/changetransferpolicy_controller.go`	Uses ancestry detection to trigger divergence resolution; also passes PromotionStrategy into PR templating data when available.
`internal/controller/changetransferpolicy_controller_test.go`	Adds a test covering external squash-merge divergence and expected PR title behavior.
`internal/controller/testdata/ControllerConfiguration.yaml`	Test fixture whitespace-only change.

Comments suppressed due to low confidence (2)

internal/controller/changetransferpolicy_controller_test.go:908

The //nolint:unparam rationale looks inaccurate here (the function does use ctx). If the intent is to silence unparam because namespace is always passed as "default" in these tests, consider updating the comment to match the pattern used in other test helpers (e.g., promotionStrategyResource) so future readers know which parameter triggered the lint suppression.

//nolint:unparam // ctx is required for test helper signature consistency
func changeTransferPolicyResources(ctx context.Context, name, namespace string) (string, *v1.Secret, *promoterv1alpha1.ScmProvider, *promoterv1alpha1.GitRepository, *promoterv1alpha1.CommitStatus, *promoterv1alpha1.ChangeTransferPolicy) {

internal/controller/testdata/ControllerConfiguration.yaml:84

This trailing whitespace-only line appears accidental and creates noise in test fixtures. Consider removing it to keep the testdata YAML stable and minimize diff churn.

Copilot · 2026-02-28T00:30:01Z

internal/types/constants/events.go

+	// ResolvedDivergenceReason is the reason for a resolved branch divergence event (e.g., after a squash merge on the SCM).
+	ResolvedDivergenceReason = "ResolvedDivergence"
+	// ResolvedDivergenceMessage is the message for a resolved branch divergence event.
+	ResolvedDivergenceMessage = "Merged %s into %s with 'ours' strategy to resolve branch divergence (possible squash merge)"


This file notes that new event constants must be documented in docs/monitoring/events.md. The newly added ResolvedDivergence event reason/message should be added to that document to keep operator event documentation complete.

@copilot code review[agent] show me the diff needed

internal/types/constants/events.go

Copilot · 2026-02-28T00:30:02Z

internal/git/git.go

+	_, stderr, err := g.runCmd(ctx, gitPath, "merge-base", "--is-ancestor", "origin/"+ancestorBranch, "origin/"+descendantBranch)
+	if err != nil {
+		// Exit code 1 means ancestorBranch is NOT an ancestor of descendantBranch.
+		// This is a normal condition (e.g., after a squash merge), not an error.
+		// We distinguish this from actual git errors by checking stderr for unexpected messages.
+		if stderr == "" || strings.Contains(stderr, "exit status 1") {
+			logger.V(4).Info("Branch is not an ancestor", "ancestor", ancestorBranch, "descendant", descendantBranch)
+			return false, nil


IsAncestor treats any git merge-base error with empty stderr as a normal "not an ancestor" condition. That can mask real failures (e.g., context cancellation/timeout) and incorrectly trigger divergence handling. Consider checking the actual process exit code (0=true, 1=false, 128+=error) via exec.ExitError/ProcessState.ExitCode and only return (false, nil) for exit code 1; return an error for anything else.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-02-28T00:30:02Z

internal/git/git.go

+	// Force push the proposed branch to point to the new commit
+	// We use --force because we're rewriting history (the old commits are no longer ancestors)
+	_, stderr, err = g.runCmd(ctx, gitPath, "push", "--force", "origin", newCommitSha+":refs/heads/"+proposedBranch)
 	if err != nil {
-		logger.Error(err, "Failed to push merged branch", "proposedBranch", proposedBranch, "activeBranch", activeBranch, "stderr", stderr)
-		return fmt.Errorf("failed to push merged branch %q: %w", proposedBranch, err)
+		logger.Error(err, "Failed to force push branch", "proposedBranch", proposedBranch, "activeBranch", activeBranch, "stderr", stderr)
+		return fmt.Errorf("failed to push branch %q: %w", proposedBranch, err)


MergeWithOursStrategy uses git push --force, which can overwrite concurrent updates to the proposed branch (e.g., hydrator pushes) and cause data loss. Prefer --force-with-lease using the currently-fetched old SHA (or an explicit lease) so the push fails if the remote branch advanced unexpectedly.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-02-28T00:30:02Z

internal/git/git.go

+// MergeWithOursStrategy reconciles diverged branch histories after a squash merge.
 // This assumes that both branches have already been fetched via GetBranchShas earlier in the reconciliation,
 // ensuring we merge the exact same refs that were checked for conflicts.
+//
+// After a squash merge, the proposed branch's commits are no longer ancestors of the active branch.
+// This function creates a new commit on the proposed branch that:
+// 1. Keeps the exact same tree (file contents) as the proposed branch
+// 2. Has ONLY the active branch commit as its parent (NOT the old proposed commits)
+// 3. Results in a clean PR that only shows the actual differences, not old squashed commits
+//
+// This is essentially "rebasing" the proposed branch's content onto active without carrying
+// the old commit history. Future PRs will be clean because the old commits are no longer
+// in the proposed branch's ancestry.
 func (g *EnvironmentOperations) MergeWithOursStrategy(ctx context.Context, proposedBranch, activeBranch string) error {


MergeWithOursStrategy no longer performs a merge nor uses Git's -s ours strategy; it rewrites the proposed branch with a new commit created via commit-tree. Consider renaming the method (and its callers) to reflect the new behavior (e.g., ReconcileBranchDivergence/ResetProposedBranchHistory) to avoid misleading future maintainers.

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-02-28T00:30:02Z

internal/controller/changetransferpolicy_controller.go

+		if err != nil {
+			return fmt.Errorf("failed to merge diverged branches %q and %q with 'ours' strategy: %w", ctp.Spec.ProposedBranch, ctp.Spec.ActiveBranch, err)
+		}
+
+		r.Recorder.Eventf(ctp, nil, "Normal", constants.ResolvedDivergenceReason, "ResolvingDivergence", constants.ResolvedDivergenceMessage, ctp.Spec.ProposedBranch, ctp.Spec.ActiveBranch)
+		return nil
+	}


After calling MergeWithOursStrategy to rewrite/force-push the proposed branch, the reconciler continues using the pre-merge ctp.Status.Proposed.Hydrated.Sha (computed earlier) when creating/updating the PullRequest (mergeSha and commit trailers). This can leave the PR referencing a SHA that is no longer the branch HEAD. Consider re-fetching the proposed branch and updating ctp.Status.Proposed (or returning the new SHA from MergeWithOursStrategy) before proceeding to PR creation.

@copilot open a new pull request to apply changes based on this feedback

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Nolan Emirot <emirot.nolan@gmail.com>

emirot · 2026-02-28T00:46:19Z

@copilot implement the changes based on the comments in this thread in that branch

zachaller and others added 6 commits February 19, 2026 12:44

feat: support squash merge

5f9d20b

Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>

undo config

9c23696

Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>

undo config

e009e5b

Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>

undo config

d402d2d

Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>

fix: commit squash

a5f3f63

Signed-off-by: emirot <emirot.nolan@gmail.com>

Merge branch 'main' into nolantestsquash

6ee4cb2

emirot marked this pull request as draft February 27, 2026 23:19

fix: commit squash

cadb071

Signed-off-by: emirot <emirot.nolan@gmail.com>

emirot requested a review from Copilot February 28, 2026 00:23

fix: squash merge

9aa0de7

Signed-off-by: emirot <emirot.nolan@gmail.com>

Copilot started reviewing on behalf of emirot February 28, 2026 00:23 View session

Copilot AI reviewed Feb 28, 2026

View reviewed changes

Update internal/types/constants/events.go

5496e03

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Nolan Emirot <emirot.nolan@gmail.com>

Merge branch 'main' into nolantestsquash

7764a13

Conversation

emirot commented Feb 27, 2026

Uh oh!

codecov-commenter commented Feb 27, 2026

Bundle Report

Uh oh!

codecov-commenter commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

emirot Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

emirot Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

emirot Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

emirot Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

emirot Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

emirot commented Feb 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Feb 27, 2026 •

edited

Loading