Repository navigation
a PR build should merge with the master branch first - #37881
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The merge may fail without Git identity/editor settings, and the fetch may leave origin/master stale.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates the Buildkite PR build to test the branch after integrating the latest master.
Changes:
- Fetches master before testing.
- Merges master before running
basic-search-test.
File summaries
| File | Description |
|---|---|
.buildkite/Makefile |
Adds master fetch and merge steps to the PR build. |
Review details
Suppressed comments (1)
.buildkite/Makefile:47
git fetch origin masterfetches the requested branch intoFETCH_HEAD; it does not refresh theorigin/masterremote-tracking ref used on the next line. If this checkout already has a staleorigin/master, the build will merge and test the wrong master revision, defeating the purpose of this target. MergeFETCH_HEADhere, or give the fetch an explicit destination such asmaster:refs/remotes/origin/master.
git merge origin/master
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Previously the "pr" build target ran basic-search-test directly against the PR branch's tip, so it never caught cases where the PR's changes conflict with, or are incompatible with, what's currently on master. Merging master into the branch before running the test suite makes the PR build validate the same integrated state that would exist after the PR is merged, catching integration issues earlier. Fetch origin/master explicitly before merging, since the checkout used for a build may not have an up-to-date (or any) local master branch.
dd05870 to
d2b35f8
Compare
| git fetch origin master:refs/remotes/origin/master && \ | ||
| git -c user.name=buildkite -c user.email=buildkite@vespa.ai merge --no-edit origin/master && \ | ||
| git tag v$${VESPA_VERSION} && \ | ||
| $(MAKE) VESPA_VERSION=$${VESPA_VERSION} -f .buildkite/Makefile basic-search-test |
There was a problem hiding this comment.
😅 That's not entirely correct, we're building Vespa X.Y.Z+1, right?
Any implications at setting the VESPA_VERSION?
There was a problem hiding this comment.
😅 That's not entirely correct, we're building Vespa
X.Y.Z+1, right? Any implications at setting the VESPA_VERSION?
since it's a PR build it doesn't really matter, but it's probably best if it's a version number that can't possibly be a "real" version. We have some things looking at version number (protocol negotiation stuff) but nothing that looks at "Z" (last change was with 8.310).
Previously the "pr" build target ran basic-search-test directly against the PR branch's tip, so it never caught cases where the PR's changes conflict with, or are incompatible with, what's currently on master. Merging master into the branch before running the test suite makes the PR build validate the same integrated state that would exist after the PR is merged, catching integration issues earlier.
Fetch origin/master explicitly before merging, since the checkout used for a build may not have an up-to-date (or any) local master branch.