Add --git-fetch-base-branch to fetch the base branch at checkout - #4285
Add --git-fetch-base-branch to fetch the base branch at checkout#4285jasonwbarnett wants to merge 1 commit into
Conversation
1ff355e to
66765f3
Compare
A job's checkout fetches refs/pull/N/head plus the commit for a pull request, and the commit alone for a branch build. Neither updates a branch's remote-tracking ref, so refs/remotes/origin/<base> holds whatever an earlier build left in a reused checkout directory. --fetch-diff-base covers `pipeline upload`'s own if_changed diff. Any other command in the job that asks what changed -- a pipeline generator, a test selector, a script running `git merge-base HEAD origin/main` -- still reads the stale ref, and a stale base only ever widens a diff, so nothing fails and nobody notices. The new option fetches the base branch during checkout, so every job in the build starts from its current tip. The base is the first non-empty value of BUILDKITE_PULL_REQUEST_BASE_BRANCH or BUILDKITE_PIPELINE_DEFAULT_BRANCH, and nothing is fetched when that is the branch being built. Available on `agent start` as well as `bootstrap`, so it can be enforced fleet-wide, and protected so a pipeline cannot turn it off. The fetch is separate from the job's own because FETCH_HEAD resolves to the first ref fetched, uses an explicit forced refspec so the remote-tracking ref lands regardless of remote.origin.fetch, and is neither fatal nor retried: the checkout is complete without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
66765f3 to
2ca99c0
Compare
|
hello. real life human being here. I reviewed the output from Claude and I'm going to be deploying this out to our production fleet shortly as it's urgent and we need this functionality to guarantee base branches are up-to-date for use when answering the question: "What changed in this PR?" or "What changed in this multi-commit push to main from the merge queue?" Altana is a customer of Buildkite and we would like to see this get merged and released, although we're having issues with the latest release and are stuck on v3.133.0 and we haven't yet diagnosed why our fleet went to hell when we released Elastic CI Stack v6.71.0 which bundles buildkite-agent v3.136.2. I didn't diagnose if it was the stack or the agent, but this came to my mind as I was typing this so I likely need to rollout a test queue using the newest CI stack and agent to see if those issues are resolved. |
|
This (https://github.com/altana-ai/buildkite-agent/releases/tag/v3.133.0-altana.1) is now deployed to production and working beautifully. For BK employee(s) to see it in action: https://buildkite.com/altana/monorepo-build/builds/152465/canvas?sid=01a05596-b6c2-4ce5-918f-85f665d5e8ca&tab=output |
|
Thanks for opening this PR @jasonwbarnett. I’ve created a ticket to track it internally. |
Description
A job's checkout fetches
refs/pull/N/headplus the commit for a pull request, and the commit alone for a branch build. Neither updates a branch's remote-tracking ref, sorefs/remotes/origin/<base>in a reused checkout directory holds whatever an earlier build left there.--fetch-diff-basecoverspipeline upload's ownif_changeddiff. Anything else in the job that asks what changed — a pipeline generator, a test selector, a script runninggit merge-base HEAD origin/main— still reads the stale ref, and a stale base only ever widens a diff, so nothing fails and nobody notices.--git-fetch-base-branchfetches the base branch during checkout instead, so every job in the build starts from its current tip. Off by default. It is onagent startas well asbootstrap, so it can be turned on once for a fleet, and it is inenv/protected.goso a pipeline cannot turn it back off.Alternatives considered:
BUILDKITE_GIT_FETCH_FLAGScan't carry a refspec (flags land before--);BUILDKITE_REFSPECreplaces the PR fetch rather than adding to it; doing it per step with a plugin is what we do today, and it's an opt-in every new step has to remember.Context
Fixes #4284.
Four decisions worth a reviewer's attention, each pinned by a test:
FETCH_HEADresolves to the first ref fetched and the checkout depends on that.+refs/heads/<base>:refs/remotes/origin/<base>), so the remote-tracking ref lands regardless ofremote.origin.fetchand follows a force-pushed base branch instead of stopping at a non-fast-forward rejection.GitSkipFetchExistingCommitsearly return, because a job that asked for the base branch asked for it whether or not its own commit still needs fetching.The base branch is the first non-empty value of
BUILDKITE_PULL_REQUEST_BASE_BRANCHorBUILDKITE_PIPELINE_DEFAULT_BRANCH— the precedencepipeline uploadalready uses for--git-diff-base, minus its final literalmain, since guessing a name would mean a failed fetch on every repository that calls its default branch something else. Nothing is fetched when the base is the branch being built.Changes
internal/job/checkout_fetch.go:baseBranchToFetchandfetchBaseBranch, called fromfetchSource; one new span attribute (git.fetch_base_branch).internal/job/config.go,clicommand/global.go,clicommand/bootstrap.go,clicommand/agent_start.go,agent/agent_configuration.go,agent/job_runner.go: the option, wired the same way as--git-skip-fetch-existing-commits(including both checkout-override modes).env/protected.go:BUILDKITE_GIT_FETCH_BASE_BRANCH.internal/job/checkout_fetch_base_branch_test.go: 9 cases against a realgithttptestremote and a checkout cloned before the base branch moved.docs/remote-git-mirrors.md: one line in §4.1, which describes what the checkout fetches today. Drop that hunk if you'd rather own that doc — I added it only so the ground-truth walkthrough stays true.The user-facing flag reference lives in
buildkite/docs, so I have not touched it; say the word if you'd like a PR there once this lands.Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)./env/...,./agent/...and the full./internal/job/suite pass, andgolangci-lint runreports 0 issues on the touched packages. One unrelated failure,clicommand.TestAgentStartupHookWithAdditionalPaths, reproduces on a pristine checkout ofmainwhen the whole package runs, and passes in isolation.Each behaviour was checked by breaking it and confirming the tests catch it: removing the call (4 failures), moving it after the skip-fetch return (1), dropping the built-branch guard (1), dropping the default-branch fallback (1), using a plain refspec (1), dropping the leading
+(1), and making the failure fatal (1).Affiliation (optional, external contributors)
Altana.
Disclosures / Credits
Claude Code (Opus) wrote the implementation and the tests, and drafted this description, working from my problem statement and reviewing decisions with me as it went — including the two corrections above that came out of testing: dropping the retry, and moving the fetch ahead of the skip-fetch early return.