Closed
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the PR ABI validation workflow to gate the ABI Validation step on a new should_run output from the review-checking step instead of relying on process.exit(0), ensuring ABI checks only run for fully approved PRs. Flow diagram for updated ABI validation gating in PR workflowflowchart TD
A[PR opened or updated] --> B[Run Check_Review_Status step]
B --> C{steps.check_reviews.outputs.should_run == 'true'?}
C -- true --> D[Run ABI_Validation step
./gradlew checkLegacyAbi -Pfull-build=true --no-configuration-cache]
C -- false --> E[Skip ABI_Validation step]
D --> F[Job completes]
E --> F[Job completes]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider setting a default value for
should_runin thecheck_reviewsstep (e.g., via::set-outputon all code paths) so that theif: steps.check_reviews.outputs.should_run == 'true'condition behaves predictably even if the script exits early or encounters an unexpected branch. - You may want to consolidate the
'true'string comparison into a reusable convention (e.g., always emitting'true'/'false'in the script and documenting that) to avoid subtle bugs if the output ever changes totrue/falsebooleans or different casing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider setting a default value for `should_run` in the `check_reviews` step (e.g., via `::set-output` on all code paths) so that the `if: steps.check_reviews.outputs.should_run == 'true'` condition behaves predictably even if the script exits early or encounters an unexpected branch.
- You may want to consolidate the `'true'` string comparison into a reusable convention (e.g., always emitting `'true'`/`'false'` in the script and documenting that) to avoid subtle bugs if the output ever changes to `true`/`false` booleans or different casing.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes the logic in the ABI validation workflow.
Previously, using
process.exit(0)in theCheck Review Statusstep only exited the script successfully but did not stop subsequent steps from running. This meant the ABI validation would run even if the PR wasn't fully approved.The fix introduces an output variable
should_runto control the execution of subsequent steps usingifconditions.