diff --git a/e2e/admin/admin_test.go b/e2e/admin/admin_test.go index 525b792921..d3b9a77d3c 100644 --- a/e2e/admin/admin_test.go +++ b/e2e/admin/admin_test.go @@ -826,7 +826,7 @@ func runUnenrollmentTest(t *testing.T, env *e2eEnv) { // watches the repo-maintenance workflow to completion before returning, // so the removal PR should already exist when this returns. output := runCLI(t, env.binary, env.token, - "admin", "disable", "repos", env.org, testRepo, "--yolo") + "admin", "disable", "repos", env.org, testRepo, "--yolo", "--direct") t.Logf("Disable repos output:\n%s", output) // Always capture the repo-maintenance run's logs. Even when the run diff --git a/internal/cli/admin.go b/internal/cli/admin.go index 2d3add2a70..13686920ac 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -2144,13 +2144,16 @@ func newDisableCmd() *cobra.Command { } // reposRunFunc is the signature for repo enable/disable operations. -type reposRunFunc func(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool) error +type reposRunFunc func(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool, pr bool) error // newReposSubcommand creates a repos enable or disable subcommand with shared setup logic. // If withYolo is true, the --yolo flag is added to skip confirmation prompts. +// By default, changes are delivered via a pull request. Use --direct to push +// changes directly to the default branch instead. func newReposSubcommand(use, short, long, allFlagHelp string, runFn reposRunFunc, withYolo bool) *cobra.Command { var all bool var yolo bool + var directFlag bool cmd := &cobra.Command{ Use: use, @@ -2186,11 +2189,15 @@ func newReposSubcommand(use, short, long, allFlagHelp string, runFn reposRunFunc printer := ui.New(os.Stdout) ctx := cmd.Context() - return runFn(ctx, client, printer, org, repos, all, yolo) + // Default is PR delivery; --direct overrides to direct push. + usePR := !directFlag + + return runFn(ctx, client, printer, org, repos, all, yolo, usePR) }, } cmd.Flags().BoolVar(&all, "all", false, allFlagHelp) + cmd.Flags().BoolVar(&directFlag, "direct", false, "push changes directly to the default branch instead of creating a PR") if withYolo { cmd.Flags().BoolVar(&yolo, "yolo", false, "skip confirmation prompt") } @@ -2223,7 +2230,7 @@ func newDisableReposCmd() *cobra.Command { // runEnableRepos enables the specified repositories for fullsend enrollment. // The yolo parameter is accepted for signature compatibility with reposRunFunc but is unused // since enable has no destructive operations that require confirmation. -func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool) error { +func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool, pr bool) error { printer.Banner(Version()) printer.Blank() printer.Header("Enabling repositories for " + org) @@ -2326,7 +2333,7 @@ func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printe // Save updated config. commitMsg := fmt.Sprintf("chore: enable %d repositories for fullsend enrollment", changed) var err error - dispatchTime, err = saveRepoConfig(ctx, client, printer, org, cfg, commitMsg) + dispatchTime, err = saveRepoConfig(ctx, client, printer, org, cfg, commitMsg, pr) if err != nil { return err } @@ -2335,7 +2342,8 @@ func runEnableRepos(ctx context.Context, client forge.Client, printer *ui.Printe // Sync org variable visibility so enrolled repos can read dispatch // variables like FULLSEND_MINT_URL. Runs even when changed == 0 to // reconcile a previously failed best-effort sync on re-run. - if cfg.Dispatch.Mode == "oidc-mint" { + // Skipped in PR mode — repo-maintenance reconciles on merge. + if cfg.Dispatch.Mode == "oidc-mint" && !pr { syncOrgVariableVisibility(ctx, client, printer, org, cfg, allOrgRepos) } @@ -2403,7 +2411,7 @@ func syncOrgVariableVisibility(ctx context.Context, client forge.Client, printer } // runDisableRepos disables the specified repositories from fullsend enrollment. -func runDisableRepos(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool) error { +func runDisableRepos(ctx context.Context, client forge.Client, printer *ui.Printer, org string, repos []string, all bool, yolo bool, pr bool) error { printer.Banner(Version()) printer.Blank() printer.Header("Disabling repositories for " + org) @@ -2491,13 +2499,14 @@ func runDisableRepos(ctx context.Context, client forge.Client, printer *ui.Print // Save updated config. commitMsg := fmt.Sprintf("chore: disable %d repositories from fullsend enrollment", changed) - dispatchTime, err := saveRepoConfig(ctx, client, printer, org, cfg, commitMsg) + dispatchTime, err := saveRepoConfig(ctx, client, printer, org, cfg, commitMsg, pr) if err != nil { return err } // Sync org variable visibility to revoke access for disabled repos. - if cfg.Dispatch.Mode == "oidc-mint" { + // Skipped in PR mode — repo-maintenance reconciles on merge. + if cfg.Dispatch.Mode == "oidc-mint" && !pr { allOrgRepos, listErr := client.ListOrgRepos(ctx, org) if listErr != nil { printer.StepWarn(fmt.Sprintf("could not list org repos for variable sync: %v", listErr)) @@ -2561,11 +2570,14 @@ func loadRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printe return cfg, nil } -// saveRepoConfig marshals and commits the updated config, then triggers the repo-maintenance workflow. // saveRepoConfig marshals the config, commits it, and dispatches the // repo-maintenance workflow. It returns the dispatch time so callers can // watch the resulting workflow run. A zero time means the dispatch failed. -func saveRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printer, org string, cfg *config.OrgConfig, commitMsg string) (time.Time, error) { +// +// When pr is true, config.yaml is delivered via a pull request instead of +// being pushed directly to the default branch. The repo-maintenance +// workflow is not dispatched in PR mode — it will run when the PR is merged. +func saveRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printer, org string, cfg *config.OrgConfig, commitMsg string, pr bool) (time.Time, error) { // Marshal updated config. updatedConfigData, err := cfg.Marshal() if err != nil { @@ -2573,6 +2585,10 @@ func saveRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printe return time.Time{}, fmt.Errorf("marshaling config.yaml: %w", err) } + if pr { + return saveRepoConfigViaPR(ctx, client, printer, org, updatedConfigData, commitMsg) + } + // Commit and push changes. printer.StepStart("Committing changes to .fullsend") if err := client.CreateOrUpdateFile(ctx, org, forge.ConfigRepoName, "config.yaml", commitMsg, updatedConfigData); err != nil { @@ -2596,6 +2612,36 @@ func saveRepoConfig(ctx context.Context, client forge.Client, printer *ui.Printe return dispatchTime, nil } +// saveRepoConfigViaPR delivers config.yaml via a pull request on a dedicated +// branch, separate from the scaffold-install branch used by sync-scaffold. +func saveRepoConfigViaPR(ctx context.Context, client forge.Client, printer *ui.Printer, org string, configData []byte, commitMsg string) (time.Time, error) { + cfgRepo, err := client.GetRepo(ctx, org, forge.ConfigRepoName) + if err != nil { + printer.StepFail("Failed to get .fullsend repo info") + return time.Time{}, fmt.Errorf("getting config repo info: %w", err) + } + + files := []forge.TreeFile{{ + Path: "config.yaml", + Content: configData, + Mode: "100644", + }} + + prBody := "This PR updates `config.yaml` in the .fullsend config repo.\n\n" + + "Merge this PR to apply the enrollment changes. The repo-maintenance workflow will run automatically on merge." + + _, prErr := layers.CommitFilesViaPR(ctx, client, printer, + org, forge.ConfigRepoName, cfgRepo.DefaultBranch, + "fullsend/enrollment-config", + commitMsg, commitMsg, prBody, files) + if prErr != nil { + return time.Time{}, prErr + } + + // No workflow dispatch in PR mode — repo-maintenance runs on merge. + return time.Time{}, nil +} + // awaitRepoMaintenance watches the repo-maintenance workflow run triggered by a // config.yaml push, waits for it to complete, and prints any PR URLs from its // annotations. diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index 83b69b1792..234ccd6ba3 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -539,7 +539,7 @@ func TestReposEnableCmd_AllIgnoresPositionalArgs(t *testing.T) { printer := ui.New(&discardWriter{}) // Pass "web-app" as a positional arg, but --all should ignore it and enable both repos - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, true, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, true, true, false) require.NoError(t, err) // Verify both repos were enabled (--all behavior), not just web-app @@ -560,7 +560,7 @@ func TestReposDisableCmd_AllIgnoresPositionalArgs(t *testing.T) { printer := ui.New(&discardWriter{}) // Pass "web-app" as a positional arg, but --all should ignore it and disable both repos - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, true, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, true, true, false) require.NoError(t, err) // Verify both repos were disabled (--all behavior), not just web-app @@ -619,7 +619,7 @@ func TestRunEnableRepos_EnableSingleRepo(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // Verify config was updated. @@ -639,7 +639,7 @@ func TestRunEnableRepos_EnableMultipleRepos(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api", "docs"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "docs"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "docs"}, false, true, false) require.NoError(t, err) // Verify config was updated. @@ -659,7 +659,7 @@ func TestRunEnableRepos_EnableAllRepos(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api", "new-repo"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", nil, true, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", nil, true, true, false) require.NoError(t, err) // Verify all repos were enabled (excluding .fullsend). @@ -681,7 +681,7 @@ func TestRunEnableRepos_NoOpWhenAlreadyEnabled(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // Verify no file was created (no changes). @@ -692,7 +692,7 @@ func TestRunEnableRepos_ErrorWhenFullsendRepoMissing(t *testing.T) { client := forge.NewFakeClient() printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), ".fullsend repository not found") } @@ -701,7 +701,7 @@ func TestRunEnableRepos_ErrorWhenConfigMissing(t *testing.T) { client := setupTestClient("testorg", nil, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), "reading config.yaml") } @@ -713,7 +713,7 @@ func TestRunEnableRepos_ErrorWhenEnablingFullsend(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{".fullsend"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{".fullsend"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), "cannot enable .fullsend repository") } @@ -725,7 +725,7 @@ func TestRunEnableRepos_ErrorWhenRepoNotFound(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"nonexistent"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"nonexistent"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), "repository nonexistent not found") } @@ -738,7 +738,7 @@ func TestRunEnableRepos_CommitMessageFormat(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "api"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "api"}, false, true, false) require.NoError(t, err) require.Len(t, client.CreatedFiles, 1) @@ -773,7 +773,7 @@ func TestRunEnableRepos_UpdatesOrgVariableVisibility(t *testing.T) { printer := ui.New(&discardWriter{}) // Action: enable repo "api". - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"api"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"api"}, false, true, false) require.NoError(t, err) // Assert: SetOrgVariableRepos was called with both enrolled repo IDs @@ -797,7 +797,7 @@ func TestRunEnableRepos_SkipsVariableSyncWhenNotOIDCMint(t *testing.T) { printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // SetOrgVariableRepos should not have been called. @@ -826,7 +826,7 @@ func TestRunEnableRepos_VariableSyncErrorDoesNotBlockEnable(t *testing.T) { printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err, "enable should succeed even when variable sync fails") } @@ -843,7 +843,7 @@ func TestRunEnableRepos_SkipsVariableSyncWhenVariableNotExists(t *testing.T) { printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // SetOrgVariableRepos should not have been called. @@ -860,7 +860,7 @@ func TestRunDisableRepos_DisableSingleRepo(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // Verify config was updated. @@ -880,7 +880,7 @@ func TestRunDisableRepos_DisableMultipleRepos(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api", "docs"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "docs"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "docs"}, false, true, false) require.NoError(t, err) // Verify config was updated. @@ -900,7 +900,7 @@ func TestRunDisableRepos_DisableAllRepos(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", nil, true, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", nil, true, true, false) require.NoError(t, err) // Verify all repos were disabled. @@ -918,7 +918,7 @@ func TestRunDisableRepos_NoOpWhenAlreadyDisabled(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) // Verify no file was created (no changes). @@ -929,7 +929,7 @@ func TestRunDisableRepos_ErrorWhenFullsendRepoMissing(t *testing.T) { client := forge.NewFakeClient() printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), ".fullsend repository not found") } @@ -938,7 +938,7 @@ func TestRunDisableRepos_ErrorWhenConfigMissing(t *testing.T) { client := setupTestClient("testorg", nil, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), "reading config.yaml") } @@ -950,7 +950,7 @@ func TestRunDisableRepos_ErrorWhenDisablingFullsend(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{".fullsend"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{".fullsend"}, false, true, false) require.Error(t, err) assert.Contains(t, err.Error(), "cannot disable .fullsend repository") } @@ -963,7 +963,7 @@ func TestRunDisableRepos_AllowsRepoNotInConfig(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"nonexistent"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"nonexistent"}, false, true, false) require.NoError(t, err) // Should succeed but make no changes (repo not in config, nothing to disable) assert.Len(t, client.CreatedFiles, 0) @@ -977,13 +977,68 @@ func TestRunDisableRepos_CommitMessageFormat(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "api"}, false, true) + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app", "api"}, false, true, false) require.NoError(t, err) require.Len(t, client.CreatedFiles, 1) assert.Contains(t, client.CreatedFiles[0].Message, "chore: disable 2 repositories") } +func TestRunEnableRepos_PRDelivery(t *testing.T) { + cfg := setupTestConfig(map[string]bool{ + "web-app": false, + "api": false, + }) + client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) + printer := ui.New(&discardWriter{}) + + // pr=true should create a branch and PR instead of pushing directly. + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, true) + require.NoError(t, err) + + // PR mode: should create a branch and proposal, not use CreateOrUpdateFile. + assert.NotEmpty(t, client.CreatedBranches, "expected a branch to be created for PR delivery") + assert.NotEmpty(t, client.CreatedProposals, "expected a PR to be created") + // CreatedFiles records CreateOrUpdateFile calls (direct commits). + // In PR mode these should be empty. + assert.Empty(t, client.CreatedFiles, "expected no direct file commits in PR mode") +} + +func TestRunDisableRepos_PRDelivery(t *testing.T) { + cfg := setupTestConfig(map[string]bool{ + "web-app": true, + "api": true, + }) + client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) + printer := ui.New(&discardWriter{}) + + // pr=true should create a branch and PR instead of pushing directly. + err := runDisableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, true) + require.NoError(t, err) + + // PR mode: should create a branch and proposal, not use CreateOrUpdateFile. + assert.NotEmpty(t, client.CreatedBranches, "expected a branch to be created for PR delivery") + assert.NotEmpty(t, client.CreatedProposals, "expected a PR to be created") + assert.Empty(t, client.CreatedFiles, "expected no direct file commits in PR mode") +} + +func TestReposEnableCmd_HasDirectFlag(t *testing.T) { + cmd := newEnableReposCmd() + directFlag := cmd.Flags().Lookup("direct") + require.NotNil(t, directFlag, "expected --direct flag") + assert.Equal(t, "false", directFlag.DefValue) + // --pr flag should not exist; PR delivery is the default. + assert.Nil(t, cmd.Flags().Lookup("pr"), "unexpected --pr flag; PR delivery is the default") +} + +func TestReposDisableCmd_HasDirectFlag(t *testing.T) { + cmd := newDisableReposCmd() + directFlag := cmd.Flags().Lookup("direct") + require.NotNil(t, directFlag, "expected --direct flag") + // --pr flag should not exist; PR delivery is the default. + assert.Nil(t, cmd.Flags().Lookup("pr"), "unexpected --pr flag; PR delivery is the default") +} + func TestPromptEnrollment_ChooseAll(t *testing.T) { tests := []struct { name string @@ -2612,7 +2667,7 @@ func TestApplyPerRepoScaffold_ProtectedBranch_ScaffoldBranchAlsoProtected(t *tes err := applyPerRepoScaffold(context.Background(), client, printer, "acme", "widget", files, nil, nil, true) require.Error(t, err) - assert.Contains(t, err.Error(), "scaffold branch") + assert.Contains(t, err.Error(), "is protected") assert.Contains(t, err.Error(), "configure branch protection") } diff --git a/internal/cli/github.go b/internal/cli/github.go index b4d4e0eaa4..b40bdb18ab 100644 --- a/internal/cli/github.go +++ b/internal/cli/github.go @@ -929,10 +929,12 @@ func runGitHubUninstall(ctx context.Context, client forge.Client, printer *ui.Pr // --- sync-scaffold command --- func newGitHubSyncScaffoldCmd() *cobra.Command { + var directFlag bool + cmd := &cobra.Command{ Use: "sync-scaffold ", Short: "Update workflow templates in .fullsend", - Long: "Re-commits scaffold files (shim and maintenance workflows) to the .fullsend repo without touching secrets, variables, or enrollment. Useful after fullsend version upgrades. Idempotent and safe to run repeatedly.", + Long: "Re-commits scaffold files (shim and maintenance workflows) to the .fullsend repo without touching secrets, variables, or enrollment. Useful after fullsend version upgrades. Idempotent and safe to run repeatedly.\n\nBy default, changes are delivered via a pull request. Use --direct to push to the default branch instead.", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { org := args[0] @@ -948,15 +950,18 @@ func newGitHubSyncScaffoldCmd() *cobra.Command { client := gh.New(token) printer := ui.New(os.Stdout) - return runGitHubSyncScaffold(cmd.Context(), client, printer, org) + // Default is PR delivery; --direct overrides to direct push. + return runGitHubSyncScaffold(cmd.Context(), client, printer, org, directFlag) }, } + cmd.Flags().BoolVar(&directFlag, "direct", false, "push scaffold files directly to the default branch instead of creating a PR") + return cmd } // runGitHubSyncScaffold runs only the WorkflowsLayer. -func runGitHubSyncScaffold(ctx context.Context, client forge.Client, printer *ui.Printer, org string) error { +func runGitHubSyncScaffold(ctx context.Context, client forge.Client, printer *ui.Printer, org string, direct bool) error { printer.Banner(Version()) printer.Blank() printer.Header("Syncing scaffold for " + org) @@ -983,7 +988,7 @@ func runGitHubSyncScaffold(ctx context.Context, client forge.Client, printer *ui } upstreamRef, upstreamTag := resolveUpstreamRef() - wfLayer := layers.NewWorkflowsLayer(org, client, printer, user, version, vendored).WithDirect(true).WithUpstreamRef(upstreamRef, upstreamTag) + wfLayer := layers.NewWorkflowsLayer(org, client, printer, user, version, vendored).WithDirect(direct).WithUpstreamRef(upstreamRef, upstreamTag) if id, idErr := client.GetAuthenticatedUserIdentity(ctx); idErr == nil { wfLayer = wfLayer.WithSignOff(id.Name, id.Email) } diff --git a/internal/cli/github_test.go b/internal/cli/github_test.go index 7f19e9362b..ecc181a893 100644 --- a/internal/cli/github_test.go +++ b/internal/cli/github_test.go @@ -237,7 +237,7 @@ func TestGitHubEnrollCmd_DelegatesCorrectly(t *testing.T) { client := setupTestClient("testorg", cfg, []string{"web-app", "api"}) printer := ui.New(&discardWriter{}) - err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true) + err := runEnableRepos(context.Background(), client, printer, "testorg", []string{"web-app"}, false, true, false) require.NoError(t, err) require.Len(t, client.CreatedFiles, 1) @@ -544,7 +544,7 @@ func TestRunGitHubSyncScaffold_CommitsFiles(t *testing.T) { client.AuthenticatedUser = "testuser" printer := ui.New(&discardWriter{}) - err := runGitHubSyncScaffold(context.Background(), client, printer, "acme") + err := runGitHubSyncScaffold(context.Background(), client, printer, "acme", true) require.NoError(t, err) // sync-scaffold uses direct mode — files are committed to the default branch. @@ -563,7 +563,7 @@ func TestRunGitHubSyncScaffold_VendoredMarker(t *testing.T) { } printer := ui.New(&discardWriter{}) - err := runGitHubSyncScaffold(context.Background(), client, printer, "acme") + err := runGitHubSyncScaffold(context.Background(), client, printer, "acme", true) require.NoError(t, err) require.NotEmpty(t, client.CommittedFiles) } @@ -577,11 +577,55 @@ func TestRunGitHubSyncScaffold_InvalidConfig(t *testing.T) { } printer := ui.New(&discardWriter{}) - err := runGitHubSyncScaffold(context.Background(), client, printer, "acme") + err := runGitHubSyncScaffold(context.Background(), client, printer, "acme", true) require.Error(t, err) assert.Contains(t, err.Error(), "parsing config.yaml") } +func TestRunGitHubSyncScaffold_DefaultCreatesPR(t *testing.T) { + client := forge.NewFakeClient() + client.Repos = []forge.Repository{ + {Name: ".fullsend", FullName: "acme/.fullsend", DefaultBranch: "main"}, + } + client.AuthenticatedUser = "acme" + printer := ui.New(&discardWriter{}) + + // direct=false means PR-based delivery (the default). + err := runGitHubSyncScaffold(context.Background(), client, printer, "acme", false) + require.NoError(t, err) + + // Should create a branch and PR, not commit directly. + assert.NotEmpty(t, client.CreatedBranches, "expected a scaffold branch to be created") + assert.NotEmpty(t, client.CreatedProposals, "expected a scaffold PR to be created") + assert.Empty(t, client.CommittedFiles, "expected no direct commits when using PR delivery") +} + +func TestGitHubSyncScaffoldCmd_HasDirectFlag(t *testing.T) { + cmd := newGitHubSyncScaffoldCmd() + directFlag := cmd.Flags().Lookup("direct") + require.NotNil(t, directFlag, "expected --direct flag") + assert.Equal(t, "false", directFlag.DefValue) + // --pr flag should not exist; PR delivery is the default. + assert.Nil(t, cmd.Flags().Lookup("pr"), "unexpected --pr flag; PR delivery is the default") +} + +func TestGitHubEnrollCmd_HasDirectFlag(t *testing.T) { + cmd := newGitHubEnrollCmd() + directFlag := cmd.Flags().Lookup("direct") + require.NotNil(t, directFlag, "expected --direct flag") + assert.Equal(t, "false", directFlag.DefValue) + // --pr flag should not exist; PR delivery is the default. + assert.Nil(t, cmd.Flags().Lookup("pr"), "unexpected --pr flag; PR delivery is the default") +} + +func TestGitHubUnenrollCmd_HasDirectFlag(t *testing.T) { + cmd := newGitHubUnenrollCmd() + directFlag := cmd.Flags().Lookup("direct") + require.NotNil(t, directFlag, "expected --direct flag") + // --pr flag should not exist; PR delivery is the default. + assert.Nil(t, cmd.Flags().Lookup("pr"), "unexpected --pr flag; PR delivery is the default") +} + func TestRunGitHubSetupPerOrg_DryRun(t *testing.T) { client := forge.NewFakeClient() client.AuthenticatedUser = "testuser" diff --git a/internal/layers/commit.go b/internal/layers/commit.go index 3912e772e5..a5ae2e391c 100644 --- a/internal/layers/commit.go +++ b/internal/layers/commit.go @@ -35,6 +35,18 @@ func CommitScaffoldFiles(ctx context.Context, client forge.Client, printer *ui.P owner, repo, defaultBranch, commitMsg, prTitle, prBody, files, in) } +// CommitFilesViaPR delivers files via a pull request on the given branch. +// Uses a fixed branch name so re-runs update the same PR. +func CommitFilesViaPR(ctx context.Context, client forge.Client, printer *ui.Printer, + owner, repo, defaultBranch, branch, commitMsg, prTitle, prBody string, + files []forge.TreeFile) (bool, error) { + + return commitViaPR(ctx, client, printer, + owner, repo, defaultBranch, branch, commitMsg, prTitle, prBody, files) +} + +const defaultScaffoldBranch = "fullsend/scaffold-install" + // commitScaffoldViaPR creates a feature branch, commits files, and opens a PR. // For non-owner users, it defaults to creating a fork and opening a cross-fork // PR rather than pushing directly to the upstream repository. @@ -42,8 +54,6 @@ func commitScaffoldViaPR(ctx context.Context, client forge.Client, printer *ui.P owner, repo, defaultBranch, commitMsg, prTitle, prBody string, files []forge.TreeFile, in io.Reader) (bool, error) { - const scaffoldBranch = "fullsend/scaffold-install" - user, err := client.GetAuthenticatedUser(ctx) if err != nil { return false, fmt.Errorf("getting authenticated user: %w", err) @@ -52,7 +62,7 @@ func commitScaffoldViaPR(ctx context.Context, client forge.Client, printer *ui.P // Owner pushes directly to the repo — no fork needed. if strings.EqualFold(user, owner) { return commitBranchAndPR(ctx, client, printer, - owner, repo, owner, repo, scaffoldBranch, defaultBranch, + owner, repo, owner, repo, defaultScaffoldBranch, defaultBranch, commitMsg, prTitle, prBody, files) } @@ -65,7 +75,7 @@ func commitScaffoldViaPR(ctx context.Context, client forge.Client, printer *ui.P if forkOwner != "" { printer.StepDone(fmt.Sprintf("Using existing fork %s/%s", forkOwner, forkRepo)) return commitViaFork(ctx, client, printer, - owner, repo, forkOwner, forkRepo, scaffoldBranch, defaultBranch, + owner, repo, forkOwner, forkRepo, defaultScaffoldBranch, defaultBranch, commitMsg, prTitle, prBody, files) } @@ -83,13 +93,13 @@ func commitScaffoldViaPR(ctx context.Context, client forge.Client, printer *ui.P if useFork { return forkAndCommit(ctx, client, printer, - owner, repo, scaffoldBranch, defaultBranch, + owner, repo, defaultScaffoldBranch, defaultBranch, commitMsg, prTitle, prBody, files) } // Upstream path: try to push directly, fail clearly on 403. return commitBranchAndPR(ctx, client, printer, - owner, repo, owner, repo, scaffoldBranch, defaultBranch, + owner, repo, owner, repo, defaultScaffoldBranch, defaultBranch, commitMsg, prTitle, prBody, files) } @@ -186,6 +196,59 @@ func commitBranchAndPR(ctx context.Context, client forge.Client, printer *ui.Pri return false, nil } +// commitViaPR creates a feature branch, commits files, and opens a PR. +// Unlike commitBranchAndPR, this is a simpler pathway for same-owner PRs +// (e.g., enrollment config updates) that don't need fork support. +func commitViaPR(ctx context.Context, client forge.Client, printer *ui.Printer, + owner, repo, defaultBranch, branch, commitMsg, prTitle, prBody string, + files []forge.TreeFile) (bool, error) { + + if branchErr := client.CreateBranch(ctx, owner, repo, branch); branchErr != nil { + if forge.IsForbidden(branchErr) { + printer.StepFail("Insufficient permissions to create branch") + return false, fmt.Errorf("cannot push to %s/%s (403 forbidden); check your token scopes: %w", + owner, repo, branchErr) + } + if !forge.IsAlreadyExists(branchErr) { + printer.StepFail("Failed to create feature branch") + return false, fmt.Errorf("creating feature branch: %w", branchErr) + } + } + + branchCommitted, commitErr := client.CommitFilesToBranch(ctx, owner, repo, branch, commitMsg, files) + if commitErr != nil { + if forge.IsBranchProtected(commitErr) { + printer.StepFail("Feature branch is protected — cannot commit") + return false, fmt.Errorf("branch %q is protected; configure branch protection to allow pushes: %w", branch, commitErr) + } + printer.StepFail("Failed to commit files to branch") + return false, fmt.Errorf("committing files to branch: %w", commitErr) + } + + proposal, prErr := client.CreateChangeProposal(ctx, owner, repo, + prTitle, prBody, branch, defaultBranch) + if prErr != nil { + if forge.IsNoChanges(prErr) { + printer.StepDone("Branch and PR up to date") + return false, nil + } + if !forge.IsAlreadyExists(prErr) { + printer.StepFail("Failed to create PR") + return false, fmt.Errorf("creating PR: %w", prErr) + } + if branchCommitted { + printer.StepDone("PR already exists — updated with new files") + printer.StepInfo("Merge the PR to apply changes") + } else { + printer.StepDone("Branch and PR up to date") + } + } else { + printer.StepDone(fmt.Sprintf("Created PR #%d: %s", proposal.Number, proposal.URL)) + printer.StepInfo("Merge the PR to apply changes") + } + return false, nil +} + // waitForFork polls GetRepo until the fork is ready or the timeout expires. // GitHub fork creation is async (202 Accepted) and can take up to several // minutes for large repos. diff --git a/internal/layers/workflows_test.go b/internal/layers/workflows_test.go index 9349609889..12d04fb486 100644 --- a/internal/layers/workflows_test.go +++ b/internal/layers/workflows_test.go @@ -357,7 +357,7 @@ func TestWorkflowsLayer_Install_ProtectedBranch_ScaffoldBranchAlsoProtected(t *t err := layer.Install(context.Background()) require.Error(t, err) - assert.Contains(t, err.Error(), "scaffold branch") + assert.Contains(t, err.Error(), "is protected") assert.Contains(t, err.Error(), "configure branch protection") }