-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add validation check for manual bp release notes #285
base: main
Are you sure you want to change the base?
feat: add validation check for manual bp release notes #285
Conversation
await updateBackportValidityCheck(context, checkRun, { | ||
title: 'Invalid Backport', | ||
summary: `The release notes of the backport PR do not match those of ${oldPRNumbers | ||
.map((pr) => `#${pr}`) | ||
.join(', ')}.`, | ||
conclusion: CheckRunStatus.FAILURE, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to get clobbered by the update a bit further down in this file:
Lines 358 to 362 in 47f78d4
await updateBackportValidityCheck(context, checkRun, { | |
title: 'Valid Backport', | |
summary: `This PR is declared as backporting "#${oldPRNumber}" which is a valid PR that has been merged into ${pr.base.repo.default_branch}`, | |
conclusion: CheckRunStatus.SUCCESS, | |
}); |
Might take some more refactoring to ensure the clobbering doesn't happen. However, I'm a bit worried about a regression since we don't have good test coverage here. Could you look at expanding test coverage in spec/index.spec.ts
to cover updateBackportValidityCheck
? A separate PR first expanding test coverage might be best since it will keep the changes in this one more concise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointers!
- Created a PR to expand test coverage of index.spec.ts test: add test for updateBackportValidityCheck #289
- Updated the logic in this PR to move the release notes check after all the failure map check so it doesn't get clobbered
- After the test PR gets merged, I will update this PR with additional
updateBackportValidityCheck
so we can be confident it works
bba9de1
to
f143e82
Compare
This PR:
updateManualBackportReleaseNotes