ci: fetch only the number of commits needed for checks#73
Conversation
c9dc3b3 to
77c6f10
Compare
77c6f10 to
cd32e5c
Compare
|
https://github.com/owncloud/reusable-workflows/actions/runs/28351542028/job/83985317067?pr=73 https://github.com/owncloud/reusable-workflows/actions/runs/28351542028/job/83985331368?pr=73 works |
DeepDiver1975
left a comment
There was a problem hiding this comment.
Reviewed as a maintainer — the approach here is correct, and I want to confirm the key worry from the description is handled.
No invalid Actions arithmetic. ${{ github.event.pull_request.commits + 1 }} indeed doesn't work (Actions expressions have no +). This PR sidesteps that cleanly: the calculate-depth job substitutes the plain integer ${{ github.event.pull_request.commits }} into a shell variable, does the math in bash ($((PR_COMMITS + 1)), valid), and passes it on via $GITHUB_OUTPUT → fetch-depth: ${{ needs.calculate-depth.outputs.calculated_depth }}. 👍
+1 is right — no off-by-one. Validating the PR's N commits requires fetching those N commits plus their merge-base parent, so N+1 is the correct depth (a bare N would risk truncating the range). Keeping the +1 is the safe choice. The empty/non-PR fallback to depth 1 is sensible.
CI proves it end-to-end: Calculate commits in the PR and Validate semantic commits both ran under the new N+1 logic and passed — so the semantic-commit check still has enough history. All checks green, MERGEABLE.
On your question — "Do we care enough to add this extra code?" — the extra job is modest and the payoff (avoiding fetch-depth: 0 full-history clones on every PR) is real, so I think it's worth it. One minor thought, no change requested: the calculate-depth job is duplicated across semantic-git-message.yml and translation-sync.yml; if it spreads further, a tiny composite/reusable step could DRY it up — but two copies is fine for now.
Changelog: this is an internal CI-workflow optimization (no behavior change in the reusable workflows themselves), and recent infra PRs here didn't carry a changelog/unreleased/ fragment, so I don't think one is required — your call.
LGTM. Leaving the merge to you since it's your PR and you raised an open design question in the description.
🤖 Generated with Claude Code
Fixes #55
I discovered that you can't just do:
Calculations do not work there.
You have to do a whole little job before the real job, which gets the number of commits in the PR, does the calculation, and passes it to the next real job.
Do we care enough to add this extra code?