Skip to content

feat(get_prs_between_tags): add opt-in fail_on_error - #772

Merged
dcalhoun merged 2 commits into
trunkfrom
feat/get-prs-between-tags-fail-on-error
Aug 19, 2026
Merged

feat(get_prs_between_tags): add opt-in fail_on_error#772
dcalhoun merged 2 commits into
trunkfrom
feat/get-prs-between-tags-fail-on-error

Conversation

@dcalhoun

Copy link
Copy Markdown
Member

What does it do?

Adds an opt-in fail_on_error: parameter to get_prs_between_tags, defaulting to false so existing callers are unaffected.

The action rescues every StandardError from generate_release_notes and returns the message as the changelog:

rescue StandardError => e
  error_msg = "❌ Error computing the list of PRs since #{previous_tag || 'last release'}: `#{e.message}`"
  UI.important(error_msg)
  error_msg # Use error message as GitHub Release body to help us be aware of what went wrong.

That is the right default when the caller creates the GitHub Release first and would rather ship degraded notes than fail the lane — the error stays visible in the release body.

It is the wrong default when the caller pushes the tag and then creates the release from the computed notes. There, a transient API failure publishes a release whose body reads ❌ Error computing the list of PRs since v0.19.0: ..., which reads as a successful release and is typically only caught by someone reading the release page later. Those callers want the lane to stop while a re-run is still cheap.

Prompted by GutenbergKit#588, where @AliSoftware suggested adding this upstream so the app can use the action directly instead of reaching past it into GithubHelper#generate_release_notes.

This follows the same opt-in shape as #771, which adds fail_on_error to the translation-download actions. No file overlap between the two.

Usage

get_prs_between_tags(
  repository: 'wordpress-mobile/GutenbergKit',
  tag_name: 'v0.20.0',
  previous_tag: 'v0.19.0',
  fail_on_error: true
)

Testing

  • Both new specs fail without the production change (verified by stashing it) and pass with it.
  • The four pre-existing error-handling specs are unchanged and still pass, confirming the default behavior is preserved.
  • Full suite: 1006/1007. The one failure is git_helper_spec.rb:59 (git-lfs detection), which reproduces on a clean trunk in my environment and is unrelated to this change.

Checklist before requesting a review

  • Run bundle exec rubocop to test for code style violations and recommendations.
  • Add Unit Tests (aka specs/*_spec.rb) if applicable.
  • Run bundle exec rspec to run the whole test suite and ensure all your tests pass.
  • Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
  • If applicable, add an entry in the MIGRATION.md file to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider. Not applicable — the new parameter is optional and defaults to the current behavior.

The action rescues every StandardError from `generate_release_notes` and
returns the message as the changelog, so a transient API failure yields a
GitHub Release whose notes read "❌ Error computing the list of PRs…".

That is the right default for callers that create the release first and
would rather ship degraded notes than fail the lane. It is the wrong one
for callers that push the tag and then create the release from the
computed notes: there the failure needs to stop the lane while a re-run
is still cheap.

Defaults to false, so existing callers are unaffected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dcalhoun dcalhoun added the enhancement New feature or request label Aug 19, 2026
@dcalhoun
dcalhoun marked this pull request as ready for review August 19, 2026 16:20
@dcalhoun
dcalhoun requested a review from a team as a code owner August 19, 2026 16:20

@AliSoftware AliSoftware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good 👍 :shipit:


cc @iangmaia FYI as you're currently also working on a PR in release-toolkit to add a fail_on_error on a different action (translations-related actions), might make sense to make sure the semantic and description of those params are consistent between the two PRs, even if they are on completely different actions, just for consistency with our wording being aligned on the description of such ConfigItem across actions and such

Comment thread CHANGELOG.md Outdated
Avoid unnecessary explanation in the CHANGELOG.

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>
@dcalhoun

dcalhoun commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

@AliSoftware thank you for the review and suggestion. 🙇🏻‍♂️

@dcalhoun
dcalhoun enabled auto-merge August 19, 2026 16:31
@dcalhoun
dcalhoun merged commit 9322fff into trunk Aug 19, 2026
6 checks passed
@dcalhoun
dcalhoun deleted the feat/get-prs-between-tags-fail-on-error branch August 19, 2026 16:32
@iangmaia

Copy link
Copy Markdown
Contributor

might make sense to make sure the semantic and description of those params are consistent between the two PRs

Gave it a go at ef119031

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants