Skip to content

ci: Add cargo to dependabot so openjd-rs releases open their own PR #336

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
leongdl wants to merge 1 commit into
base: mainline
Choose a base branch
from
Open

ci: Add cargo to dependabot so openjd-rs releases open their own PR #336

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .github/dependabot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,25 @@ updates:
update-types:
- "minor"
- "patch"
- package-ecosystem: "cargo"

@github-actions github-actions Bot Aug 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
fi

The 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 --update and 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.

@leongdl leongdl Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

directory: "/" # Workspace root; rust-bindings is discovered as a member
schedule:
interval: "weekly"
day: "monday"
commit-message:
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

@github-actions github-actions Bot Aug 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

@leongdl leongdl Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# as breaking, so openjd-expr 0.3 -> 0.4 deserves its own review rather
# than riding along with a patch.

@github-actions github-actions Bot Aug 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

@leongdl leongdl Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

groups:
cargo-patch:
patterns:
- "*"
update-types:
- "patch"

- package-ecosystem: "github-actions"
directory: "/" # Location of package manifests
schedule:
Expand Down
Loading