feat: add sparse checkout support#3903
Conversation
| // current working tree. It returns true if sparse checkout was successfully | ||
| // applied for this build, so callers can adjust later behaviour (e.g. skip | ||
| // submodule init, which requires the full tree). | ||
| func (e *Executor) setupSparseCheckout(ctx context.Context) (bool, error) { |
There was a problem hiding this comment.
sparse-checkout set --cone requires git ≥ 2.26. On older git we warn and fall through to a normal full checkout rather than failing the build.
| setEnv("BUILDKITE_GIT_CHECKOUT_FLAGS", r.conf.AgentConfiguration.GitCheckoutFlags) | ||
| setEnv("BUILDKITE_GIT_CLONE_FLAGS", r.conf.AgentConfiguration.GitCloneFlags) | ||
| setEnv("BUILDKITE_GIT_FETCH_FLAGS", r.conf.AgentConfiguration.GitFetchFlags) | ||
| setEnv("BUILDKITE_SPARSE_CHECKOUT_PATHS", r.conf.AgentConfiguration.SparseCheckoutPaths) |
There was a problem hiding this comment.
Always exported (matching all other git flag env vars); empty value = full checkout
mcncl
force-pushed
the
SUP-6409/sparse_checkout
branch
from
318d9ef to
163cbea
Compare
DrJosh9000
left a comment
DrJosh9000
left a comment
There was a problem hiding this comment.
My first reaction is "why not _Git_SparseCheckoutPaths", but I'm not fussed
| EnvVar: "BUILDKITE_GIT_FETCH_FLAGS", | ||
| } | ||
|
|
||
| SparseCheckoutPathsFlag = cli.StringFlag{ |
There was a problem hiding this comment.
Consider using StringSliceFlag so that the cli package does the splitting for you (the field type can then be []string), but then of course you will have to re-join the strings for passing between agent and bootstrap
|
@DrJosh9000 thats a fair call and removes any chance of ambiguity |
mcncl
force-pushed
the
SUP-6409/sparse_checkout
branch
from
163cbea to
9e93039
Compare
zhming0
left a comment
zhming0
left a comment
There was a problem hiding this comment.
I left some suggestions re code organization, increase signal-to-noise ratio on integration test and adding an e2e test. 🙏🏿
so far no blocker, but I plan to take another look later 👀
| if len(r.conf.AgentConfiguration.GitSparseCheckoutPaths) > 0 { | ||
| setCheckoutEnv("BUILDKITE_GIT_SPARSE_CHECKOUT_PATHS", strings.Join(r.conf.AgentConfiguration.GitSparseCheckoutPaths, ",")) | ||
| } | ||
| setEnv("BUILDKITE_GIT_CLONE_MIRROR_FLAGS", r.conf.AgentConfiguration.GitCloneMirrorFlags) |
There was a problem hiding this comment.
This change does not seem to get mentioned on the PR description?
| requireCheckoutPath(t, tester.CheckoutDir(), "docs/readme.md", false) | ||
| } | ||
|
|
||
| func TestCheckingOutLocalGitProjectWithSparseCheckoutExistingGitDir(t *testing.T) { |
There was a problem hiding this comment.
It's getting a bit difficult to tell the difference between this test case and the case above. Code comment will be ideal, or alternatively maybe we can consolidate to reduce repetition and increase signal-to-noise ratio?
There was a problem hiding this comment.
Given the size of checkout.go and the fact that this code change is quite isolated, I recommend opening a internal/job/checkout_sparse.go file.
There was a problem hiding this comment.
Same recommendation for this code change, we can move these new tests to their own focused test file, so checkout_test.go focus on testing the core, happy path.
| if len(paths) == 0 { | ||
| e.disableSparseCheckoutIfConfigured(ctx) | ||
| return false, nil | ||
| } | ||
|
|
||
| ok, err := gitVersionAtLeast(ctx, e.shell, 2, 26) | ||
| if err != nil { | ||
| e.shell.Warningf("Sparse checkout requires git >= 2.26; falling back to full checkout (%v)", err) | ||
| e.disableSparseCheckoutIfConfigured(ctx) | ||
| return false, nil | ||
| } | ||
| if !ok { | ||
| e.shell.Warningf("Sparse checkout requires git >= 2.26; falling back to full checkout") | ||
| e.disableSparseCheckoutIfConfigured(ctx) | ||
| return false, nil | ||
| } | ||
|
|
||
| e.shell.Commentf("Setting up sparse checkout for paths: %s", strings.Join(paths, ",")) | ||
| args := append([]string{"sparse-checkout", "set", "--cone"}, paths...) | ||
| if err := e.shell.Command("git", args...).Run(ctx); err != nil { | ||
| return false, fmt.Errorf("setting sparse checkout paths: %w", err) | ||
| } |
There was a problem hiding this comment.
I suggest writing an end-to-end test to verify that the behavior of setupSparseCheckout is safe when an agent executes jobs that require sparse checkout interleaved with other jobs that don't require it.
4 participants
Description
This adds support for sparse checkout logic within the agent, allowing it to read
BUILDKITE_SPARSE_CHECKOUT_PATHSfrom the job environment, running the necessarygitcheckout process.Changes
sparse-checkout-pathsin the agent CLI onagent startandbootstrapExecutor.setupSparseCheckoutruns after fetch and before checkoutTesting
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)Disclosures / Credits
I used Codex to assist with writing this change. The core function (non
_test.go) changes were implemented by me, I then used Codex to assist with writing theTestfunctions.I used Amp to check the changes, which it highlighted a couple of issues with (fixed) and a missing test case (added).