[Admin] Lower pull request approval requirement to 1

[bitinn]

Hi @node-fetch/core,

• Per @jimmywarting's request, I will consider lowering our merge approval count from 2 to 1 in the coming week.

• Given the number of pending changes and our team member schedule, I believe this will speed up 3.0 development considerably.

• There are definitely some risk involved; as an alternative, I am open to recruiting more active reviewers.

• If you have any other suggestions or concerns, please feel free to voice it here.

• If you support the move to lower approval requirement, please leave your vote here so we know :)

Thx and have a nice day.

[jimmywarting]

There are definitely some risk involved; as an alternative, I am open to recruiting more active reviewers.

on the other end of it, we also get security bugs and hotfixes out faster.

it would still be okey to keep it at 2, but that would require that we recruit more active reviewers. Some ppl have lost touch and interest in maintaining this. it is especially slow to get approvals when a team member like myself makes a pr and needs 2 approvals from somebody other then myself. it feels like there is 3 ppl that sometimes approves PRs and when you are one of them who can't approve your own PR then it's VERY long and tedious process of getting something approved cuz you are waiting for that one last person who isn't involved so much

Merging some random persons PR is sometimes quicker.

[jimmywarting]

as i discussed with @bitinn over email also... I think a automated deployment would be nice to have also. @gr2m already made a PR about it: #1270 but it would require @bitinn help with admin privileges

cuz the way it's right now is: you make a PR, wait for two approvals and when it's done, someone needs to make a new PR with a major, minor or patch version and needs to wait for that to be approved by two ppl as well. it takes very long to get something out in production

[bitinn]

Ping @node-fetch/reviewers for comments too, if you got any suggestions to help maintain node-fetch better, let us know here :)

[JefferyHus]

I agree with what @jimmywarting stated in his comment about the PR process. The restrictions are good, but if there could be more people that can code review and approve, it will make the process more fluid & fast even if we do 1 PR/Merge approval.

Automating the deploy of a new version should be a great addition save time. As long as the PR was approved a merged, publishing a new version is a step that is both complicated & easy, complicated because if you automate you have to decide how to upgrade the version from a patch to a major or a minor & also if it is manual we need someone to do it and that someone may not be free.

[Richienb]

Ideally, the amount of reviewers should not be constrained by a number we set and forget, but instead by common sense and pure logic. Following the same threshold for all pull requests doesn't make sense since some can have a bigger impact than others.

However, having 1 as a minimum requires at least 1 other person to take a took which is fine in most circumstances though we should still internalise the fact that we should be waiting for approvals as needed rather than to just satisfy the quota.

[LinusU]

I'm okay with this, but personally I don't feel too confident merging most of the PRs that are open now with only one reviewer. Some minor ones could benefit from it though ☺️

[NotMoni]

I agree with LinusU.

[bitinn]

OK, I understand the hesitation here, then maybe we can establish an active PR review assignment table, here is how I think it might work:

• We open a pinned issue with all candidate PRs as a single task list.

• Members who have time and feel confident to review an PR, can take it formally.

• We will mark a PR as active when 2 members have taken it.

• And it will be accepted when these 2 members have reviewed and author has cleared all concerns.

This way we have a clear picture of (1) which PR needs reviewing and (2) who are reviewing a PR.

It's a bit old-fashioned but I like to have a central place for (1) project transparency, (2) not GitHub project or tag filter so any new members can understand.

It may look something like:

• feat(Body): Added support for BodyMixin.formData() and constructing bodies with FormData #1314 (taken by @LinusU and @Richienb)
• ci: semantic-release #1270 (taken by @jimmywarting, missing one reviewer)
• Implement Happy Eyeballs #1326 (missing 2 reviewers)

How do you think about this?

(Personally, I no longer code in nodejs, but I am happy to help maintain such a dashboard)

[jimmywarting]

I don't know, I think I would rather prefer GitHub project or tags

[gr2m]

I'd rather use a project, too. I think it's something that should be automated, not add additional maintenance overhead.

I added my 👍🏼 to reduce the approval requirement to 1. From my experiences, open source projects that lowered the barrier of contribution and maintenance are the ones that thrive in the long term. Even with only a single review required, every maintainer can still block a PR, either by rejecting a PR in its current state or requiring a review of themselves if they want to review the PR but need more time. And we can add required reviews manually to any PRs where we think certain maintainers should at least acknowledge it.

And lastly, when things slip through, it's easy enough to revert a change.

[jimmywarting]

regarding:

ci: semantic-release #1270 (taken by @jimmywarting, missing one reviewer)

...That missing reviewer needs to be you @bitinn, only you can add NPM_TOKEN, and do the other recommendations

[bitinn]

...That missing reviewer needs to be you @bitinn, only you can add NPM_TOKEN, and do the other recommendations

Please let me know when you have that token, and I will do the steps: I don't even have npm installed on my work machine at this point, that's how far removed I am from nodejs.

I am not the only admin for this right? I was given admin right after node-fetch org migration.

[jimmywarting]

I don't think i have github-admin privileges (?)
I can't see the settings-tab

---

I think i can generate a NPM token, cuz i can publish to npm
(Don't think you need to install npm to generate a token, you can do it all from the web in npm & github)

I have wanted to enable discussion in this repo, but i can't

[bitinn]

both @Richienb @xxczaki should have organization admin permission.

I don't think i have github-admin privileges (?) I can't see the settings-tab

I think i can generate a NPM token, cuz i can publish to npm (Don't think you need to install npm to generate a token, you can do it all from the web in npm & github)

I have wanted to enable discussion in this repo, but i can't

both @Richienb @xxczaki should have organization admin permissions. the exact work to be done can be discussed in the issue thread.

[bitinn]

Hi @jimmywarting,

If you are looking for admin access to this repo to setup some automation stuffs, please ping @Richienb and @xxczaki directly to setup a role for it. I wasn't involved in the node-fetch org's role settings, it would be better if they are involved.

Given current input I have changed required reviewer count to 1, to get the balls rolling on PRs.

Thx.