Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

alicelovescake
Copy link
Contributor

This PR:

@alicelovescake alicelovescake requested a review from a team as a code owner April 21, 2024 20:48
@alicelovescake alicelovescake changed the title Manual bp release notes feat: add validation check for manual bp release notes Apr 21, 2024
src/utils.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
spec/utils.spec.ts Outdated Show resolved Hide resolved
Comment on lines +349 to +355
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,
});
Copy link
Member

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:

trop/src/index.ts

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.

Copy link
Contributor Author

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

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.

Validate Manual Backport Release Notes
2 participants