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

Cleanup if removed needs-security-action #254

Open
thypon opened this issue Jul 3, 2023 · 3 comments
Open

Cleanup if removed needs-security-action #254

thypon opened this issue Jul 3, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@thypon
Copy link
Member

thypon commented Jul 3, 2023

If an assignee removes needs-security-action the next run:

  1. cleanups all the comments without any interaction
  2. removes the assignees
@thypon thypon added the enhancement New feature or request label Jul 3, 2023
@kdenhartog
Copy link
Member

Can we make this a fix for when anyone on sec-team removes it? Sometimes I'm reviewing PRs and either of you get flagged for it and then it ends up retriggering if another commit gets added after I remove the label.

@thypon
Copy link
Member Author

thypon commented Sep 13, 2023

Possible, but not the direction I'd like this to go. I'd prefer if only the assignee for the given finding would be able to remove the needs-security-action label permanently.

@kdenhartog
Copy link
Member

kdenhartog commented Sep 14, 2023

Could we add additional assignees on a per repo/file basis on top of the rule based assignees as well? There's times where the context matters that's relevant to the repo and these are flagging redundantly to other sec reviews in progress. For example, just recently I was working on https://github.com/brave/reviews/issues/1376 and the bot flagged no use of HTTPS. In this case, this is one of the areas that was included in the review because we're utilizing the attestation documents for integrity protection and isolating the endpoints within the VPN for confidentiality plus having issues getting enough certs issued via letsencrypt so we need to sync the certs.

If it were the case that only the assignees from the rule were to be able to trigger this removal and the label is now blocking then we'll end up in a situation where sec reviews are complete but PRs won't be able to be merged until the assignee for the rule removes the label. This seems like it would lead to unnecessary double reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants