diff --git a/.coverage-baseline b/.coverage-baseline index 7a60a910..9336dfa7 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -79.0 +79.1 diff --git a/api/v1alpha3/gittarget_types.go b/api/v1alpha3/gittarget_types.go index bfa6c811..3b12f0d6 100644 --- a/api/v1alpha3/gittarget_types.go +++ b/api/v1alpha3/gittarget_types.go @@ -49,6 +49,23 @@ type GitTargetSpec struct { // +kubebuilder:validation:MinLength=1 Branch string `json:"branch"` + // Why "parent" and not base/source/revision: docs/definitions.md reserves those words, and the + // field is flat because it is one fact. It is mutable because it only says where a branch + // starts; changing it abandons nothing already written, unlike branch and path. It can never + // hold a commit or tag: a pinned starting point would be a sibling field. + + // ParentBranch is the branch the write branch (spec.branch) is created from, and the branch the + // target compares against while the write branch does not exist on the remote. Omitted: the + // branch the remote's HEAD points at (its default branch), or a branch with no history when + // the repository is empty. When set, it must exist on the remote for the write branch to be + // created; otherwise the GitTarget reports ParentBranchNotFound and writes nothing. Once the + // write branch exists, it is used as it is and the parent is not followed. It is only read, so + // it does not need to be in the GitProvider's allowedBranches. All GitTargets on one GitProvider + // and branch must name the same parent branch (omitted counts as its own value). + // +optional + // +kubebuilder:validation:MinLength=1 + ParentBranch string `json:"parentBranch,omitempty"` + // Path within the repository to write resources to, relative to the repository // root. Required and must be non-empty — there is no default, so a GitTarget can // never silently write to the repository root. To deliberately target the diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index dfccbdf4..38729338 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -348,6 +348,18 @@ spec: - Ignore - PushEmptyCommit type: string + parentBranch: + description: |- + ParentBranch is the branch the write branch (spec.branch) is created from, and the branch the + target compares against while the write branch does not exist on the remote. Omitted: the + branch the remote's HEAD points at (its default branch), or a branch with no history when + the repository is empty. When set, it must exist on the remote for the write branch to be + created; otherwise the GitTarget reports ParentBranchNotFound and writes nothing. Once the + write branch exists, it is used as it is and the parent is not followed. It is only read, so + it does not need to be in the GitProvider's allowedBranches. All GitTargets on one GitProvider + and branch must name the same parent branch (omitted counts as its own value). + minLength: 1 + type: string path: description: |- Path within the repository to write resources to, relative to the repository diff --git a/docs/architecture.md b/docs/architecture.md index cb125f10..e7a8fb8f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -359,6 +359,10 @@ Key fields: - `spec.gitProviderRef`: a `GitProvider` in the same namespace, by name. - `spec.clusterProviderRef`: a cluster-scoped source `ClusterProvider`; it defaults to `{name: default}`. - `spec.branch`: immutable branch, validated against `GitProvider.spec.allowedBranches`. +- `spec.parentBranch`: optional, mutable branch the write branch is created from and compared + against while it does not exist. Omitted means the remote's default branch. An explicit parent + must exist when creating the write branch. It does not need to be in `allowedBranches` because + it is only read. - `spec.path`: immutable, required path under the repo (`MinLength=1`; `.` means repo root and must be chosen explicitly). - `spec.encryption`: optional SOPS/age encryption settings for sensitive resources. @@ -372,7 +376,8 @@ resolves the window per open window, since a window is bound to exactly one targ `gitProviderRef`, `clusterProviderRef`, `branch`, and `path` are immutable so a target cannot silently orphan an old materialization or change its source cluster. The controller also rejects path overlaps -between GitTargets sharing a provider and branch. +between GitTargets sharing a provider and branch, and rejects two GitTargets on one branch that name +different parent branches. Status has a kstatus-compatible summary layer plus domain conditions: @@ -1240,9 +1245,10 @@ A branch whose remote has been **deleted** takes the same path. The advertisemen branch at all, which is reported as a moved remote with a zero hash, so the replay re-roots on the remote's default branch and the retry re-creates the branch with the retained writes on top. -**A branch that does not exist yet** is created from the remote's default branch (its parent) only -when a write has something to commit, so an idle or in-sync target never creates it. Until then the -worker compares the folder against the parent. Periodic refresh, ten minutes by default, follows the +**A branch that does not exist yet** is created from its parent (`spec.parentBranch`, or the +remote's default branch when that is omitted) only when a write has something to commit, so an +idle or in-sync target never creates it. Until then the worker compares the folder against the +parent. Periodic refresh, ten minutes by default, follows the parent from the same advertisement and updates the idle view independently of the publication check. The push records the parent and the commit it started from as its root, and treats a moved parent like a moved branch: fetch, reset, replay, check again. A branch somebody else created in the diff --git a/docs/configuration.md b/docs/configuration.md index 31a71c4c..6a43ff29 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -79,6 +79,7 @@ The destination fields are immutable: to move a target, delete it and create a n |---|---|---| | `gitProviderRef` | **required** | The `GitProvider` backing this target, in the same namespace | | `branch` | **required** | Branch to write. Must be in the provider's `allowedBranches` | +| `parentBranch` | the remote's default branch | Branch a `branch` the remote does not have yet is created from. Mutable, and only read, so it need not be allowed. See [starting the write branch from another branch](#starting-the-write-branch-from-another-branch-specparentbranch) | | `path` | **required** | Folder within the repository. `.` targets the root; empty is rejected | | `clusterProviderRef` | `{"name":"default"}` | Source cluster to mirror from. The default names a `ClusterProvider` called `default` | | `commit.window.idleTimeout` | `5s` | Close a commit window after this much silence. `0s` commits every write on its own. See [the commit window](#the-commit-window-speccommitwindow) | @@ -551,6 +552,8 @@ The important fields are: - `spec.clusterProviderRef`: which `ClusterProvider` supplies resources; omit it to reference the user-created `default` provider - `spec.branch`: which allowed branch to write to +- `spec.parentBranch`: which branch `spec.branch` is created from while it does not exist yet; omit it + for the remote's default branch - `spec.path`: required relative path inside the repository; use `.` only when you deliberately want the repository root - `spec.encryption`: how `Secret` resources should be encrypted before commit @@ -747,6 +750,34 @@ two are always separate objects, and deleting the preview cannot disturb the rea For inspecting a repository without a cluster at all, use the `manifest-analyzer` CLI. +### Starting the write branch from another branch (`spec.parentBranch`) + +A target whose `spec.branch` does not exist on the remote waits on standby: it compares the cluster +against the **parent branch** and creates nothing while they agree. The first write that has +something to commit creates the branch from the parent's current tip, checked when it pushes, so a +target that waited a week does not publish onto a week-old checkout. A write that finds nothing to +change creates no branch at all. + +The parent is the remote's default branch unless you name one: + +```yaml +spec: + gitProviderRef: {name: homelab} + branch: reverser/prod-edits # the write branch: created on the first commit + parentBranch: env/prod # what it is created from + path: apps/checkout +``` + +- An explicit parent must exist when the write branch is created. Otherwise the target reports + `Ready=False` with reason `ParentBranchNotFound`, writes nothing, and recovers once the branch is + pushed. An omitted parent never fails this way, and in an empty repository the first commit + starts a branch with no history. +- Once the write branch exists, it is used as it is: new commits on the parent do not reach it. + Delete the write branch after its changes are merged, and the next edit starts a fresh one. +- `parentBranch` is mutable, and it is only read, so it does not need to be in `allowedBranches`. +- All `GitTarget` objects on one `GitProvider` and branch must name the same parent branch; omitted + counts as its own value. The later-created one reports `TargetConflict`. + `spec.suspend` is **not** this. It is the next section. ### Stopping a target from writing (`spec.suspend`) diff --git a/docs/definitions.md b/docs/definitions.md index 0196be68..02dd6726 100644 --- a/docs/definitions.md +++ b/docs/definitions.md @@ -124,6 +124,15 @@ Observation failure supplies no evidence of absence. The code calls this write-s branch)` tuple. Every write to that branch goes through it. Two words, lowercase in prose, `BranchWorker` as the Go type. +**Write branch.** The branch a `GitTarget` writes to: `spec.branch`. It must be in the +`GitProvider`'s `allowedBranches`. + +**Parent branch.** The branch a write branch is created from, and compared against while the write +branch does not exist on the remote: `spec.parentBranch`, or the remote's default branch when that +is omitted. Always with its noun: "parent" alone already names a parent resource. Not "base", which +names a kustomize read-only base and base trust, and not "source", which names the source cluster. +Once the write branch exists, the parent is not followed. + **Commit window.** The interval a branch worker batches one author's writes to one target over before it commits. Only a write opens one; nothing else does. It closes on its `idleTimeout` of silence or its `maxDuration` after opening, whichever comes first. Windows do not merge across authors, which diff --git a/hack/crdfields/fields.yaml b/hack/crdfields/fields.yaml index 67ed1952..15c90547 100644 --- a/hack/crdfields/fields.yaml +++ b/hack/crdfields/fields.yaml @@ -88,6 +88,12 @@ kinds: doc: The `GitProvider` backing this target, in the same namespace - path: branch doc: Branch to write. Must be in the provider's `allowedBranches` + - path: parentBranch + default: the remote's default branch + doc: >- + Branch a `branch` the remote does not have yet is created from. Mutable, and only read, so + it need not be allowed. See + [starting the write branch from another branch](#starting-the-write-branch-from-another-branch-specparentbranch) - path: path doc: Folder within the repository. `.` targets the root; empty is rejected - path: clusterProviderRef diff --git a/internal/controller/condition_helper.go b/internal/controller/condition_helper.go index e523eb31..a6a288de 100644 --- a/internal/controller/condition_helper.go +++ b/internal/controller/condition_helper.go @@ -44,3 +44,9 @@ func conditionIsTrue(conditions []metav1.Condition, conditionType string) bool { func conditionIsFalse(conditions []metav1.Condition, conditionType string) bool { return apimeta.IsStatusConditionFalse(conditions, conditionType) } + +// readyReasonIs reports whether the Ready condition carries the given reason. +func readyReasonIs(conditions []metav1.Condition, reason string) bool { + ready := apimeta.FindStatusCondition(conditions, ConditionTypeReady) + return ready != nil && ready.Reason == reason +} diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 945023cb..0b0760a8 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -53,9 +53,12 @@ const ( const ( // GitTargetReasonOK is the healthy reason. It is the shared Succeeded vocabulary rather than // a per-kind spelling; the name is kept for call-site stability. - GitTargetReasonOK = ReasonSucceeded - GitTargetReasonProviderNotFound = "ProviderNotFound" - GitTargetReasonBranchNotAllowed = "BranchNotAllowed" + GitTargetReasonOK = ReasonSucceeded + GitTargetReasonProviderNotFound = "ProviderNotFound" + GitTargetReasonBranchNotAllowed = "BranchNotAllowed" + // GitTargetReasonParentBranchNotFound is the Ready reason for a write branch that cannot be + // created: the remote carries neither it nor the spec.parentBranch it would start from. + GitTargetReasonParentBranchNotFound = "ParentBranchNotFound" GitTargetReasonTargetConflict = "TargetConflict" GitTargetReasonNotChecked = "NotChecked" GitTargetReasonBlocked = "Blocked" @@ -181,6 +184,9 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( st := beginStatus(r.Client, r.Recorder, &target) gitPathWasRefused := conditionIsFalse(target.Status.Conditions, GitTargetConditionGitPathAccepted) + // Writes are dropped while the parent is missing, so its return is recovered the way a refused + // folder is: by a recheck that re-derives what the cluster holds. + parentWasMissing := readyReasonIs(target.Status.Conditions, GitTargetReasonParentBranchNotFound) providerNS := target.Namespace // One read of the GitProvider for everything below it; see getGitProvider. @@ -266,7 +272,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // A standing reconcile request forces the same re-check a refused Git path does: the watch // plane re-anchors the target's streams, which is what makes it re-read the folder rather than // wait for the periodic pass. Taken once per distinct annotation value. - forceRecheck := gitPathWasRefused || r.reconcileRequests.take( + forceRecheck := gitPathWasRefused || parentWasMissing || r.reconcileRequests.take( types.NewResourceReference(target.Name, target.Namespace), reconcileRequestedAt(&target)) observed := r.observeDataPlane(&target, sourceProvider, forceRecheck, log) st.setValue(GitTargetConditionStreamsRunning, observed.axes.Streams) @@ -277,7 +283,8 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( rd := newGitTargetReadiness() convergeAsSuspended(rd, &target) - gitTargetReadinessGates(rd, observed, refs.gitProvider, refs.clusterProvider, refs.sourceCluster) + gitTargetReadinessGates(rd, observed, r.parentBranchReadiness(&target, providerNS, repoIdentityOf(gitProvider)), + refs.gitProvider, refs.clusterProvider, refs.sourceCluster) st.applyReadiness(rd) if err := st.commit(ctx); err != nil { @@ -762,12 +769,14 @@ func (r *GitTargetReconciler) publishReferenceReadiness( func gitTargetReadinessGates( rd *readiness, observed dataPlaneObservation, - provider, clusterProvider, sourceReach conditionValue, + parent, provider, clusterProvider, sourceReach conditionValue, ) { // Terminal, most specific first. Each of these needs a human: the folder holds content the - // operator will not manage, a watch is refused, or what was written no longer matches live. + // operator will not manage, the write branch has no parent to be created from, a watch is + // refused, or what was written no longer matches live. rd.stalledIf(observed.axes.GitPath.Status == metav1.ConditionFalse, observed.axes.GitPath.Reason, observed.axes.GitPath.Message) + rd.stalledIf(parent.Status == metav1.ConditionFalse, parent.Reason, parent.Message) rd.stalledIf(observed.streams.Blocked > 0, observed.streams.Reason, observed.streams.Message) rd.stalledIf(observed.axes.Render.Status == metav1.ConditionFalse, observed.axes.Render.Reason, observed.axes.Render.Message) @@ -803,6 +812,37 @@ func gitTargetReadinessGates( "Stream declaration has not landed yet; the data-plane surface is not observable") } +// parentBranchReadiness reports whether the write branch can be created: False when the last +// look at the remote found neither the write branch nor the configured spec.parentBranch. +// +// It is read from the branch worker's remote observation, so it costs no round trip and holds +// with periodic refresh disabled: every fetch on the write path records the same observation. +// An omitted parent is never missing; it falls back to the remote's default branch. An +// observation proved against another repository than the one the GitProvider names now (a +// repoint) says nothing about this one. +func (r *GitTargetReconciler) parentBranchReadiness( + target *configbutleraiv1alpha3.GitTarget, + providerNS string, + repo git.RepoIdentity, +) conditionValue { + ok := conditionValue{Status: metav1.ConditionTrue, Reason: GitTargetReasonOK} + if target.Spec.ParentBranch == "" { + return ok + } + observed, seen := r.observeRemote(target, providerNS) + if !seen || observed.MissingParent != target.Spec.ParentBranch || differentRepository(observed.Repo, repo) { + return ok + } + return conditionValue{ + Status: metav1.ConditionFalse, + Reason: GitTargetReasonParentBranchNotFound, + Message: fmt.Sprintf( + "Branch '%s' does not exist on the remote, and neither does its parent branch '%s' to create "+ + "it from; nothing is written until one of them exists", + target.Spec.Branch, target.Spec.ParentBranch), + } +} + // requestRemoteRefresh asks this target's branch worker to re-prove where the branch is, if // nothing has proved it within the interval. // @@ -902,6 +942,7 @@ func (r *GitTargetReconciler) ensureEventStream( providerNS, target.Spec.Branch, repo, + target.Spec.ParentBranch, ) if err != nil { return nil, fmt.Errorf( @@ -1089,13 +1130,6 @@ func (r *GitTargetReconciler) checkForConflicts( target *configbutleraiv1alpha3.GitTarget, providerNS string, ) (bool, string, string, error) { - // A path the writer would reject (absolute, backslashes, ".." traversal) owns - // nothing, so it must neither block others nor be blocked here — its own write - // path fails it. Only well-formed paths participate in overlap detection. - if !git.IsValidTargetPath(target.Spec.Path) { - return false, "", "", nil - } - var allTargets configbutleraiv1alpha3.GitTargetList if err := r.List(ctx, &allTargets); err != nil { return false, "", "", fmt.Errorf("list GitTargets for conflict validation: %w", err) @@ -1109,41 +1143,18 @@ func (r *GitTargetReconciler) checkForConflicts( if existing.Namespace != providerNS || existing.Spec.GitProviderRef.Name != target.Spec.GitProviderRef.Name { continue } - if existing.Spec.Branch != target.Spec.Branch || - !git.IsValidTargetPath(existing.Spec.Path) || - !gitTargetPathsOverlap(target.Spec.Path, existing.Spec.Path) { + if existing.Spec.Branch != target.Spec.Branch { continue } - // Two GitTargets on the same provider+branch whose paths are equal or - // nested fight over which documents each one owns. The later-created - // target loses (ties broken deterministically by identity) so every - // materialized folder keeps exactly one owner. - if gitTargetLosesConflict(target, existing) { - var msg string - if normalizeGitTargetPath(target.Spec.Path) == normalizeGitTargetPath(existing.Spec.Path) { - msg = fmt.Sprintf( - "Conflict detected. Another GitTarget '%s/%s' (created at %s) is already using GitProvider '%s/%s', branch '%s', path '%s'. This GitTarget was created later and will not be processed.", - existing.Namespace, - existing.Name, - existing.CreationTimestamp.Format(time.RFC3339), - providerNS, - target.Spec.GitProviderRef.Name, - target.Spec.Branch, - target.Spec.Path, - ) - } else { - msg = fmt.Sprintf( - "Conflict detected. This GitTarget's path '%s' overlaps the path '%s' of GitTarget '%s/%s' (created at %s) on GitProvider '%s/%s', branch '%s' — one path nests inside the other (sibling paths are allowed). This GitTarget was created later and will not be processed.", - target.Spec.Path, - existing.Spec.Path, - existing.Namespace, - existing.Name, - existing.CreationTimestamp.Format(time.RFC3339), - providerNS, - target.Spec.GitProviderRef.Name, - target.Spec.Branch, - ) - } + // Checked whatever either path looks like: a target with a path its writer will reject + // still wires the shared worker, and would set its parent. + if lost, msg := losesParentBranchConflict(target, existing, providerNS); lost { + return true, msg, GitTargetReasonTargetConflict, nil + } + if existing.Spec.ParentBranch != target.Spec.ParentBranch { + continue // the other target loses, on its own reconcile + } + if lost, msg := losesPathConflict(target, existing, providerNS); lost { return true, msg, GitTargetReasonTargetConflict, nil } } @@ -1151,6 +1162,77 @@ func (r *GitTargetReconciler) checkForConflicts( return false, "", "", nil } +// losesPathConflict reports whether target loses to existing, on the same write branch, because +// their paths are equal or nested. Two such targets fight over which documents each one owns. The +// later-created target loses (ties broken deterministically by identity) so every materialized +// folder keeps exactly one owner. +// +// A path the writer would reject (absolute, backslashes, ".." traversal) owns nothing, so it must +// neither block others nor be blocked here — its own write path fails it. Only well-formed paths +// participate in overlap detection. +func losesPathConflict(target, existing *configbutleraiv1alpha3.GitTarget, providerNS string) (bool, string) { + if !git.IsValidTargetPath(target.Spec.Path) || + !git.IsValidTargetPath(existing.Spec.Path) || + !gitTargetPathsOverlap(target.Spec.Path, existing.Spec.Path) || + !gitTargetLosesConflict(target, existing) { + return false, "" + } + if normalizeGitTargetPath(target.Spec.Path) == normalizeGitTargetPath(existing.Spec.Path) { + return true, fmt.Sprintf( + "Conflict detected. Another GitTarget '%s/%s' (created at %s) is already using GitProvider '%s/%s', branch '%s', path '%s'. This GitTarget was created later and will not be processed.", + existing.Namespace, + existing.Name, + existing.CreationTimestamp.Format(time.RFC3339), + providerNS, + target.Spec.GitProviderRef.Name, + target.Spec.Branch, + target.Spec.Path, + ) + } + return true, fmt.Sprintf( + "Conflict detected. This GitTarget's path '%s' overlaps the path '%s' of GitTarget '%s/%s' (created at %s) on GitProvider '%s/%s', branch '%s' — one path nests inside the other (sibling paths are allowed). This GitTarget was created later and will not be processed.", + target.Spec.Path, + existing.Spec.Path, + existing.Namespace, + existing.Name, + existing.CreationTimestamp.Format(time.RFC3339), + providerNS, + target.Spec.GitProviderRef.Name, + target.Spec.Branch, + ) +} + +// losesParentBranchConflict reports whether target loses to existing, on the same write branch, +// because the two name different parent branches. One branch worker owns one checkout per write +// branch, so every target on it has to agree on where the branch starts. Compared as written, with +// omitted as its own value: the answer needs no remote round trip and cannot flip when somebody +// changes the remote's default branch. +func losesParentBranchConflict(target, existing *configbutleraiv1alpha3.GitTarget, providerNS string) (bool, string) { + if existing.Spec.ParentBranch == target.Spec.ParentBranch || !gitTargetLosesConflict(target, existing) { + return false, "" + } + return true, parentBranchConflictMessage(target, existing, providerNS) +} + +// parentBranchConflictMessage explains a TargetConflict over spec.parentBranch. +func parentBranchConflictMessage(target, existing *configbutleraiv1alpha3.GitTarget, providerNS string) string { + return fmt.Sprintf( + "Conflict detected. GitTarget '%s/%s' (created at %s) writes GitProvider '%s/%s', branch '%s' with parent "+ + "branch %s, and this GitTarget names parent branch %s. All GitTargets on one branch must name the same "+ + "parent branch. This GitTarget was created later and will not be processed.", + existing.Namespace, existing.Name, existing.CreationTimestamp.Format(time.RFC3339), + providerNS, target.Spec.GitProviderRef.Name, target.Spec.Branch, + describeParentBranch(existing.Spec.ParentBranch), describeParentBranch(target.Spec.ParentBranch), + ) +} + +func describeParentBranch(name string) string { + if name == "" { + return "(omitted: the remote's default branch)" + } + return "'" + name + "'" +} + func (r *GitTargetReconciler) ensureEncryptionSecret( ctx context.Context, target *configbutleraiv1alpha3.GitTarget, diff --git a/internal/controller/gittarget_immutability_test.go b/internal/controller/gittarget_immutability_test.go index d73d15f1..2ab6f899 100644 --- a/internal/controller/gittarget_immutability_test.go +++ b/internal/controller/gittarget_immutability_test.go @@ -10,6 +10,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" @@ -75,6 +76,49 @@ var _ = Describe("GitTarget Destination Immutability", func() { }, "spec.gitProviderRef is immutable") }) + // spec.parentBranch is NOT part of the destination: it says where a write branch starts, so it + // can change on a live target. It can be omitted, but not set to the empty string. + It("allows spec.parentBranch to be set, changed and cleared, but never empty", func() { + ctx := context.Background() + key := types.NamespacedName{Name: "parent-branch-target", Namespace: "default"} + + gitTarget := &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: key.Name, Namespace: key.Namespace}, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + GitProviderRef: meta.LocalObjectReference{Name: "prov-a"}, + Branch: "edits", + Path: "apps", + ParentBranch: "main", + }, + } + Expect(k8sClient.Create(ctx, gitTarget)).Should(Succeed()) + DeferCleanup(func() { _ = k8sClient.Delete(ctx, gitTarget) }) + + for _, parent := range []string{"release", ""} { + Eventually(func(g Gomega) { + current := &configbutleraiv1alpha3.GitTarget{} + g.Expect(k8sClient.Get(ctx, key, current)).To(Succeed()) + current.Spec.ParentBranch = parent + g.Expect(k8sClient.Update(ctx, current)).To(Succeed()) + }, timeout, interval).Should(Succeed()) + } + + empty := &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "configbutler.ai/v1alpha3", + "kind": "GitTarget", + "metadata": map[string]interface{}{"name": "empty-parent-target", "namespace": "default"}, + "spec": map[string]interface{}{ + "gitProviderRef": map[string]interface{}{"name": "prov-a"}, + "branch": "edits", + "path": "apps", + "parentBranch": "", + }, + }} + err := k8sClient.Create(ctx, empty) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("spec.parentBranch")) + }) + It("requires a non-empty path: rejects an omitted or empty path but allows an explicit \".\" root", func() { ctx := context.Background() key := types.NamespacedName{Name: "root-policy-target", Namespace: "default"} diff --git a/internal/controller/gittarget_layout_test.go b/internal/controller/gittarget_layout_test.go index 391dde11..bab5a610 100644 --- a/internal/controller/gittarget_layout_test.go +++ b/internal/controller/gittarget_layout_test.go @@ -206,7 +206,7 @@ func TestGitTargetReadiness_StalledFollowsGitPathAccepted(t *testing.T) { }, Render: conditionValue{Status: metav1.ConditionTrue}, }, - }, healthyDependency(), healthyDependency(), healthyDependency()) + }, healthyDependency(), healthyDependency(), healthyDependency(), healthyDependency()) trio := rd.trio() assert.Equal(t, wantStalled.Status, string(trio.Stalled.Status)) diff --git a/internal/controller/gittarget_parent_branch_test.go b/internal/controller/gittarget_parent_branch_test.go new file mode 100644 index 00000000..85fa854a --- /dev/null +++ b/internal/controller/gittarget_parent_branch_test.go @@ -0,0 +1,226 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "testing" + "time" + + meta "github.com/fluxcd/pkg/apis/meta" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/types" + "github.com/ConfigButler/gitops-reverser/internal/watch" +) + +func parentBranchTarget(name, branch, path, parent string, created time.Time) *configbutleraiv1alpha3.GitTarget { + return &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, Namespace: "shop", CreationTimestamp: metav1.NewTime(created), + }, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + GitProviderRef: meta.LocalObjectReference{Name: "repo1"}, + Branch: branch, + Path: path, + ParentBranch: parent, + }, + } +} + +func parentBranchScheme(t *testing.T) *runtime.Scheme { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configbutleraiv1alpha3.AddToScheme(scheme)) + return scheme +} + +// TestCheckForConflicts_ParentBranch: one write branch, one checkout, one parent. Two targets on the +// same branch that name different parents conflict even on sibling paths, and the later one loses. +// Omitted is its own value, so it conflicts with an explicit "main". +func TestCheckForConflicts_ParentBranch(t *testing.T) { + earlier := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + later := earlier.Add(time.Hour) + + cases := []struct { + name string + existing *configbutleraiv1alpha3.GitTarget + target *configbutleraiv1alpha3.GitTarget + wantConflict bool + }{ + { + name: "omitted versus main conflicts", + existing: parentBranchTarget("first", "edits", "apps/a", "", earlier), + target: parentBranchTarget("second", "edits", "apps/b", "main", later), + wantConflict: true, + }, + { + name: "the earlier target wins", + existing: parentBranchTarget("second", "edits", "apps/b", "main", later), + target: parentBranchTarget("first", "edits", "apps/a", "", earlier), + }, + { + name: "the same parent on sibling paths is fine", + existing: parentBranchTarget("first", "edits", "apps/a", "release", earlier), + target: parentBranchTarget("second", "edits", "apps/b", "release", later), + }, + { + name: "another write branch is another checkout", + existing: parentBranchTarget("first", "edits", "apps/a", "", earlier), + target: parentBranchTarget("second", "other", "apps/b", "release", later), + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + k8sClient := fake.NewClientBuilder().WithScheme(parentBranchScheme(t)).WithObjects(tc.existing).Build() + r := &GitTargetReconciler{Client: k8sClient} + + conflict, msg, reason, err := r.checkForConflicts(context.Background(), tc.target, "shop") + + require.NoError(t, err) + assert.Equal(t, tc.wantConflict, conflict, msg) + if tc.wantConflict { + assert.Equal(t, GitTargetReasonTargetConflict, reason) + assert.Contains(t, msg, "(omitted: the remote's default branch)") + assert.Contains(t, msg, "'main'") + } + }) + } +} + +func startParentBranchWorkers(t *testing.T) (*GitTargetReconciler, *git.WorkerManager) { + t.Helper() + k8sClient := fake.NewClientBuilder().WithScheme(parentBranchScheme(t)).Build() + workers := git.NewWorkerManager(k8sClient, logr.Discard(), git.BranchWorkerLimits{}, + types.SensitiveResourcePolicy{}) + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + go func() { _ = workers.Start(ctx) }() + time.Sleep(100 * time.Millisecond) // Start installs the context EnsureWorker starts workers under + router := watch.NewEventRouter(workers, nil, k8sClient, logr.Discard()) + return &GitTargetReconciler{Client: k8sClient, WorkerManager: workers, EventRouter: router}, workers +} + +// TestEnsureEventStream_AParentBranchChangeKeepsTheWorker: changing spec.parentBranch abandons +// nothing written, so it is not a repoint. The worker stays and re-reads the remote. +func TestEnsureEventStream_AParentBranchChangeKeepsTheWorker(t *testing.T) { + r, workers := startParentBranchWorkers(t) + repo := git.RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/repo.git"} + target := parentBranchTarget("apps", "edits", "apps", "", time.Now()) + + _, err := r.ensureEventStream(context.Background(), target, "shop", repo, logr.Discard()) + require.NoError(t, err) + before, ok := workers.GetWorkerForTarget("repo1", "shop", "edits") + require.True(t, ok) + before.SeedRemoteObservationForTest("", time.Now(), "Fetch") + + target.Spec.ParentBranch = "release" + _, err = r.ensureEventStream(context.Background(), target, "shop", repo, logr.Discard()) + require.NoError(t, err) + + after, ok := workers.GetWorkerForTarget("repo1", "shop", "edits") + require.True(t, ok) + assert.Same(t, before, after, "the worker and its clone are kept") + assert.Equal(t, "release", after.ParentBranch()) + assert.False(t, after.BaseTrustedForTest(), "and its checkout of the old parent is no longer trusted") +} + +// TestParentBranchReadiness: a fetch that found neither the write branch nor its configured parent +// stalls the GitTarget as ParentBranchNotFound. An omitted parent never does. +func TestParentBranchReadiness(t *testing.T) { + r, workers := startParentBranchWorkers(t) + repo := git.RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/repo.git"} + target := parentBranchTarget("apps", "edits", "apps", "release", time.Now()) + + assert.Equal(t, metav1.ConditionTrue, r.parentBranchReadiness(target, "shop", repo).Status, + "nothing has looked yet") + + _, err := r.ensureEventStream(context.Background(), target, "shop", repo, logr.Discard()) + require.NoError(t, err) + worker, _ := workers.GetWorkerForTarget("repo1", "shop", "edits") + worker.RecordMissingParentForTest("release") + + got := r.parentBranchReadiness(target, "shop", repo) + assert.Equal(t, metav1.ConditionFalse, got.Status) + assert.Equal(t, GitTargetReasonParentBranchNotFound, got.Reason) + assert.Contains(t, got.Message, "'release'") + + rd := newGitTargetReadiness() + gitTargetReadinessGates(rd, dataPlaneObservation{axes: gitTargetAxes{ + Streams: conditionValue{Status: metav1.ConditionTrue}, + GitPath: conditionValue{Status: metav1.ConditionTrue}, + Render: conditionValue{Status: metav1.ConditionTrue}, + }}, got, healthyDependency(), healthyDependency(), healthyDependency()) + trio := rd.trio() + assert.Equal(t, metav1.ConditionTrue, trio.Stalled.Status, "it needs somebody to create the branch") + assert.Equal(t, GitTargetReasonParentBranchNotFound, trio.Ready.Reason) + + target.Spec.ParentBranch = "other" + assert.Equal(t, metav1.ConditionTrue, r.parentBranchReadiness(target, "shop", repo).Status, + "the observation is about another parent") + target.Spec.ParentBranch = "" + assert.Equal(t, metav1.ConditionTrue, r.parentBranchReadiness(target, "shop", repo).Status) +} + +func TestReadyReasonIs(t *testing.T) { + conditions := []metav1.Condition{{Type: ConditionTypeReady, Status: metav1.ConditionFalse, + Reason: GitTargetReasonParentBranchNotFound}} + assert.True(t, readyReasonIs(conditions, GitTargetReasonParentBranchNotFound)) + assert.False(t, readyReasonIs(conditions, GitTargetReasonTargetConflict)) + assert.False(t, readyReasonIs(nil, GitTargetReasonParentBranchNotFound)) +} + +// TestCheckForConflicts_ParentBranchAppliesToInvalidPaths: a target whose path its writer will +// reject still wires the shared worker, so it must not escape the parent check in either role and +// repoint the healthy sibling's parent. +func TestCheckForConflicts_ParentBranchAppliesToInvalidPaths(t *testing.T) { + earlier := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + healthy := parentBranchTarget("first", "edits", "apps/a", "main", earlier) + invalid := parentBranchTarget("second", "edits", "../bad", "release", earlier.Add(time.Hour)) + + r, workers := startParentBranchWorkers(t) + require.NoError(t, r.Client.Create(context.Background(), healthy)) + _, err := r.ensureEventStream(context.Background(), healthy, "shop", firstRepo, logr.Discard()) + require.NoError(t, err) + + conflict, _, reason, err := r.checkForConflicts(context.Background(), invalid, "shop") + require.NoError(t, err) + assert.True(t, conflict, "an invalid path must not bypass the parent conflict") + assert.Equal(t, GitTargetReasonTargetConflict, reason) + worker, _ := workers.GetWorkerForTarget("repo1", "shop", "edits") + assert.Equal(t, "main", worker.ParentBranch()) + + // And as the earlier object, the invalid target still wins the parent. + k8sClient := fake.NewClientBuilder().WithScheme(parentBranchScheme(t)). + WithObjects(parentBranchTarget("older", "edits", "../bad", "release", earlier)).Build() + conflict, _, _, err = (&GitTargetReconciler{Client: k8sClient}).checkForConflicts( + context.Background(), parentBranchTarget("newer", "edits", "apps/a", "main", earlier.Add(time.Hour)), "shop") + require.NoError(t, err) + assert.True(t, conflict) +} + +// TestParentBranchReadiness_IgnoresAnotherRepository: the manager keeps a branch's observation across +// a repoint, and a missing parent in the old repository says nothing about the new one. +func TestParentBranchReadiness_IgnoresAnotherRepository(t *testing.T) { + r, workers := startParentBranchWorkers(t) + target := parentBranchTarget("apps", "edits", "apps", "release", time.Now()) + _, err := r.ensureEventStream(context.Background(), target, "shop", firstRepo, logr.Discard()) + require.NoError(t, err) + worker, _ := workers.GetWorkerForTarget("repo1", "shop", "edits") + worker.RecordMissingParentForTest("release") + require.Equal(t, metav1.ConditionFalse, r.parentBranchReadiness(target, "shop", firstRepo).Status) + + _, err = r.ensureEventStream(context.Background(), target, "shop", secondRepo, logr.Discard()) + require.NoError(t, err) + assert.Equal(t, metav1.ConditionTrue, r.parentBranchReadiness(target, "shop", secondRepo).Status, + "the old repository's missing parent is not evidence about the replacement") +} diff --git a/internal/controller/gittarget_status_test.go b/internal/controller/gittarget_status_test.go index 714abe0c..e5a1d656 100644 --- a/internal/controller/gittarget_status_test.go +++ b/internal/controller/gittarget_status_test.go @@ -164,7 +164,7 @@ func TestGitTargetReadinessGates(t *testing.T) { st.setValue(GitTargetConditionRenderMatchesLive, observed.axes.Render) rd := newGitTargetReadiness() - gitTargetReadinessGates(rd, observed, tt.provider, tt.clusterProvider, tt.sourceReach) + gitTargetReadinessGates(rd, observed, conditionValue{}, tt.provider, tt.clusterProvider, tt.sourceReach) st.applyReadiness(rd) ready := findCondition(target.Status.Conditions, GitTargetConditionReady) diff --git a/internal/controller/gittarget_worker_cleanup_test.go b/internal/controller/gittarget_worker_cleanup_test.go index a354e6b8..a79fd7ec 100644 --- a/internal/controller/gittarget_worker_cleanup_test.go +++ b/internal/controller/gittarget_worker_cleanup_test.go @@ -45,7 +45,7 @@ func TestCleanupDeletedGitTarget_StopsAWorkerNothingNeedsAnyMore(t *testing.T) { time.Sleep(100 * time.Millisecond) func() { - err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}) + err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}, "") require.NoError(t, err) }() _, exists := workers.GetWorkerForTarget("repo1", "shop", "main") @@ -81,7 +81,7 @@ func TestCleanupDeletedGitTarget_LeavesAWorkerAnotherGitTargetStillUses(t *testi go func() { _ = workers.Start(ctx) }() time.Sleep(100 * time.Millisecond) func() { - err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}) + err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}, "") require.NoError(t, err) }() @@ -125,7 +125,7 @@ func TestCleanupDeletedGitTarget_RetriesWhenTheSweepCannotRead(t *testing.T) { go func() { _ = workers.Start(ctx) }() time.Sleep(100 * time.Millisecond) func() { - err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}) + err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}, "") require.NoError(t, err) }() @@ -158,7 +158,7 @@ func TestHandleFetchError_RequeuesAFailedCleanup(t *testing.T) { go func() { _ = workers.Start(ctx) }() time.Sleep(100 * time.Millisecond) func() { - err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}) + err := workers.EnsureWorker(ctx, "repo1", "shop", "main", git.RepoIdentity{URL: "file:///repo1.git"}, "") require.NoError(t, err) }() diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 3c3b85a7..eb313e89 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -56,7 +56,7 @@ var ( //nolint:gochecknoglobals fetchRemoteBranchHashFn = fetchRemoteBranchHash //nolint:gochecknoglobals - syncToRemoteFn = syncToRemote + syncToRemoteFn = syncToRemoteFrom ) // BranchWorker processes events for a single (GitProvider, Branch) combination. @@ -238,6 +238,15 @@ type BranchWorker struct { newBranchParent plumbing.ReferenceName newBranchParentHash plumbing.Hash + // parentBranchName is the GitTarget's spec.parentBranch: the branch a write branch the remote + // does not carry is created from. Empty means the remote's default branch. It is set by the + // reconcile and read by the event loop, hence atomic. + parentBranchName atomic.Pointer[string] + // parentChangedState is raised by SetParentBranch and consumed by the event loop: writes it + // retains were planned against the old parent, and invalidating base trust alone does not + // reach them (ensureBaseForCycle ignores trust while writes are retained). + parentChangedState atomic.Bool + // firsts surfaces the first successful commit and push at default verbosity. firsts branchWorkerLogFirsts @@ -923,7 +932,7 @@ func (w *BranchWorker) prepareBootstrapRepository( repoPath := w.repoPath() w.recordFetch(fetchReasonBootstrap) - pullReport, err := PrepareBranch(ctx, w.repo.URL, repoPath, w.Branch, auth) + pullReport, err := w.prepareBranch(ctx, repoPath, auth) if err != nil { return "", fmt.Errorf("failed to prepare repository: %w", err) } @@ -1402,6 +1411,9 @@ func (l *branchWorkerEventLoop) resetCommitTimer(delay time.Duration) { // The case only arises with retained work. With nothing retained, commitPendingWrites' own guard // resets for us, and it can do that safely because there is nothing to lose. func (l *branchWorkerEventLoop) recoverRetainedWrites() error { + // Taken first, so a change that found nothing retained does not linger: with nothing retained + // the invalidated base makes the next cycle fetch the new parent anyway. + parentChanged := l.w.parentChangedState.Swap(false) if len(l.pendingWrites) == 0 { // Nothing is retained, so there is nothing a stale replay could strand. A replay only // ever marks itself required while writes are retained, but clear it here too so the flag @@ -1410,14 +1422,15 @@ func (l *branchWorkerEventLoop) recoverRetainedWrites() error { return nil } dirty, needsReplay := l.w.worktreeDirty(), l.w.replayRequired() - if !dirty && !needsReplay { + if !dirty && !needsReplay && !parentChanged { return nil } l.w.Log.Info("Rebuilding retained writes onto the remote tip", "pendingWrites", len(l.pendingWrites), "worktreeDirty", dirty, - "replayRequired", needsReplay) + "replayRequired", needsReplay, + "parentBranchChanged", parentChanged) // fetchReasonRecovery, the same series the no-retained-writes case records in // ensureBaseForCycle: this is one event, and which half of it an operator sees must not depend // on whether a push happened to be in cooldown at the time. @@ -1858,7 +1871,7 @@ func (w *BranchWorker) ensureBaseForCycle( } w.recordFetch(reason) - pullReport, err := PrepareBranch(w.ctx, w.repo.URL, repoPath, w.Branch, auth) + pullReport, err := w.prepareBranch(w.ctx, repoPath, auth) if err != nil { return fmt.Errorf("prepare repository: %w", err) } @@ -2035,7 +2048,7 @@ func (w *BranchWorker) runPushCycle(pendingWrites []PendingWrite) error { // changed nothing. w.markReplayRequired() w.recordFetch(fetchReasonContention) - pullReport, syncErr := syncToRemoteFn(w.ctx, repo, plumbing.NewBranchReferenceName(w.Branch), auth) + pullReport, syncErr := w.syncToRemote(w.ctx, repo, auth) if syncErr != nil { w.invalidateBase("sync during replay failed") return fmt.Errorf("sync remote during replay: %w", syncErr) @@ -2185,7 +2198,7 @@ func (w *BranchWorker) refreshRemoteAndRebuildPendingWrites( // false, which is precisely the state no other flag describes. w.markReplayRequired() w.recordFetch(reason) - pullReport, err := syncToRemoteFn(ctx, repo, plumbing.NewBranchReferenceName(w.Branch), auth) + pullReport, err := w.syncToRemote(ctx, repo, auth) if err != nil { w.invalidateBase("sync before replay failed") return fmt.Errorf("sync remote before replay: %w", err) @@ -2345,6 +2358,53 @@ func (w *BranchWorker) endPushCycle(kind PushOutcomeKind) { } } +// ParentBranch is the configured parent branch, or empty for the remote's default branch. +func (w *BranchWorker) ParentBranch() string { + if name := w.parentBranchName.Load(); name != nil { + return *name + } + return "" +} + +// SetParentBranch configures the parent branch. A change invalidates the base, so the next cycle +// fetches and compares against the new parent; the worker and its clone are kept, because +// nothing already written is abandoned. It reports whether the value changed. +func (w *BranchWorker) SetParentBranch(name string) bool { + if w.ParentBranch() == name { + return false + } + w.parentBranchName.Store(&name) + w.invalidateBase("parent branch changed") + w.parentChangedState.Store(true) + return true +} + +// prepareBranch is PrepareBranch for this worker's branch and parent. A configured parent the +// remote does not carry is recorded as an observation, because it is one: the advertisement said +// so, and the GitTarget reports it as ParentBranchNotFound. +func (w *BranchWorker) prepareBranch( + ctx context.Context, repoPath string, auth []gitclient.Option, +) (*PullReport, error) { + report, err := PrepareBranchFrom(ctx, w.repo.URL, repoPath, w.Branch, w.ParentBranch(), auth) + w.noteMissingParent(err) + return report, err +} + +// syncToRemote is syncToRemoteFn for this worker's branch and parent; see prepareBranch. +func (w *BranchWorker) syncToRemote( + ctx context.Context, repo *gogit.Repository, auth []gitclient.Option, +) (*PullReport, error) { + report, err := syncToRemoteFn(ctx, repo, plumbing.NewBranchReferenceName(w.Branch), w.ParentBranch(), auth) + w.noteMissingParent(err) + return report, err +} + +func (w *BranchWorker) noteMissingParent(err error) { + if errors.Is(err, ErrParentBranchNotFound) { + w.recordMissingParent(w.ParentBranch(), ObservedByFetch) + } +} + // clearNewBranchParent forgets the parent once the remote carries the write branch. func (w *BranchWorker) clearNewBranchParent() { w.newBranchParent = "" @@ -2711,9 +2771,17 @@ func (w *BranchWorker) LastRemoteObservation() (RemoteObservation, bool) { // that died mid-upload or an advertisement that never arrived observed nothing, and recording a // guess there is how a stale revision reaches status. func (w *BranchWorker) recordRemoteObservation(revision string, by ObservationSource) { + w.publishObservation(RemoteObservation{Commit: revision, At: time.Now(), By: by, Repo: w.repo}) +} + +// recordMissingParent records that the write branch is absent and its configured parent is too. +func (w *BranchWorker) recordMissingParent(parent string, by ObservationSource) { + w.publishObservation(RemoteObservation{At: time.Now(), By: by, Repo: w.repo, MissingParent: parent}) +} + +func (w *BranchWorker) publishObservation(observed RemoteObservation) { // Stamped with the repository it is about, so that whoever publishes it can tell whether it // still describes the repository the GitTarget points at. See RemoteObservation.Repo. - observed := RemoteObservation{Commit: revision, At: time.Now(), By: by, Repo: w.repo} w.lastObservation.Store(&observed) // Delivered here, at the one point where an observation is made, rather than by each caller // against whichever GitTargets it happened to be holding. The fact is about the BRANCH, so it @@ -2751,7 +2819,7 @@ func (w *BranchWorker) syncWithRemote(ctx context.Context, reason string) error // PrepareBranch handles both initial and update cases w.recordFetch(reason) - report, err := PrepareBranch(ctx, w.repo.URL, repoPath, w.Branch, auth) + report, err := w.prepareBranch(ctx, repoPath, auth) if err != nil { return fmt.Errorf("failed to sync with remote: %w", err) } @@ -2786,7 +2854,7 @@ func (w *BranchWorker) ensureRepositoryInitialized(ctx context.Context) error { } // Use new PrepareBranch abstraction - pullReport, err := PrepareBranch(ctx, w.repo.URL, repoPath, w.Branch, auth) + pullReport, err := w.prepareBranch(ctx, repoPath, auth) if err != nil { return fmt.Errorf("failed to prepare repository: %w", err) } @@ -2875,5 +2943,11 @@ func (w *BranchWorker) SeedRemoteObservationForTest(revision string, at time.Tim w.lastObservation.Store(&observed) } +// RecordMissingParentForTest records the observation a fetch makes when neither the write branch +// nor its configured parent exists, for tests in another package. +func (w *BranchWorker) RecordMissingParentForTest(parent string) { + w.recordMissingParent(parent, ObservedByFetch) +} + // BaseTrustedForTest exposes the flag to tests in another package. func (w *BranchWorker) BaseTrustedForTest() bool { return w.baseTrusted() } diff --git a/internal/git/branch_worker_base_trust_test.go b/internal/git/branch_worker_base_trust_test.go index 437efe52..bcb9bd21 100644 --- a/internal/git/branch_worker_base_trust_test.go +++ b/internal/git/branch_worker_base_trust_test.go @@ -188,7 +188,7 @@ func TestBaseTrust_LostWhenSomebodySaysTheRemoteMoved(t *testing.T) { syncFailed := errors.New("sync failed") original := syncToRemoteFn syncToRemoteFn = func( - _ context.Context, _ *gogit.Repository, _ plumbing.ReferenceName, _ []gitclient.Option, + _ context.Context, _ *gogit.Repository, _ plumbing.ReferenceName, _ string, _ []gitclient.Option, ) (*PullReport, error) { return nil, syncFailed } diff --git a/internal/git/branch_worker_new_branch_test.go b/internal/git/branch_worker_new_branch_test.go index c399ff5f..c40357a5 100644 --- a/internal/git/branch_worker_new_branch_test.go +++ b/internal/git/branch_worker_new_branch_test.go @@ -37,8 +37,8 @@ import ( ) const ( - newBranchTarget = "target-a" - newBranchFolder = "live" + newBranchTarget = "standby-target" + newBranchFolder = "standby" ) // newBranchFixture is a remote whose main is seeded and whose write branch `feature` is absent, @@ -88,7 +88,8 @@ func newNewBranchFixture(t *testing.T, seedFiles map[string]string) *newBranchFi f.worker.ctx = ctx f.worker.mapper = configMapMapper() t.Cleanup(func() { _ = os.RemoveAll(f.worker.repoRootPath()) }) - createGitTargetWithPruneMode(t, f.worker, newBranchTarget, newBranchFolder, configv1alpha3.PruneAlways) + createGitTarget(t, f.worker, newBranchTarget, newBranchFolder, + &configv1alpha3.PrunePolicy{Mode: configv1alpha3.PruneAlways}) return f } @@ -448,9 +449,10 @@ func TestBranchWorker_NewBranchIsNotPublishedOnAnUncheckedParent(t *testing.T) { t.Cleanup(func() { syncToRemoteFn = original }) moves := 0 syncToRemoteFn = func( - ctx context.Context, repo *git.Repository, branch plumbing.ReferenceName, auth []gitclient.Option, + ctx context.Context, repo *git.Repository, branch plumbing.ReferenceName, parent string, + auth []gitclient.Option, ) (*PullReport, error) { - report, err := original(ctx, repo, branch, auth) + report, err := original(ctx, repo, branch, parent, auth) moves++ f.pushToMain(map[string]string{"MOVING.md": time.Now().String() + string(rune('a'+moves))}) return report, err diff --git a/internal/git/branch_worker_parent_branch_test.go b/internal/git/branch_worker_parent_branch_test.go new file mode 100644 index 00000000..a7f60f26 --- /dev/null +++ b/internal/git/branch_worker_parent_branch_test.go @@ -0,0 +1,259 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +// spec.parentBranch names the branch a write branch the remote does not carry is created from. +// Omitted keeps the remote's default branch, which the tests in branch_worker_new_branch_test.go +// cover; these cover a configured one: it is followed while the write branch is absent, it must +// exist for the write branch to be created, and it stops mattering once the write branch exists. + +import ( + "context" + "path/filepath" + "testing" + "time" + + "github.com/go-git/go-git/v6/plumbing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// pushToRelease commits one file to `release` from outside, creating it from main when absent. +func (f *newBranchFixture) pushToRelease(file, content string) plumbing.Hash { + f.t.Helper() + return simulateClientCommitOnDisk(f.t, f.remote, "release", file, content) +} + +func (f *newBranchFixture) commitParent(tip plumbing.Hash) plumbing.Hash { + f.t.Helper() + commit, err := f.server.CommitObject(tip) + require.NoError(f.t, err) + require.Len(f.t, commit.ParentHashes, 1) + return commit.ParentHashes[0] +} + +func TestSmartFetchFrom_ConfiguredParent(t *testing.T) { + ctx := context.Background() + tempDir := t.TempDir() + remoteURL := "file://" + filepath.Join(tempDir, "remote.git") + createBareRepo(t, filepath.Join(tempDir, "remote.git")) + simulateClientCommitOnDisk(t, remoteURL, "main", "README.md", "main\n") + simulateClientCommitOnDisk(t, remoteURL, "release", "RELEASE.md", "release\n") + simulateClientCommitOnDisk(t, remoteURL, "existing", "EXISTING.md", "existing\n") + + cases := []struct { + name, target, parent string + want plumbing.ReferenceName + wantErr error + }{ + {name: "omitted falls back to the default branch", target: "feature", want: "refs/heads/main"}, + {name: "a configured parent replaces the default", target: "feature", parent: "release", + want: "refs/heads/release"}, + {name: "a missing configured parent is refused", target: "feature", parent: "typo", + wantErr: ErrParentBranchNotFound}, + {name: "an existing target ignores a missing parent", target: "existing", parent: "typo", + want: "refs/heads/existing"}, + {name: "a parent equal to an absent target is refused", target: "feature", parent: "feature", + wantErr: ErrParentBranchNotFound}, + {name: "a parent equal to an existing target is that branch", target: "existing", parent: "existing", + want: "refs/heads/existing"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + repo, _ := initLocalRepo(t, filepath.Join(t.TempDir(), "local"), remoteURL, "") + got, err := SmartFetchFrom(ctx, repo, plumbing.NewBranchReferenceName(tc.target), tc.parent, nil) + if tc.wantErr != nil { + require.ErrorIs(t, err, tc.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } + + t.Run("an empty repository refuses a configured parent", func(t *testing.T) { + emptyURL := "file://" + filepath.Join(t.TempDir(), "empty.git") + createBareRepo(t, emptyURL[len("file://"):]) + repo, _ := initLocalRepo(t, filepath.Join(t.TempDir(), "local"), emptyURL, "") + _, err := SmartFetchFrom(ctx, repo, plumbing.NewBranchReferenceName("main"), "main", nil) + require.ErrorIs(t, err, ErrParentBranchNotFound) + + got, err := SmartFetchFrom(ctx, repo, plumbing.NewBranchReferenceName("main"), "", nil) + require.NoError(t, err) + assert.Empty(t, got, "an omitted parent keeps the empty-repository bootstrap") + }) +} + +// TestBranchWorker_NewBranchStartsFromTheConfiguredParent: the write branch is created on the +// configured parent's tip as publication saw it, even when the parent moved after the checkout. +func TestBranchWorker_NewBranchStartsFromTheConfiguredParent(t *testing.T) { + f := newNewBranchFixture(t, nil) + f.pushToRelease("RELEASE.md", "release at R1\n") + f.worker.SetParentBranch("release") + loop := newBranchWorkerEventLoop(f.worker, 0) + defer loop.stopTimers() + + require.NoError(t, f.worker.ensureRepositoryInitialized(f.worker.ctx)) + branch, _ := f.localHead() + require.Equal( + t, + plumbing.NewBranchReferenceName("release"), + branch, + "the absent write branch checks out its parent", + ) + r2 := f.pushToRelease("RELEASE-2.md", "release at R2\n") + + liveWrite(loop, "cm1") + + tip, onRemote := f.featureOnRemote() + require.True(t, onRemote) + assert.Equal(t, r2, f.commitParent(tip), "created on the configured parent's current tip") + assert.True(t, treeHas(t, f.server, tip, "RELEASE-2.md")) +} + +// TestBranchWorker_MissingConfiguredParentWritesNothingUntilItExists: a configured parent the +// remote does not carry refuses the write, starts no orphan branch, and is reported; once it is +// pushed the next write creates the branch on it. +func TestBranchWorker_MissingConfiguredParentWritesNothingUntilItExists(t *testing.T) { + f := newNewBranchFixture(t, nil) + f.worker.SetParentBranch("release") + loop := newBranchWorkerEventLoop(f.worker, 0) + defer loop.stopTimers() + + liveWrite(loop, "cm1") + + assert.Empty(t, loop.pendingWrites, "nothing was committed") + assert.True(t, loop.lastPushAt.IsZero(), "and nothing was pushed") + _, onRemote := f.featureOnRemote() + assert.False(t, onRemote, "no orphan branch") + observed, known := f.worker.LastRemoteObservation() + require.True(t, known) + assert.Equal(t, "release", observed.MissingParent) + + r1 := f.pushToRelease("RELEASE.md", "now it exists\n") + liveWrite(loop, "cm2") + + tip, onRemote := f.featureOnRemote() + require.True(t, onRemote, "it recovers once the parent exists") + assert.Equal(t, r1, f.commitParent(tip)) + observed, _ = f.worker.LastRemoteObservation() + assert.Empty(t, observed.MissingParent) +} + +// TestBranchWorker_ParentBranchEqualToTheWriteBranch: legal when the branch exists, and then it is +// simply that branch; refused when it does not, because nothing names where to start it. +func TestBranchWorker_ParentBranchEqualToTheWriteBranch(t *testing.T) { + t.Run("the branch exists", func(t *testing.T) { + f := newNewBranchFixture(t, nil) + existing := simulateClientCommitOnDisk(t, f.remote, "feature", "FEATURE.md", "feature\n") + f.worker.SetParentBranch("feature") + loop := newBranchWorkerEventLoop(f.worker, 0) + defer loop.stopTimers() + + liveWrite(loop, "cm1") + + tip, _ := f.featureOnRemote() + assert.Equal(t, existing, f.commitParent(tip)) + }) + + t.Run("the branch is absent", func(t *testing.T) { + f := newNewBranchFixture(t, nil) + f.worker.SetParentBranch("feature") + loop := newBranchWorkerEventLoop(f.worker, 0) + defer loop.stopTimers() + + liveWrite(loop, "cm1") + + _, onRemote := f.featureOnRemote() + assert.False(t, onRemote) + observed, _ := f.worker.LastRemoteObservation() + assert.Equal(t, "feature", observed.MissingParent) + }) + + t.Run("an empty repository", func(t *testing.T) { + worker, server, _ := setupCommitPushSplitWorkerOnEmptyRemote(t) + worker.mapper = configMapMapper() + createGitTarget(t, worker, newBranchTarget, newBranchFolder, + &configv1alpha3.PrunePolicy{Mode: configv1alpha3.PruneAlways}) + worker.SetParentBranch("main") + loop := newBranchWorkerEventLoop(worker, 0) + defer loop.stopTimers() + + liveWrite(loop, "cm1") + + _, err := server.Reference(plumbing.NewBranchReferenceName("main"), true) + require.ErrorIs(t, err, plumbing.ErrReferenceNotFound, "an explicit parent never bootstraps") + }) +} + +// TestBranchWorker_AnExistingWriteBranchDoesNotFollowItsParent is the rule kept from Flux and Argo +// CD: the parent decides where the branch starts, and nothing after that. +func TestBranchWorker_AnExistingWriteBranchDoesNotFollowItsParent(t *testing.T) { + f := newNewBranchFixture(t, nil) + f.pushToRelease("RELEASE.md", "release at R1\n") + f.worker.SetParentBranch("release") + loop := newBranchWorkerEventLoop(f.worker, 0) + defer loop.stopTimers() + + liveWrite(loop, "cm1") + first, onRemote := f.featureOnRemote() + require.True(t, onRemote) + + f.pushToRelease("RELEASE-2.md", "release at R2\n") + loop.lastPushAt = time.Time{} + liveWrite(loop, "cm2") + + tip, _ := f.featureOnRemote() + assert.Equal(t, first, f.commitParent(tip), "the write branch builds on itself") + assert.False(t, treeHas(t, f.server, tip, "RELEASE-2.md")) +} + +// TestBranchWorker_SetParentBranchInvalidatesTheBase: a changed parent is re-read on the next +// cycle; an unchanged one costs nothing. +func TestBranchWorker_SetParentBranchInvalidatesTheBase(t *testing.T) { + f := newNewBranchFixture(t, nil) + f.warmOnParent() + + assert.False(t, f.worker.SetParentBranch(""), "unchanged") + assert.True(t, f.worker.baseTrusted()) + + assert.True(t, f.worker.SetParentBranch("release")) + assert.Equal(t, "release", f.worker.ParentBranch()) + assert.False(t, f.worker.baseTrusted(), "the checkout holds the old parent") +} + +// TestBranchWorker_AParentChangeRebuildsRetainedWrites: writes retained during the push cooldown +// were planned against the old parent, and an invalidated base does not reach them. The change is +// taken by the event loop, which refetches and replays before the next commit or push, so the +// branch starts on the new parent, or is not created at all when the new parent is missing. +func TestBranchWorker_AParentChangeRebuildsRetainedWrites(t *testing.T) { + for _, parent := range []string{"release", "missing"} { + t.Run(parent, func(t *testing.T) { + f := newNewBranchFixture(t, map[string]string{cmFile("cm1"): cmDocument("cm1")}) + release := f.pushToRelease("RELEASE.md", "release\n") + f.warmOnParent() + loop := newBranchWorkerEventLoop(f.worker, 0) + defer loop.stopTimers() + + loop.lastPushAt = time.Now() + liveWrite(loop, "cm1") + require.Len(t, loop.pendingWrites, 1) + require.True(t, loop.pendingWrites[0].CommitSHA.IsZero(), "a no-op is retained during the cooldown") + + require.True(t, f.worker.SetParentBranch(parent)) + liveWrite(loop, "cm2") + loop.pushPending() + + tip, exists := f.featureOnRemote() + if parent == "missing" { + assert.False(t, exists, "a missing configured parent prevents branch creation") + assert.NotEmpty(t, loop.pendingWrites, "and the retained work is kept") + return + } + require.True(t, exists) + assert.Equal(t, release, f.commitParent(tip), "the first push uses the configured parent") + }) + } +} diff --git a/internal/git/branch_worker_queue_depth_test.go b/internal/git/branch_worker_queue_depth_test.go index 30f6ce2e..f4ba4e0b 100644 --- a/internal/git/branch_worker_queue_depth_test.go +++ b/internal/git/branch_worker_queue_depth_test.go @@ -130,7 +130,7 @@ func TestEnsureWorker_WaitsForTheWorkerItReplacesToStop(t *testing.T) { ensured := make(chan struct{}) go func() { - _ = manager.EnsureWorker(context.Background(), "test-provider", "test-ns", "main", RepoIdentity{}) + _ = manager.EnsureWorker(context.Background(), "test-provider", "test-ns", "main", RepoIdentity{}, "") close(ensured) }() @@ -178,7 +178,7 @@ func TestReconcileWorkers_DoesNotStopAWorkerEnsureWorkerJustHandedOut(t *testing // The SAME repository the retiring worker is about, deliberately: that is what makes // an unguarded EnsureWorker take its "already there" path, which is the regression // this test is here to catch. - _ = manager.EnsureWorker(context.Background(), "test-provider", "test-ns", "main", retiring.repo) + _ = manager.EnsureWorker(context.Background(), "test-provider", "test-ns", "main", retiring.repo, "") }() // Long enough for an unguarded EnsureWorker to finish inside the window; a guarded one is // blocked on lifecycleMu and finishes after the sweep instead. @@ -243,7 +243,7 @@ func TestReconcileWorkers_DoesNotStopAWorkerCreatedAfterItsSnapshot(t *testing.T go func() { defer close(ensured) _ = manager.EnsureWorker( - context.Background(), "newcomer-provider", "test-ns", "main", RepoIdentity{}) + context.Background(), "newcomer-provider", "test-ns", "main", RepoIdentity{}, "") }() time.Sleep(200 * time.Millisecond) }) diff --git a/internal/git/branch_worker_split_test.go b/internal/git/branch_worker_split_test.go index 8d7ee5c6..3bcb70cf 100644 --- a/internal/git/branch_worker_split_test.go +++ b/internal/git/branch_worker_split_test.go @@ -615,6 +615,7 @@ func TestBranchWorker_TransientPushFailure_RetriesSameLocalCommits(t *testing.T) _ context.Context, _ *git.Repository, _ plumbing.ReferenceName, + _ string, _ []gitclient.Option, ) (*PullReport, error) { syncCalled = true @@ -694,6 +695,7 @@ func TestBranchWorker_PushFollowedByFetchFailure_TreatsAsTransient(t *testing.T) _ context.Context, _ *git.Repository, _ plumbing.ReferenceName, + _ string, _ []gitclient.Option, ) (*PullReport, error) { syncCalled = true diff --git a/internal/git/git.go b/internal/git/git.go index 3e4f1529..d46ca9af 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -98,9 +98,20 @@ func PrepareBranch( ctx context.Context, repoURL, repoPath, targetBranchName string, auth []gitclient.Option, +) (*PullReport, error) { + return PrepareBranchFrom(ctx, repoURL, repoPath, targetBranchName, "", auth) +} + +// PrepareBranchFrom is PrepareBranch with an explicit parent branch for a target branch the remote +// does not carry; empty means the remote's default branch. See SmartFetchFrom. +func PrepareBranchFrom( + ctx context.Context, + repoURL, repoPath, targetBranchName, parentBranch string, + auth []gitclient.Option, ) (*PullReport, error) { logger := log.FromContext(ctx) - logger.Info("Preparing branch for operations", "url", repoURL, "path", repoPath, "branch", targetBranchName) + logger.Info("Preparing branch for operations", "url", repoURL, "path", repoPath, "branch", targetBranchName, + "parentBranch", parentBranch) // Ensure the directory exists if err := os.MkdirAll(filepath.Dir(repoPath), 0750); err != nil { @@ -136,7 +147,7 @@ func PrepareBranch( } targetBranch := plumbing.NewBranchReferenceName(targetBranchName) - pullReport, err := syncToRemote(ctx, repo, targetBranch, auth) + pullReport, err := syncToRemoteFrom(ctx, repo, targetBranch, parentBranch, auth) if err != nil { return nil, err } @@ -366,13 +377,24 @@ func syncToRemote( repo *git.Repository, branch plumbing.ReferenceName, auth []gitclient.Option, +) (*PullReport, error) { + return syncToRemoteFrom(ctx, repo, branch, "", auth) +} + +// syncToRemoteFrom is syncToRemote with an explicit parent branch; see SmartFetchFrom. +func syncToRemoteFrom( + ctx context.Context, + repo *git.Repository, + branch plumbing.ReferenceName, + parent string, + auth []gitclient.Option, ) (*PullReport, error) { _, currentHash, err := GetCurrentBranch(repo) if err != nil { return nil, fmt.Errorf("unexpected fail to read HEAD: %w", err) } - availableBranch, err := SmartFetch(ctx, repo, branch, auth) + availableBranch, err := SmartFetchFrom(ctx, repo, branch, parent, auth) if err != nil { return nil, fmt.Errorf("failed to fetch: %w", err) } diff --git a/internal/git/git_smart_fetch.go b/internal/git/git_smart_fetch.go index 5458afb1..47447e55 100644 --- a/internal/git/git_smart_fetch.go +++ b/internal/git/git_smart_fetch.go @@ -27,6 +27,26 @@ func SmartFetch( repo *git.Repository, target plumbing.ReferenceName, // e.g. "refs/heads/feature" or "HEAD" auth []gitclient.Option, +) (plumbing.ReferenceName, error) { + return SmartFetchFrom(ctx, repo, target, "", auth) +} + +// ErrParentBranchNotFound reports that a configured parent branch is absent on the remote while +// the target branch is absent too, so there is nothing to create the target branch from. It is +// never reported for an omitted parent: that falls back to the remote's default branch, and to +// an unborn branch in an empty repository. +var ErrParentBranchNotFound = errors.New("parent branch not found on the remote") + +// SmartFetchFrom is SmartFetch with an explicit parent: the branch to fall back to when the target +// branch is absent. Empty means the remote's default branch. A configured parent that is absent is +// an error rather than a fallback, so a typo can never start an orphan branch; it does not matter +// while the target branch exists. +func SmartFetchFrom( + ctx context.Context, + repo *git.Repository, + target plumbing.ReferenceName, + parent string, + auth []gitclient.Option, ) (plumbing.ReferenceName, error) { remoteName := "origin" remote, err := repo.Remote(remoteName) @@ -39,12 +59,17 @@ func SmartFetch( if err != nil { return "", err } - if len(refs) == 0 { + if len(refs) == 0 && parent == "" { return "", nil } // 2. Analyze: Find default branch and check target existence - defaultFull, defaultShort, targetExists := analyzeRemoteRefs(ctx, refs, target.String()) + headFull, headShort, targetExists := analyzeRemoteRefs(ctx, refs, target.String()) + + defaultFull, defaultShort, err := fallbackBranch(refs, parent, headFull, headShort, targetExists) + if err != nil { + return "", err + } // 3. Plan: Build RefSpecs based on analysis refSpecs := buildSmartRefSpecs(remoteName, defaultFull, defaultShort, target, targetExists) @@ -75,12 +100,39 @@ func SmartFetch( } } - // 5. Repair: Fix local symbolic HEAD - repairRemoteSymbolicHead(repo, remoteName, defaultShort) + // 5. Repair: Fix local symbolic HEAD. It mirrors the remote's HEAD, never a configured parent. + repairRemoteSymbolicHead(repo, remoteName, headShort) return result, nil } +// fallbackBranch is the branch to fall back on, and to fetch as the safety net: the configured +// parent, or the remote's default branch when none is configured. A configured parent the remote +// does not carry is an error only while the target is absent too. +func fallbackBranch( + refs []*plumbing.Reference, parent, headFull, headShort string, targetExists bool, +) (string, string, error) { + if parent == "" { + return headFull, headShort, nil + } + full, short := configuredParent(refs, parent) + if full == "" && !targetExists { + return "", "", fmt.Errorf("%w: %q", ErrParentBranchNotFound, parent) + } + return full, short, nil +} + +// configuredParent finds a configured parent branch in the advertisement, or returns empty names. +func configuredParent(refs []*plumbing.Reference, parent string) (string, string) { + full := plumbing.NewBranchReferenceName(parent) + for _, ref := range refs { + if ref.Name() == full && ref.Type() == plumbing.HashReference { + return full.String(), parent + } + } + return "", "" +} + func listRemoteRefs(remote *git.Remote, auth []gitclient.Option) ([]*plumbing.Reference, error) { refs, err := remote.List(&git.ListOptions{ClientOptions: auth}) if errors.Is(err, transport.ErrEmptyRemoteRepository) { diff --git a/internal/git/refresh.go b/internal/git/refresh.go index d0492480..bccb940c 100644 --- a/internal/git/refresh.go +++ b/internal/git/refresh.go @@ -190,12 +190,19 @@ func (l *branchWorkerEventLoop) refreshFromRemote(provider *configv1alpha3.GitPr // is what makes it honest for the two targets a clone-first version skipped silently — one // declared but never published to, and one whose GitProvider was recreated against a // different repository. + parentBranch := w.ParentBranch() advertisement, err := advertiseRemoteBranch( - w.repo.URL, plumbing.NewBranchReferenceName(w.Branch), auth) + w.repo.URL, plumbing.NewBranchReferenceName(w.Branch), parentBranch, auth) if err != nil { return fmt.Errorf("read the remote advertisement: %w", err) } advertised := advertisement.branch + // Neither the branch nor the parent it would be created from: there is nothing to follow, + // and the GitTarget reports the parent as missing until somebody creates it. + if advertised.IsZero() && parentBranch != "" && advertisement.parent.IsZero() { + w.recordMissingParent(parentBranch, ObservedByFetch) + return nil + } revision := "" if !advertised.IsZero() { revision = advertised.String() @@ -347,13 +354,14 @@ func (w *BranchWorker) rescanLayoutForTarget(ctx context.Context, req *RefreshRe type remoteAdvertisement struct { // branch is where the write branch is; zero when the remote does not carry it. branch plumbing.Hash - // parent is where the remote's default branch is: the parent a new write branch is created - // from. Zero for an empty repository, or a HEAD that names no branch. + // parent is where the parent a new write branch is created from is: the configured parent + // branch, or the remote's default branch when none is configured. Zero when the remote does + // not carry it, which includes an empty repository. parent plumbing.Hash } // advertiseRemoteBranch asks the remote where a branch and its parent are, and transfers nothing -// else. +// else. An empty parentBranch means the branch the remote's HEAD names. // // It is built on a storage-less remote, the way CheckRepo is, so it needs no clone on disk. Zero // means the advertisement did not carry the branch, which includes an empty repository — an @@ -361,6 +369,7 @@ type remoteAdvertisement struct { func advertiseRemoteBranch( remoteURL string, branch plumbing.ReferenceName, + parentBranch string, auth []gitclient.Option, ) (remoteAdvertisement, error) { remote := gogit.NewRemote(nil, &config.RemoteConfig{Name: "origin", URLs: []string{remoteURL}}) @@ -378,6 +387,9 @@ func advertiseRemoteBranch( defaultBranch = ref.Target() } } + if parentBranch != "" { + defaultBranch = plumbing.NewBranchReferenceName(parentBranch) + } return remoteAdvertisement{branch: hashes[branch], parent: hashes[defaultBranch]}, nil } diff --git a/internal/git/refresh_test.go b/internal/git/refresh_test.go index 2e0aba5d..b70f7a08 100644 --- a/internal/git/refresh_test.go +++ b/internal/git/refresh_test.go @@ -644,3 +644,42 @@ func TestRefresh_ANewBranchDoesNotDependOnARecentRefresh(t *testing.T) { require.NoError(t, err) assert.Equal(t, moved, strings.TrimSpace(string(out)), "the new branch starts on the parent's current tip") } + +// TestRefresh_FollowsTheConfiguredParent: with spec.parentBranch set, the refresher follows that +// branch rather than the remote's default, and a parent changed on a live target is re-read on the +// next refresh. +func TestRefresh_FollowsTheConfiguredParent(t *testing.T) { + reader, err := telemetry.InitTestExporter() + require.NoError(t, err) + + h := newRefreshHarnessForBranch(t, "refresh-configured-parent", true, "feature") + release := simulateClientCommitOnDisk(t, h.repoDir, "release", "RELEASE.md", "release\n") + require.NoError(t, h.worker.ensureRepositoryInitialized(h.worker.ctx)) + + require.True(t, h.worker.SetParentBranch("release"), "changed on a live target") + h.refresh(time.Nanosecond) + + assert.Equal(t, int64(1), fetchCount(t, reader, h.worker, fetchReasonRefresh)) + repo, err := gogit.PlainOpen(h.worker.repoPath()) + require.NoError(t, err) + assert.Equal(t, release, localHead(repo), "the checkout is on the configured parent") + assert.NotEmpty(t, h.scanVerdicts, "and the folder was re-read from it") +} + +// TestRefresh_ReportsAMissingConfiguredParent: one advertisement shows neither the write branch nor +// its configured parent, which is reported and fetches nothing. +func TestRefresh_ReportsAMissingConfiguredParent(t *testing.T) { + reader, err := telemetry.InitTestExporter() + require.NoError(t, err) + + h := newRefreshHarnessForBranch(t, "refresh-missing-parent", true, "feature") + h.worker.SetParentBranch("release") + h.reported = nil + + assert.Equal(t, int64(1), h.refresh(time.Nanosecond)) + assert.Zero(t, fetchCount(t, reader, h.worker, fetchReasonRefresh)) + require.NotEmpty(t, h.reported) + last := h.reported[len(h.reported)-1] + assert.Equal(t, "release", last.MissingParent) + assert.Empty(t, last.Commit) +} diff --git a/internal/git/remote_observation.go b/internal/git/remote_observation.go index 465f3b6c..26b82b88 100644 --- a/internal/git/remote_observation.go +++ b/internal/git/remote_observation.go @@ -50,6 +50,9 @@ type RemoteObservation struct { // It is zero for a worker that has no identity of its own: the CLI, and tests that never reach // a remote. A comparison against an unknown identity proves nothing, so it is not made. Repo RepoIdentity + // MissingParent names the configured parent branch when the remote carries neither it nor the + // branch, so the branch cannot be created. Empty otherwise, including for an omitted parent. + MissingParent string } // Age is how long ago the observation was made, against now. diff --git a/internal/git/remote_report_test.go b/internal/git/remote_report_test.go index b17ac9f4..d8ffddeb 100644 --- a/internal/git/remote_report_test.go +++ b/internal/git/remote_report_test.go @@ -56,12 +56,12 @@ func TestRemoteForBranch_SurvivesTheWorkerItWasProvedBy(t *testing.T) { before := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} after := RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} - require.NoError(t, m.EnsureWorker(ctx, "repo1", "gitops-system", "main", before)) + require.NoError(t, m.EnsureWorker(ctx, "repo1", "gitops-system", "main", before, "")) worker, ok := m.GetWorkerForTarget("repo1", "gitops-system", "main") require.True(t, ok) worker.recordRemoteObservation("aaaa", ObservedByPush) - require.NoError(t, m.EnsureWorker(ctx, "repo1", "gitops-system", "main", after)) + require.NoError(t, m.EnsureWorker(ctx, "repo1", "gitops-system", "main", after, "")) observed, known := m.RemoteForBranch(key) require.True(t, known, diff --git a/internal/git/replay_failure_test.go b/internal/git/replay_failure_test.go index 45941630..574a7ace 100644 --- a/internal/git/replay_failure_test.go +++ b/internal/git/replay_failure_test.go @@ -334,7 +334,7 @@ func TestReplayFailure_AResetThatDiesHalfwayIsAlsoHeld(t *testing.T) { original := syncToRemoteFn syncToRemoteFn = func( ctx context.Context, repo *gogit.Repository, - branch plumbing.ReferenceName, auth []gitclient.Option, + branch plumbing.ReferenceName, _ string, auth []gitclient.Option, ) (*PullReport, error) { if _, err := SmartFetch(ctx, repo, branch, auth); err != nil { return nil, err diff --git a/internal/git/worker_manager.go b/internal/git/worker_manager.go index 566b6af7..2b149fd1 100644 --- a/internal/git/worker_manager.go +++ b/internal/git/worker_manager.go @@ -283,12 +283,17 @@ func (m *WorkerManager) RemoteForBranch(key BranchKey) (RemoteObservation, bool) // and the caller can fail. See ReplacementPending; the delivery is the GitTarget reconcile's // worker wiring gate. // +// parentBranch is the GitTarget's spec.parentBranch. A change is not a replacement: it says where +// a write branch the remote does not carry is created from, so it abandons nothing already +// written. The worker keeps its clone and drops its base trust instead. +// // Worker creation/start is protected by the manager lock. func (m *WorkerManager) EnsureWorker( _ context.Context, providerName, providerNamespace string, branch string, repo RepoIdentity, + parentBranch string, ) error { // Held across the whole check-and-create so a replacement cannot start while the worker it // replaces is still stopping; they would share a clone. @@ -307,6 +312,10 @@ func (m *WorkerManager) EnsureWorker( if exists { if existing.repo == repo { + if existing.SetParentBranch(parentBranch) { + m.Log.Info("Parent branch changed; the worker re-reads the remote on its next cycle", + "key", key.String(), "parentBranch", parentBranch) + } return nil } // Both identities, at default verbosity: a comparison that is not stable across steady @@ -338,6 +347,7 @@ func (m *WorkerManager) EnsureWorker( // Inject the resolver before Start: the field is read only by the event-loop // goroutine Start spawns, so setting it here (under m.mu, before that goroutine // exists) is race-free. + worker.SetParentBranch(parentBranch) worker.mapper = m.mapper worker.clusterMapper = m.clusterMapper worker.sshHostKeys = m.sshHostKeys diff --git a/internal/git/worker_manager_test.go b/internal/git/worker_manager_test.go index 5fe9c1e7..3ce329b9 100644 --- a/internal/git/worker_manager_test.go +++ b/internal/git/worker_manager_test.go @@ -68,7 +68,7 @@ func mustEnsureWorker( repo RepoIdentity, ) bool { t.Helper() - require.NoError(t, m.EnsureWorker(ctx, providerName, providerNamespace, branch, repo)) + require.NoError(t, m.EnsureWorker(ctx, providerName, providerNamespace, branch, repo, "")) key := BranchKey{RepoNamespace: providerNamespace, RepoName: providerName, Branch: branch} pending := m.ReplacementPending(key) m.AcknowledgeReplacement(key) @@ -144,7 +144,7 @@ func TestEnsureWorker_CreatesAWorkerWithTheIdentityItWasAskedFor(t *testing.T) { createTargetForRegister(ctx, t, client, "target1", "repo1", "main", "clusters/prod") // Register first target - err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", repo) + err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", repo, "") if err != nil { t.Fatalf("Failed to register target: %v", err) } @@ -237,12 +237,12 @@ func TestWorkerManagerDifferentBranches(t *testing.T) { createTargetForRegister(ctx, t, client, "target-dev", "repo1", "develop", "base/") // Register targets for same repo, different branches - err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", repo) + err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", repo, "") if err != nil { t.Fatalf("Failed to register target-main: %v", err) } - err = manager.EnsureWorker(ctx, "repo1", "gitops-system", "develop", repo) + err = manager.EnsureWorker(ctx, "repo1", "gitops-system", "develop", repo, "") if err != nil { t.Fatalf("Failed to register target-dev: %v", err) } @@ -362,7 +362,7 @@ func TestWorkerManagerConcurrentRegistration(t *testing.T) { done := make(chan bool, 10) for i := range 10 { go func(index int) { - err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", repo) + err := manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", repo, "") if err != nil { t.Errorf("Failed to ensure worker %d: %v", index, err) } @@ -529,7 +529,7 @@ func TestWorkerLifecycle_BuildsAndReclaimsItsOwnState(t *testing.T) { root := workerStateRoot repo := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} - require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", repo)) + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", repo, "")) worker, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") require.True(t, ok) require.True(t, strings.HasPrefix(worker.repoPath(), root+string(filepath.Separator)), @@ -573,7 +573,7 @@ func TestWorkerLifecycle_AReplacementDoesNotReclaimItsPredecessorsCheckout(t *te before := RepoIdentity{ProviderUID: "uid-1", URL: sameURL} after := RepoIdentity{ProviderUID: "uid-2", URL: sameURL} - require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", before)) + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", before, "")) old, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") require.True(t, ok) require.NoError(t, os.MkdirAll(old.repoPath(), 0o750)) @@ -585,7 +585,7 @@ func TestWorkerLifecycle_AReplacementDoesNotReclaimItsPredecessorsCheckout(t *te sentinel := filepath.Join(successor, "HEAD") require.NoError(t, os.WriteFile(sentinel, []byte("ref: refs/heads/main\n"), 0o600)) - require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after)) + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after, "")) replacement, ok := manager.GetWorkerForTarget("repo1", "gitops-system", "main") require.True(t, ok) require.NotSame(t, old, replacement) diff --git a/internal/git/worker_repo_identity_test.go b/internal/git/worker_repo_identity_test.go index bc23e70c..6fd70ab0 100644 --- a/internal/git/worker_repo_identity_test.go +++ b/internal/git/worker_repo_identity_test.go @@ -352,21 +352,21 @@ func TestEnsureWorker_AnUndeliveredRecoveryIsStillPendingOnTheNextPass(t *testin before := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} after := RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} - require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", before)) + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", before, "")) require.False(t, manager.ReplacementPending(key)) - require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after)) + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after, "")) require.True(t, manager.ReplacementPending(key)) // The delivery fails, so nothing acknowledges it. Two more reconciles of this target, and of // a sibling on the same branch, still find the recovery waiting. - require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after)) + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after, "")) assert.True(t, manager.ReplacementPending(key), "a recovery nobody delivered must survive the calls that come after it") manager.AcknowledgeReplacement(key) assert.False(t, manager.ReplacementPending(key)) - require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after)) + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after, "")) assert.False(t, manager.ReplacementPending(key), "and a delivered one must not be asked for again on every steady tick") } @@ -379,8 +379,8 @@ func TestReconcileWorkers_DropsARecoveryForABranchNothingNeeds(t *testing.T) { before := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} after := RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} - require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", before)) - require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after)) + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", before, "")) + require.NoError(t, manager.EnsureWorker(ctx, "repo1", "gitops-system", "main", after, "")) require.True(t, manager.ReplacementPending(key)) // No GitTarget names this branch, so the sweep retires its worker. diff --git a/internal/watch/event_router_test.go b/internal/watch/event_router_test.go index db2c3946..7a5c6222 100644 --- a/internal/watch/event_router_test.go +++ b/internal/watch/event_router_test.go @@ -353,8 +353,8 @@ func TestServiceCommitRequest_RegisteredWorkerResolvesNoOpenWindow(t *testing.T) ensureErr := workerManager.EnsureWorker( ctx, "team-a-provider", "team-a", "main", - git.RepoIdentity{URL: "file:///tmp/does-not-need-to-exist"}, - ) + git.RepoIdentity{URL: "file:///tmp/does-not-need-to-exist"}, "") + require.NoError(t, ensureErr) router := NewEventRouter(workerManager, nil, client, logr.Discard()) diff --git a/test/e2e/helpers.go b/test/e2e/helpers.go index 3300d14c..81d74f2a 100644 --- a/test/e2e/helpers.go +++ b/test/e2e/helpers.go @@ -252,6 +252,26 @@ func createGitTargetWithOptions( encryptionSecretName string, generateWhenMissing bool, commit gitTargetCommitOptions, +) { + applyGitTarget(name, namespace, providerName, path, branch, "", encryptionSecretName, generateWhenMissing, commit) +} + +// createGitTargetWithParentBranch is createGitTarget with spec.parentBranch; empty omits it. +func createGitTargetWithParentBranch(name, namespace, providerName, path, branch, parentBranch string) { + applyGitTarget(name, namespace, providerName, path, branch, parentBranch, + e2eEncryptionRefName, false, gitTargetCommitOptions{Window: e2eCommitWindow}) +} + +func applyGitTarget( + name, + namespace, + providerName, + path, + branch, + parentBranch, + encryptionSecretName string, + generateWhenMissing bool, + commit gitTargetCommitOptions, ) { By(fmt.Sprintf("creating GitTarget '%s' in ns '%s' for GitProvider '%s' with path '%s'", name, namespace, providerName, path)) @@ -261,6 +281,7 @@ func createGitTargetWithOptions( Namespace string ProviderName string Branch string + ParentBranch string Path string EncryptionSecretName string GenerateWhenMissing bool @@ -273,6 +294,7 @@ func createGitTargetWithOptions( Namespace: namespace, ProviderName: providerName, Branch: branch, + ParentBranch: parentBranch, Path: path, EncryptionSecretName: encryptionSecretName, GenerateWhenMissing: generateWhenMissing, diff --git a/test/e2e/new_branch_parent_e2e_test.go b/test/e2e/new_branch_parent_e2e_test.go index be3f483b..f335393a 100644 --- a/test/e2e/new_branch_parent_e2e_test.go +++ b/test/e2e/new_branch_parent_e2e_test.go @@ -31,7 +31,19 @@ import ( // `contention` fetch (the rejected push), with no `refresh` fetch in between. That takes the // periodic refresher out of the proof without redeploying the controller with it disabled, which // would disturb every other spec on the leg. -var _ = Describe("New write branch starts from its parent", Label("manager"), Ordered, func() { +// +// It runs twice: once with spec.parentBranch omitted (the remote's default branch, main), and once +// naming `release`, which the GitProvider does not allow writing to because it is only read. +var _ = Describe("New write branch starts from its parent", Label("manager"), func() { + Context("with spec.parentBranch omitted", Ordered, func() { + describeNewWriteBranchSpec("", "main", "parent-branch") + }) + Context("with spec.parentBranch naming release", Ordered, func() { + describeNewWriteBranchSpec("release", "release", "parent-release") + }) +}) + +func describeNewWriteBranchSpec(parentBranch, parent, slug string) { const ( providerName = "parent-provider" destName = "parent-dest" @@ -94,11 +106,11 @@ data: BeforeAll(func() { ensurePrometheusClient() - testNs = testNamespaceFor("manager-parent-branch") + testNs = testNamespaceFor("manager-" + slug) _, _ = kubectlRun("create", "namespace", testNs) writeBranch = fmt.Sprintf("reverser-e2e-%d", GinkgoRandomSeed()) - repo = SetupRepo(resolveE2EContext(), testNs, fmt.Sprintf("e2e-parent-branch-%d", GinkgoRandomSeed())) + repo = SetupRepo(resolveE2EContext(), testNs, fmt.Sprintf("e2e-%s-%d", slug, GinkgoRandomSeed())) _, err := kubectlRunInNamespace(testNs, "apply", "-f", repo.SecretsYAML) Expect(err).NotTo(HaveOccurred(), "failed to apply git secrets to test namespace") applySOPSAgeKeyToNamespace(testNs) @@ -127,14 +139,19 @@ spec: }) It("stays on standby, then creates the write branch on the parent's current tip", func() { - By("seeding main with commit A, whose folder already holds a document the cluster will match") - commitFilesToMainFromOutside(repo, testNs, "e2e: seed A", map[string]string{ + By("seeding " + parent + " with commit A, whose folder already holds a document the cluster will match") + if parent != "main" { + commitFilesToBranchFromOutside(repo, testNs, "main", "e2e: seed main", map[string]string{ + "README.md": "main is not the parent here\n", + }) + } + commitFilesToBranchFromOutside(repo, testNs, parent, "e2e: seed A", map[string]string{ "README.md": "seed\n", path.Join(gitPath, testNs, "configmaps", "standby-present.yaml"): mirroredDocument("standby-present"), }) By("creating a GitTarget on an absent write branch, and a WatchRule that selects nothing yet") - createGitTarget(destName, testNs, providerName, gitPath, writeBranch) + createGitTargetWithParentBranch(destName, testNs, providerName, gitPath, writeBranch, parentBranch) rule := fmt.Sprintf(`apiVersion: configbutler.ai/v1alpha3 kind: WatchRule metadata: @@ -178,10 +195,10 @@ spec: Expect(remoteBranchHeadOf(Default, repo.CheckoutDir, writeBranch)).To(BeEmpty(), "a write that commits nothing must not create the write branch") - By("main moves to B from outside, touching the target's folder") + By(parent + " moves to B from outside, touching the target's folder") contentionBefore, refreshBefore := fetches("contention"), fetches("refresh") - hashB := commitFilesToMainFromOutside(repo, testNs, "e2e: B moves main", map[string]string{ - path.Join(gitPath, "NOTES.md"): "added on main at B\n", + hashB := commitFilesToBranchFromOutside(repo, testNs, parent, "e2e: B moves the parent", map[string]string{ + path.Join(gitPath, "NOTES.md"): "added on the parent at B\n", }) By("a live edit arrives with no resync in between") @@ -211,17 +228,20 @@ spec: Expect(fetches("refresh")).To(Equal(refreshBefore), "no refresh fetch may stand in for the publication check") }) -}) +} -// commitFilesToMainFromOutside commits files to main and pushes it directly, the way another -// writer would, and returns the new tip. It starts main when the repository is still empty, and -// retries because losing a push race with the controller says nothing about the behavior under test. -func commitFilesToMainFromOutside(repo *RepoArtifacts, namespace, message string, files map[string]string) string { +// commitFilesToBranchFromOutside commits files to a branch and pushes it directly, the way another +// writer would, and returns the new tip. A branch the remote does not carry yet starts from main, +// or from nothing in an empty repository. It retries because losing a push race with the +// controller says nothing about the behavior under test. +func commitFilesToBranchFromOutside( + repo *RepoArtifacts, namespace, branch, message string, files map[string]string, +) string { GinkgoHelper() configureRepoOriginWithCredentials(repo, namespace) Eventually(func() error { - return attemptCommitFilesToMain(repo, message, files) + return attemptCommitFilesToBranch(repo, branch, message, files) }, seedPushTimeout, seedPushInterval).Should(Succeed(), "the outside writer kept losing the push race") head, err := gitRun(repo.CheckoutDir, "rev-parse", "HEAD") @@ -229,27 +249,17 @@ func commitFilesToMainFromOutside(repo *RepoArtifacts, namespace, message string return strings.TrimSpace(head) } -// attemptCommitFilesToMain is one attempt, from the CURRENT remote tip. It returns errors rather +// attemptCommitFilesToBranch is one attempt, from the CURRENT remote tip. It returns errors rather // than asserting, so Eventually can rebuild on the tip that beat it. -func attemptCommitFilesToMain(repo *RepoArtifacts, message string, files map[string]string) error { +func attemptCommitFilesToBranch(repo *RepoArtifacts, branch, message string, files map[string]string) error { runGit := func(args ...string) error { if out, err := gitRun(repo.CheckoutDir, args...); err != nil { return fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), err, out) } return nil } - if _, err := gitRun(repo.CheckoutDir, "fetch", "origin", "main"); err == nil { - if err := runGit("checkout", "-B", "main", "origin/main"); err != nil { - return err - } - if err := runGit("reset", "--hard", "origin/main"); err != nil { - return err - } - } else { - if err := runGit("checkout", "--orphan", "main"); err != nil { - return err - } - _, _ = gitRun(repo.CheckoutDir, "rm", "-rf", ".") + if err := checkoutRemoteTipOf(repo, branch, runGit); err != nil { + return err } for name, content := range files { full := filepath.Join(repo.CheckoutDir, name) @@ -266,5 +276,24 @@ func attemptCommitFilesToMain(repo *RepoArtifacts, message string, files map[str if err := runGit("commit", "-m", message); err != nil { return err } - return runGit("push", "origin", "HEAD:main") + return runGit("push", "origin", "HEAD:refs/heads/"+branch) +} + +// checkoutRemoteTipOf checks out branch at its remote tip; a branch the remote does not carry +// starts from main, and from an orphan when main is absent too. +func checkoutRemoteTipOf(repo *RepoArtifacts, branch string, runGit func(...string) error) error { + for _, start := range []string{branch, "main"} { + if _, err := gitRun(repo.CheckoutDir, "fetch", "origin", start); err != nil { + continue + } + if err := runGit("checkout", "-B", branch, "origin/"+start); err != nil { + return err + } + return runGit("reset", "--hard", "origin/"+start) + } + if err := runGit("checkout", "--orphan", branch); err != nil { + return err + } + _, _ = gitRun(repo.CheckoutDir, "rm", "-rf", ".") + return nil } diff --git a/test/e2e/templates/gittarget.tmpl b/test/e2e/templates/gittarget.tmpl index 3511362c..3a5fccc9 100644 --- a/test/e2e/templates/gittarget.tmpl +++ b/test/e2e/templates/gittarget.tmpl @@ -7,6 +7,9 @@ spec: gitProviderRef: name: {{ .ProviderName }} branch: {{ .Branch }} +{{- if .ParentBranch }} + parentBranch: {{ .ParentBranch }} +{{- end }} path: {{ .Path }} commit: window: