Skip to content

check-pr-changes-allowed.py now considers review comments#30493

Merged
hcallahan-lowrisc merged 1 commit into
lowRISC:masterfrom
Ed-Baker1:fix-pr-auth-comments
Jun 23, 2026
Merged

check-pr-changes-allowed.py now considers review comments#30493
hcallahan-lowrisc merged 1 commit into
lowRISC:masterfrom
Ed-Baker1:fix-pr-auth-comments

Conversation

@Ed-Baker1

@Ed-Baker1 Ed-Baker1 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Updated so the script now looks at regular comments and review comments when looking for "CHANGE AUTHORIZED: " before allow a PR to be merged.

Fixes #29864

@Ed-Baker1 Ed-Baker1 requested a review from a team as a code owner June 23, 2026 10:42
@hcallahan-lowrisc hcallahan-lowrisc self-requested a review June 23, 2026 10:55
Updated so the script now looks at regular comments and review comments when looking for "CHANGE AUTHORIZED: " before allow a PR to be merged.

Signed-off-by: Ed Baker <edward.baker@lowrisc.org>
@Ed-Baker1 Ed-Baker1 force-pushed the fix-pr-auth-comments branch from 00407ca to 0192fd7 Compare June 23, 2026 12:09

@hcallahan-lowrisc hcallahan-lowrisc 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.

Thanks @Ed-Baker1 ! This is a nice usability fix.
As this is an important feature to prevent accidental changes to significant files in the tree, I will wait for another review before authorizing and merging.
@rswarbrick @vogelpi PTAL

@vogelpi vogelpi 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.

Thanks, LGTM!

@vogelpi

vogelpi commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

CHANGE AUTHORIZED: ci/scripts/check-pr-changes-allowed.py

This PR just changes the script to also look for CHANGE AUTHORIZED comments in the review comments.

@rswarbrick rswarbrick 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.

Looks sensible to me (and I'm thrilled to see that there's such a clean solution!)

@rswarbrick

Copy link
Copy Markdown
Contributor

CHANGE AUTHORIZED: ci/scripts/check-pr-changes-allowed.py

This change is to match the desired behaviour of the script. Definitely no "risk to the design" :-)

@hcallahan-lowrisc

Copy link
Copy Markdown
Contributor

Failing FPGA tests are clearly unrelated to this CI workflow tooling change.

@hcallahan-lowrisc hcallahan-lowrisc added this pull request to the merge queue Jun 23, 2026
Merged via the queue into lowRISC:master with commit fc79657 Jun 23, 2026
41 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ci, scripts] check-pr-changes-allowed.py does not consider review comments

4 participants