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

added script to dismiss reviews #76

Merged

Conversation

Bullrich
Copy link
Contributor

@Bullrich Bullrich commented Oct 24, 2023

Added a script that will require new reviews in a PR if the author (who is not a fellow) pushed a new commit.

Resolves #60

Copied from paritytech/polkadot-sdk#4152 and paritytech/polkadot-sdk#3431

Summary

Added a new step in the action that triggers review bot to stop approval from new pushes.

This step works in the following way:

  • If the author of the PR, who is not a fellow, pushed a new commit then:
  • Review-Trigger requests new reviews from the reviewers and fails.

It does not dismiss reviews. It simply request them again, but they will still be available.

This way, if the author changed something in the code, they will still need to have this latest change approved to stop them from uploading malicious code.

  • Does not require a CHANGELOG entry

@Bullrich Bullrich requested a review from bkchr October 24, 2023 13:55
@Bullrich
Copy link
Contributor Author

/merge

@github-actions
Copy link

There was a problem running the action.

❌😵❌

Please find more information in the logs.

@Bullrich
Copy link
Contributor Author

There was a problem running the action.

❌😵❌

Please find more information in the logs.

Ah yeah, I forgot that actions can not merge PRs which modify workflows

@bkchr
Copy link
Contributor

bkchr commented Oct 25, 2023

@Bullrich and this works as well for external reviews?

@Bullrich
Copy link
Contributor Author

@Bullrich and this works as well for external reviews?

Yes, I tried it in Bullrich/auto-merge-bot#2 where @rzadp is not a member and the system dismissed his PR

@Bullrich
Copy link
Contributor Author

/merge

@fellowship-merge-bot
Copy link
Contributor

There was a problem running the action.

❌😵❌

Please find more information in the logs.

@Bullrich
Copy link
Contributor Author

/merge

@fellowship-merge-bot fellowship-merge-bot bot enabled auto-merge (squash) November 22, 2023 11:18
@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

@Bullrich
Copy link
Contributor Author

/merge cancel

@fellowship-merge-bot
Copy link
Contributor

Disabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

@Bullrich
Copy link
Contributor Author

I'm a bit stuck with this one, as it will discard reviews when the branch gets updated with the "Keep branch up to date" action (or by pressing the button).

I'm researching into differentiating this kind of push from regular pushes.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

I hope this does not dismiss on merge commits?

@mordamax
Copy link
Contributor

mordamax commented Apr 5, 2024

@Bullrich ^^

@Bullrich Bullrich requested review from bkchr and ggwpez April 16, 2024 15:28
@Bullrich
Copy link
Contributor Author

I hope this does not dismiss on merge commits?

Hi @ggwpez,

After a very long process I was finally able to make a final version work.

I modified the code to work in the same way as paritytech/polkadot-sdk#4152 and paritytech/polkadot-sdk#3431, so now it only requires that the last commit is reviewed, but it doesn't require this for fellows.

It also doesn't dismiss reviews on merge commits if the commit is made by the merge-bot or a fellow.

Please, take another look to the PR and let me know if it satisfies your requirements.

Thanks!

Bullrich added 4 commits May 7, 2024 14:13
This stops the dismissal on different events and only executes it when the user pushes new commits to the PR
It should not be a separated step
@Bullrich Bullrich force-pushed the dismiss-reviews branch from 6ef0844 to bfe299d Compare May 7, 2024 12:13
@Bullrich
Copy link
Contributor Author

Bullrich commented May 7, 2024

/merge

@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

@fellowship-merge-bot fellowship-merge-bot bot enabled auto-merge (squash) May 7, 2024 12:38
@fellowship-merge-bot fellowship-merge-bot bot merged commit 633d4e7 into polkadot-fellows:main May 7, 2024
36 checks passed
@Bullrich Bullrich deleted the dismiss-reviews branch May 7, 2024 14:08
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.

Dismiss approval on new changes
8 participants