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

Request for vote to reduce inactive collaborator duration #1536

Merged
merged 11 commits into from
Apr 26, 2024

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Apr 23, 2024

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

@anonrig anonrig changed the base branch from main to initiateNewVote April 23, 2024 17:00
@anonrig anonrig changed the base branch from initiateNewVote to main April 23, 2024 17:00
@anonrig anonrig force-pushed the anonrig/vote-collaborator-status branch from 909063c to 5c9e18c Compare April 23, 2024 17:14
Co-authored-by: Antoine du Hamel <[email protected]>
@github-actions github-actions bot changed the base branch from main to initiateNewVote April 23, 2024 18:32
Copy link
Contributor

@aduh95 aduh95 left a 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)

@anonrig
Copy link
Member Author

anonrig commented Apr 23, 2024

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)

@aduh95
Copy link
Contributor

aduh95 commented Apr 23, 2024

I think it will make the decision harder.

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.

@GeoffreyBooth
Copy link
Member

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.

votes/initiateNewVote/_EDIT_ME.yml Outdated Show resolved Hide resolved
votes/initiateNewVote/_EDIT_ME.yml Outdated Show resolved Hide resolved
votes/initiateNewVote/_EDIT_ME.yml Outdated Show resolved Hide resolved
votes/initiateNewVote/_EDIT_ME.yml Outdated Show resolved Hide resolved
votes/initiateNewVote/_EDIT_ME.yml Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Apr 24, 2024

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

Yagiz already opened that exact PR, and it did get blocked.

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.

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

  1. there’s no agreed upon definition of how long is “enough time”. Read “at some vague point in the far future”.

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).
Copy link
Member

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.

Copy link
Member

@benjamingr benjamingr left a 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.

@benjamingr
Copy link
Member

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).

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.

@anonrig
Copy link
Member Author

anonrig commented Apr 24, 2024

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?

@benjamingr
Copy link
Member

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).

Copy link
Member

Choose a reason for hiding this comment

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

nit: redundant whitespace

Suggested change


# 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:

Copy link
Member

Choose a reason for hiding this comment

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

nit: redundant whitespace

Suggested change

@benjamingr
Copy link
Member

It would be beneficial to add the other options discussed in the TSC meeting here as options

@anonrig
Copy link
Member Author

anonrig commented Apr 26, 2024

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.

@anonrig anonrig merged commit 874e413 into initiateNewVote Apr 26, 2024
3 checks passed
@anonrig anonrig deleted the anonrig/vote-collaborator-status branch April 26, 2024 19:15
@benjamingr
Copy link
Member

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

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.

9 participants