-
Notifications
You must be signed in to change notification settings - Fork 134
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
Request for vote to reduce inactive collaborator duration #1536
Conversation
909063c
to
5c9e18c
Compare
Co-authored-by: Joyee Cheung <[email protected]>
Co-authored-by: Joyee Cheung <[email protected]>
Co-authored-by: Antoine du Hamel <[email protected]>
Co-authored-by: Joyee Cheung <[email protected]>
Co-authored-by: Joyee Cheung <[email protected]>
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.
If you’re still pushing for reducing the inactivity period to 6 months, I think you should add it as a candidate (there’s no down side for having more candidates with the vote system we use)
I think it will make the decision harder. I'll push for 9 for now, and see in the future (maybe in a couple of months) |
I don’t see how. If the voter knows which option they prefer, they can simply order them. If not they can tie the two/three options for which they have no relative preference. |
I prefer to keep votes simple. Voting to override blocks on a particular PR leaves no ambiguity as to what the outcome of the vote determines: that PR can land. If we want to consider 6 months, then open a PR for that, let it get blocked, and open a vote to override that block. I'd prefer that process to a ranked choice vote, even though it takes more rounds. Yes it's more inefficient, but we learn information after the first round: how many people support that option and why. That's useful data to have when considering how to vote in later rounds. |
Co-authored-by: Tobias Nießen <[email protected]>
Co-authored-by: Tobias Nießen <[email protected]>
Co-authored-by: Tobias Nießen <[email protected]>
Co-authored-by: Tobias Nießen <[email protected]>
Co-authored-by: Tobias Nießen <[email protected]>
Yagiz already opened that exact PR, and it did get blocked.
I don’t think adding a third candidate makes the vote less simple, or its outcome more ambiguous. In any case, if Yagiz does not want to push for 6 months anymore, that’s certainly fine by me. I’d like the result of the vote to be somewhat final, and not have to discuss the issue again (I mean not until something changes and/or enough1 time elapses). Footnotes
|
Currently, the bot looks through all commits on `main` that have landed in the last 12 months, and makes two lists: | ||
|
||
- a list of all the commit authors. | ||
- a list of all the collaborators who are listed as reviewers in the commit message (i.e. folks who have approved the 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 would prefer we add a third option of asking @nodejs/security-wg to review the membership durations and then the TSC to implement their feedback.
I for example would vote against reducing the duration since I feel like it has little security benefit and can alienate people but if I was convinced there would be actual security merit I'd be in favor.
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.
Approving this since it's always allowed to call for a vote but this seems premature.
The issue with that is that we don't fully understand the implications yet and calling for a vote now instead of figuring the data out would mean I could more easily vote in some way I'd later regret. |
@benjamingr What is the data you're referring to? |
The motivation is to reduce security risk (right?) in that case I want to know what roles in the org have access to what, what risk it entails and what attack vectors there are. If a triager is equally as risky or someone can abuse the fact a PR was already OKd once to rerun stuff or if there are other issues (e.g maybe we fix triggers being able to run CI or ensure CI is isolated and then they can still run benchmark ci which isn’t for example). Basically I would prefer a solution where collaborators can’t do anything that risks us (other than approving PRs obviously) but if we take the limiting inactivity route I at least wanna know it actually helps.. |
|
||
- a list of all the commit authors. | ||
- a list of all the collaborators who are listed as reviewers in the commit message (i.e. folks who have approved the 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.
nit: redundant whitespace
|
||
# 6. Optionally, insert a brief introduction for the vote PR, in the markdown format. | ||
prBody: | | ||
Currently, the bot looks through all commits on `main` that have landed in the last 12 months, and makes two lists: | ||
|
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.
nit: redundant whitespace
It would be beneficial to add the other options discussed in the TSC meeting here as options |
I'd like to keep the options as it is for now since the original block to my pull request does not involve anything other than 9 months. We can always follow up and vote on the other options if anybody from TSC wants to pursue. |
I mean, we spent time in the TSC meeting discussing several options (reducing it further to 6 months but making it voluntary for example) - merging this as is forced me to vote between two options I don't actually like all while there is ongoing discussion at nodejs/node#52459 |
Requesting vote due to the blockers of pull-request nodejs/node#52459.
Currently, there are 4 blockers (2 TSC members) and 6 approvals (5 TSC members) in the original pull-request.
cc @nodejs/tsc