diff --git a/pkg/agent/workspace.go b/pkg/agent/workspace.go index c535868ee..2cf59985b 100644 --- a/pkg/agent/workspace.go +++ b/pkg/agent/workspace.go @@ -313,12 +313,17 @@ func ensureGit(ctx context.Context, agentConfig *provider2.ProviderAgentConfig) if command.Exists("git") { return nil } - if local, _ := agentConfig.Local.Bool(); local { + if isLocalAgent(agentConfig) { return errors.New("git not installed: install git and add it to PATH") } return git.InstallBinary(ctx) } +func isLocalAgent(agentConfig *provider2.ProviderAgentConfig) bool { + local, _ := agentConfig.Local.Bool() + return local +} + func setupGitSSH( options provider2.CLIOptions, agentConfig *provider2.ProviderAgentConfig, @@ -418,7 +423,8 @@ func cloneViaGit(ctx context.Context, p CloneWorkspaceParams, extraEnv []string) repo := git.At(p.WorkspaceDir, git.WithStrictHostKeyChecking(p.Options.StrictHostKeyChecking), git.WithEnv(extraEnv)) - if err := repo.CloneFromInfo(ctx, gitInfo, p.Helper, getGitOptions(p.Options)...); err != nil { + gitOpts := getGitOptions(p.Options, p.AgentConfig) + if err := repo.CloneFromInfo(ctx, gitInfo, p.Helper, gitOpts...); err != nil { return failedClone(p.WorkspaceDir, "clone repository", err) } return nil @@ -452,7 +458,10 @@ func applyDevsyIgnore(workspaceDir string) error { return nil } -func getGitOptions(options provider2.CLIOptions) []git.Option { +func getGitOptions( + options provider2.CLIOptions, + agentConfig *provider2.ProviderAgentConfig, +) []git.Option { var gitOpts []git.Option if options.GitCloneStrategy != "" { gitOpts = append(gitOpts, git.WithCloneStrategy(options.GitCloneStrategy)) @@ -468,6 +477,7 @@ func getGitOptions(options provider2.CLIOptions) []git.Option { } else { gitOpts = append(gitOpts, git.WithLFSMode(options.GitLFSMode)) } + gitOpts = append(gitOpts, git.WithAllowLFSInstall(!isLocalAgent(agentConfig))) if options.GitCloneRecursiveSubmodules { gitOpts = append(gitOpts, git.WithRecursiveSubmodules()) } diff --git a/pkg/agent/workspace_test.go b/pkg/agent/workspace_test.go index 039baf80f..cda85ebf7 100644 --- a/pkg/agent/workspace_test.go +++ b/pkg/agent/workspace_test.go @@ -2,10 +2,33 @@ package agent import ( "testing" + + provider2 "github.com/devsy-org/devsy/pkg/provider" + "github.com/devsy-org/devsy/pkg/types" ) const explicitAgentDir = "/some/dir" +func TestIsLocalAgent(t *testing.T) { + cases := []struct { + name string + local types.StrBool + want bool + }{ + {"local true", "true", true}, + {"local false", "false", false}, + {"unset defaults to remote", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := isLocalAgent(&provider2.ProviderAgentConfig{Local: tc.local}) + if got != tc.want { + t.Errorf("isLocalAgent(%q) = %v, want %v", tc.local, got, tc.want) + } + }) + } +} + func withContainerDetector(t *testing.T, fn func() bool) { t.Helper() prev := containerDetector diff --git a/pkg/git/clone.go b/pkg/git/clone.go index 2a683e30d..55c7a18a1 100644 --- a/pkg/git/clone.go +++ b/pkg/git/clone.go @@ -107,12 +107,18 @@ func WithSkipLFS() Option { return WithLFSMode(LFSSkip) } +// WithAllowLFSInstall permits SetupLFS to install the git-lfs binary itself. +func WithAllowLFSInstall(allow bool) Option { + return func(c *cloneConfig) { c.allowLFSInstall = allow } +} + // cloneConfig is the resolved set of clone options. type cloneConfig struct { strategy CloneStrategy branch string credentialHelper string recurseSubmodules bool + allowLFSInstall bool lfsMode LFSMode } diff --git a/pkg/git/lfs.go b/pkg/git/lfs.go index 1a7034935..b97b9aaff 100644 --- a/pkg/git/lfs.go +++ b/pkg/git/lfs.go @@ -41,8 +41,11 @@ func cloneEnvForLFS() []string { return nil } +// lfsInstaller is overridable in tests. +var lfsInstaller = InstallLFS + // SetupLFS configures Git LFS in the repository. -func (r *Repo) SetupLFS(ctx context.Context, mode LFSMode) { +func (r *Repo) SetupLFS(ctx context.Context, mode LFSMode, allowInstall bool) { if mode == LFSSkip { return } @@ -50,14 +53,8 @@ func (r *Repo) SetupLFS(ctx context.Context, mode LFSMode) { return } - if !command.Exists(binGitLFS) { - if err := InstallLFS(ctx); err != nil { - log.Warnf( - "repository uses git-lfs but it could not be installed, LFS files will be pointer stubs: %v", - err, - ) - return - } + if !command.Exists(binGitLFS) && !ensureLFSBinary(ctx, allowInstall) { + return } if err := r.lfs(ctx, "install", "--local"); err != nil { @@ -74,6 +71,25 @@ func (r *Repo) SetupLFS(ctx context.Context, mode LFSMode) { } } +// ensureLFSBinary installs git-lfs when allowInstall is set, reporting +// whether the binary is now available. +func ensureLFSBinary(ctx context.Context, allowInstall bool) bool { + if !allowInstall { + log.Info( + "repository uses git-lfs but the binary is not installed, LFS files will be pointer stubs", + ) + return false + } + if err := lfsInstaller(ctx); err != nil { + log.Warnf( + "repository uses git-lfs but it could not be installed, LFS files will be pointer stubs: %v", + err, + ) + return false + } + return true +} + // lfs runs a `git lfs ` subcommand in the repository, returning any // captured output alongside the error for diagnostics. func (r *Repo) lfs(ctx context.Context, args ...string) error { diff --git a/pkg/git/lfs_test.go b/pkg/git/lfs_test.go index c2b8116c9..018713d4e 100644 --- a/pkg/git/lfs_test.go +++ b/pkg/git/lfs_test.go @@ -2,7 +2,9 @@ package git import ( "context" + "errors" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -89,7 +91,7 @@ func TestSetupLFSModeCommands(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { dir, fake := newLFSRepo(t) - At(dir, WithRunner(fake)).SetupLFS(context.Background(), tc.mode) + At(dir, WithRunner(fake)).SetupLFS(context.Background(), tc.mode, false) var got []string for _, args := range lfsSubcommands(fake) { @@ -100,6 +102,63 @@ func TestSetupLFSModeCommands(t *testing.T) { } } +func TestSetupLFSSkipsWhenBinaryMissingAndInstallNotAllowed(t *testing.T) { + hideGitLFSFromPath(t) + stubLFSInstaller(t, func(context.Context) error { + t.Fatal("lfsInstaller must not be called when allowInstall is false") + return nil + }) + + dir, fake := newLFSRepo(t) + At(dir, WithRunner(fake)).SetupLFS(context.Background(), LFSFull, false) + + if got := lfsSubcommands(fake); got != nil { + t.Errorf("lfs subcommands = %v, want none", got) + } +} + +func TestSetupLFSInstallsWhenBinaryMissingAndInstallAllowed(t *testing.T) { + hideGitLFSFromPath(t) + + var called bool + stubLFSInstaller(t, func(context.Context) error { + called = true + return errors.New("install unavailable in this environment") + }) + + dir, fake := newLFSRepo(t) + At(dir, WithRunner(fake)).SetupLFS(context.Background(), LFSFull, true) + + if !called { + t.Error("lfsInstaller was not called despite allowInstall being true") + } + if got := lfsSubcommands(fake); got != nil { + t.Errorf("lfs subcommands = %v, want none (install failed)", got) + } +} + +func stubLFSInstaller(t *testing.T, fn func(context.Context) error) { + t.Helper() + original := lfsInstaller + lfsInstaller = fn + t.Cleanup(func() { lfsInstaller = original }) +} + +func hideGitLFSFromPath(t *testing.T) { + t.Helper() + + gitPath, err := exec.LookPath("git") + if err != nil { + t.Skip("git not found on PATH") + } + + binDir := t.TempDir() + if err := os.Symlink(gitPath, filepath.Join(binDir, "git")); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", binDir) +} + func TestRepoUsesLFS(t *testing.T) { cases := []struct { name string diff --git a/pkg/git/repo.go b/pkg/git/repo.go index 656a90bb4..8703213f6 100644 --- a/pkg/git/repo.go +++ b/pkg/git/repo.go @@ -173,7 +173,7 @@ func (r *Repo) CloneFromInfo( // Bare clones have no worktree to hydrate. if c.strategy != BareCloneStrategy { - r.SetupLFS(ctx, c.lfsMode) + r.SetupLFS(ctx, c.lfsMode, c.allowLFSInstall) } return nil }