From 9908029629e236f5278e005eca06363f44e26076 Mon Sep 17 00:00:00 2001 From: eding Date: Wed, 16 Sep 2026 20:26:43 +0200 Subject: [PATCH] fix: avoid stale onboarding branches --- internal/bootstrap/onboard.go | 18 +++++++++++------- internal/bootstrap/onboard_test.go | 9 +++++---- internal/forgejo/repo.go | 12 +++++++++++- internal/forgejo/repo_test.go | 28 ++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 12 deletions(-) diff --git a/internal/bootstrap/onboard.go b/internal/bootstrap/onboard.go index 227e9c0..0bf458d 100644 --- a/internal/bootstrap/onboard.go +++ b/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) diff --git a/internal/bootstrap/onboard_test.go b/internal/bootstrap/onboard_test.go index f68036e..c0f251d 100644 --- a/internal/bootstrap/onboard_test.go +++ b/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) } } diff --git a/internal/forgejo/repo.go b/internal/forgejo/repo.go index be10d33..ea31315 100644 --- a/internal/forgejo/repo.go +++ b/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 diff --git a/internal/forgejo/repo_test.go b/internal/forgejo/repo_test.go index cf6e77d..86192d1 100644 --- a/internal/forgejo/repo_test.go +++ b/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 {