Skip to content

refactor(sdk-release): replace regex-based PR subject parsing with GitHub commit-to-PR association API in release workflow - #37219

Merged
sfreudenthaler merged 2 commits into
mainfrom
37200-sdk-breaking-change-gate-replace-squash-subject-regex-with-commitsprs-api
Aug 27, 2026
Merged

refactor(sdk-release): replace regex-based PR subject parsing with GitHub commit-to-PR association API in release workflow#37219
sfreudenthaler merged 2 commits into
mainfrom
37200-sdk-breaking-change-gate-replace-squash-subject-regex-with-commitsprs-api

Conversation

@KevinDavilaDotCMS

@KevinDavilaDotCMS KevinDavilaDotCMS commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

To fix the new mechanism without squash-merge

This PR fixes: #37200

@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @KevinDavilaDotCMS's task in 3m 33s —— View job


Code Review

Reviewed the current state of .github/workflows/cicd_6-release.yml (both commits in this PR) against origin/main.

  • Read the changed workflow file
  • Analyze the commit→PR API migration + the follow-up "fail loudly" commit
  • Address @sfreudenthaler's rate-limit (429) question

New Issues

  • 🟠 High: .github/workflows/cicd_6-release.yml:155 — The COMMIT_SHAS fetch still swallows errors with || true, re-introducing the exact silent-pass this PR exists to fix. The follow-up commit hardened the per-commit lookup (lines 165–171) to fail loudly, but the initial compare call was left with --jq '.commits[].sha' || true. If that call hits a 429 / transient 5xx, COMMIT_SHAS becomes empty, the for loop iterates zero times, PR_NUMBERS is empty → BREAKING_PRS is empty → the gate passes silently. This is precisely the failure mode the PR's own comment (lines 158–164) warns against — the compare call is just as prone to rate-limit failure as the per-commit calls, arguably more so since --paginate may issue several requests. Recommend the same explicit check, e.g.:
    if ! COMMIT_SHAS=$(gh api "repos/${REPO}/compare/${PREV_TAG}...main" --paginate --jq '.commits[].sha'); then
      echo "::error::Failed to compare ${PREV_TAG}...main — likely a transient GitHub API error or rate limit. Re-run the release."
      exit 1
    fi
    Fix this →

Answering @sfreudenthaler's 429 question

What happens if we hit an api rate limit 429? … at least we need to fail gracefully

The follow-up commit (9be1d6d1) does now make the per-commit lookup fail loudly on any API error (429 included) rather than mis-counting it as "no PR" — that satisfies "fail gracefully" for those calls, with re-running the release as the mitigation. Two caveats worth deciding on before merge:

  • Not fully covered — the compare call (High finding above) and the gh pr view label lookup at line 177 (2>/dev/null || true) still swallow errors. The label lookup being pre-existing, but it has the same "transient failure looks like no breaking label → silent pass" shape, so it's worth hardening in the same spirit as the rest of this PR.
  • Higher rate-limit exposure — the old code made one compare call; this design makes 1 + N calls (one commits/{sha}/pulls per commit in the range). For a release spanning many commits that's a real increase in 429 probability. There's no backoff/retry — gh api doesn't retry primary rate limits by default. Options: batch via GraphQL (associatedPullRequests on each commit in one query), or add a retry/Retry-After sleep loop around the lookups. Not blocking if "fail loud + re-run" is acceptable operationally, but flagging since the reviewer explicitly raised it.

Note (pre-existing, not introduced here)

  • GitHub's compare endpoint returns at most 250 commits even with --paginate; ranges larger than that silently omit commits. This was true of the old regex approach too, so it's not a regression — just calling it out since correctness of this gate depends on the commit list being complete.

Overall: the API-based approach is a solid correctness improvement over subject-regex parsing. The one blocking item is the || true on the compare call (line 155), which leaves a hole in exactly the guarantee this PR is meant to restore.

· branch 37200-sdk-breaking-change-gate-replace-squash-subject-regex-with-commitsprs-api

@github-actions github-actions Bot added the Area : CI/CD PR changes GitHub Actions/workflows label Aug 25, 2026
Comment thread .github/workflows/cicd_6-release.yml Outdated
…ing them

A bare `2>/dev/null || true` per commit made a real gh api failure (rate
limit, transient 5xx, network blip) indistinguishable from "this commit
has no associated PR", silently dropping it from the SDK breaking-change
count. Addresses review feedback on #37219 from sfreudenthaler and the
Claude bot.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@sfreudenthaler sfreudenthaler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

love it thanks @KevinDavilaDotCMS

@sfreudenthaler
sfreudenthaler added this pull request to the merge queue Aug 27, 2026
Merged via the queue into main with commit 5e23602 Aug 27, 2026
53 checks passed
@sfreudenthaler
sfreudenthaler deleted the 37200-sdk-breaking-change-gate-replace-squash-subject-regex-with-commitsprs-api branch August 27, 2026 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : CI/CD PR changes GitHub Actions/workflows

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

SDK breaking-change gate: replace squash-subject regex with commits→PRs API

2 participants