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

Merged
merged 2 commits into from
Nov 17, 2021
Merged

feat: reduce gthub app/secret requirements #554

merged 2 commits into from
Nov 17, 2021

Conversation

mmarchini
Copy link
Contributor

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

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
Contributor Author

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
Contributor Author

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

@targos
Copy link
Member

targos commented Sep 22, 2020

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

@mhdawson
Copy link
Member

@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
Contributor Author

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
Contributor Author

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

@mhdawson
Copy link
Member

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 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
Contributor Author

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.

Copy link
Contributor

@bnb bnb left a comment

Choose a reason for hiding this comment

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

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.

Base automatically changed from master to main March 18, 2021 15:55
@bnb
Copy link
Contributor

bnb commented Sep 27, 2021

bringing this up again: I think we should merge.

@mcollina
Copy link
Member

+1 for landing

@@ -133,7 +133,7 @@ a) a link showing how the GitHub App or the secret being requested is already
in use, and b) ask for approvals to fast-track the request. Two members of
either TSC or CommComm must approve the fast track request. Fast-tracked
requests only need one approval from either TSC or CommComm is required, and
the request must remain open for 72 hours.
the request must remain open for 24 hours.
Copy link
Member

Choose a reason for hiding this comment

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

Tiniest of nits:

Suggested change
the request must remain open for 24 hours.
the request must remain open for 24 hours.

@bnb
Copy link
Contributor

bnb commented Nov 15, 2021

I assume this can be merged now given nobody's objected?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@Trott Trott merged commit ae3a26b into main Nov 17, 2021
@Trott Trott deleted the reduce-time-gh-apps branch November 17, 2021 03:13
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.

6 participants