Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Branch protection: Possibility to not use whitelist but allow anyone with write access #9055
Branch protection: Possibility to not use whitelist but allow anyone with write access #9055
Changes from 2 commits
22be3e9
11a5954
5bcf80e
b488f1d
21f8590
2461d52
d1ecef6
d48fcb4
459c59c
283be88
9d3da22
148e1d6
8afcf90
c296126
5ee7ae2
b63974e
6b04006
e6908f5
2cad7bb
8af34e3
2187c6f
b1537d4
8198531
5aa2e18
6ca746a
f81f209
52760dd
93ede5a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
IMHO approvals should "remember" if the users were whitelisted at the moment they approved and this value should not be recalculated. Two reasons for this:
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.
Your argumentation makes sense, but that would require quite large code change (need new database columns). I think it should be a different PR.
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 don't think it's that big of a change. I think you only need a new "Verified bool" column to the review table and move this check to the code that inserts the review. One day that "verified" column can reflect a 2FA, for example.
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.
Can it be called
OfficialReviewer (bool)
?You mean there will be an option to only accept 2FA users as reviewers?
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.
Yes, it can be called OfficialReviewer, although that would lock the meaning. What I mean is that what today is a bool (verified/not verified) tomorrow can be an enum (not verified/verified/verified+signed, etc.). But whatever we may want in the future can be solved with a migration no problem, so feel free to choose whatever you feel that fits best. 👍
Large diffs are not rendered by default.
Large diffs are not rendered by default.