Conversation
… squash Brings a feature branch into dogfooding as a single squash commit, opened as a PR from a dogfood-<branch>-<sha12> branch. The feature's own commits never enter dogfooding's history, so a later reset can't make them look "already merged" and a later sync can't silently drop the feature when it graduates to develop. Each squash commit carries a Dogfood-Source: <branch>@<sha> trailer (plus Dogfood-Author for notifications); the first dogfood since the last reset squashes the whole feature, later ones apply only the changes since the previous dogfood (a commit-tree delta), so updates, removed files and force-pushed rebases apply correctly. --at <sha> dogfoods a specific commit (e.g. after the branch was deleted on graduation) and --full forces a full squash (e.g. after a revert). It refuses when another dogfood PR for the branch or a reset PR is open, or when the feature is based on develop commits dogfooding doesn't have yet (sync first). It runs in a temporary worktree (kept on conflicts, with the steps to finish by hand). Verified in a sandbox with a fake gh.
… a PR, preserving history Resets dogfooding's content to exactly match develop without force-pushing: a merge -s ours --no-commit + read-tree -u --reset recipe produces a commit whose tree is develop's exact tree and whose parents are the dogfooding tip and develop's tip, so future syncs see develop as already incorporated. A reset therefore never misses anything from develop: after it, dogfooding has exactly develop's content. The script never touches dogfooding directly: it pushes a reset-dogfooding-<dogfooding sha12>-<develop sha12> branch and opens a PR, so it works with branch protection. Re-running against the same tips prints the open PR; a leftover branch without an open PR makes it fail instead of being overwritten. It runs in a temporary worktree, so the checkout it's run from is never touched. Exits without opening a PR if dogfooding already has develop's exact content. The reset commit skips local pre-commit hooks: it only contains develop's exact content. Features reach dogfooding through dogfooding/feature.sh as squash commits with a Dogfood-Source trailer, so the reset can tell what it removes: the features dogfooded since the last reset (found by the reset commit's subject) that haven't graduated to develop are listed in the prompt and the PR body, mentioning their authors, with the dogfooding/feature.sh command to run once the reset is merged. Features whose dogfooded commit can't be found locally or on origin are listed separately with an unknown status; plain sync merges aren't listed as discarded. Open dogfood PRs are listed too, since they were computed against content the reset removes: they must be closed (deleting their branch) and re-created afterwards. Verified in a sandbox with a fake gh: graduated features (including ones rebased after being dogfooded), branches that kept going after an earlier PR merged, branch names reused by an older merged PR, deleted branches, missing dogfooded commits, open delta PRs and back-to-back resets.
… develop Pushes develop's current tip as a sync-dogfooding-<develop sha12> side branch and opens a PR against dogfooding, without discarding dogfooding's in-flight work. Nothing is checked out or merged locally: the side branch is develop's tip as-is, so GitHub computes the merge itself, and a conflicting sync still produces a visible PR marked "must be resolved" instead of failing with nothing pushed. The PR body includes the resolution steps (check out the branch, merge dogfooding into it, fix conflicts, push back to the same branch). Exits without pushing or opening a PR if dogfooding already contains develop. Re-running against the same develop tip prints the open PR; a leftover branch without an open PR makes it fail instead of being overwritten. Older open sync PRs are mentioned in the new PR's body. Features dogfooded with dogfooding/feature.sh (tracked by their Dogfood-Source trailer since the last reset) that graduate to develop in this sync at a different commit than the one dogfooded are flagged in the output and the PR body, mentioning their authors: the sync brings develop's version in, but code that only existed in the dogfooded version would stay in dogfooding, so they must be dogfooded again at their graduated commit (dogfooding/feature.sh --at) once the sync is merged. Verified in a sandbox with a fake gh: the warning fires only for features graduating in this sync at a different commit (not for same-commit graduations, older PRs reusing the branch name, or merges of an earlier part of a branch that kept going), following it removes the leftover code, and a failing gh only skips the check.
|
✅ All CI checks and tests passed. Datadog automation helped this PR pass. 🎉 All green!🧪 All tests passed 🔄 Datadog retried 1 test - 1 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: 0a8cca7 | Docs | View more details | Give us feedback! |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06accbb612
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| echo "Creating $RESET_BRANCH from origin/$DOGFOODING_BRANCH in a temporary worktree..." | ||
| git worktree add --quiet -b "$RESET_BRANCH" "$worktree_dir" "origin/$DOGFOODING_BRANCH" |
There was a problem hiding this comment.
When a dogfood PR merges after this reset branch is cut—possible for an already-open PR or while feature.sh has not yet observed the pending reset—GitHub merges the reset against the newer base and preserves nonconflicting base-side changes. The reset can therefore merge successfully while dogfooding still contains feature code that is absent from the discard and re-dogfood lists; revalidate the target tip before opening the PR or otherwise prevent dogfooding from advancing until the reset merges.
AGENTS.md reference: AGENTS.md:L139-L139
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I would say this is a real gap, but quite limited thanks to the already implemented checks in the scripts. Let's accept this for the moment, and we can implement branch protection enabling "require branches to be up to date before merging" if we see this is a real and frequent problem.
…o Sonatype snapshots The nexus publish plugin routes uploads based only on whether the version ends in -SNAPSHOT. Without it, 3.15.0-dogfood-<sha> was sent to the Maven Central staging API: nothing consumers could use, and a risk of a dogfood build being released to Maven Central if someone closed the deployment. With the suffix, publish:dogfooding uploads to the Sonatype snapshots repository, which Shop.ist and the Datadog app already read from. Each commit still gets its own coordinate (3.15.0-dogfood-<sha>-SNAPSHOT), but snapshot builds can be overwritten and Sonatype cleans them up after a retention period. This is temporary until dogfood builds can go to an internal repository (e.g. Depot), and this commit is meant to be reverted then.
Documents how to work with the dogfooding branch through ci/scripts/dogfooding: the rules (features only reach dogfooding through feature.sh, every PR is merged with a merge commit), the common tasks (dogfood or update a feature, sync, clean up after a feature graduates to develop, reset, remove a single feature), what happens on conflicts, publishing, what to do when a script stops, and how the scripts work. It also explains why features can't be merged straight into dogfooding: a reset keeps the old commits in history, so git would treat such a feature as already merged and a later sync would silently leave it out.
Adds a Dogfooding Branch section to AGENTS.md (CLAUDE.md links to it): agents read ci/scripts/dogfooding/README.md before helping with the dogfooding branch, never push to it, open feature PRs into it or merge into it with squash or rebase, use the scripts instead (reverting a feature is the only manual change), and ask before running them since they push branches and open PRs.
Wording and style fixes from the review: format develop as code, write the reset list items as full sentences, and small punctuation fixes in the "When a script stops" table. Co-authored-by: Esther Kim <41168354+estherk15@users.noreply.github.com>
06accbb to
07e1a79
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07e1a7921d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
I did a quick look, didn't go through ci/scripts/dogfooding yet.
| } | ||
|
|
||
| data class Dogfood(val shortSha: String) : Type() { | ||
| override val suffix: String = "-dogfood-$shortSha-SNAPSHOT" |
There was a problem hiding this comment.
I don't think we need SNAPSHOT suffix anymore, is it really needed?
There was a problem hiding this comment.
We still need it for now. Dogfood builds are still published to the Sonatype snapshots repository, since we don't have S3 or an internal repository for these artifacts yet. The nexus publish plugin picks the destination based only on whether the version ends in -SNAPSHOT: without it, publish:dogfooding would send the build to the Maven Central staging API.
We should have a proper internal solution for these artifacts in the coming months (Depot, once it supports Java artifacts), so I'd keep using Sonatype snapshots until then and do a single migration. Moving to S3 now would mean doing the work twice: once for S3, and again for the internal tool. This change is in its own commit 1714957 so it's easy to revert when we migrate.
| tags: [ "arch:amd64" ] | ||
| only: | ||
| - dogfooding | ||
| when: manual |
There was a problem hiding this comment.
do we actually need have it manual? seems every dogfood build has unique coordinates and published to S3 anyway, I guess we can afford publishing automatically on every merge to dogfooding?
There was a problem hiding this comment.
Good point. As mentioned in the other comment, we're still publishing to the Sonatype snapshots repository, not S3, but the reasoning still holds: every build has a unique coordinate, and I don't expect we'll publish many artifacts, so we can publish automatically. I removed when: manual, so the job now runs on every pipeline on dogfooding, but only after the verify and test jobs pass, same as publish:release-all on develop.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a8cca7b46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [[ "$head" =~ ^dogfood-(.+)-[0-9a-f]{12}$ ]]; then | ||
| OPEN_DOGFOOD="$OPEN_DOGFOOD- #$number $head @$author: gh pr close $number --delete-branch, then once this reset is merged ./ci/scripts/dogfooding/feature.sh ${BASH_REMATCH[1]} | ||
| " | ||
| elif [[ "$head" == reset-dogfooding-* ]]; then | ||
| older_prs="${older_prs:+$older_prs, }#$number" |
There was a problem hiding this comment.
When an open sync-dogfooding-* PR has conflicts, the documented resolution merges the current dogfooding branch into its head and may record conflict-resolution changes that are not in develop. This loop ignores such sync PRs, so they remain open after the reset; merging one afterward can reapply those resolutions or create conflicts against the freshly reset tree, violating the reset's promise to leave dogfooding exactly matching develop. Include open sync PRs in the close-and-recreate/supersede list just like dogfood PRs.
Useful? React with 👍 / 👎.
| elif git merge-base --is-ancestor "$sha" origin/develop; then | ||
| graduated=true | ||
| else | ||
| graduated=false |
There was a problem hiding this comment.
When the gh pr list request fails because of authentication, rate limiting, or a transient network error, a dogfooded SHA that is not an ancestor of develop is classified here as not graduated. That non-ancestry is normal when the feature was squash-merged or rebased, so the reset can instruct users to dogfood the obsolete SHA again and reintroduce old code after the newer feature is already in develop. If the API lookup failed and ancestry cannot positively prove graduation, put the feature in UNKNOWN rather than IN_FLIGHT.
Useful? React with 👍 / 👎.
| seen="$seen$branch " | ||
| unknown_entry="- $branch (dogfooded at ${sha:0:12}) ${author:-} | ||
| " | ||
| if git ls-remote --exit-code --heads origin "$branch" >/dev/null 2>&1; then |
There was a problem hiding this comment.
When the recorded source branch foo has been deleted but another branch such as team/foo exists, this check succeeds because git ls-remote patterns match against slash-separated ref tails. The reset then emits feature.sh foo instead of the recoverable feature.sh foo --at <sha> command, and that command fails because origin/foo does not exist. Query refs/heads/$branch so only the exact source branch counts.
Useful? React with 👍 / 👎.
| [ "$head" != "$sha" ] || continue | ||
| # The merged head only brought in part of what was dogfooded (the branch kept going after | ||
| # it): dogfooding already has everything develop now brings, so there's nothing left over. | ||
| ! git merge-base --is-ancestor "$head" "$sha" 2>/dev/null || continue | ||
| GRADUATED="$GRADUATED- $branch ${author:-}: dogfooded at ${sha:0:12}, graduated at ${head:0:12}. After merging this sync, run ./ci/scripts/dogfooding/feature.sh $branch --at $head |
There was a problem hiding this comment.
When the same feature branch has more than one PR merged into develop after the dogfooded SHA, this loop appends a cleanup command for every merged head. Following all listed commands can first update dogfooding to the newest head and then apply the reverse delta to an older head, leaving the branch behind the version now in develop. Select the latest relevant merged head and emit one feature.sh --at command for the feature.
Useful? React with 👍 / 👎.
| class AndroidConfigTest { | ||
|
|
||
| @Test | ||
| fun `M declare VERSION on a single line with forCi() W reading AndroidConfig source`() { |
There was a problem hiding this comment.
Is this test needed? I understand what it does - checks that file structure is okay for ci/scripts/merge-release-develop.sh to be able to parse it, but this requirement feels strange. Maybe we can avoid imposing this by using defined strategy for the merge conflict resolution? I would maybe suggest to check how devflow is doing this - it tries to resolve conflicts and if fails, it suggests to resolve them manully.
|
|
||
| | Script | What it does | | ||
| |---|---| | ||
| | `feature.sh <branch>` | Brings a feature branch into `dogfooding` as a single squash commit. | |
There was a problem hiding this comment.
what is the reason behind squashing everything?
3 participants
What does this PR do?
Adds the tooling to run the
dogfoodingbranch without force-pushes and compatible with branch protection:ci/scripts/dogfooding/feature.sh: dogfoods a feature branch as a single squash commit (tracked with aDogfood-Sourcetrailer); later runs only apply the changes since the previous dogfood.ci/scripts/dogfooding/sync.sh: opens a PR bringingdevelopintodogfooding, and flags features that graduated todevelopat a different commit than the one dogfooded.ci/scripts/dogfooding/reset.sh: opens a PR resettingdogfoodingto exactlydevelop's content while preserving history, listing the dogfooded features to re-add afterwards.<version>-dogfood-<sha>-SNAPSHOTversion computed from CI env vars (forCi()), instead of a committed patch, and are published by a new manualpublish:dogfoodingjob.AGENTS.mdsection pointing AI agents at it.Motivation
dogfoodingwas managed by hand with force-pushes and a committed version patch, which doesn't work once the branch is protected and loses in-flight work. Resetting without force-pushing keeps old commits in history: a feature merged directly intodogfoodingwould then look "already merged" after a reset, and a later sync would silently leave it out. Bringing features in only as squash commits (feature.sh) avoids that.Additional Notes
ci/scripts/dogfooding/README.md: it explains the workflow, the rules and what each script does, which makes the scripts easier to follow.dogfoodingonce withdevelop's tip (one-time force-push, before protection), adddogfoodingto the protected branches ruleset, then use the scripts.-SNAPSHOTsuffix is temporary, until dogfood builds can go to an internal repository (RUM-17785). Consumers (Shopist, Datadog app) need to switch to the pinned-dogfood-<sha>-SNAPSHOTversions.Review checklist (to be filled by reviewers)