Merge pull request 'fix: avoid stale onboarding branches' (#64) from fix/stale-registration-branches into main

Reviewed-on: #64

internal/assets/templates/delivery-pipeline.yaml.tmpl

@@ -27,10 +27,6 @@ spec:
     - name: environment
     - name: pr-number
       default: ""
-    - name: platform-url
-      default: ""
-    - name: platform-revision
-      default: ""
     - name: forgejo-base-url
       default: ""
     - name: forgejo-owner
@@ -57,10 +53,6 @@ spec:
           value: $(params.environment)
         - name: PR_NUMBER
           value: $(params.pr-number)
-        - name: PLATFORM_URL
-          value: $(params.platform-url)
-        - name: PLATFORM_REVISION
-          value: $(params.platform-revision)
         - name: FORGEJO_BASE_URL
           value: $(params.forgejo-base-url)
         - name: FORGEJO_OWNER
@@ -102,8 +94,6 @@ spec:
         cd /tmp/manifests
         if [ "$ENVIRONMENT" = preview ]; then
           valid_pr_number "$PR_NUMBER"
-          valid_url "$PLATFORM_URL"
-          valid_revision "$PLATFORM_REVISION"
           app_dir="apps/previews/$APP_NAME-pr-$PR_NUMBER"
           marker="$app_dir/ownership.yaml"
           if [ -e "$app_dir" ]; then
@@ -129,12 +119,7 @@ spec:
         EOF
             cmp -s "$expected_marker" "$marker" || fail
           fi
-          git clone "$PLATFORM_URL" /tmp/platform
-          git -C /tmp/platform checkout "$PLATFORM_REVISION"
-          [ -f /tmp/platform/preview/values.yaml ] || fail
           mkdir -p "$app_dir"
-          cp /tmp/platform/preview/values.yaml "$app_dir/values.yaml"
-          sed -i "s/PLACEHOLDER_PR/$PR_NUMBER/g" "$app_dir/values.yaml"
           cat > "$marker" <<EOF
         apiVersion: v1
         kind: ConfigMap
@@ -169,14 +154,20 @@ spec:
           chart:
             spec:
               chart: ./charts/$APP_NAME
+              reconcileStrategy: Revision
               sourceRef:
                 kind: GitRepository
                 name: $APP_NAME
                 namespace: flux-system
+              valuesFiles:
+                - ./charts/$APP_NAME/values.yaml
+                - ./preview/values.yaml
           values:
             image:
               repository: $IMAGE
               tag: $TAG
+            gateway:
+              hostname: $APP_NAME-pr-$PR_NUMBER.{{ .ClusterDomain }}
         EOF
           cat > "$app_dir/kustomization.yaml" <<EOF
         apiVersion: kustomize.config.k8s.io/v1beta1
@@ -212,10 +203,14 @@ spec:
           chart:
             spec:
               chart: ./charts/$APP_NAME
+              reconcileStrategy: Revision
               sourceRef:
                 kind: GitRepository
                 name: $APP_NAME
                 namespace: flux-system
+              valuesFiles:
+                - ./charts/$APP_NAME/values.yaml
+                - ./staging/values.yaml
           values:
             image:
               repository: $IMAGE
@@ -259,10 +254,14 @@ spec:
           chart:
             spec:
               chart: ./charts/$APP_NAME
+              reconcileStrategy: Revision
               sourceRef:
                 kind: GitRepository
                 name: $APP_NAME
                 namespace: flux-system
+              valuesFiles:
+                - ./charts/$APP_NAME/values.yaml
+                - ./production/values.yaml
           values:
             image:
               repository: $IMAGE
@@ -628,10 +627,6 @@ spec:
       default: {{ quote .ForgejoOwner }}
     - name: manifests-repo
       default: {{ quote .ManifestsRepo }}
-    - name: platform-url
-      default: {{ quote .AppRepoURL }}
-    - name: platform-revision
-      default: {{ quote .PlatformBranch }}
   tasks:
     - name: build-layer
       when:
@@ -681,10 +676,6 @@ spec:
           value: preview
         - name: pr-number
           value: $(params.pr-number)
-        - name: platform-url
-          value: $(params.platform-url)
-        - name: platform-revision
-          value: $(params.platform-revision)
     - name: update-staging
       runAfter: [build-layer]
       when:

internal/bootstrap/bootstrap.go

@@ -622,7 +622,6 @@ type appDeliveryTemplateConfig struct {
 	AppRepository        string
 	AppRepoURL           string
 	AppRepoRef           string
-	PlatformBranch       string
 	ProductionBranch     string
 	ImageRepository      string
 	BuildStrategy        string
@@ -633,6 +632,7 @@ type appDeliveryTemplateConfig struct {
 	ManifestsURL         string
 	ManifestsRepo        string
 	ManifestsBranch      string
+	ClusterDomain        string
 }
 
 var deliveryAppName = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]{0,45}[a-z0-9])?$`)
@@ -654,11 +654,11 @@ func renderAppDelivery(cfg config.Config) ([]byte, error) {
 	}
 	values := appDeliveryTemplateConfig{
 		AppName: cfg.Delivery.AppName, AppRepository: appRepository, AppRepoURL: cfg.Delivery.AppRepoURL,
-		AppRepoRef: cfg.Delivery.AppRepoRef, PlatformBranch: "maidn/platform-" + cfg.Delivery.AppName, ProductionBranch: cfg.Delivery.ProductionBranch, ImageRepository: cfg.Delivery.ImageRepository,
+		AppRepoRef: cfg.Delivery.AppRepoRef, ProductionBranch: cfg.Delivery.ProductionBranch, ImageRepository: cfg.Delivery.ImageRepository,
 		BuildStrategy: cfg.Delivery.BuildStrategy, BuildOutputDirectory: cfg.Delivery.BuildOutputDirectory, BuildConfiguration: cfg.Delivery.BuildConfiguration,
-		ForgejoBaseURL: origin, ForgejoOwner: cfg.Git.Owner, ManifestsURL: forgejo.CloneURL(origin, cfg.Git.Owner, cfg.Flux.ManifestsRepo), ManifestsRepo: cfg.Flux.ManifestsRepo, ManifestsBranch: cfg.Flux.Branch,
+		ForgejoBaseURL: origin, ForgejoOwner: cfg.Git.Owner, ManifestsURL: forgejo.CloneURL(origin, cfg.Git.Owner, cfg.Flux.ManifestsRepo), ManifestsRepo: cfg.Flux.ManifestsRepo, ManifestsBranch: cfg.Flux.Branch, ClusterDomain: cfg.Flux.ClusterDomain,
 	}
-	for name, value := range map[string]string{"appRepository": values.AppRepository, "appRepoUrl": values.AppRepoURL, "appRepoRef": values.AppRepoRef, "platformBranch": values.PlatformBranch, "productionBranch": values.ProductionBranch, "imageRepository": values.ImageRepository, "buildOutputDirectory": values.BuildOutputDirectory, "buildConfiguration": values.BuildConfiguration, "forgejoBaseUrl": values.ForgejoBaseURL, "forgejoOwner": values.ForgejoOwner, "manifestsUrl": values.ManifestsURL, "manifestsRepo": values.ManifestsRepo, "manifestsBranch": values.ManifestsBranch} {
+	for name, value := range map[string]string{"appRepository": values.AppRepository, "appRepoUrl": values.AppRepoURL, "appRepoRef": values.AppRepoRef, "productionBranch": values.ProductionBranch, "imageRepository": values.ImageRepository, "buildOutputDirectory": values.BuildOutputDirectory, "buildConfiguration": values.BuildConfiguration, "forgejoBaseUrl": values.ForgejoBaseURL, "forgejoOwner": values.ForgejoOwner, "manifestsUrl": values.ManifestsURL, "manifestsRepo": values.ManifestsRepo, "manifestsBranch": values.ManifestsBranch, "clusterDomain": values.ClusterDomain} {
 		if value == "" || strings.ContainsAny(value, "\r\n") || config.RedactURL(value) != value {
 			return nil, fmt.Errorf("delivery %s cannot be empty or contain credentials", name)
 		}

internal/bootstrap/bootstrap_test.go

@@ -111,7 +111,7 @@ func TestRemoveDuplicateAppDeliverySource(t *testing.T) {
 func TestGeneratedDeliveryUsesCombinedStaticBuildArtifactContract(t *testing.T) {
 	cfg := config.Config{
 		Git:      config.GitConfig{BaseURL: "https://git.example.test", Owner: "platform"},
-		Flux:     config.FluxConfig{Branch: "main", ManifestsRepo: "manifests"},
+		Flux:     config.FluxConfig{Branch: "main", ClusterDomain: "example.test", ManifestsRepo: "manifests"},
 		Delivery: config.DeliveryConfig{AppName: "web-ui", AppRepoURL: "https://git.example.test/apps/web-ui.git", AppRepoRef: "main", ProductionBranch: "production", ImageRepository: "registry.example.test/apps/web-ui", BuildStrategy: "static", BuildOutputDirectory: "dist/web-ui", BuildConfiguration: "production"},
 	}
 	content, err := renderAppDelivery(cfg)
@@ -123,12 +123,12 @@ func TestGeneratedDeliveryUsesCombinedStaticBuildArtifactContract(t *testing.T)
 			t.Fatalf("generated delivery does not contain %q", expected)
 		}
 	}
-	for _, expected := range []string{"name: platform-url\n      default: \"https://git.example.test/apps/web-ui.git\"", "name: platform-revision\n      default: \"maidn/platform-web-ui\"", "git clone \"$PLATFORM_URL\" /tmp/platform", "/tmp/platform/preview/values.yaml"} {
+	for _, expected := range []string{"reconcileStrategy: Revision", "valuesFiles:\n                - ./charts/$APP_NAME/values.yaml\n                - ./preview/values.yaml", "valuesFiles:\n                - ./charts/$APP_NAME/values.yaml\n                - ./staging/values.yaml", "valuesFiles:\n                - ./charts/$APP_NAME/values.yaml\n                - ./production/values.yaml", "hostname: $APP_NAME-pr-$PR_NUMBER.example.test"} {
 		if !strings.Contains(string(content), expected) {
-			t.Fatalf("generated delivery does not contain protected platform input %q", expected)
+			t.Fatalf("generated delivery does not contain protected platform values %q", expected)
 		}
 	}
-	for _, unexpected := range []string{"maidn-git-clone", "maidn-node-static-build", "maidn-node-static-push", "runAfter: [clone]", "name: source", "workspace: source", "workspace: artifact", "easycsr", "test-org", "git rm -r", "$APP_URL", "$APP_REVISION", "/tmp/app/preview/values.yaml"} {
+	for _, unexpected := range []string{"maidn-git-clone", "maidn-node-static-build", "maidn-node-static-push", "runAfter: [clone]", "name: source", "workspace: source", "workspace: artifact", "easycsr", "test-org", "git rm -r", "$APP_URL", "$APP_REVISION", "platform-url", "platform-revision", "/tmp/platform", "/tmp/app/preview/values.yaml"} {
 		if strings.Contains(string(content), unexpected) {
 			t.Fatalf("generated delivery contains unexpected %q", unexpected)
 		}
@@ -138,7 +138,7 @@ func TestGeneratedDeliveryUsesCombinedStaticBuildArtifactContract(t *testing.T)
 func TestGeneratedDeliveryUsesRuntimeImageWithoutStaticParameters(t *testing.T) {
 	cfg := config.Config{
 		Git:      config.GitConfig{BaseURL: "https://git.example.test", Owner: "platform"},
-		Flux:     config.FluxConfig{Branch: "main", ManifestsRepo: "manifests"},
+		Flux:     config.FluxConfig{Branch: "main", ClusterDomain: "example.test", ManifestsRepo: "manifests"},
 		Delivery: config.DeliveryConfig{AppName: "web-ui", AppRepoURL: "https://git.example.test/apps/web-ui.git", AppRepoRef: "main", ProductionBranch: "production", ImageRepository: "registry.example.test/apps/web-ui", BuildStrategy: "runtime", BuildOutputDirectory: "dist/web-ui", BuildConfiguration: "production"},
 	}
 	content, err := renderAppDelivery(cfg)
@@ -160,7 +160,7 @@ func TestGeneratedDeliveryUsesRuntimeImageWithoutStaticParameters(t *testing.T)
 func TestGeneratedDeliveryInitializesStagingAndPromotesByPullRequest(t *testing.T) {
 	cfg := config.Config{
 		Git:      config.GitConfig{BaseURL: "https://git.example.test", Owner: "platform"},
-		Flux:     config.FluxConfig{Branch: "main", ManifestsRepo: "manifests"},
+		Flux:     config.FluxConfig{Branch: "main", ClusterDomain: "example.test", ManifestsRepo: "manifests"},
 		Delivery: config.DeliveryConfig{AppName: "web-ui", AppRepoURL: "https://git.example.test/apps/web-ui.git", AppRepoRef: "main", ProductionBranch: "production", ImageRepository: "registry.example.test/apps/web-ui", BuildStrategy: "static", BuildOutputDirectory: "dist", BuildConfiguration: "production"},
 	}
 	content, err := renderAppDelivery(cfg)

internal/bootstrap/onboard.go

@@ -43,6 +43,10 @@ func OnboardApp(cfg config.Config, sourceDir string) error {
 	if sourceBranch != resolved.Delivery.AppRepoRef {
 		return errors.New("--from branch must match delivery appRepoRef")
 	}
+	sourceRevision, err := forgejo.BranchRevision(sourceDir, sourceBranch)
+	if err != nil {
+		return err
+	}
 	owner, repository, err := forgejo.RepositoryFromURL(resolved.Delivery.AppRepoURL)
 	if err != nil {
 		return err
@@ -51,7 +55,7 @@ func OnboardApp(cfg config.Config, sourceDir string) error {
 	if _, err := sourceManager.EnsureRepository(repository, "Application build input for Maidn CI/CD"); err != nil {
 		return err
 	}
-	if err := publishInitialAppBranches(sourceManager, sourceDir, resolved.Delivery.AppRepoURL, sourceBranch, resolved.Delivery.AppRepoRef, resolved.Delivery.ProductionBranch); err != nil {
+	if err := publishInitialAppBranches(sourceManager, sourceDir, resolved.Delivery.AppRepoURL, sourceRevision, resolved.Delivery.AppRepoRef, resolved.Delivery.ProductionBranch); err != nil {
 		return err
 	}
 	if err := sourceManager.EnsureProtectedBranch(repository, resolved.Delivery.ProductionBranch); err != nil {
@@ -60,7 +64,7 @@ func OnboardApp(cfg config.Config, sourceDir string) error {
 	if err := ensurePlatformBranch(sourceManager, repository, resolved.Delivery.AppRepoURL, resolved.Delivery.AppName); err != nil {
 		return err
 	}
-	registrationBranch := "maidn/register-" + resolved.Delivery.AppName
+	registrationBranch := registrationBranch(resolved.Delivery.AppName, sourceRevision)
 	clusterManager := newOnboardingRepoManager(resolved.Git.BaseURL, resolved.Git.Token, resolved.Git.Owner, resolved.Git.Username, "", "", resolved.Flux.Branch, "")
 	if _, err := clusterManager.PublishRepositoryPullRequest(resolved.Flux.RepoName, "feat: register "+resolved.Delivery.AppName+" delivery", registrationBranch, resolved.Flux.Branch, func(dir string) error {
 		return registerAppInCluster(dir, resolved)
@@ -86,6 +90,10 @@ func OnboardApp(cfg config.Config, sourceDir string) error {
 	return nil
 }
 
+func registrationBranch(appName, revision string) string {
+	return "maidn/register-" + appName + "-" + revision[:12]
+}
+
 func platformBranch(appName string) string {
 	return "maidn/platform-" + appName
 }
@@ -106,11 +114,7 @@ func ensurePlatformBranch(manager onboardingRepoManager, repository, repositoryU
 }
 
 // publishInitialAppBranches establishes the immutable source baseline before central registration.
-func publishInitialAppBranches(manager onboardingRepoManager, sourceDir, targetURL, sourceBranch, targetBranch, productionBranch string) error {
-	sourceRevision, err := forgejo.BranchRevision(sourceDir, sourceBranch)
-	if err != nil {
-		return err
-	}
+func publishInitialAppBranches(manager onboardingRepoManager, sourceDir, targetURL, sourceRevision, targetBranch, productionBranch string) error {
 	mainRevision, err := manager.RemoteBranchRevision(targetURL, targetBranch)
 	if err != nil {
 		return fmt.Errorf("read target base branch: %w", err)

internal/bootstrap/onboard_test.go

@@ -112,7 +112,7 @@ func TestPublishInitialAppBranchesCreatesAndPreservesProduction(t *testing.T) {
 	onboardingGit(t, "", "init", "--bare", target)
 
 	manager := forgejo.NewRepoManager("https://git.example.test", "", "owner", "", "", "", "main", "")
-	if err := publishInitialAppBranches(manager, source, target, "source", "main", "production"); err != nil {
+	if err := publishInitialAppBranches(manager, source, target, sourceRevision, "main", "production"); err != nil {
 		t.Fatal(err)
 	}
 	for _, branch := range []string{"main", "production"} {
@@ -139,7 +139,7 @@ func TestPublishInitialAppBranchesCreatesAndPreservesProduction(t *testing.T) {
 	existingProduction := onboardingGit(t, production, "rev-parse", "production")
 	onboardingGit(t, production, "push", preservedTarget, "production:production")
 
-	if err := publishInitialAppBranches(manager, source, preservedTarget, "source", "main", "production"); err != nil {
+	if err := publishInitialAppBranches(manager, source, preservedTarget, sourceRevision, "main", "production"); err != nil {
 		t.Fatal(err)
 	}
 	if got := onboardingGit(t, "", "--git-dir", preservedTarget, "rev-parse", "refs/heads/production"); got != existingProduction {
@@ -149,7 +149,7 @@ func TestPublishInitialAppBranchesCreatesAndPreservesProduction(t *testing.T) {
 		t.Fatal(err)
 	}
 	onboardingGit(t, source, "commit", "-am", "updated source")
-	if err := publishInitialAppBranches(manager, source, preservedTarget, "source", "main", "production"); err != nil {
+	if err := publishInitialAppBranches(manager, source, preservedTarget, sourceRevision, "main", "production"); err != nil {
 		t.Fatal(err)
 	}
 	if got := onboardingGit(t, "", "--git-dir", preservedTarget, "rev-parse", "refs/heads/main"); got != sourceRevision {
@@ -269,7 +269,8 @@ func TestOnboardAppUsesCanonicalSourceAndCentralClusterManagers(t *testing.T) {
 	if strings.Contains(sourceCalls, "delivery") || strings.Contains(sourceCalls, "ensure-pr") || strings.Contains(sourceCalls, "merge-pr") {
 		t.Fatalf("source manager published a delivery change: %q", sourceCalls)
 	}
-	if got := strings.Join(clusterManager.calls, "\n"); got != "register test-org-2/cluster:maidn/register-web-ui" {
+	sourceRevision := onboardingGit(t, source, "rev-parse", "main")
+	if got := strings.Join(clusterManager.calls, "\n"); got != "register test-org-2/cluster:maidn/register-web-ui-"+sourceRevision[:12] {
 		t.Fatalf("cluster manager calls = %q", got)
 	}
 }

internal/forgejo/repo.go

@@ -97,6 +97,7 @@ type accessToken struct {
 }
 
 var copyGit = runGit
+var hasRemoteBranch = (*RepoManager).HasRemoteBranch
 
 func (e *APIError) Error() string {
 	return fmt.Sprintf("forgejo returned %s", e.Status)
@@ -515,10 +516,19 @@ func (rm *RepoManager) PublishRepositoryPullRequest(repo, title, branch, base st
 		return false, errors.New("repository pull request requires distinct non-empty branches")
 	}
 	repoURL := CloneURL(rm.BaseURL, rm.Owner, repo)
-	hasBranch, err := rm.HasRemoteBranch(repoURL, branch)
+	hasBranch, err := hasRemoteBranch(rm, repoURL, branch)
 	if err != nil {
 		return false, err
 	}
+	if hasBranch {
+		open, err := rm.HasOpenPullRequest(repo, branch)
+		if err != nil {
+			return false, err
+		}
+		if !open {
+			return false, fmt.Errorf("Forgejo branch %q exists without exactly one open pull request; refusing to reuse it", branch)
+		}
+	}
 	temporary, err := os.MkdirTemp("", "maidn-registration-*")
 	if err != nil {
 		return false, err

internal/forgejo/repo_test.go

@@ -400,6 +400,34 @@ func TestHasRemoteBranchReturnsFalseForMissingBranch(t *testing.T) {
 	}
 }
 
+func TestPublishRepositoryPullRequestRejectsExistingBranchWithoutOpenPullRequest(t *testing.T) {
+	original := hasRemoteBranch
+	t.Cleanup(func() { hasRemoteBranch = original })
+	server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
+		if request.Method != http.MethodGet || request.URL.Path != "/api/v1/repos/owner/cluster/pulls" || request.URL.Query().Get("state") != "open" || request.URL.Query().Get("head") != "maidn/register-web-ui-deadbeefcafe" {
+			t.Fatalf("unexpected pull request lookup: %s %s", request.Method, request.URL.String())
+		}
+		_ = json.NewEncoder(writer).Encode([]pullRequest{})
+	}))
+	defer server.Close()
+	manager := NewRepoManager(server.URL, "test-token", "owner", "user", "", "", "main", "")
+	manager.HTTPClient = server.Client()
+	hasRemoteBranch = func(got *RepoManager, repoURL, branch string) (bool, error) {
+		if got != manager || repoURL != CloneURL(server.URL, "owner", "cluster") || branch != "maidn/register-web-ui-deadbeefcafe" {
+			t.Fatalf("unexpected remote branch lookup: %q %q", repoURL, branch)
+		}
+		return true, nil
+	}
+
+	_, err := manager.PublishRepositoryPullRequest("cluster", "register web-ui", "maidn/register-web-ui-deadbeefcafe", "main", func(string) error {
+		t.Fatal("change ran for a stale registration branch")
+		return nil
+	})
+	if err == nil || !strings.Contains(err.Error(), "without exactly one open pull request") {
+		t.Fatalf("PublishRepositoryPullRequest() error = %v", err)
+	}
+}
+
 func TestMergePullRequest(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
 		switch request.Method {