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

feat: reduce gthub app/secret requirements #554

Open
wants to merge 1 commit into
base: master
from

Conversation

@mmarchini
Copy link
Member

@mmarchini mmarchini commented Sep 14, 2020

Seven days wait + 2 approvals from both committees is quite bureaucratic, and
so is 72 hours fast-tracking for an already approved app/secret, Reducing the
number of required approvals to one per committee as well as the wait time to
72 hours as well as reducing the fast-track time to 24 hours should allow
collaborators to move forward with their work faster, and since it still
require approval from committees it shouldn't have a negative impact on
security concerns.

Seven days wait + 2 approvals from both committees is quite bureaucratic, and
so is 72 hours fast-tracking for an already approved app/secret, Reducing the
number of required approvals to one per committee as well as the wait time to
72 hours as well as reducing the fast-track time to 24 hours should allow
collaborators to move forward with their work faster, and since it still
require approval from committees it shouldn't have a negative impact on
security concerns.
@targos
targos approved these changes Sep 15, 2020
@mhdawson
Copy link
Member

@mhdawson mhdawson commented Sep 18, 2020

I'm comfortable with the change for the one related to already approved apps/secrets, less so for the first time apps/secrets, although mostly for the apps versus secrets.

I'd suggest just making the change for the ones related to already approved apps/secrets.

@mmarchini
Copy link
Member Author

@mmarchini mmarchini commented Sep 19, 2020

Can you elaborate on your concerns for the not-yet-approved apps/secrets? We currently have a very heavy requirement and sometimes it's hard to get all approvals (recent examples: #553 9 days and still needs one approval, #523 took almost a month, #535 also took almost a month).

@mmarchini
Copy link
Member Author

@mmarchini mmarchini commented Sep 22, 2020

Cc @nodejs/tsc @nodejs/community-committee since it affects the required approvals from both committees.

@targos
Copy link
Member

@targos targos commented Sep 22, 2020

Nit: s/gthub/github/ in the commit message

@mhdawson
Copy link
Member

@mhdawson mhdawson commented Sep 22, 2020

@mmarchini not-yet-approved apps/secrets are more of a concern to me because that's where we should be doing most of the due diligence. It's a one-time thing so I think erring on the side of talking more time/giving more time to review makes sense. Once we've agreed to add to one repo, then adding to another is something I think we can do more quickly.

@mmarchini
Copy link
Member Author

@mmarchini mmarchini commented Sep 22, 2020

I understand that, but do you disagree that having two people doing the due diligence (one from each committee) is enough? We have a total of four people today. I'd rather have two doing proper due diligence than four "empty" +1s.

@mmarchini
Copy link
Member Author

@mmarchini mmarchini commented Oct 1, 2020

@mhdawson just double-checking: are you blocking this from landing?

@mhdawson
Copy link
Member

@mhdawson mhdawson commented Oct 2, 2020

@mmarchini I'd say yes unless we get more TSC/CommComm members approving.

I'd be ok if for the first time request we change either the number of approvers or the time, but not both.

I'll also withdraw my objection if more TSC/CommComm members approve. I'm just not comfortable with this change landing with just 1 approval.

@mhdawson
Copy link
Member

@mhdawson mhdawson commented Oct 2, 2020

I understand that, but do you disagree that having two people doing the due diligence (one from each committee) is enough? We have a total of four people today. I'd rather have two doing proper due diligence than four "empty" +1s.

so in the context of I'd be ok if for the first time request we change either the number of approvers or the time, but not both.. Yes if we keep the time at 7 days I'm good with dropping it to be 1 person from each committee.

@mmarchini
Copy link
Member Author

@mmarchini mmarchini commented Oct 2, 2020

Ok, I'll split this PR into two: one to change the number of approvals on normal requests, and another to change the time on fast-track requests. I might follow up later with a PR suggesting changes to the wait time on normal requests so we can continue discussion.

@bnb
bnb approved these changes Oct 12, 2020
Copy link
Member

@bnb bnb left a comment

I see the comment about splitting this up, but I just want to add my approval here for the PR as it currently stands - if we are requiring at least two people from the TSC/CommComm to approve, there is an assumed level of competence in those approvals. I can't see a reason we'd need to wait up to 4 additional days for something we can choose to reverse if a concern is raised later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.