diff --git a/docs/guides/getting-started/repo-management.md b/docs/guides/getting-started/repo-management.md index ba66b8d4b2..7a53ccbb5e 100644 --- a/docs/guides/getting-started/repo-management.md +++ b/docs/guides/getting-started/repo-management.md @@ -82,16 +82,15 @@ repos: forge: gitlab ``` +All repos under the same owner must use the same forge. A GitHub org +and a GitLab group with the same name are different entities, and +mixing forges under one owner would route API calls incorrectly. + For GitLab repos, set the `GITLAB_TOKEN` environment variable or pass `--gitlab-token` to `fullsend repos` subcommands. For self-hosted GitLab instances, set `GITLAB_API_URL` to the API base URL (e.g. `https://gitlab.example.com/api/v4`). -> **Note:** Mixed-forge manifests currently require that all repos share the -> same forge as `defaults.forge`. Per-entry client switching (connecting to -> both GitHub and GitLab APIs in a single run) is planned but not yet -> implemented. - See `fullsend repos init --help` or the [CLI reference](../../cli/repos.md) for all flags. diff --git a/docs/plans/repos-init.md b/docs/plans/repos-init.md index 2260677b5d..76bada4e38 100644 --- a/docs/plans/repos-init.md +++ b/docs/plans/repos-init.md @@ -103,7 +103,7 @@ type InitResult struct { } func Init(ctx context.Context, cfg InitConfig, - client forge.Client, + clients ForgeClientFactory, selectRepos RepoSelectFunc, progress ProgressFunc) (*InitResult, error) ``` diff --git a/docs/plans/repos-management.md b/docs/plans/repos-management.md index bb65b420b1..7222edf01e 100644 --- a/docs/plans/repos-management.md +++ b/docs/plans/repos-management.md @@ -561,7 +561,7 @@ func LoadManifest(pathOrURL string) (*Manifest, error) func (m *Manifest) Validate() error func (m *Manifest) ExpandGlobs(ctx context.Context, - client forge.Client) ([]ResolvedRepo, error) + clients ForgeClientFactory) ([]ResolvedRepo, error) func (m *Manifest) ResolveConfig(owner, repo string) ResolvedConfig ``` @@ -819,7 +819,7 @@ type Drift struct { } func Status(ctx context.Context, manifest *Manifest, - client forge.Client, maxConcurrency int) ([]RepoStatus, error) + clients ForgeClientFactory, maxConcurrency int) ([]RepoStatus, error) ``` Per-repo discovery (parallelizable, read-only): @@ -907,7 +907,7 @@ type BatchInstallResult struct { } func BatchInstall(ctx context.Context, cfg BatchInstallConfig, - client forge.Client, provisionerFactory ProvisionerFactory, + clients ForgeClientFactory, provisionerFactory ProvisionerFactory, progress ProgressFunc) (*BatchInstallResult, error) ``` @@ -1025,10 +1025,10 @@ type Change struct { } func Diff(ctx context.Context, manifest *Manifest, - client forge.Client, maxConcurrency int) ([]Change, error) + clients ForgeClientFactory, maxConcurrency int) ([]Change, error) func Sync(ctx context.Context, manifest *Manifest, - client forge.Client, maxConcurrency int, + clients ForgeClientFactory, maxConcurrency int, progress ProgressFunc) ([]Change, error) ``` @@ -1124,7 +1124,7 @@ type UpgradeResult struct { } func Upgrade(ctx context.Context, cfg UpgradeConfig, - client forge.Client, + clients ForgeClientFactory, progress ProgressFunc) ([]UpgradeResult, error) func UpgradeMint(ctx context.Context, manifest *Manifest, @@ -1264,7 +1264,7 @@ type RemoveResult struct { } func Remove(ctx context.Context, cfg RemoveConfig, - client forge.Client, provisionerFactory ProvisionerFactory, + clients ForgeClientFactory, provisionerFactory ProvisionerFactory, progress ProgressFunc) ([]RemoveResult, error) ``` diff --git a/internal/cli/forge_client.go b/internal/cli/forge_client.go index 9d08bc62e5..d755afb6eb 100644 --- a/internal/cli/forge_client.go +++ b/internal/cli/forge_client.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "strings" + "sync" "github.com/fullsend-ai/fullsend/internal/forge" gl "github.com/fullsend-ai/fullsend/internal/forge/gitlab" @@ -52,18 +53,72 @@ func newForgeClient(forgeName, gitlabToken string) (forge.Client, error) { } } -// forgeClientFromManifest resolves the dominant forge from the manifest -// defaults and returns the appropriate client. For manifests with a -// single forge type, this returns the correct client. Mixed-forge -// manifests (e.g., both GitHub and GitLab entries) are not yet fully -// supported — the default forge client is used and GitLab-specific -// entries will fail with a clear error. -func forgeClientFromManifest(m *repos.Manifest, gitlabToken string) (forge.Client, error) { - forgeName := m.Defaults.Forge +// forgeClientFactory lazily creates and caches per-forge API clients. +// Each client is created on first use and reused for subsequent calls +// with the same forge name. The sync.Mutex protects the client cache +// for concurrent goroutines in per-repo batch loops. +type forgeClientFactory struct { + gitlabToken string + mu sync.Mutex + clients map[string]forge.Client +} + +// newForgeClientFactory returns a ForgeClientFactory that lazily creates +// and caches forge clients. A GitLab token is only resolved if the +// factory is asked for a GitLab client, so single-forge GitHub manifests +// never require GITLAB_TOKEN. +func newForgeClientFactory(gitlabToken string) repos.ForgeClientFactory { + return &forgeClientFactory{ + gitlabToken: gitlabToken, + clients: make(map[string]forge.Client), + } +} + +// ConfigFor returns a ForgeConfig with a live Client for the named forge. +// Clients are created lazily and cached — at most 2 clients per command +// invocation (one GitHub, one GitLab). +func (f *forgeClientFactory) ConfigFor(forgeName string) (repos.ForgeConfig, error) { + f.mu.Lock() + defer f.mu.Unlock() + + // Normalize empty forge name to github (backward compat). + if forgeName == "" { + forgeName = repos.ForgeGitHub + } + + client, ok := f.clients[forgeName] + if !ok { + var err error + client, err = newForgeClient(forgeName, f.gitlabToken) + if err != nil { + return repos.ForgeConfig{}, err + } + f.clients[forgeName] = client + } + + cfg := repos.ForgeConfigFor(forgeName) + cfg.Client = client + return cfg, nil +} + +// singleClientFactory wraps a single forge.Client as a ForgeClientFactory, +// returning the same client for any forge name. Used in tests and CLI +// test-override paths where a single FakeClient backs all operations. +type singleClientFactory struct { + client forge.Client +} + +func newSingleClientFactory(client forge.Client) repos.ForgeClientFactory { + return &singleClientFactory{client: client} +} + +func (f *singleClientFactory) ConfigFor(forgeName string) (repos.ForgeConfig, error) { if forgeName == "" { forgeName = repos.ForgeGitHub } - return newForgeClient(forgeName, gitlabToken) + cfg := repos.ForgeConfigFor(forgeName) + cfg.Client = f.client + return cfg, nil } // getGitLabToken extracts the --gitlab-token flag from the command chain. diff --git a/internal/cli/forge_client_test.go b/internal/cli/forge_client_test.go index dc1dfb58fa..e691586674 100644 --- a/internal/cli/forge_client_test.go +++ b/internal/cli/forge_client_test.go @@ -70,33 +70,27 @@ func TestNewForgeClient_Unsupported(t *testing.T) { assert.Contains(t, err.Error(), "unsupported forge") } -func TestForgeClientFromManifest_GitHub(t *testing.T) { +func TestNewForgeClientFactory_GitHub(t *testing.T) { t.Setenv("GH_TOKEN", "ghp-test-token") - m := &repos.Manifest{ - Defaults: repos.DefaultsConfig{Forge: repos.ForgeGitHub}, - } - client, err := forgeClientFromManifest(m, "") + factory := newForgeClientFactory("") + cfg, err := factory.ConfigFor(repos.ForgeGitHub) require.NoError(t, err) - assert.NotNil(t, client) + assert.NotNil(t, cfg.Client) } -func TestForgeClientFromManifest_EmptyForgeDefaultsToGitHub(t *testing.T) { +func TestNewForgeClientFactory_EmptyForgeDefaultsToGitHub(t *testing.T) { t.Setenv("GH_TOKEN", "ghp-test-token") - m := &repos.Manifest{ - Defaults: repos.DefaultsConfig{Forge: ""}, - } - client, err := forgeClientFromManifest(m, "") + factory := newForgeClientFactory("") + cfg, err := factory.ConfigFor("") require.NoError(t, err) - assert.NotNil(t, client) + assert.NotNil(t, cfg.Client) } -func TestForgeClientFromManifest_GitLab(t *testing.T) { - m := &repos.Manifest{ - Defaults: repos.DefaultsConfig{Forge: repos.ForgeGitLab}, - } - client, err := forgeClientFromManifest(m, "glpat-direct") +func TestNewForgeClientFactory_GitLab(t *testing.T) { + factory := newForgeClientFactory("glpat-direct") + cfg, err := factory.ConfigFor(repos.ForgeGitLab) require.NoError(t, err) - assert.NotNil(t, client) + assert.NotNil(t, cfg.Client) } func TestGetGitLabToken_FromFlag(t *testing.T) { @@ -121,3 +115,31 @@ func TestGetGitLabToken_Empty(t *testing.T) { cmd := &cobra.Command{} assert.Equal(t, "", getGitLabToken(cmd)) } + +func TestNewForgeClientFactory_Caching(t *testing.T) { + t.Setenv("GH_TOKEN", "ghp-test-token") + factory := newForgeClientFactory("") + + cfg1, err := factory.ConfigFor(repos.ForgeGitHub) + require.NoError(t, err) + + cfg2, err := factory.ConfigFor(repos.ForgeGitHub) + require.NoError(t, err) + + assert.Same(t, cfg1.Client, cfg2.Client, "same forge should return the same cached client instance") +} + +func TestNewForgeClientFactory_MixedForge(t *testing.T) { + t.Setenv("GH_TOKEN", "ghp-test-token") + factory := newForgeClientFactory("glpat-test-token") + + ghCfg, err := factory.ConfigFor(repos.ForgeGitHub) + require.NoError(t, err) + + glCfg, err := factory.ConfigFor(repos.ForgeGitLab) + require.NoError(t, err) + + assert.NotSame(t, ghCfg.Client, glCfg.Client, "different forges should return different clients") + assert.NotNil(t, ghCfg.Client) + assert.NotNil(t, glCfg.Client) +} diff --git a/internal/cli/repos.go b/internal/cli/repos.go index 8391a96b67..4d80d8d1cb 100644 --- a/internal/cli/repos.go +++ b/internal/cli/repos.go @@ -74,8 +74,9 @@ that reflects current reality.`, target := args[0] cfg.gitlabToken = getGitLabToken(cmd) - client, err := newForgeClient(cfg.forge, cfg.gitlabToken) - if err != nil { + clients := newForgeClientFactory(cfg.gitlabToken) + // Eagerly validate that a client can be created for the forge. + if _, err := clients.ConfigFor(cfg.forge); err != nil { return err } printerOut := os.Stdout @@ -119,7 +120,7 @@ that reflects current reality.`, printer.StepInfo(fmt.Sprintf("[%s] %s: %s", phase, repo, message)) } - result, err := repos.Init(ctx, initCfg, client, nil, progress) + result, err := repos.Init(ctx, initCfg, clients, nil, progress) if err != nil { return err } @@ -219,12 +220,9 @@ func runReposStatus(cmd *cobra.Command, manifestPath string, jsonOutput bool, re return fmt.Errorf("manifest validation failed: %w", err) } - client, err := forgeClientFromManifest(m, getGitLabToken(cmd)) - if err != nil { - return err - } + clients := newForgeClientFactory(getGitLabToken(cmd)) - result, err := repos.Status(ctx, m, client, concurrency, repoFilter) + result, err := repos.Status(ctx, m, clients, concurrency, repoFilter) if err != nil { return err } @@ -382,18 +380,14 @@ func runReposInstall(ctx context.Context, opts *reposInstallConfig) error { } printer.StepDone(fmt.Sprintf("Loaded manifest with %d repo entries", len(manifest.Repos))) - var client forge.Client + var clients repos.ForgeClientFactory if opts.testClient != nil { - client = opts.testClient + clients = newSingleClientFactory(opts.testClient) } else { - var clientErr error - client, clientErr = forgeClientFromManifest(manifest, opts.gitlabToken) - if clientErr != nil { - return clientErr - } + clients = newForgeClientFactory(opts.gitlabToken) } - if err := checkPerRepoScopes(ctx, client, printer); err != nil { + if err := checkAllForgeScopes(ctx, manifest, clients, printer); err != nil { return err } @@ -426,14 +420,22 @@ func runReposInstall(ctx context.Context, opts *reposInstallConfig) error { } scaffoldCommitFn := func(ctx context.Context, owner, repo string, files []forge.TreeFile, direct bool) error { - targetRepo, repoErr := client.GetRepo(ctx, owner, repo) + rc, ok := manifest.ResolveConfigWithGlobs(owner, repo) + if !ok { + return fmt.Errorf("repo %s/%s not found in manifest", owner, repo) + } + fc, fcErr := clients.ConfigFor(rc.Forge) + if fcErr != nil { + return fcErr + } + targetRepo, repoErr := fc.Client.GetRepo(ctx, owner, repo) if repoErr != nil { return fmt.Errorf("getting repo info: %w", repoErr) } commitMsg := "chore: initialize fullsend per-repo installation" prTitle := "chore: initialize fullsend per-repo installation" prBody := defaultScaffoldPRBody - _, commitErr := layers.CommitScaffoldFiles(ctx, client, printer, owner, repo, + _, commitErr := layers.CommitScaffoldFiles(ctx, fc.Client, printer, owner, repo, targetRepo.DefaultBranch, commitMsg, prTitle, prBody, files, direct, nil) return commitErr } @@ -468,7 +470,7 @@ func runReposInstall(ctx context.Context, opts *reposInstallConfig) error { printer.StepStart("Installing fullsend on manifest repos") } - result, err := repos.BatchInstall(ctx, cfg, client, provisionerFactory, scaffoldCommitFn, progressFn) + result, err := repos.BatchInstall(ctx, cfg, clients, provisionerFactory, scaffoldCommitFn, progressFn) if err != nil { return err } @@ -546,15 +548,11 @@ func runReposAdd(ctx context.Context, opts *reposAddConfig, repoArgs []string) e } printer.StepDone(fmt.Sprintf("Loaded manifest with %d repo entries", len(manifest.Repos))) - var client forge.Client + var clients repos.ForgeClientFactory if opts.testClient != nil { - client = opts.testClient + clients = newSingleClientFactory(opts.testClient) } else { - var clientErr error - client, clientErr = forgeClientFromManifest(manifest, opts.gitlabToken) - if clientErr != nil { - return clientErr - } + clients = newForgeClientFactory(opts.gitlabToken) } entries := make([]repos.RepoEntry, len(repoArgs)) @@ -575,7 +573,7 @@ func runReposAdd(ctx context.Context, opts *reposAddConfig, repoArgs []string) e Manifest: manifest, ManifestPath: opts.manifest, DryRun: opts.dryRun, - }, entries, client, progressFn) + }, entries, clients, progressFn) if err != nil { return err } @@ -698,15 +696,11 @@ func runReposRemove(ctx context.Context, opts *reposRemoveConfig, repoArgs []str printer.StepStart("Uninstalling fullsend from repos before removing from manifest") } - var client forge.Client + var uninstallClients repos.ForgeClientFactory if opts.testClient != nil { - client = opts.testClient - } else if !opts.dryRun { - var clientErr error - client, clientErr = forgeClientFromManifest(manifest, opts.gitlabToken) - if clientErr != nil { - return clientErr - } + uninstallClients = newSingleClientFactory(opts.testClient) + } else { + uninstallClients = newForgeClientFactory(opts.gitlabToken) } uninstallCfg := repos.UninstallConfig{ @@ -725,7 +719,7 @@ func runReposRemove(ctx context.Context, opts *reposRemoveConfig, repoArgs []str printer.StepInfo(fmt.Sprintf("[%s] %s", repo, msg)) } } - results, uninstallErr := repos.Uninstall(ctx, uninstallCfg, client, provFactory, progressFn) + results, uninstallErr := repos.Uninstall(ctx, uninstallCfg, uninstallClients, provFactory, progressFn) if uninstallErr != nil { return uninstallErr } @@ -862,18 +856,14 @@ func runReposUninstall(ctx context.Context, opts *reposUninstallConfig, repoArgs } } - var client forge.Client + var uninstallClients repos.ForgeClientFactory if opts.testClient != nil { - client = opts.testClient + uninstallClients = newSingleClientFactory(opts.testClient) } else { - var clientErr error - client, clientErr = forgeClientFromManifest(manifest, opts.gitlabToken) - if clientErr != nil { - return clientErr - } + uninstallClients = newForgeClientFactory(opts.gitlabToken) } - if err := checkPerRepoScopes(ctx, client, printer); err != nil { + if err := checkAllForgeScopes(ctx, manifest, uninstallClients, printer); err != nil { return err } @@ -903,7 +893,7 @@ func runReposUninstall(ctx context.Context, opts *reposUninstallConfig, repoArgs printer.StepStart("Uninstalling fullsend from repos") } - results, err := repos.Uninstall(ctx, cfg, client, provFactory, progressFn) + results, err := repos.Uninstall(ctx, cfg, uninstallClients, provFactory, progressFn) if err != nil { return err } @@ -1099,22 +1089,18 @@ func runReposDiff(ctx context.Context, opts *reposDiffConfig) error { return fmt.Errorf("manifest validation failed: %w", err) } - var client forge.Client + var diffClients repos.ForgeClientFactory if opts.testClient != nil { - client = opts.testClient + diffClients = newSingleClientFactory(opts.testClient) } else { - var clientErr error - client, clientErr = forgeClientFromManifest(m, opts.gitlabToken) - if clientErr != nil { - return clientErr - } + diffClients = newForgeClientFactory(opts.gitlabToken) } - if err := checkPerRepoScopes(ctx, client, printer); err != nil { + if err := checkAllForgeScopes(ctx, m, diffClients, printer); err != nil { return err } - result, err := repos.Diff(ctx, m, client, opts.concurrency, opts.repoFilter) + result, err := repos.Diff(ctx, m, diffClients, opts.concurrency, opts.repoFilter) if err != nil { return err } @@ -1188,23 +1174,19 @@ func runReposSync(ctx context.Context, opts *reposSyncConfig) error { return fmt.Errorf("manifest validation failed: %w", err) } - var client forge.Client + var syncClients repos.ForgeClientFactory if opts.testClient != nil { - client = opts.testClient + syncClients = newSingleClientFactory(opts.testClient) } else { - var clientErr error - client, clientErr = forgeClientFromManifest(m, opts.gitlabToken) - if clientErr != nil { - return clientErr - } + syncClients = newForgeClientFactory(opts.gitlabToken) } - if err := checkPerRepoScopes(ctx, client, printer); err != nil { + if err := checkAllForgeScopes(ctx, m, syncClients, printer); err != nil { return err } if opts.dryRun { - result, diffErr := repos.Diff(ctx, m, client, opts.concurrency, opts.repoFilter) + result, diffErr := repos.Diff(ctx, m, syncClients, opts.concurrency, opts.repoFilter) if diffErr != nil { return diffErr } @@ -1231,7 +1213,7 @@ func runReposSync(ctx context.Context, opts *reposSyncConfig) error { } } - result, err := repos.Sync(ctx, m, client, opts.concurrency, opts.repoFilter, progressFn) + result, err := repos.Sync(ctx, m, syncClients, opts.concurrency, opts.repoFilter, progressFn) if err != nil && result == nil { return err } @@ -1328,18 +1310,14 @@ func runReposUpgrade(ctx context.Context, opts *reposUpgradeConfig, repoFilter [ } printer.StepDone(fmt.Sprintf("Loaded manifest with %d repo entries", len(m.Repos))) - var client forge.Client + var upgradeClients repos.ForgeClientFactory if opts.testClient != nil { - client = opts.testClient + upgradeClients = newSingleClientFactory(opts.testClient) } else { - var clientErr error - client, clientErr = forgeClientFromManifest(m, opts.gitlabToken) - if clientErr != nil { - return clientErr - } + upgradeClients = newForgeClientFactory(opts.gitlabToken) } - if err := checkPerRepoScopes(ctx, client, printer); err != nil { + if err := checkAllForgeScopes(ctx, m, upgradeClients, printer); err != nil { return err } @@ -1360,7 +1338,15 @@ func runReposUpgrade(ctx context.Context, opts *reposUpgradeConfig, repoFilter [ } commitFn := func(ctx context.Context, owner, repo string, files []forge.TreeFile, isDirect bool) error { - targetRepo, repoErr := client.GetRepo(ctx, owner, repo) + rc, ok := m.ResolveConfigWithGlobs(owner, repo) + if !ok { + return fmt.Errorf("repo %s/%s not found in manifest", owner, repo) + } + fc, fcErr := upgradeClients.ConfigFor(rc.Forge) + if fcErr != nil { + return fcErr + } + targetRepo, repoErr := fc.Client.GetRepo(ctx, owner, repo) if repoErr != nil { return fmt.Errorf("getting repo info: %w", repoErr) } @@ -1368,7 +1354,7 @@ func runReposUpgrade(ctx context.Context, opts *reposUpgradeConfig, repoFilter [ prTitle := "chore: upgrade fullsend scaffold ref" prBody := "This PR upgrades the fullsend scaffold workflow ref.\n\n" + "Merge this PR to activate the updated workflows." - _, commitErr := layers.CommitScaffoldFiles(ctx, client, printer, owner, repo, + _, commitErr := layers.CommitScaffoldFiles(ctx, fc.Client, printer, owner, repo, targetRepo.DefaultBranch, commitMsg, prTitle, prBody, files, isDirect, nil) return commitErr } @@ -1399,7 +1385,7 @@ func runReposUpgrade(ctx context.Context, opts *reposUpgradeConfig, repoFilter [ printer.StepStart("Upgrading repos") } - results, err := repos.Upgrade(ctx, cfg, client, commitFn, progressFn) + results, err := repos.Upgrade(ctx, cfg, upgradeClients, commitFn, progressFn) if err != nil { return err } @@ -1485,3 +1471,22 @@ func runReposUpgradeMint(ctx context.Context, opts *reposUpgradeMintConfig) erro printer.StepDone("Mint verified successfully") return nil } + +// checkAllForgeScopes validates GitHub token permissions for forges used +// in the manifest. Only GitHub forges are checked because scope +// introspection is not supported by other forge providers. +func checkAllForgeScopes(ctx context.Context, m *repos.Manifest, clients repos.ForgeClientFactory, printer *ui.Printer) error { + for _, forgeName := range m.DistinctForges() { + if forgeName != "" && forgeName != repos.ForgeGitHub { + continue + } + fc, err := clients.ConfigFor(forgeName) + if err != nil { + return err + } + if err := checkPerRepoScopes(ctx, fc.Client, printer); err != nil { + return err + } + } + return nil +} diff --git a/internal/cli/repos_test.go b/internal/cli/repos_test.go index 5e5d92ddaf..0dd0e0ec64 100644 --- a/internal/cli/repos_test.go +++ b/internal/cli/repos_test.go @@ -131,12 +131,13 @@ defaults: inference_region: us-central1 repos: [] ` + // With lazy client creation, status on an empty GitLab manifest + // succeeds without a token — no repos means no API calls. manifestPath := writeTestManifest(t, manifestYAML) cmd := newRootCmd() cmd.SetArgs([]string{"repos", "status", "--manifest", manifestPath}) err := cmd.Execute() - require.Error(t, err) - assert.Contains(t, err.Error(), "no GitLab token found") + require.NoError(t, err) } func TestRunReposStatus_GitLabWithToken(t *testing.T) { @@ -1944,7 +1945,7 @@ func TestRunReposSync_DryRun_JSON(t *testing.T) { } // --- forge-aware CLI integration tests --- -// These tests exercise the RunE closures and forgeClientFromManifest paths +// These tests exercise the RunE closures and newForgeClientFactory paths // that are only reachable through the Cobra command chain. var emptyReposManifestYAML = `version: 1 @@ -1960,50 +1961,54 @@ repos: [] ` func TestReposDiffCmd_GitLabNoToken(t *testing.T) { + // With zero repos, a GitLab-default manifest does not require a token. t.Setenv("GITLAB_TOKEN", "") m := strings.Replace(emptyReposManifestYAML, "forge: github", "forge: gitlab", 1) manifestPath := writeTestManifest(t, m) cmd := newRootCmd() cmd.SetArgs([]string{"repos", "diff", "--manifest", manifestPath}) err := cmd.Execute() - require.Error(t, err) - assert.Contains(t, err.Error(), "no GitLab token found") + require.NoError(t, err) } func TestReposSyncCmd_GitLabNoToken(t *testing.T) { + // With zero repos, a GitLab-default manifest does not require a token. t.Setenv("GITLAB_TOKEN", "") m := strings.Replace(emptyReposManifestYAML, "forge: github", "forge: gitlab", 1) manifestPath := writeTestManifest(t, m) cmd := newRootCmd() cmd.SetArgs([]string{"repos", "sync", "--manifest", manifestPath}) err := cmd.Execute() - require.Error(t, err) - assert.Contains(t, err.Error(), "no GitLab token found") + require.NoError(t, err) } func TestReposUpgradeCmd_GitLabNoToken(t *testing.T) { + // With zero repos, a GitLab-default manifest does not require a token; + // the command may still fail on mint verification but NOT on token lookup. t.Setenv("GITLAB_TOKEN", "") m := strings.Replace(emptyReposManifestYAML, "forge: github", "forge: gitlab", 1) manifestPath := writeTestManifest(t, m) cmd := newRootCmd() cmd.SetArgs([]string{"repos", "upgrade", "--manifest", manifestPath}) err := cmd.Execute() - require.Error(t, err) - assert.Contains(t, err.Error(), "no GitLab token found") + if err != nil { + assert.NotContains(t, err.Error(), "no GitLab token found") + } } func TestReposInstallCmd_GitLabNoToken(t *testing.T) { + // With zero repos, a GitLab-default manifest does not require a token. t.Setenv("GITLAB_TOKEN", "") m := strings.Replace(emptyReposManifestYAML, "forge: github", "forge: gitlab", 1) manifestPath := writeTestManifest(t, m) cmd := newRootCmd() cmd.SetArgs([]string{"repos", "install", "--manifest", manifestPath}) err := cmd.Execute() - require.Error(t, err) - assert.Contains(t, err.Error(), "no GitLab token found") + require.NoError(t, err) } func TestReposUninstallCmd_GitLabNoToken(t *testing.T) { + // The token error now surfaces per-repo instead of at scope checking. t.Setenv("GITLAB_TOKEN", "") m := `version: 1 mint: @@ -2022,18 +2027,44 @@ repos: cmd.SetArgs([]string{"repos", "uninstall", "--yes", "--manifest", manifestPath, "acme/repo"}) err := cmd.Execute() require.Error(t, err) - assert.Contains(t, err.Error(), "no GitLab token found") + assert.Contains(t, err.Error(), "failed to uninstall") } func TestReposAddCmd_GitLabNoToken(t *testing.T) { + // With lazy client creation, adding a repo to a GitLab manifest + // succeeds without a token — the client is only needed for probing + // existing installation state, which is non-fatal when it fails. t.Setenv("GITLAB_TOKEN", "") m := strings.Replace(emptyReposManifestYAML, "forge: github", "forge: gitlab", 1) manifestPath := writeTestManifest(t, m) cmd := newRootCmd() cmd.SetArgs([]string{"repos", "add", "--manifest", manifestPath, "acme/repo"}) err := cmd.Execute() - require.Error(t, err) - assert.Contains(t, err.Error(), "no GitLab token found") + require.NoError(t, err) +} + +func TestReposDiffCmd_GitLabNoToken_WithRepos(t *testing.T) { + // With actual repos, the missing GitLab token surfaces per-repo. + t.Setenv("GITLAB_TOKEN", "") + m := `version: 1 +mint: + url: https://mint.example.com + project: p + region: us-central1 +defaults: + forge: gitlab + inference_project: proj + inference_region: us-central1 +repos: + - acme/repo +` + manifestPath := writeTestManifest(t, m) + cmd := newRootCmd() + cmd.SetArgs([]string{"repos", "diff", "--manifest", manifestPath}) + err := cmd.Execute() + require.NoError(t, err) + // The error is reported as a warning, not a fatal error, because + // the diff operation treats per-repo forge client errors as warnings. } func TestReposInitCmd_GitLabNoToken(t *testing.T) { diff --git a/internal/repos/batch_install.go b/internal/repos/batch_install.go index 607f57db22..c8b76d7e10 100644 --- a/internal/repos/batch_install.go +++ b/internal/repos/batch_install.go @@ -55,7 +55,7 @@ type ProvisionerFactory func(cfg ResolvedConfig) WIFProvisioner // // Errors on individual repos do not abort the batch. func BatchInstall(ctx context.Context, cfg BatchInstallConfig, - client forge.Client, provisionerFactory ProvisionerFactory, + clients ForgeClientFactory, provisionerFactory ProvisionerFactory, commitScaffold ScaffoldCommitFunc, progress ProgressFunc) (*BatchInstallResult, error) { @@ -72,7 +72,7 @@ func BatchInstall(ctx context.Context, cfg BatchInstallConfig, return nil, fmt.Errorf("invalid manifest: %w", err) } - repos, err := manifest.ExpandGlobs(ctx, client) + repos, err := manifest.ExpandGlobs(ctx, clients) if err != nil { return nil, fmt.Errorf("expanding globs: %w", err) } @@ -125,15 +125,21 @@ func BatchInstall(ctx context.Context, cfg BatchInstallConfig, fullName := rr.Owner + "/" + rr.Repo progress(fullName, "discover", "Checking installation status") - guardVal, guardExists, guardErr := client.GetRepoVariable(ctx, rr.Owner, rr.Repo, forge.PerRepoGuardVar) + fc, fcErr := clients.ConfigFor(resolved.Forge) + if fcErr != nil { + discoveries[idx] = discoveryResult{repo: rr, resolved: resolved, err: fcErr} + return + } + resolved.ForgeConfig = fc + + guardVal, guardExists, guardErr := fc.Client.GetRepoVariable(ctx, rr.Owner, rr.Repo, forge.PerRepoGuardVar) if guardErr != nil { discoveries[idx] = discoveryResult{repo: rr, resolved: resolved, err: guardErr} return } installed := false if guardExists && guardVal == "true" { - fc := ForgeConfigFor(resolved.Forge) - fullyInstalled, checkErr := checkInstallComponents(ctx, client, rr.Owner, rr.Repo, fc) + fullyInstalled, checkErr := checkInstallComponents(ctx, fc.Client, rr.Owner, rr.Repo, fc) if checkErr != nil { discoveries[idx] = discoveryResult{repo: rr, resolved: resolved, err: checkErr} return @@ -280,7 +286,9 @@ func BatchInstall(ctx context.Context, cfg BatchInstallConfig, fullName := d.repo.Owner + "/" + d.repo.Repo // TOCTOU re-check: guard variable may have changed since Phase 1. - guardVal, guardExists, guardErr := client.GetRepoVariable(ctx, d.repo.Owner, d.repo.Repo, forge.PerRepoGuardVar) + // ForgeConfig was resolved during Phase 1 and cached in d.resolved. + repoClient := d.resolved.ForgeConfig.Client + guardVal, guardExists, guardErr := repoClient.GetRepoVariable(ctx, d.repo.Owner, d.repo.Repo, forge.PerRepoGuardVar) if guardErr != nil { result.Failed = append(result.Failed, InstallResult{ Owner: d.repo.Owner, @@ -291,8 +299,7 @@ func BatchInstall(ctx context.Context, cfg BatchInstallConfig, continue } if guardExists && guardVal == "true" { - fc := ForgeConfigFor(d.resolved.Forge) - fullyInstalled, checkErr := checkInstallComponents(ctx, client, d.repo.Owner, d.repo.Repo, fc) + fullyInstalled, checkErr := checkInstallComponents(ctx, repoClient, d.repo.Owner, d.repo.Repo, d.resolved.ForgeConfig) if checkErr != nil { result.Failed = append(result.Failed, InstallResult{ Owner: d.repo.Owner, @@ -406,7 +413,7 @@ func BatchInstall(ctx context.Context, cfg BatchInstallConfig, Direct: cfg.Direct, } - installResult, installErr := Install(ctx, installCfg, client, nil, commitScaffold, progress) + installResult, installErr := Install(ctx, installCfg, dr.resolved.ForgeConfig.Client, nil, commitScaffold, progress) mu.Lock() defer mu.Unlock() diff --git a/internal/repos/batch_install_test.go b/internal/repos/batch_install_test.go index f14edb06e3..740010a214 100644 --- a/internal/repos/batch_install_test.go +++ b/internal/repos/batch_install_test.go @@ -198,7 +198,7 @@ func TestBatchInstall_AllFresh(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -237,7 +237,7 @@ func TestBatchInstall_SomeAlreadyInstalled(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -273,7 +273,7 @@ func TestBatchInstall_RepoFilter(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -302,7 +302,7 @@ func TestBatchInstall_DryRun(t *testing.T) { Roles: []string{"triage"}, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -344,7 +344,7 @@ func TestBatchInstall_DryRunSkipsInstalled(t *testing.T) { Roles: []string{"triage"}, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -392,7 +392,7 @@ func TestBatchInstall_WIFSerialization(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc, noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc, noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -447,7 +447,7 @@ func TestBatchInstall_OrgMintFailure(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -478,7 +478,7 @@ func TestBatchInstall_SkipMintCheck(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -509,7 +509,7 @@ func TestBatchInstall_WIFProvisionFailure(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -543,7 +543,7 @@ func TestBatchInstall_RegisterWIFFailure_OneRepo(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -588,7 +588,7 @@ func TestBatchInstall_RegisterWIFFailure_CleanupErrorInResult(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -619,7 +619,7 @@ func TestBatchInstall_ScaffoldFailure_WIFCleanup(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -648,7 +648,7 @@ func TestBatchInstall_EmptyManifest(t *testing.T) { MaxConcurrency: 1, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -671,7 +671,7 @@ func TestBatchInstall_InvalidManifest(t *testing.T) { MaxConcurrency: 1, } - _, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + _, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err == nil { t.Fatal("expected error for invalid manifest") } @@ -695,7 +695,7 @@ func TestBatchInstall_MissingInferenceProject(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -733,7 +733,7 @@ func TestBatchInstall_MissingInferenceRegion(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -776,7 +776,7 @@ func TestBatchInstall_MultiOrg(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -829,7 +829,7 @@ func TestBatchInstall_TOCTOUReCheck(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -862,7 +862,7 @@ func TestBatchInstall_PartialInstall_RepairsWhenComponentsMissing(t *testing.T) Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -899,7 +899,7 @@ func TestBatchInstall_TOCTOUReCheck_PartialInstallProceeds(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -938,7 +938,7 @@ func TestBatchInstall_ScaffoldFailure_OneRepo(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc, noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc, noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -972,7 +972,7 @@ func TestBatchInstall_NilProgress(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), nil) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), nil) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -997,7 +997,7 @@ func TestBatchInstall_DefaultConcurrency(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -1049,7 +1049,7 @@ func TestBatchInstall_ConcurrencyCap(t *testing.T) { close(done) }() - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc, noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc, noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -1092,7 +1092,7 @@ func TestBatchInstall_InvalidConcurrency(t *testing.T) { Direct: true, } - _, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + _, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err == nil { t.Errorf("expected error for concurrency=%d, got nil", tt.concurrency) } @@ -1117,7 +1117,7 @@ func TestBatchInstall_RepoFilterCaseInsensitive(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -1143,7 +1143,7 @@ func TestBatchInstall_DiscoveryError(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -1173,7 +1173,7 @@ func TestBatchInstall_ScaffoldErrorCollection(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() unexpected top-level error: %v", err) } @@ -1218,7 +1218,7 @@ func TestBatchInstall_ContextCancellation_Phase1(t *testing.T) { Direct: true, } - result, err := BatchInstall(ctx, cfg, client, factory, sc.fn(), noopProgress) + result, err := BatchInstall(ctx, cfg, newTestClientFactory(client), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() unexpected top-level error: %v", err) } @@ -1259,7 +1259,7 @@ func TestBatchInstall_ContextCancellation_Phase2(t *testing.T) { Direct: true, } - result, err := BatchInstall(ctx, cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(ctx, cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() unexpected top-level error: %v", err) } @@ -1289,7 +1289,7 @@ func TestBatchInstall_ContextCancellation_OrgMintLoop(t *testing.T) { Direct: true, } - result, err := BatchInstall(ctx, cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(ctx, cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() unexpected top-level error: %v", err) } @@ -1422,7 +1422,7 @@ func TestBatchInstall_Phase1_CheckInstallComponentsError(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } @@ -1459,7 +1459,7 @@ func TestBatchInstall_TOCTOUReCheck_ComponentCheckError(t *testing.T) { Direct: true, } - result, err := BatchInstall(context.Background(), cfg, fc, factory, sc.fn(), noopProgress) + result, err := BatchInstall(context.Background(), cfg, newTestClientFactory(fc), factory, sc.fn(), noopProgress) if err != nil { t.Fatalf("BatchInstall() error: %v", err) } diff --git a/internal/repos/factory_test_helper_test.go b/internal/repos/factory_test_helper_test.go new file mode 100644 index 0000000000..3d9db9d0de --- /dev/null +++ b/internal/repos/factory_test_helper_test.go @@ -0,0 +1,23 @@ +package repos + +import "github.com/fullsend-ai/fullsend/internal/forge" + +// testClientFactory wraps a single forge client as a ForgeClientFactory +// for use in tests. It returns the same client for any forge name, matching +// the single-client test pattern that existed before ForgeClientFactory. +type testClientFactory struct { + client forge.Client +} + +func newTestClientFactory(fc forge.Client) ForgeClientFactory { + return &testClientFactory{client: fc} +} + +func (f *testClientFactory) ConfigFor(forgeName string) (ForgeConfig, error) { + if forgeName == "" { + forgeName = ForgeGitHub + } + cfg := ForgeConfigFor(forgeName) + cfg.Client = f.client + return cfg, nil +} diff --git a/internal/repos/forge_config.go b/internal/repos/forge_config.go index 8bf9677d44..a615018a3c 100644 --- a/internal/repos/forge_config.go +++ b/internal/repos/forge_config.go @@ -1,6 +1,10 @@ package repos -import "regexp" +import ( + "regexp" + + "github.com/fullsend-ai/fullsend/internal/forge" +) var ( ghWorkflowRefPattern = regexp.MustCompile( @@ -19,8 +23,13 @@ var ( // ForgeConfig holds forge-specific CI paths and regex patterns used by // status and upgrade operations. Each forge has different workflow file -// conventions and ref syntax. +// conventions and ref syntax. When populated by a ForgeClientFactory, +// the Client field carries a live API client for the forge. type ForgeConfig struct { + // Client is the API client for this forge. Set by ForgeClientFactory; + // nil when ForgeConfig is constructed by ForgeConfigFor (pattern-only). + Client forge.Client + // WorkflowPaths lists the shim workflow file paths to try, in order. WorkflowPaths []string @@ -32,6 +41,15 @@ type ForgeConfig struct { ShimRefPattern *regexp.Regexp } +// ForgeClientFactory creates ForgeConfig instances with a live Client. +// The CLI layer implements this with lazy client creation and caching. +type ForgeClientFactory interface { + // ConfigFor returns a ForgeConfig with a live Client for the named forge. + // The factory lazily creates and caches clients, so a GitLab token is + // only required if the manifest actually contains GitLab entries. + ConfigFor(forgeName string) (ForgeConfig, error) +} + // GitHubForgeConfig returns the ForgeConfig for GitHub repositories. // GitHub Actions workflow files live under .github/workflows/ and use // the "uses: owner/repo/.github/workflows/file@ref" syntax. diff --git a/internal/repos/init.go b/internal/repos/init.go index 6263ca5356..9aaa8f07e9 100644 --- a/internal/repos/init.go +++ b/internal/repos/init.go @@ -63,7 +63,7 @@ type InitResult struct { // Init discovers existing fullsend installations and generates a // repos.yaml manifest. It supports both greenfield onboarding and // migration from existing per-repo or per-org installations. -func Init(ctx context.Context, cfg InitConfig, client forge.Client, +func Init(ctx context.Context, cfg InitConfig, clients ForgeClientFactory, selectRepos RepoSelectFunc, progress ProgressFunc) (*InitResult, error) { if cfg.MaxConcurrency <= 0 { @@ -88,9 +88,9 @@ func Init(ctx context.Context, cfg InitConfig, client forge.Client, if cfg.Repos != nil { return nil, fmt.Errorf("--repos flag cannot be used with a single repo target") } - return initSingleRepo(ctx, cfg, client, owner, repo, progress) + return initSingleRepo(ctx, cfg, clients, owner, repo, progress) } - return initOrg(ctx, cfg, client, owner, selectRepos, progress) + return initOrg(ctx, cfg, clients, owner, selectRepos, progress) } func parseInitTarget(target string) (owner, repo string, isRepo bool, err error) { @@ -111,11 +111,17 @@ func parseInitTarget(target string) (owner, repo string, isRepo bool, err error) } // initSingleRepo discovers a single repo and generates a one-entry manifest. -func initSingleRepo(ctx context.Context, cfg InitConfig, client forge.Client, +func initSingleRepo(ctx context.Context, cfg InitConfig, clients ForgeClientFactory, owner, repo string, progress ProgressFunc) (*InitResult, error) { progress(owner+"/"+repo, "discover", "checking installation status") + fc, fcErr := clients.ConfigFor(cfg.Forge) + if fcErr != nil { + return nil, fcErr + } + client := fc.Client + // Check for per-org config if the repo isn't per-repo installed. var orgCfg config.OrgConfigReader progress(owner, "discover", "checking for per-org config") @@ -145,9 +151,15 @@ func initSingleRepo(ctx context.Context, cfg InitConfig, client forge.Client, } // initOrg discovers all repos in an org and generates a manifest. -func initOrg(ctx context.Context, cfg InitConfig, client forge.Client, +func initOrg(ctx context.Context, cfg InitConfig, clients ForgeClientFactory, org string, selectRepos RepoSelectFunc, progress ProgressFunc) (*InitResult, error) { + fc, fcErr := clients.ConfigFor(cfg.Forge) + if fcErr != nil { + return nil, fcErr + } + client := fc.Client + progress(org, "discover", "listing org repos") allOrgRepos, err := client.ListOrgRepos(ctx, org, false) if err != nil { diff --git a/internal/repos/init_test.go b/internal/repos/init_test.go index e2454b8524..f8e219e1e2 100644 --- a/internal/repos/init_test.go +++ b/internal/repos/init_test.go @@ -63,7 +63,7 @@ func TestInit_GreenfieldOrg_AllFlag(t *testing.T) { InferenceProject: "my-inference", CLIVersion: "2.3.0", MaxConcurrency: 2, - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 0, result.PerRepoCount) @@ -95,7 +95,7 @@ func TestInit_GreenfieldOrg_ExplicitRepos(t *testing.T) { MintRegion: "r", InferenceProject: "inf", CLIVersion: "1.0.0", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 2, result.NewCount) @@ -112,7 +112,7 @@ func TestInit_GreenfieldOrg_ExplicitRepos_NotFound(t *testing.T) { _, err := Init(context.Background(), InitConfig{ Target: "acme", Repos: []string{"acme/api", "acme/nonexistent"}, - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) assert.Error(t, err) assert.ErrorContains(t, err, "not found in org") @@ -159,7 +159,7 @@ repos: MintRegion: "us-central1", InferenceProject: "inf", MaxConcurrency: 4, - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 1, result.PerRepoCount) @@ -194,7 +194,7 @@ func TestInit_OnlyPerRepoInstallations(t *testing.T) { MintProject: "proj", MintRegion: "us-central1", MaxConcurrency: 2, - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 2, result.PerRepoCount) @@ -226,7 +226,7 @@ repos: MintProject: "proj", MintRegion: "us-central1", InferenceProject: "inf", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 0, result.PerRepoCount) @@ -250,7 +250,7 @@ func TestInit_SingleRepo_PerRepoInstalled(t *testing.T) { Target: "acme/api", MintProject: "proj", MintRegion: "us-central1", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 1, result.PerRepoCount) @@ -281,7 +281,7 @@ repos: MintProject: "proj", MintRegion: "us-central1", InferenceProject: "inf", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 1, result.PerOrgCount) @@ -294,7 +294,7 @@ func TestInit_SingleRepo_RejectsAllFlag(t *testing.T) { _, err := Init(context.Background(), InitConfig{ Target: "acme/api", All: true, - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) assert.Error(t, err) assert.ErrorContains(t, err, "--all flag cannot be used with a single repo target") @@ -306,7 +306,7 @@ func TestInit_SingleRepo_RejectsReposFlag(t *testing.T) { _, err := Init(context.Background(), InitConfig{ Target: "acme/api", Repos: []string{"acme/other"}, - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) assert.Error(t, err) assert.ErrorContains(t, err, "--repos flag cannot be used with a single repo target") @@ -321,7 +321,7 @@ func TestInit_SingleRepo_NotInstalled(t *testing.T) { MintRegion: "us-central1", InferenceProject: "inf", CLIVersion: "2.5.0", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 1, result.NewCount) @@ -358,7 +358,7 @@ func TestInit_DefaultsComputation_MostCommonRef(t *testing.T) { All: true, MintProject: "proj", MintRegion: "us-central1", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) // v2.3.0 is most common, should be the default. @@ -404,7 +404,7 @@ func TestInit_PerRepoOverrides_DifferentRegion(t *testing.T) { All: true, MintProject: "proj", MintRegion: "us-central1", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) @@ -446,7 +446,7 @@ func TestInit_InteractiveSelection(t *testing.T) { MintProject: "proj", MintRegion: "r", InferenceProject: "inf", - }, fc, selectFn, nopProgress) + }, newTestClientFactory(fc), selectFn, nopProgress) require.NoError(t, err) require.Len(t, result.Manifest.Repos, 2) @@ -469,7 +469,7 @@ func TestInit_NilCallback_RequiresFlag(t *testing.T) { _, err := Init(context.Background(), InitConfig{ Target: "acme", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) assert.Error(t, err) assert.ErrorContains(t, err, "org target requires --all or --repos flag") @@ -484,7 +484,7 @@ func TestInit_TODOs_NoMintProject(t *testing.T) { Target: "acme/api", MintRegion: "us-central1", CLIVersion: "1.0.0", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Contains(t, result.TODOs, "mint.project: provide via --mint-project flag") @@ -499,7 +499,7 @@ func TestInit_TODOs_NoMintURL_Greenfield(t *testing.T) { MintProject: "proj", MintRegion: "us-central1", CLIVersion: "1.0.0", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Contains(t, result.TODOs, "mint.url: set the Cloud Run endpoint URL") @@ -530,7 +530,7 @@ func TestInit_TODOs_MultipleMintURLs(t *testing.T) { All: true, MintProject: "proj", MintRegion: "r", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) // Most common URL should be used. @@ -713,7 +713,7 @@ func TestInit_RoundTrip(t *testing.T) { MintProject: "proj", MintRegion: "us-central1", InferenceProject: "inf", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) // Marshal and re-parse. @@ -738,7 +738,7 @@ func TestInit_ListOrgReposError(t *testing.T) { _, err := Init(context.Background(), InitConfig{ Target: "acme", All: true, - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) assert.Error(t, err) assert.ErrorContains(t, err, "listing repos for org") @@ -750,7 +750,7 @@ func TestInit_ListRepoVariablesError(t *testing.T) { _, err := Init(context.Background(), InitConfig{ Target: "acme/api", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) assert.Error(t, err) assert.ErrorContains(t, err, "listing variables") @@ -773,7 +773,7 @@ func TestInit_OrgConfigParseError_SingleRepo_Warns(t *testing.T) { MintProject: "proj", MintRegion: "us-central1", CLIVersion: "1.0.0", - }, fc, nil, progress) + }, newTestClientFactory(fc), nil, progress) require.NoError(t, err) assert.Equal(t, 1, result.NewCount) @@ -794,7 +794,7 @@ func TestInit_OrgConfigFetchError_SingleRepo(t *testing.T) { _, err := Init(context.Background(), InitConfig{ Target: "acme/api", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) assert.Error(t, err) assert.ErrorContains(t, err, "fetching org config") @@ -809,7 +809,7 @@ func TestInit_OrgConfigFetchError_Org(t *testing.T) { _, err := Init(context.Background(), InitConfig{ Target: "acme", All: true, - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) assert.Error(t, err) assert.ErrorContains(t, err, "fetching org config") @@ -831,7 +831,7 @@ func TestInit_ConfigRepoExcluded(t *testing.T) { MintRegion: "us-central1", InferenceProject: "inf", CLIVersion: "1.0.0", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 2, result.NewCount) @@ -856,7 +856,7 @@ func TestInit_DiscoveryErrors_Tracked(t *testing.T) { MintRegion: "us-central1", InferenceProject: "inf", CLIVersion: "1.0.0", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) // Repo with error should be excluded from manifest. @@ -1184,7 +1184,7 @@ func TestInit_SingleRepo_GitLabForge(t *testing.T) { Forge: ForgeGitLab, MintProject: "proj", MintRegion: "us-central1", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 1, result.PerRepoCount) @@ -1242,7 +1242,7 @@ func TestInit_CLIVersionFallback(t *testing.T) { MintRegion: "us-central1", InferenceProject: "inf", CLIVersion: "3.0.0", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, "v3.0.0", result.Manifest.Defaults.FullsendRef) @@ -1257,7 +1257,7 @@ func TestInit_CLIVersionWithVPrefix_NoDoubleV(t *testing.T) { MintRegion: "us-central1", InferenceProject: "inf", CLIVersion: "v0.32.0-82-gcb2bcd9f", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, "v0.32.0-82-gcb2bcd9f", result.Manifest.Defaults.FullsendRef) @@ -1272,7 +1272,7 @@ func TestInit_CLIVersionDev_FallsBackToDefault(t *testing.T) { MintRegion: "us-central1", InferenceProject: "inf", CLIVersion: "dev", - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, config.DefaultUpstreamRef, result.Manifest.Defaults.FullsendRef) @@ -1293,7 +1293,7 @@ func TestInit_DefaultConcurrency(t *testing.T) { InferenceProject: "inf", CLIVersion: "1.0.0", MaxConcurrency: 0, // should default to 8 - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 1, result.NewCount) @@ -1312,7 +1312,7 @@ func TestInit_ConcurrencyUpperBound(t *testing.T) { InferenceProject: "inf", CLIVersion: "1.0.0", MaxConcurrency: 200, // should clamp to 64 - }, fc, nil, nopProgress) + }, newTestClientFactory(fc), nil, nopProgress) require.NoError(t, err) assert.Equal(t, 1, result.NewCount) diff --git a/internal/repos/manifest.go b/internal/repos/manifest.go index b508b306fb..3557c1202c 100644 --- a/internal/repos/manifest.go +++ b/internal/repos/manifest.go @@ -194,11 +194,14 @@ type ResolvedRepo struct { // ResolvedConfig is the fully resolved configuration for a single // repository after merging per-repo overrides, manifest defaults, -// and built-in defaults. +// and built-in defaults. The ForgeConfig field carries per-forge +// patterns and (when populated by ForgeClientFactory.ConfigFor) a +// live API client. type ResolvedConfig struct { Owner string Repo string Forge string + ForgeConfig ForgeConfig MintURL string MintProject string MintRegion string @@ -423,6 +426,21 @@ func (m *Manifest) Validate() error { seen[entry.Repo] = true } + // Reject manifests where the same owner has entries with different forges. + // A GitHub org and a GitLab group named the same thing are different + // entities; mixing them under one owner would route API calls incorrectly. + ownerForge := make(map[string]string) + for i, entry := range m.Repos { + parts := strings.SplitN(entry.Repo, "/", 2) + owner := parts[0] + entryForge := resolveField(entry.Forge, m.Defaults.Forge, "") + if prev, ok := ownerForge[owner]; ok && prev != entryForge { + return fmt.Errorf("repos[%d]: owner %q has entries with forge %q and %q; "+ + "all repos under the same owner must use the same forge", i, owner, prev, entryForge) + } + ownerForge[owner] = entryForge + } + return nil } @@ -434,7 +452,10 @@ func (m *Manifest) Validate() error { // ListOrgRepos is called with includePrivate=true because repos.yaml // manifests are used in per-repo mode, where agents run on the target // repo itself. Archived and forked repos remain excluded. -func (m *Manifest) ExpandGlobs(ctx context.Context, client forge.Client) ([]ResolvedRepo, error) { +// +// The clients factory provides per-forge API clients so glob entries +// targeting different forges resolve against the correct API. +func (m *Manifest) ExpandGlobs(ctx context.Context, clients ForgeClientFactory) ([]ResolvedRepo, error) { // First pass: separate explicit entries from glob patterns. explicit := make(map[string]RepoEntry) type globEntry struct { @@ -474,8 +495,13 @@ func (m *Manifest) ExpandGlobs(ctx context.Context, client forge.Client) ([]Reso for _, g := range globs { repos, ok := orgRepoCache[g.org] if !ok { - var err error - repos, err = client.ListOrgRepos(ctx, g.org, true) + // Resolve the forge for this glob entry to get the right client. + entryForge := resolveField(g.entry.Forge, m.Defaults.Forge, "") + fc, err := clients.ConfigFor(entryForge) + if err != nil { + return nil, fmt.Errorf("expanding glob %q: creating client for forge %q: %w", g.org+"/"+g.pattern, entryForge, err) + } + repos, err = fc.Client.ListOrgRepos(ctx, g.org, true) if err != nil { return nil, fmt.Errorf("expanding glob %q: listing repos for org %q: %w", g.org+"/"+g.pattern, g.org, err) } @@ -555,6 +581,21 @@ func (m *Manifest) ResolveConfig(owner, repo string) (ResolvedConfig, bool) { return m.resolveWithEntry(owner, repo, RepoEntry{}), false } +// ResolveConfigWithGlobs resolves config for a repo, falling back to +// glob-pattern matching when the exact entry lookup fails. +func (m *Manifest) ResolveConfigWithGlobs(owner, repo string) (ResolvedConfig, bool) { + if resolved, ok := m.ResolveConfig(owner, repo); ok { + return resolved, true + } + fullName := owner + "/" + repo + for _, e := range m.Repos { + if ok, _ := matchesPattern(e.Repo, fullName); ok { + return m.ResolveConfigForEntry(owner, repo, e), true + } + } + return ResolvedConfig{}, false +} + // ResolveConfigForEntry computes the fully merged configuration for // the given owner/repo using the provided RepoEntry. Use this with // entries returned by ExpandGlobs, which carry per-glob overrides @@ -604,6 +645,26 @@ func resolveField(override NullableString, fallback string, builtinDefault strin return builtinDefault } +// DistinctForges returns the deduplicated set of forge names actually +// used by entries in the manifest, after resolving per-entry overrides +// against defaults. Only forges referenced by at least one repo entry +// are included. The order is deterministic (sorted). +func (m *Manifest) DistinctForges() []string { + seen := make(map[string]bool) + for _, entry := range m.Repos { + f := resolveField(entry.Forge, m.Defaults.Forge, "") + if f != "" { + seen[f] = true + } + } + forges := make([]string, 0, len(seen)) + for f := range seen { + forges = append(forges, f) + } + sort.Strings(forges) + return forges +} + // Marshal serializes the manifest back to YAML. func (m *Manifest) Marshal() ([]byte, error) { return yaml.Marshal(m) diff --git a/internal/repos/manifest_edit.go b/internal/repos/manifest_edit.go index a25d865e1b..e14f7c685c 100644 --- a/internal/repos/manifest_edit.go +++ b/internal/repos/manifest_edit.go @@ -7,8 +7,6 @@ import ( "path/filepath" "regexp" "strings" - - "github.com/fullsend-ai/fullsend/internal/forge" ) var repoNamePattern = regexp.MustCompile(`^[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+$`) @@ -38,7 +36,7 @@ type ManifestRemoveResult struct { // discovered values differ from manifest defaults. // Returns the result and the modified manifest. The manifest is written to // disk only when ManifestPath is set and DryRun is false. -func AddToManifest(ctx context.Context, cfg ManifestEditConfig, entries []RepoEntry, client forge.Client, progress ProgressFunc) (*ManifestAddResult, *Manifest, error) { +func AddToManifest(ctx context.Context, cfg ManifestEditConfig, entries []RepoEntry, clients ForgeClientFactory, progress ProgressFunc) (*ManifestAddResult, *Manifest, error) { if cfg.Manifest == nil { return nil, nil, fmt.Errorf("manifest is required") } @@ -60,7 +58,7 @@ func AddToManifest(ctx context.Context, cfg ManifestEditConfig, entries []RepoEn } } - if client != nil { + if clients != nil { for i := range entries { if isGlob(entries[i].Repo) || existing[strings.ToLower(entries[i].Repo)] { continue @@ -70,8 +68,12 @@ func AddToManifest(ctx context.Context, cfg ManifestEditConfig, entries []RepoEn continue } entryForge := resolveField(entries[i].Forge, cfg.Manifest.Defaults.Forge, ForgeGitHub) - fc := ForgeConfigFor(entryForge) - state, err := ProbeRepoState(ctx, client, parts[0], parts[1], fc) + fc, fcErr := clients.ConfigFor(entryForge) + if fcErr != nil { + progress(entries[i].Repo, "discover", fmt.Sprintf("forge client error: %v", fcErr)) + continue + } + state, err := ProbeRepoState(ctx, fc.Client, parts[0], parts[1], fc) if err != nil && !state.Installed { progress(entries[i].Repo, "discover", fmt.Sprintf("probe failed: %v", err)) continue diff --git a/internal/repos/manifest_edit_test.go b/internal/repos/manifest_edit_test.go index cf8e8b8d1b..54c2b44184 100644 --- a/internal/repos/manifest_edit_test.go +++ b/internal/repos/manifest_edit_test.go @@ -203,7 +203,7 @@ func TestAddToManifest_DiscoverInstalled(t *testing.T) { result, updated, err := AddToManifest(context.Background(), ManifestEditConfig{ Manifest: manifest, - }, []RepoEntry{{Repo: "acme/api"}}, fc, nil) + }, []RepoEntry{{Repo: "acme/api"}}, newTestClientFactory(fc), nil) if err != nil { t.Fatalf("AddToManifest() error = %v", err) @@ -236,7 +236,7 @@ func TestAddToManifest_DiscoverInstalledMatchesDefaults(t *testing.T) { _, updated, err := AddToManifest(context.Background(), ManifestEditConfig{ Manifest: manifest, - }, []RepoEntry{{Repo: "acme/api"}}, fc, nil) + }, []RepoEntry{{Repo: "acme/api"}}, newTestClientFactory(fc), nil) if err != nil { t.Fatalf("AddToManifest() error = %v", err) @@ -261,7 +261,7 @@ func TestAddToManifest_DiscoverNotInstalled(t *testing.T) { _, updated, err := AddToManifest(context.Background(), ManifestEditConfig{ Manifest: manifest, - }, []RepoEntry{{Repo: "acme/api"}}, fc, nil) + }, []RepoEntry{{Repo: "acme/api"}}, newTestClientFactory(fc), nil) if err != nil { t.Fatalf("AddToManifest() error = %v", err) @@ -281,7 +281,7 @@ func TestAddToManifest_DiscoverGlobSkipped(t *testing.T) { manifest := testManifest() result, _, err := AddToManifest(context.Background(), ManifestEditConfig{ Manifest: manifest, - }, []RepoEntry{{Repo: "acme/*"}}, fc, nil) + }, []RepoEntry{{Repo: "acme/*"}}, newTestClientFactory(fc), nil) if err != nil { t.Fatalf("AddToManifest() error = %v", err) @@ -300,7 +300,7 @@ func TestAddToManifest_DiscoverProbeError(t *testing.T) { result, _, err := AddToManifest(context.Background(), ManifestEditConfig{ Manifest: manifest, - }, []RepoEntry{{Repo: "acme/api"}}, fc, nil) + }, []RepoEntry{{Repo: "acme/api"}}, newTestClientFactory(fc), nil) if err != nil { t.Fatalf("AddToManifest() error = %v, want graceful skip on probe error", err) diff --git a/internal/repos/manifest_test.go b/internal/repos/manifest_test.go index 1bf2cabb53..914e84ca71 100644 --- a/internal/repos/manifest_test.go +++ b/internal/repos/manifest_test.go @@ -657,7 +657,7 @@ repos: } ctx := context.Background() - resolved, err := m.ExpandGlobs(ctx, fc) + resolved, err := m.ExpandGlobs(ctx, newTestClientFactory(fc)) require.NoError(t, err) // Should have: explicit-repo, service-api, service-priv, service-web @@ -703,7 +703,7 @@ repos: } ctx := context.Background() - resolved, err := m.ExpandGlobs(ctx, fc) + resolved, err := m.ExpandGlobs(ctx, newTestClientFactory(fc)) require.NoError(t, err) // Private repos should be included (per-repo mode), but archived @@ -743,7 +743,7 @@ repos: } ctx := context.Background() - resolved, err := m.ExpandGlobs(ctx, fc) + resolved, err := m.ExpandGlobs(ctx, newTestClientFactory(fc)) require.NoError(t, err) require.Len(t, resolved, 2) @@ -778,7 +778,7 @@ repos: } ctx := context.Background() - _, err := m.ExpandGlobs(ctx, fc) + _, err := m.ExpandGlobs(ctx, newTestClientFactory(fc)) assert.Error(t, err) assert.ErrorContains(t, err, "expanding glob") assert.ErrorContains(t, err, "listing repos for org") @@ -800,7 +800,7 @@ repos: fc := forge.NewFakeClient() ctx := context.Background() - resolved, err := m.ExpandGlobs(ctx, fc) + resolved, err := m.ExpandGlobs(ctx, newTestClientFactory(fc)) require.NoError(t, err) require.Len(t, resolved, 2) @@ -949,7 +949,7 @@ repos: } ctx := context.Background() - resolved, err := m.ExpandGlobs(ctx, fc) + resolved, err := m.ExpandGlobs(ctx, newTestClientFactory(fc)) require.NoError(t, err) require.Len(t, resolved, 2) @@ -1220,7 +1220,7 @@ repos: } ctx := context.Background() - resolved, err := m.ExpandGlobs(ctx, fc) + resolved, err := m.ExpandGlobs(ctx, newTestClientFactory(fc)) require.NoError(t, err) // org-a/* matches app, lib (from org-a). @@ -1236,3 +1236,77 @@ repos: assert.True(t, repoNames["org-b/service-api"]) assert.False(t, repoNames["org-b/other"], "other should not match service-*") } + +func TestValidate_RejectsSameOwnerMixedForge(t *testing.T) { + input := ` +version: 1 +mint: + url: https://mint.example.com + project: p + region: r +defaults: + forge: github +repos: + - acme/api + - repo: acme/ml-pipeline + forge: gitlab +` + var m Manifest + require.NoError(t, yaml.Unmarshal([]byte(input), &m)) + + err := m.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "all repos under the same owner must use the same forge") + assert.Contains(t, err.Error(), `owner "acme"`) +} + +func TestValidate_AllowsDifferentOwnersDifferentForges(t *testing.T) { + input := ` +version: 1 +mint: + url: https://mint.example.com + project: p + region: r +defaults: + forge: github +repos: + - acme/api + - repo: gitlab-group/ml-pipeline + forge: gitlab +` + var m Manifest + require.NoError(t, yaml.Unmarshal([]byte(input), &m)) + + err := m.Validate() + require.NoError(t, err) +} + +func TestDistinctForges(t *testing.T) { + input := ` +version: 1 +mint: + url: https://mint.example.com + project: p + region: r +defaults: + forge: github +repos: + - acme/api + - acme/web + - repo: gitlab-group/ml + forge: gitlab +` + var m Manifest + require.NoError(t, yaml.Unmarshal([]byte(input), &m)) + + forges := m.DistinctForges() + assert.Equal(t, []string{"github", "gitlab"}, forges) +} + +func TestDistinctForges_SingleForge(t *testing.T) { + var m Manifest + require.NoError(t, yaml.Unmarshal([]byte(validManifest), &m)) + + forges := m.DistinctForges() + assert.Equal(t, []string{"github"}, forges) +} diff --git a/internal/repos/status.go b/internal/repos/status.go index 5c37769aab..4c32cdc6a1 100644 --- a/internal/repos/status.go +++ b/internal/repos/status.go @@ -91,8 +91,8 @@ type StatusResult struct { // Status compares the manifest's desired state against the actual forge // state for each repo. It returns a StatusResult with per-repo status // and aggregate counts. API calls are parallelised up to maxConcurrency. -func Status(ctx context.Context, manifest *Manifest, client forge.Client, maxConcurrency int, repoFilter []string) (*StatusResult, error) { - resolved, err := manifest.ExpandGlobs(ctx, client) +func Status(ctx context.Context, manifest *Manifest, clients ForgeClientFactory, maxConcurrency int, repoFilter []string) (*StatusResult, error) { + resolved, err := manifest.ExpandGlobs(ctx, clients) if err != nil { return nil, fmt.Errorf("resolving repos: %w", err) } @@ -131,8 +131,17 @@ func Status(ctx context.Context, manifest *Manifest, client forge.Client, maxCon defer func() { <-sem }() cfg := manifest.ResolveConfigForEntry(rr.Owner, rr.Repo, rr.Entry) - fc := ForgeConfigFor(cfg.Forge) - status := checkRepoStatus(ctx, client, rr.Owner, rr.Repo, cfg, fc) + fc, fcErr := clients.ConfigFor(cfg.Forge) + if fcErr != nil { + results[idx] = RepoStatus{ + Owner: rr.Owner, + Repo: rr.Repo, + Error: fcErr.Error(), + } + return + } + cfg.ForgeConfig = fc + status := checkRepoStatus(ctx, cfg) results[idx] = status }(i, rr) } @@ -156,7 +165,12 @@ func Status(ctx context.Context, manifest *Manifest, client forge.Client, maxCon return &StatusResult{Repos: results, Summary: summary, Warnings: warnings}, nil } -func checkRepoStatus(ctx context.Context, client forge.Client, owner, repo string, cfg ResolvedConfig, fc ForgeConfig) RepoStatus { +func checkRepoStatus(ctx context.Context, cfg ResolvedConfig) RepoStatus { + owner := cfg.Owner + repo := cfg.Repo + client := cfg.ForgeConfig.Client + fc := cfg.ForgeConfig + status := RepoStatus{ Owner: owner, Repo: repo, diff --git a/internal/repos/status_test.go b/internal/repos/status_test.go index 50c1756892..fafb94b452 100644 --- a/internal/repos/status_test.go +++ b/internal/repos/status_test.go @@ -113,7 +113,7 @@ func TestStatus_AllInstalled_NoDrift(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "web-frontend", "v2.3.0", "https://mint.example.com", "us-central1") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -149,7 +149,7 @@ func TestStatus_RepoNotInstalled(t *testing.T) { "https://mint.example.com", "us-central1") // web-frontend has no variables — not installed. - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -179,7 +179,7 @@ func TestStatus_MintURLDrift(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "web-frontend", "v2.3.0", "https://old-mint.example.com", "us-central1") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -215,7 +215,7 @@ func TestStatus_RefDrift(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "web-frontend", "v2.1.0", "https://mint.example.com", "us-central1") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -243,7 +243,7 @@ func TestStatus_RegionDrift(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.3.0", "https://mint.example.com", "us-west1") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -275,7 +275,7 @@ func TestStatus_MultipleDrifts(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.1.0", "https://old.example.com", "us-west1") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -314,7 +314,7 @@ func TestStatus_WorkflowMissing_NotInstalled(t *testing.T) { } // Guard variable not set → not installed, workflow not checked. - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -345,7 +345,7 @@ func TestStatus_WorkflowYAMLExtension(t *testing.T) { // Use .yaml extension instead of .yml fc.FileContents["acme-corp/api-server/.github/workflows/fullsend.yaml"] = []byte(shimWorkflow) - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -367,7 +367,7 @@ func TestStatus_RepoFilter(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "web-frontend", "v2.3.0", "https://mint.example.com", "us-central1") - result, err := Status(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -387,7 +387,7 @@ func TestStatus_RepoFilterCaseInsensitive(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.3.0", "https://mint.example.com", "us-central1") - result, err := Status(context.Background(), m, fc, 4, []string{"ACME-CORP/API-SERVER"}) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, []string{"ACME-CORP/API-SERVER"}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -411,7 +411,7 @@ func TestStatus_APIError(t *testing.T) { fc.Errors["ListRepoVariables"] = fmt.Errorf("API rate limit exceeded") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -453,7 +453,7 @@ func TestStatus_GlobExpansion(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.3.0", "https://mint.example.com", "us-central1") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -496,7 +496,7 @@ func TestStatus_PerRepoOverride(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "legacy", "v2.1.0", "https://mint.example.com", "us-central1") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -518,7 +518,7 @@ func TestStatus_DefaultConcurrency(t *testing.T) { Repos: []RepoEntry{{Repo: "org/repo"}}, } - result, err := Status(context.Background(), m, fc, 0, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 0, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -538,7 +538,7 @@ func TestStatus_EmptyManifest(t *testing.T) { }, } - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -567,7 +567,7 @@ func TestStatus_InstalledButWorkflowGetError(t *testing.T) { fc.VariableValues["org/repo/FULLSEND_GCP_REGION"] = "us-central1" fc.Errors["GetFileContent"] = fmt.Errorf("server error") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -602,7 +602,7 @@ func TestStatus_NoWorkflowFiles(t *testing.T) { fc.VariableValues["org/repo/FULLSEND_MINT_URL"] = "https://mint.example.com" fc.VariableValues["org/repo/FULLSEND_GCP_REGION"] = "us-central1" - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -838,7 +838,7 @@ func TestStatus_GuardVarFalse(t *testing.T) { fc.VariableValues["org/repo/FULLSEND_PER_REPO_INSTALL"] = "false" - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -870,7 +870,7 @@ func TestStatus_MultiOrg(t *testing.T) { populateInstalledRepo(fc, "org-a", "repo1", "v2.3.0", "https://mint.example.com", "us-central1") populateInstalledRepo(fc, "org-b", "repo2", "v2.3.0", "https://mint.example.com", "us-central1") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -897,7 +897,7 @@ func TestStatus_GlobExpandError(t *testing.T) { Repos: []RepoEntry{{Repo: "bad-org/*"}}, } - _, err := Status(context.Background(), m, fc, 4, nil) + _, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err == nil { t.Fatal("expected error from glob expansion") } @@ -921,7 +921,7 @@ func TestStatus_EmptyMintURL_NoDrift(t *testing.T) { populateInstalledRepo(fc, "org", "repo", "v2.3.0", "https://some-mint.example.com", "us-central1") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -945,7 +945,7 @@ func TestStatus_EmptyExpectedRef_NoDrift(t *testing.T) { populateInstalledRepo(fc, "org", "repo", "v2.3.0", "https://mint.example.com", "us-central1") - result, err := Status(context.Background(), m, fc, 4, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -978,7 +978,7 @@ func TestStatus_Concurrency(t *testing.T) { populateInstalledRepo(fc, "org", repo, "v2.3.0", "https://mint.example.com", "us-central1") } - result, err := Status(context.Background(), m, fc, 2, nil) + result, err := Status(context.Background(), m, newTestClientFactory(fc), 2, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -998,7 +998,7 @@ func TestStatus_RepoFilterAllUnmatched(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.3.0", "https://mint.example.com", "us-central1") - _, err := Status(context.Background(), m, fc, 4, []string{"org/nonexistent"}) + _, err := Status(context.Background(), m, newTestClientFactory(fc), 4, []string{"org/nonexistent"}) if err == nil { t.Fatal("expected error when --repo filter matches nothing") } @@ -1011,7 +1011,7 @@ func TestStatus_RepoFilterPartialUnmatched(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.3.0", "https://mint.example.com", "us-central1") - result, err := Status(context.Background(), m, fc, 4, + result, err := Status(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server", "org/nonexistent"}) if err != nil { t.Fatalf("unexpected error: %v", err) diff --git a/internal/repos/sync.go b/internal/repos/sync.go index 3a6e6dc985..b85c3f6f6b 100644 --- a/internal/repos/sync.go +++ b/internal/repos/sync.go @@ -70,12 +70,12 @@ func validateConcurrency(n int) error { // // For secrets, Diff only reports missing secrets (action "create") // because secret values cannot be read back for comparison. -func Diff(ctx context.Context, manifest *Manifest, client forge.Client, maxConcurrency int, repoFilter []string) (*DiffResult, error) { +func Diff(ctx context.Context, manifest *Manifest, clients ForgeClientFactory, maxConcurrency int, repoFilter []string) (*DiffResult, error) { if err := validateConcurrency(maxConcurrency); err != nil { return nil, err } - resolved, err := manifest.ExpandGlobs(ctx, client) + resolved, err := manifest.ExpandGlobs(ctx, clients) if err != nil { return nil, fmt.Errorf("resolving repos: %w", err) } @@ -114,7 +114,13 @@ func Diff(ctx context.Context, manifest *Manifest, client forge.Client, maxConcu defer func() { <-sem }() cfg := manifest.ResolveConfigForEntry(rr.Owner, rr.Repo, rr.Entry) - changes, warnings, _ := diffRepo(ctx, client, rr.Owner, rr.Repo, cfg) + fc, fcErr := clients.ConfigFor(cfg.Forge) + if fcErr != nil { + results[idx] = repoResult{warnings: []string{fmt.Sprintf("%s/%s: forge client error: %v", rr.Owner, rr.Repo, fcErr)}} + return + } + cfg.ForgeConfig = fc + changes, warnings, _ := diffRepo(ctx, cfg) results[idx] = repoResult{changes: changes, warnings: warnings} }(i, rr) } @@ -134,7 +140,9 @@ func Diff(ctx context.Context, manifest *Manifest, client forge.Client, maxConcu // The returned bool is true when the repo was successfully examined; // false means a fatal condition (API error, guard missing) and callers // should not attempt further writes. -func diffRepo(ctx context.Context, client forge.Client, owner, repo string, cfg ResolvedConfig) ([]Change, []string, bool) { +func diffRepo(ctx context.Context, cfg ResolvedConfig) ([]Change, []string, bool) { + owner, repo := cfg.Owner, cfg.Repo + client := cfg.ForgeConfig.Client vars, err := client.ListRepoVariables(ctx, owner, repo) if err != nil { return nil, []string{fmt.Sprintf("%s/%s: error listing variables: %v", owner, repo, err)}, false @@ -207,7 +215,7 @@ func diffRepo(ctx context.Context, client forge.Client, owner, repo string, cfg // // Sync does NOT touch scaffold shim version (@ref) or harness files. // Version changes are managed by `repos upgrade`. -func Sync(ctx context.Context, manifest *Manifest, client forge.Client, maxConcurrency int, repoFilter []string, progress ProgressFunc) (*SyncResult, error) { +func Sync(ctx context.Context, manifest *Manifest, clients ForgeClientFactory, maxConcurrency int, repoFilter []string, progress ProgressFunc) (*SyncResult, error) { if err := validateConcurrency(maxConcurrency); err != nil { return nil, err } @@ -216,7 +224,7 @@ func Sync(ctx context.Context, manifest *Manifest, client forge.Client, maxConcu progress = func(_, _, _ string) {} } - resolved, err := manifest.ExpandGlobs(ctx, client) + resolved, err := manifest.ExpandGlobs(ctx, clients) if err != nil { return nil, fmt.Errorf("resolving repos: %w", err) } @@ -258,7 +266,14 @@ func Sync(ctx context.Context, manifest *Manifest, client forge.Client, maxConcu cfg := manifest.ResolveConfigForEntry(rr.Owner, rr.Repo, rr.Entry) repoFullName := rr.Owner + "/" + rr.Repo - changes, diffWarnings, ok := diffRepo(ctx, client, rr.Owner, rr.Repo, cfg) + fc, fcErr := clients.ConfigFor(cfg.Forge) + if fcErr != nil { + results[idx] = repoResult{warnings: []string{fmt.Sprintf("%s: forge client error: %v", repoFullName, fcErr)}} + return + } + cfg.ForgeConfig = fc + + changes, diffWarnings, ok := diffRepo(ctx, cfg) var res repoResult res.warnings = append(res.warnings, diffWarnings...) @@ -268,7 +283,7 @@ func Sync(ctx context.Context, manifest *Manifest, client forge.Client, maxConcu } if len(changes) == 0 { - secretChanges, secretErr := ensureSecrets(ctx, client, rr.Owner, rr.Repo, cfg, progress) + secretChanges, secretErr := ensureSecrets(ctx, cfg, progress) res.applied = append(res.applied, secretChanges...) if secretErr != nil { res.warnings = append(res.warnings, secretErr.Error()) @@ -279,7 +294,7 @@ func Sync(ctx context.Context, manifest *Manifest, client forge.Client, maxConcu return } - applied, applyErr := applyChanges(ctx, client, rr.Owner, rr.Repo, cfg, changes, progress) + applied, applyErr := applyChanges(ctx, cfg, changes, progress) res.applied = append(res.applied, applied...) if applyErr != nil { res.warnings = append(res.warnings, applyErr.Error()) @@ -313,7 +328,9 @@ func Sync(ctx context.Context, manifest *Manifest, client forge.Client, maxConcu // ensureSecrets writes all managed secrets for convergence, since their // values cannot be read back for comparison. -func ensureSecrets(ctx context.Context, client forge.Client, owner, repo string, cfg ResolvedConfig, progress ProgressFunc) ([]Change, error) { +func ensureSecrets(ctx context.Context, cfg ResolvedConfig, progress ProgressFunc) ([]Change, error) { + owner, repo := cfg.Owner, cfg.Repo + client := cfg.ForgeConfig.Client repoFullName := owner + "/" + repo var applied []Change @@ -338,7 +355,9 @@ func ensureSecrets(ctx context.Context, client forge.Client, owner, repo string, return applied, nil } -func applyChanges(ctx context.Context, client forge.Client, owner, repo string, cfg ResolvedConfig, changes []Change, progress ProgressFunc) ([]Change, error) { +func applyChanges(ctx context.Context, cfg ResolvedConfig, changes []Change, progress ProgressFunc) ([]Change, error) { + owner, repo := cfg.Owner, cfg.Repo + client := cfg.ForgeConfig.Client repoFullName := owner + "/" + repo var applied []Change @@ -353,7 +372,7 @@ func applyChanges(ctx context.Context, client forge.Client, owner, repo string, applied = append(applied, c) } - secretChanges, secretErr := ensureSecrets(ctx, client, owner, repo, cfg, progress) + secretChanges, secretErr := ensureSecrets(ctx, cfg, progress) applied = append(applied, secretChanges...) return applied, secretErr diff --git a/internal/repos/sync_test.go b/internal/repos/sync_test.go index 858cd47d18..13869c7364 100644 --- a/internal/repos/sync_test.go +++ b/internal/repos/sync_test.go @@ -21,7 +21,7 @@ func TestDiff_NoDrift(t *testing.T) { fc.Secrets["acme-corp/api-server/FULLSEND_GCP_PROJECT_ID"] = true fc.Secrets["acme-corp/web-frontend/FULLSEND_GCP_PROJECT_ID"] = true - result, err := Diff(context.Background(), m, fc, 4, nil) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -38,7 +38,7 @@ func TestDiff_VariableDrift_MintURL(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.3.0", "https://old-mint.example.com", "us-central1") - result, err := Diff(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -70,7 +70,7 @@ func TestDiff_VariableDrift_Region(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.3.0", "https://mint.example.com", "us-west1") - result, err := Diff(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -99,7 +99,7 @@ func TestDiff_MissingGuardVariable(t *testing.T) { fc.VariableValues["acme-corp/api-server/FULLSEND_MINT_URL"] = "https://mint.example.com" fc.VariableValues["acme-corp/api-server/FULLSEND_GCP_REGION"] = "us-central1" - result, err := Diff(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -127,7 +127,7 @@ func TestDiff_SecretMissing(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.3.0", "https://mint.example.com", "us-central1") - result, err := Diff(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -155,7 +155,7 @@ func TestDiff_SecretExists_NoChange(t *testing.T) { "https://mint.example.com", "us-central1") fc.Secrets["acme-corp/api-server/FULLSEND_GCP_PROJECT_ID"] = true - result, err := Diff(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -171,7 +171,7 @@ func TestDiff_RepoNotInstalled_Warning(t *testing.T) { fc := forge.NewFakeClient() m := newTestManifest() - result, err := Diff(context.Background(), m, fc, 4, nil) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -205,7 +205,7 @@ func TestDiff_APIError_Warning(t *testing.T) { fc.Errors["ListRepoVariables"] = fmt.Errorf("rate limit exceeded") - result, err := Diff(context.Background(), m, fc, 4, nil) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -227,7 +227,7 @@ func TestDiff_MultipleRepos(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "web-frontend", "v2.3.0", "https://mint.example.com", "us-west1") - result, err := Diff(context.Background(), m, fc, 4, nil) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -255,7 +255,7 @@ func TestDiff_RepoFilter(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "web-frontend", "v2.3.0", "https://old-mint.example.com", "us-central1") - result, err := Diff(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -292,7 +292,7 @@ func TestDiff_GlobExpansion(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.3.0", "https://old.example.com", "us-central1") - result, err := Diff(context.Background(), m, fc, 4, nil) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -319,7 +319,7 @@ func TestDiff_EmptyManifest(t *testing.T) { }, } - result, err := Diff(context.Background(), m, fc, 4, nil) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -347,7 +347,7 @@ func TestDiff_EmptyDesiredValue_Skips(t *testing.T) { populateInstalledRepo(fc, "org", "repo", "v2.3.0", "https://some-mint.example.com", "us-west1") - result, err := Diff(context.Background(), m, fc, 4, nil) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -373,7 +373,7 @@ func TestDiff_ConcurrencyValidation(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, err := Diff(context.Background(), m, fc, tt.concurrency, nil) + _, err := Diff(context.Background(), m, newTestClientFactory(fc), tt.concurrency, nil) if err == nil { t.Error("expected error for invalid concurrency") } @@ -404,7 +404,7 @@ func TestDiff_SecretCheckError_Warning(t *testing.T) { "https://mint.example.com", "us-central1") fc.Errors["RepoSecretExists"] = fmt.Errorf("secret API error") - result, err := Diff(context.Background(), m, fc, 4, nil) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -430,7 +430,7 @@ func TestSync_NoDrift_NoVariableWrites(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "web-frontend", "v2.3.0", "https://mint.example.com", "us-central1") - result, err := Sync(context.Background(), m, fc, 4, nil, nil) + result, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -455,7 +455,7 @@ func TestSync_AppliesVariableChanges(t *testing.T) { progressCalls = append(progressCalls, fmt.Sprintf("%s/%s/%s", repo, phase, msg)) } - result, err := Sync(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}, progress) + result, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}, progress) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -487,7 +487,7 @@ func TestSync_AppliesSecretChanges(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.3.0", "https://mint.example.com", "us-central1") - result, err := Sync(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}, nil) + result, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -522,7 +522,7 @@ func TestSync_VariableWriteError(t *testing.T) { fc.Errors["CreateOrUpdateRepoVariable"] = fmt.Errorf("write error") - result, err := Sync(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}, nil) + result, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}, nil) if err == nil { t.Fatal("expected error from variable write failure") } @@ -541,7 +541,7 @@ func TestSync_SecretWriteError(t *testing.T) { fc.Errors["CreateRepoSecret"] = fmt.Errorf("secret write error") - _, err := Sync(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}, nil) + _, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}, nil) if err == nil { t.Fatal("expected error from secret write failure") } @@ -554,7 +554,7 @@ func TestSync_NilProgress(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "api-server", "v2.3.0", "https://old-mint.example.com", "us-central1") - _, err := Sync(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}, nil) + _, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}, nil) if err != nil { t.Fatalf("unexpected error with nil progress: %v", err) } @@ -567,7 +567,7 @@ func TestSync_DiffAPIError_SkipsReconciliation(t *testing.T) { fc.Errors["ListRepoVariables"] = fmt.Errorf("API rate limit exceeded") - result, err := Sync(context.Background(), m, fc, 4, nil, nil) + result, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, nil, nil) if err != nil { t.Fatalf("expected no error (warnings only), got: %v", err) } @@ -592,7 +592,7 @@ func TestSync_MultipleRepos(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "web-frontend", "v2.3.0", "https://old.example.com", "us-central1") - result, err := Sync(context.Background(), m, fc, 4, nil, nil) + result, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -613,7 +613,7 @@ func TestSync_ConcurrencyValidation(t *testing.T) { fc := forge.NewFakeClient() m := newTestManifest() - _, err := Sync(context.Background(), m, fc, 0, nil, nil) + _, err := Sync(context.Background(), m, newTestClientFactory(fc), 0, nil, nil) if err == nil { t.Fatal("expected error for invalid concurrency") } @@ -648,7 +648,7 @@ func TestSync_GlobWithPerEntryOverride(t *testing.T) { populateInstalledRepo(fc, "acme", "api", "v2.3.0", "https://mint.example.com", "us-central1") - result, err := Sync(context.Background(), m, fc, 4, nil, nil) + result, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -784,7 +784,7 @@ func TestDiff_GlobExpandError(t *testing.T) { Repos: []RepoEntry{{Repo: "bad-org/*"}}, } - _, err := Diff(context.Background(), m, fc, 4, nil) + _, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err == nil { t.Fatal("expected error from glob expansion") } @@ -813,7 +813,7 @@ func TestDiff_PerRepoOverride(t *testing.T) { populateInstalledRepo(fc, "org", "repo", "v2.3.0", "https://mint.example.com", "eu-west1") - result, err := Diff(context.Background(), m, fc, 4, nil) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -834,7 +834,7 @@ func TestSync_RepoFilter(t *testing.T) { populateInstalledRepo(fc, "acme-corp", "web-frontend", "v2.3.0", "https://old.example.com", "us-central1") - result, err := Sync(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}, nil) + result, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -860,7 +860,7 @@ func TestDiff_GuardVarFalse_Warning(t *testing.T) { fc.VariableValues["org/repo/FULLSEND_PER_REPO_INSTALL"] = "false" - result, err := Diff(context.Background(), m, fc, 4, nil) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -902,7 +902,7 @@ func TestSync_GlobExpansion(t *testing.T) { populateInstalledRepo(fc, "acme", "api", "v2.3.0", "https://old.example.com", "us-central1") - result, err := Sync(context.Background(), m, fc, 4, nil, nil) + result, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -939,7 +939,7 @@ func TestDiff_MultiOrg(t *testing.T) { populateInstalledRepo(fc, "org-a", "repo1", "v2.3.0", "https://old.example.com", "us-central1") populateInstalledRepo(fc, "org-b", "repo2", "v2.3.0", "https://old.example.com", "us-central1") - result, err := Diff(context.Background(), m, fc, 4, nil) + result, err := Diff(context.Background(), m, newTestClientFactory(fc), 4, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -977,7 +977,7 @@ func TestSync_EnsuresSecretsOnNoDrift(t *testing.T) { "https://mint.example.com", "us-central1") fc.Secrets["acme-corp/api-server/FULLSEND_GCP_PROJECT_ID"] = true - result, err := Sync(context.Background(), m, fc, 4, []string{"acme-corp/api-server"}, nil) + result, err := Sync(context.Background(), m, newTestClientFactory(fc), 4, []string{"acme-corp/api-server"}, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/internal/repos/uninstall.go b/internal/repos/uninstall.go index 032b246cf7..f58305a376 100644 --- a/internal/repos/uninstall.go +++ b/internal/repos/uninstall.go @@ -48,7 +48,7 @@ type UninstallResult struct { // // Does NOT modify repos.yaml — use RemoveFromManifest for that. func Uninstall(ctx context.Context, cfg UninstallConfig, - client forge.Client, provisionerFactory ProvisionerFactory, + clients ForgeClientFactory, provisionerFactory ProvisionerFactory, progress ProgressFunc) ([]UninstallResult, error) { if len(cfg.Repos) == 0 { @@ -107,12 +107,16 @@ func Uninstall(ctx context.Context, cfg UninstallConfig, forgeName := "" if cfg.Manifest != nil { - if rc, ok := resolveConfigWithGlobs(cfg.Manifest, owner, repo); ok { + if rc, ok := cfg.Manifest.ResolveConfigWithGlobs(owner, repo); ok { forgeName = rc.Forge } } - fc := ForgeConfigFor(forgeName) - results[idx] = uninstallRepoResources(ctx, owner, repo, client, fc, progress) + fc, fcErr := clients.ConfigFor(forgeName) + if fcErr != nil { + results[idx] = UninstallResult{Owner: owner, Repo: repo, Error: fcErr} + return + } + results[idx] = uninstallRepoResources(ctx, ResolvedConfig{Owner: owner, Repo: repo, ForgeConfig: fc}, progress) }(i, p.owner, p.repo) } wg.Wait() @@ -128,7 +132,7 @@ func Uninstall(ctx context.Context, cfg UninstallConfig, if results[j].Error != nil || !results[j].WorkflowDeleted { continue } - if _, ok := resolveConfigWithGlobs(cfg.Manifest, results[j].Owner, results[j].Repo); ok { + if _, ok := cfg.Manifest.ResolveConfigWithGlobs(results[j].Owner, results[j].Repo); ok { results[j].Error = fmt.Errorf("WIF cleanup skipped: %w", ctx.Err()) } } @@ -136,7 +140,7 @@ func Uninstall(ctx context.Context, cfg UninstallConfig, } fullName := results[i].Owner + "/" + results[i].Repo - resolved, ok := resolveConfigWithGlobs(cfg.Manifest, results[i].Owner, results[i].Repo) + resolved, ok := cfg.Manifest.ResolveConfigWithGlobs(results[i].Owner, results[i].Repo) if !ok { progress(fullName, "wif", "Not in manifest, skipping WIF cleanup") results[i].Success = true @@ -164,9 +168,10 @@ func Uninstall(ctx context.Context, cfg UninstallConfig, return results, nil } -func uninstallRepoResources(ctx context.Context, owner, repo string, - client forge.Client, fc ForgeConfig, progress ProgressFunc) UninstallResult { - +func uninstallRepoResources(ctx context.Context, cfg ResolvedConfig, progress ProgressFunc) UninstallResult { + owner, repo := cfg.Owner, cfg.Repo + client := cfg.ForgeConfig.Client + fc := cfg.ForgeConfig fullName := owner + "/" + repo result := UninstallResult{Owner: owner, Repo: repo} @@ -231,21 +236,6 @@ func uninstallRepoResources(ctx context.Context, owner, repo string, return result } -// resolveConfigWithGlobs resolves config for a repo, falling back to -// glob-pattern matching when the exact entry lookup fails. -func resolveConfigWithGlobs(m *Manifest, owner, repo string) (ResolvedConfig, bool) { - if resolved, ok := m.ResolveConfig(owner, repo); ok { - return resolved, true - } - fullName := owner + "/" + repo - for _, e := range m.Repos { - if ok, _ := matchesPattern(e.Repo, fullName); ok { - return m.ResolveConfigForEntry(owner, repo, e), true - } - } - return ResolvedConfig{}, false -} - // splitOwnerRepo splits "owner/repo" and rejects glob characters. Callers // that accept glob patterns must filter them out before calling this. func splitOwnerRepo(fullName string) (string, string, error) { diff --git a/internal/repos/uninstall_test.go b/internal/repos/uninstall_test.go index 6c548f759e..04dae8bf72 100644 --- a/internal/repos/uninstall_test.go +++ b/internal/repos/uninstall_test.go @@ -92,7 +92,7 @@ func TestUninstall_InstalledRepo(t *testing.T) { Manifest: testManifest("acme/api"), Repos: []string{"acme/api"}, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -143,7 +143,7 @@ func TestUninstall_GlobManifestEntry_WIFCleanup(t *testing.T) { Manifest: manifest, Repos: []string{"acme/api"}, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -161,7 +161,7 @@ func TestUninstall_GlobManifestEntry_WIFCleanup(t *testing.T) { func TestResolveConfigWithGlobs_ExactMatch(t *testing.T) { m := testManifest("acme/api") - resolved, ok := resolveConfigWithGlobs(m, "acme", "api") + resolved, ok := m.ResolveConfigWithGlobs("acme", "api") if !ok { t.Fatal("expected ok=true for exact match") } @@ -173,7 +173,7 @@ func TestResolveConfigWithGlobs_ExactMatch(t *testing.T) { func TestResolveConfigWithGlobs_GlobMatch(t *testing.T) { m := testManifest() m.Repos = []RepoEntry{{Repo: "acme/*"}} - resolved, ok := resolveConfigWithGlobs(m, "acme", "api") + resolved, ok := m.ResolveConfigWithGlobs("acme", "api") if !ok { t.Fatal("expected ok=true for glob match") } @@ -184,7 +184,7 @@ func TestResolveConfigWithGlobs_GlobMatch(t *testing.T) { func TestResolveConfigWithGlobs_NoMatch(t *testing.T) { m := testManifest("other/repo") - _, ok := resolveConfigWithGlobs(m, "acme", "api") + _, ok := m.ResolveConfigWithGlobs("acme", "api") if ok { t.Error("expected ok=false for no match") } @@ -199,7 +199,7 @@ func TestUninstall_NonInstalledRepo(t *testing.T) { Manifest: testManifest("acme/api"), Repos: []string{"acme/api"}, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -224,7 +224,7 @@ func TestUninstall_YamlExtensionFallback(t *testing.T) { Manifest: testManifest("acme/api"), Repos: []string{"acme/api"}, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -257,7 +257,7 @@ func TestUninstall_SkipWIFCleanup(t *testing.T) { Repos: []string{"acme/api"}, SkipWIFCleanup: true, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -284,7 +284,7 @@ func TestUninstall_DryRun(t *testing.T) { Repos: []string{"acme/api"}, DryRun: true, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -317,7 +317,7 @@ func TestUninstall_MultipleRepos(t *testing.T) { Manifest: manifest, Repos: []string{"acme/api", "acme/web", "acme/docs"}, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -350,7 +350,7 @@ func TestUninstall_PartialFailure(t *testing.T) { Manifest: manifest, Repos: []string{"acme/api", "acme/web"}, MaxConcurrency: 1, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -382,7 +382,7 @@ func TestUninstall_WorkflowFailure_SkipsVarsAndSecrets(t *testing.T) { Manifest: testManifest("acme/api"), Repos: []string{"acme/api"}, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -449,7 +449,7 @@ func TestUninstall_WIFSequential(t *testing.T) { Manifest: manifest, Repos: repos, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -481,7 +481,7 @@ func TestUninstall_WIFFailure_DoesNotAffectOtherRepos(t *testing.T) { Manifest: manifest, Repos: []string{"acme/api", "acme/web"}, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -517,7 +517,7 @@ func TestUninstall_WIFFailure_DoesNotAffectOtherRepos(t *testing.T) { func TestUninstall_EmptyRepos(t *testing.T) { _, err := Uninstall(context.Background(), UninstallConfig{ MaxConcurrency: 4, - }, forge.NewFakeClient(), nil, nil) + }, newTestClientFactory(forge.NewFakeClient()), nil, nil) if err == nil { t.Fatal("Uninstall() error = nil, want error for empty repos") @@ -528,7 +528,7 @@ func TestUninstall_InvalidRepoFormat(t *testing.T) { _, err := Uninstall(context.Background(), UninstallConfig{ Repos: []string{"just-a-name"}, MaxConcurrency: 4, - }, forge.NewFakeClient(), nil, nil) + }, newTestClientFactory(forge.NewFakeClient()), nil, nil) if err == nil { t.Fatal("Uninstall() error = nil, want error for invalid repo format") @@ -539,7 +539,7 @@ func TestUninstall_InvalidConcurrency(t *testing.T) { _, err := Uninstall(context.Background(), UninstallConfig{ Repos: []string{"acme/api"}, MaxConcurrency: 0, - }, forge.NewFakeClient(), nil, nil) + }, newTestClientFactory(forge.NewFakeClient()), nil, nil) if err == nil { t.Fatal("Uninstall() error = nil, want error for invalid concurrency") @@ -554,7 +554,7 @@ func TestUninstall_NoManifest_SkipsWIF(t *testing.T) { results, err := Uninstall(context.Background(), UninstallConfig{ Repos: []string{"acme/api"}, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -581,7 +581,7 @@ func TestUninstall_RepoNotInManifest_SkipsWIF(t *testing.T) { Manifest: manifest, Repos: []string{"acme/api"}, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -606,7 +606,7 @@ func TestUninstall_VariableDeleteError(t *testing.T) { Manifest: testManifest("acme/api"), Repos: []string{"acme/api"}, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -634,7 +634,7 @@ func TestUninstall_SecretDeleteError(t *testing.T) { Manifest: testManifest("acme/api"), Repos: []string{"acme/api"}, MaxConcurrency: 4, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -665,7 +665,7 @@ func TestUninstall_ProgressCallbacks(t *testing.T) { Manifest: testManifest("acme/api"), Repos: []string{"acme/api"}, MaxConcurrency: 4, - }, client, factory, progress) + }, newTestClientFactory(client), factory, progress) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -711,7 +711,7 @@ func TestUninstall_ContextCancelled_SkipsWIF(t *testing.T) { Manifest: manifest, Repos: []string{"acme/api", "acme/web"}, MaxConcurrency: 1, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -732,7 +732,7 @@ func TestUninstall_ContextCancelled_SkipsWIF(t *testing.T) { Manifest: testManifest("acme/api2"), Repos: []string{"acme/api2"}, MaxConcurrency: 4, - }, client2, factory2, nil) + }, newTestClientFactory(client2), factory2, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) @@ -756,7 +756,7 @@ func TestUninstall_ContextCancelledDuringPhase2_MarksRemaining(t *testing.T) { Manifest: manifest, Repos: []string{"acme/api", "acme/web"}, MaxConcurrency: 1, - }, client, factory, nil) + }, newTestClientFactory(client), factory, nil) if err != nil { t.Fatalf("Uninstall() error = %v", err) diff --git a/internal/repos/upgrade.go b/internal/repos/upgrade.go index 0129caf05f..504e3a7c5b 100644 --- a/internal/repos/upgrade.go +++ b/internal/repos/upgrade.go @@ -83,7 +83,7 @@ func replaceShimRef(content []byte, newRef, newTag string, fc ForgeConfig) ([]by // It reads each repo's current workflow file, determines whether an upgrade // is needed, and commits the updated workflow with the new ref. func Upgrade(ctx context.Context, cfg UpgradeConfig, - client forge.Client, + clients ForgeClientFactory, commitFn ScaffoldCommitFunc, progress ProgressFunc) ([]UpgradeResult, error) { @@ -91,7 +91,7 @@ func Upgrade(ctx context.Context, cfg UpgradeConfig, progress = func(_, _, _ string) {} } - resolved, err := cfg.Manifest.ExpandGlobs(ctx, client) + resolved, err := cfg.Manifest.ExpandGlobs(ctx, clients) if err != nil { return nil, fmt.Errorf("resolving repos: %w", err) } @@ -128,8 +128,13 @@ func Upgrade(ctx context.Context, cfg UpgradeConfig, defer func() { <-sem }() resolvedCfg := cfg.Manifest.ResolveConfigForEntry(rr.Owner, rr.Repo, rr.Entry) - fc := ForgeConfigFor(resolvedCfg.Forge) - result := upgradeRepo(ctx, client, commitFn, rr.Owner, rr.Repo, resolvedCfg, cfg, fc, progress) + fc, err := clients.ConfigFor(resolvedCfg.Forge) + if err != nil { + results[idx] = UpgradeResult{Owner: rr.Owner, Repo: rr.Repo, Error: err} + return + } + resolvedCfg.ForgeConfig = fc + result := upgradeRepo(ctx, resolvedCfg, cfg, commitFn, progress) results[idx] = result }(i, rr) } @@ -138,14 +143,16 @@ func Upgrade(ctx context.Context, cfg UpgradeConfig, return results, nil } -func upgradeRepo(ctx context.Context, client forge.Client, - commitFn ScaffoldCommitFunc, - owner, repo string, +func upgradeRepo(ctx context.Context, resolvedCfg ResolvedConfig, cfg UpgradeConfig, - fc ForgeConfig, + commitFn ScaffoldCommitFunc, progress ProgressFunc) UpgradeResult { + owner := resolvedCfg.Owner + repo := resolvedCfg.Repo + client := resolvedCfg.ForgeConfig.Client + fc := resolvedCfg.ForgeConfig repoFullName := owner + "/" + repo result := UpgradeResult{Owner: owner, Repo: repo} diff --git a/internal/repos/upgrade_test.go b/internal/repos/upgrade_test.go index 61f539bde6..12004df322 100644 --- a/internal/repos/upgrade_test.go +++ b/internal/repos/upgrade_test.go @@ -58,7 +58,7 @@ func TestUpgrade_AllBehindTarget(t *testing.T) { MaxConcurrency: 2, } - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -92,7 +92,7 @@ func TestUpgrade_AllAtTarget(t *testing.T) { MaxConcurrency: 2, } - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -135,7 +135,7 @@ func TestUpgrade_MixedStates(t *testing.T) { MaxConcurrency: 4, } - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -168,7 +168,7 @@ func TestUpgrade_ForceOverridesNewerCheck(t *testing.T) { MaxConcurrency: 2, } - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -193,7 +193,7 @@ func TestUpgrade_RefOverride(t *testing.T) { MaxConcurrency: 2, } - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -220,7 +220,7 @@ func TestUpgrade_RepoFilter(t *testing.T) { MaxConcurrency: 2, } - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -251,7 +251,7 @@ func TestUpgrade_DryRun(t *testing.T) { MaxConcurrency: 2, } - results, err := Upgrade(context.Background(), cfg, fc, dryRunCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), dryRunCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -281,7 +281,7 @@ func TestUpgrade_FloatingTargetRefSkipped(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -305,7 +305,7 @@ func TestUpgrade_FloatingCurrentRefSkipped(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -329,7 +329,7 @@ func TestUpgrade_PartialVersionTargetSkipped(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -353,7 +353,7 @@ func TestUpgrade_PartialVersionCurrentRefSkipped(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -377,7 +377,7 @@ func TestUpgrade_WorkflowNotFound(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -405,7 +405,7 @@ func TestUpgrade_CommitError(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, errCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), errCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -438,7 +438,7 @@ func TestUpgrade_VerifiesWorkflowContent(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, recordingCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), recordingCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -514,7 +514,7 @@ func TestUpgrade_NoTargetRef(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -543,7 +543,7 @@ func TestUpgrade_NonSemverCurrentRef(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -572,7 +572,7 @@ func TestUpgrade_PerRepoOverrideRef(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 2} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -616,7 +616,7 @@ func TestUpgrade_YAMLExtension(t *testing.T) { return nil } - results, err := Upgrade(context.Background(), cfg, fc, commitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), commitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -648,7 +648,7 @@ func TestUpgrade_ProgressCallback(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - _, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, progressFn) + _, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, progressFn) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -918,7 +918,7 @@ func TestUpgrade_ContextCancellation(t *testing.T) { m := newUpgradeManifest("v2.3.0") cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - _, err := Upgrade(ctx, cfg, fc, noopCommitFn, nil) + _, err := Upgrade(ctx, cfg, newTestClientFactory(fc), noopCommitFn, nil) if err == nil { t.Error("expected context cancellation error") } @@ -1044,7 +1044,7 @@ func TestUpgrade_APIErrorOnWorkflowRead(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1104,7 +1104,7 @@ func TestUpgrade_DirectFlagPassedToCommitFn(t *testing.T) { MaxConcurrency: 1, } - results, err := Upgrade(context.Background(), cfg, fc, trackingCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), trackingCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1119,7 +1119,7 @@ func TestUpgrade_DirectFlagPassedToCommitFn(t *testing.T) { fc.FileContents["acme-corp/api-server/.github/workflows/fullsend.yml"] = makeWorkflow("v2.1.0") cfg.Direct = true - results, err = Upgrade(context.Background(), cfg, fc, trackingCommitFn, nil) + results, err = Upgrade(context.Background(), cfg, newTestClientFactory(fc), trackingCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1162,7 +1162,7 @@ jobs: } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1, Direct: true} - results, err := Upgrade(context.Background(), cfg, fc, commitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), commitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1231,7 +1231,7 @@ func TestUpgrade_PrereleaseDowngradeBlocked(t *testing.T) { MaxConcurrency: 1, } - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1285,7 +1285,7 @@ func TestUpgrade_InvalidManifestRef(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1383,7 +1383,7 @@ func TestUpgrade_SHAPinnedRepoPreservesPin(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, recordingCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), recordingCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1433,7 +1433,7 @@ func TestUpgrade_TagOnlyRepoStaysTagOnly(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, recordingCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), recordingCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1469,7 +1469,7 @@ func TestUpgrade_SHAPinnedTagResolutionError(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1517,7 +1517,7 @@ func TestUpgrade_MixedPinningStyles(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 2} - results, err := Upgrade(context.Background(), cfg, fc, recordingCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), recordingCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1571,7 +1571,7 @@ func TestUpgrade_DryRunSHAPinnedSkipsGetRef(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, DryRun: true, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, commitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), commitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1643,7 +1643,7 @@ func TestUpgrade_SkipReasonMessages(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1677,7 +1677,7 @@ func TestUpgrade_SHAPinnedAlreadyAtTarget(t *testing.T) { } cfg := UpgradeConfig{Manifest: m, MaxConcurrency: 1} - results, err := Upgrade(context.Background(), cfg, fc, noopCommitFn, nil) + results, err := Upgrade(context.Background(), cfg, newTestClientFactory(fc), noopCommitFn, nil) if err != nil { t.Fatalf("unexpected error: %v", err) }