ci: Add cargo to dependabot so openjd-rs releases open their own PR - #336
ci: Add cargo to dependabot so openjd-rs releases open their own PR#336leongdl wants to merge 1 commit into
Conversation
Signed-off-by: David Leong <leongdl@amazon.com>
| update-types: | ||
| - "minor" | ||
| - "patch" | ||
| - package-ecosystem: "cargo" |
There was a problem hiding this comment.
Enabling the cargo ecosystem will make every Dependabot Rust PR fail CI, because Dependabot updates Cargo.lock but cannot regenerate THIRD-PARTY-LICENSES.txt.
The third_party_licenses job in .github/workflows/rust_quality.yml runs scripts/check_third_party_licenses.sh, which renders the Rust section from Cargo.lock via cargo about generate and then hard-fails on any diff:
if ! diff -u "$OUTPUT_FILE" "$generated"; then
...
exit 1
fiThe committed file embeds exact crate versions (** tokio; version 1.53.1, ** memchr; version 2.8.3, ...), so a bump of any crate — including a transitive patch bump inside the cargo-patch group — changes the rendered output and trips the check. There is no path for a bot-authored PR to fix this on its own, so cargo Dependabot PRs will land permanently red and need a manual scripts/check_third_party_licenses.sh --update commit pushed onto each one.
Worth deciding up front which way to go, e.g.:
- add a job (or a Dependabot-triggered workflow with
contents: write) that runs--updateand commits back to the Dependabot branch, or - make the license check advisory / non-blocking for
dependabot[bot]-authored PRs, or - accept the manual step and document it.
Note this is specific to cargo: the existing pip group only bumps requirements-*.txt (dev/test), which the script does not include since it resolves Python deps from the installed wheel's runtime closure.
There was a problem hiding this comment.
Correct, and already known — the PR description covers it under "The one manual step this leaves", measured the same way. Taking the "decide up front" point, though, so here is the decision and the reason none of the three options is the right one.
The diagnosis is scoped too narrowly. The check is already non-hermetic, with no cargo involved. The Python section is regenerated by resolving pyproject.toml into a fresh venv, and pyproject.toml declares pydantic >= 2.10, < 3 — unpinned. The committed file records:
** pydantic; version 2.13.5 -- https://pypi.org/project/pydantic/
PyPI serves 2.13.5 today, so third_party_licenses is green by coincidence of timing. The next pydantic patch release reddens it on every PR from every author. So the closing note here — that this is specific to cargo because the pip group only touches requirements-*.txt — holds for what dependabot bumps, but not for what the script resolves.
That rules out option 2. Making the check advisory for dependabot[bot] leaves the Python side untouched and would mask the broader failure.
Option 1 is also not as cheap as it reads: a dependabot pull_request gets a read-only token, so committing back needs pull_request_target or a PAT, and the script builds a wheel from PR head — a write-capable token running PR-authored build code.
So: option 3 for now, with the manual step moved out of the PR description and into a comment in dependabot.yml (#355), since a description is not where anyone will look after merge. Fixing the check properly — regenerate-and-commit for any author, or pinning the Python resolve — is worth its own PR and would subsume the cargo case rather than special-casing it.
| # group and so each get their own individual PR, because every crate this | ||
| # package depends on is pre-1.0: for a 0.x crate cargo treats a minor bump | ||
| # as breaking, so openjd-expr 0.3 -> 0.4 deserves its own review rather | ||
| # than riding along with a patch. |
There was a problem hiding this comment.
The stated goal — "openjd-expr 0.3 -> 0.4 deserves its own review rather than riding along with a patch" — is defeated by .github/workflows/auto_approve.yml, which approves any PR authored by dependabot[bot] with no filtering on update type:
if: ${{ github.actor == 'dependabot[bot]' }}
steps:
- uses: dependabot/fetch-metadata@v3
id: metadata
- run: gh pr review --approve "$PR_URL"So the isolated-PR-per-minor-bump split buys separation but not review: a 0.3 -> 0.4 bump of openjd-expr/openjd-model/openjd-sessions — the crates that actually carry the API surface this package binds to — arrives pre-approved. That is the highest-risk update class this config can produce, and it is exactly the one that gets waved through.
fetch-metadata is already wired up (id: metadata) but its outputs are unused, so gating is cheap, e.g. restrict auto-approve to patch bumps:
if: steps.metadata.outputs.update-type == 'version-update:semver-patch'(That is a change to auto_approve.yml, not this file, but it is this PR that makes it load-bearing for Rust deps.)
There was a problem hiding this comment.
Correct. Raised as #356 rather than fixed here, because it is a change to a different file that also affects the existing pip and github-actions PRs.
Verified: no update-type filter, and fetch-metadata is wired up with id: metadata whose outputs are never read. So a breaking 0.x minor of a wrapped crate would indeed arrive pre-approved.
One severity correction: there is no auto-merge workflow in .github/workflows/, and dependabot auto-merge is not configured, so what an unconditional approval costs is the "someone looked at this" signal, not the merge. A human still clicks merge. Still worth closing, since giving those bumps their own review is the stated purpose of this entry.
Not taking the suggested condition as written. A global update-type == 'version-update:semver-patch' gate is larger than it looks: the pip and github-actions entries group minor with patch into one PR, and update-type is the highest change in the PR, so those grouped PRs report semver-minor and would stop being auto-approved — nearly every PR from both ecosystems. Their dependencies are also 1.x, where a minor is additive, so the risk motivating the gate does not apply. #356 scopes it:
if: >-
steps.metadata.outputs.package-ecosystem != 'cargo' ||
steps.metadata.outputs.update-type == 'version-update:semver-patch'That holds cargo minors, cargo majors, and any cargo group containing a minor, and leaves pip and github-actions exactly as they are today. #356 also names the gap it deliberately leaves: a pip or github-actions major is still auto-approved.
| prefix: "chore(deps):" | ||
| # Patch bumps are combined into a single PR. Minor bumps do not match this | ||
| # group and so each get their own individual PR, because every crate this | ||
| # package depends on is pre-1.0: for a 0.x crate cargo treats a minor bump |
There was a problem hiding this comment.
"every crate this package depends on is pre-1.0" is not accurate, and it is the premise the grouping choice rests on. Per rust-bindings/Cargo.toml:
| crate | req | pre-1.0? |
|---|---|---|
openjd-expr |
0.3.0 |
yes |
openjd-model |
0.5.0 |
yes |
openjd-sessions |
0.5.0 |
yes |
pyo3 |
0.29 |
yes |
pyo3-log |
0.13 |
yes |
pyo3-stub-gen |
0.22 |
yes |
log |
0.4 |
yes |
windows |
0.62 |
yes |
tokio |
1 |
no |
uuid |
1 |
no |
serde_json |
1 |
no |
For the three 1.x crates a minor bump is explicitly non-breaking under cargo's semver rules, so the "0.x minor == breaking, so isolate it" rationale does not apply to them — yet they are excluded from cargo-patch and will each open a standalone PR (tokio 1.53 -> 1.54, etc.) for a compatible update. Combined with transitive deps in Cargo.lock, that is a fair amount of avoidable PR churn.
If the intent is "isolate only the genuinely-breaking 0.x minors", consider adding a second group that sweeps up the 1.x minors, e.g.:
groups:
cargo-patch:
patterns: ["*"]
update-types: ["patch"]
cargo-stable-minor:
patterns: ["tokio", "uuid", "serde_json"]
update-types: ["minor"]Otherwise the comment should just be corrected to say most deps are pre-1.0 and the per-PR split is being accepted for all minors for simplicity.
There was a problem hiding this comment.
Correct, and fixed in #355, which supersedes this PR.
Verified against rust-bindings/Cargo.toml on current mainline: tokio = "1", uuid = "1" and serde_json = "1" are 1.x, so "every crate this package depends on is pre-1.0" is false, and it was the premise the grouping rested on. The comment no longer claims it.
One correction to the table: it is reading an older mainline. Current pins are openjd-expr 0.6.0, openjd-model 0.6.0, openjd-sessions 0.5.5, pyo3-stub-gen 0.23. Doesn't change the conclusion — all still pre-1.0 — but it does mean this branch was two weeks behind, which #355 also fixes by rebasing.
Not taking the suggested fix. A cargo-stable-minor group listing ["tokio", "uuid", "serde_json"] does not reach the transitive 1.x crates in Cargo.lock — memchr 2.x and friends — and cargo supports dependency-type: indirect, so dependabot will raise those. They are where most of the churn the finding objects to actually comes from. Inverting it covers them and states the intent directly:
groups:
cargo-patch:
patterns: ["*"]
update-types: ["patch"]
cargo-minor:
patterns: ["*"]
exclude-patterns: ["openjd-*"]
update-types: ["minor"]Dependabot places a dependency in the first group it matches, so an openjd-* patch still rides in cargo-patch and only minors reach the exclusion; openjd-* minors match no group and get their own PR, which is the whole point of the entry.
Checked by simulating those documented rules over 19 cases — openjd-* minors and patches, every 1.x and 0.x direct dep, two transitive crates, majors. All 19 route as intended. Against the single-group config in this PR the same check fails 10 cases, including tokio/uuid/serde_json minors, so the second group is load-bearing rather than decorative.
|
Superseded by #355 (this config, rebased and with the grouping corrected) and #356 (the auto-approve gate the review flagged). All three findings on this PR were correct on the facts and are answered inline. Summary of where each went:
Two places I did not take the suggested fix, both explained inline: the One thing the review missed that changed the decision on the license check: it is already non-hermetic without cargo. The Python section is regenerated by resolving Happy to close this one — leaving it open in case you would rather review the diff between the two. |
Dependabot watches
pipandgithub-actionsbut notcargo, so a newopenjd-expr/openjd-model/openjd-sessionsrelease is only noticed when somebody goes looking.#335 is the worked
example: the crates published, and picking them up was a manual chase for pins,
Cargo.lock, andTHIRD-PARTY-LICENSES.txt. This makes that arrive as a PR on its own.Loosening the version requirements would not have helped
Worth stating because it is the obvious alternative. The requirements are already permissive
enough —
openjd-model = "0.5.2"is^0.5.2, so>=0.5.2, <0.6.0, and a 0.5.3 satisfies itwithout an edit.
Cargo.lockis what actually pins the build, and it is committed and is what CIresolves from. So a patch pickup needs a
cargo updateand a commit no matter how loose therequirement string is, and a
*requirement would buy nothing while giving up the reproducibilitythe lock exists for.
Why patch-only grouping, unlike the pip entry
openjd-model0.5.2 -> 0.5.3openjd-expr0.3.0 -> 0.4.0Every crate here is pre-1.0, and cargo treats a minor bump of a 0.x crate as breaking. That is not
theoretical:
openjd-expr0.4.0 carried a[**breaking**]coercion change, andopenjd-model0.5.2 changed the job-side
StepScriptwire format. Those deserve their own CI run and their ownreview rather than riding along with a patch. The
pipentry groups minor and patch togetherbecause its dependencies are 1.0+, where minor is additive.
The one manual step this leaves
scripts/check_third_party_licenses.shfails on aCargo.lockchange alone, so that check will bered on every cargo PR until the file is regenerated [measured — a lock-only diff of the three
openjd versions failed the check]. The follow-up is one command pushed to the dependabot branch:
(Alternate, if you would rather it be zero-touch) a scheduled workflow that runs
cargo update -p openjd-expr -p openjd-model -p openjd-sessions, regenerates the license file, andopens the PR itself. That removes the manual step and scopes updates to the three crates that
matter, at the cost of ~40 lines of workflow YAML and a token with PR-write permission. I went with
dependabot because it matches what this repo already does; happy to switch if you prefer the
workflow.
Testing
Config parses as YAML and declares all three ecosystems; the
cargoentry'sdirectory: "/"iswhere
Cargo.tomlandCargo.lockactually live, withrust-bindingspicked up as a workspacemember. I cannot run dependabot itself, so the schedule and grouping behaviour are unverified
until it runs on Monday.