Tier editor approvals required for ambiguous PRs.

[MicahZoltu]

Replaces the solution proposed in #46, lower priority but a superior solution:

If a PR is ambiguous according to the bot and it fulfills one of the following conditions, then it should auto-merge:

• It has received 4 editor approvals and no editor rejections.
• It has received 3 editor approvals and no editor rejections and has been open for at least 1 week.
• It has received 2 editor approvals and no editor rejections and has been open for at least 1 month.
• It has received 1 editor approval and no editor rejections it has been open for at least 2 months.

The goal here is to make it so editors failing to show up doesn't prevent us from making changes to things like EIP-1, the EIP template, Jekyll pages, etc. but at the same time it is preferred that we get editor consensus before making changes to such things.

cc @gcolvin @axic @lightclient @SamWilsn

[MicahZoltu]

If we move forward with this, the same change should be made to edits to EIP-1. Currently it is just a flat 2-editor approval required.

[Pandapip1]

If a PR is ambiguous according to the bot

Suggested change: If a PR is only ambiguous according to the bot.

[JEAlfonsoP]

Proposed General idea:

Try to implement MicahZoltu solution.

Implementation:
To Calculate: approvals count value-number
/EIP-Bot/blob/master/src/modules/approvals/modules/get_approvals.ts

To Calculate: Created_at / updated_at days number
Calculate for open PR created / open days, using octokit get pull request (), “created_at / updated_at” (this could be added here: /EIP-Bot/tree/master/src/modules/assertions) another possible solution to obtain (PR open days) could be in: /EIP-Bot/tree/master/src/infra/github.ts , obtained Created_at from getPullRequestFromNumber APIs.

Once the two values have been calculated, Micah’s solution would be implemented, and making possible: for (according to the Bot) ambiguous-PR the “auto-merge”.

Note:
Still needs to define where into the Bot’s code this solution (if decided) would be added.

[MicahZoltu]

Even simpler solution:
Have the bot not touch EIP-1, and require manual merging. This is hopefully trivial to implement, and it will at least make it so EIP-1 changes don't accidentally get merged.

[gcolvin]

I very much prefer the simpler solution for EIP-1. It's not just any EIP, and the editors themselves are the authors.

For the rest, all of it just seems way too complicated to me, both for the coding of the bot, and for humans to understand the rules. I'd be happy to just leave edge cases like this to wait for a manual merge.

[JEAlfonsoP]

If this is the consensus could we close this issue ?

[SamWilsn]

+1 on the simpler solution.

I might suggest expanding this to any "Living" EIP, and not just EIP-1.

[Pandapip1]

Maybe just have eip-editors be allowed to be included as an author.

[JEAlfonsoP]

This action is included in Issue 58. If this is the only pending action could we close this issue and work on Issue 58 ?

[MicahZoltu]

Maybe just have eip-editors be allowed to be included as an author.

We specifically don't want the bot auto-merging EIP-1 changes with a single editor proposing them or approving them, so we shouldn't just make editors authors of EIP-1 and then lean on existing functionality.

[JEAlfonsoP]

I think that the key for editors/authors approval is PR reviewers, if reviewers intersected editors is greater than X then execute.
X to be defined:

It has received 4 editor approvals and no editor rejections.
It has received 3 editor approvals and no editor rejections and has been open for at least 1 week.
It has received 2 editor approvals and no editor rejections and has been open for at least 1 month.
It has received 1 editor approval and no editor rejections it has been open for at least 2 months.

[gcolvin]

EIP-1 should change when the editors have come to consensus that it should be. There is no formula for measuring that.

[JEAlfonsoP]

may be EIP-BOT can change EIP-1 if reviewers == editors (all editors have approved it and non has rejected it) and PR has been open for X days (30?)

but I agree this is an specific call for the editors.

another idea, EIP-BOT will auto merge (using Micah's formula) everything except EIP-1, actually we can set first check for the EIP-BOT: if EIP-1 Exit

[MicahZoltu]

EIP-1 should change when the editors have come to consensus that it should be. There is no formula for measuring that.

What is your recommendation for codifying that in the bot? Do you think the bot should just never automerge EIP-1 changes and they should always be manually merged by an admin?

[gcolvin]

@MicahZoltu
Do you think the bot should just never automerge EIP-1 changes and they should always be manually merged by an admin?

Exactly.

[JEAlfonsoP]

EIP-1 should change when the editors have come to consensus that it should be. There is no formula for measuring that.

What is your recommendation for codifying that in the bot? Do you think the bot should just never automerge EIP-1 changes and they should always be manually merged by an admin?

test for eip 1 (signature) at the beginning of context.payload?.pull_request?.body:

eip1-signatureRegEx =
/^eip: 1
title: EIP Purpose and Guidelines
status: Living
type: Meta
author: Martin Becze mb@ethereum.org, Hudson Jameson hudson@ethereum.org, et al.
created: 2015-10-27);\n/g

if eip1-signatureRegEx.test(context.payload?.pull_request?.body) {console.log(Critical Error: EIP 1); System.exit(1);}

somethig like this.

[MicahZoltu]

Do you think the bot should just never automerge EIP-1 changes and they should always be manually merged by an admin?

Exactly.

While I'm not opposed to this on principal, I would like a solution to the following problem (which is what this change was trying to solve):

The current set of editors (and historic editors) don't reliably comment on all process change proposals, nor do they all show up regularly to meetings. Sometimes we'll have 1 editor present for very extended periods (many months to a year). Sometimes we'll have a couple editors present for extended periods (months on end). Sometimes we'll have many editors present/available at the same time.

When we are in a situation where many editors are present/participating, I would like to wait on sign off from everyone. When we are in a situation where only one or two editors are present, I would still like to be able to make process changes.

The problem with no automation is that it is very easy to file an issue suggesting a change, get no feedback on it for an extended period, and then it gets forgotten. Often the same issue will be suggested again and we have duplicates.

What if the bot would just monitor for the above number of approvals, but instead of automerging it just comments on the issue to draw attention to it once the threshold is passed?

[MicahZoltu]

@JEAlfonsoP No need for regex. Just look at the filepath of the touched file and if it is eip-1 then follow whatever rules we decide on.

[JEAlfonsoP]

Ok, I just feel that BOT has more control Regex.Test eip1 signature , but I agree filepath is another valid signature (it will just need a call to outside:EIP repo).

EIP-Bot can follows any rule inside its scope.
ie:
Micah: What if the bot would just monitor for the above number of approvals, but instead of automerging it just comments on the issue to draw attention to it once the threshold is passed?

[gcolvin]

@MicahZoltu It's on the person wanting a change to discuss it online and get it on the agenda for meetings. At some point those not participating in the discussions or meetings are in "silence indicates consent" mode, or perhaps in "do you really want be an editor" mode. I don't see that a bot can help much, but if someone wants to do as you suggest I don't object.

[JEAlfonsoP]

const eip1_html_url = https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1.md;
if (context.payload?.pull_request?.html_url == eip1_html_url) {
console.log(Critical Error: EIP 1);
System.exit(1);}

@MicahZoltu If agree, I can PR this as the very first EIP-BOT check

[MicahZoltu]

I think disabling automerge on EIP-1 is a good first step. I think we all agree at least that it shouldn't be automerging with a single editor approval.

[Pandapip1]

How about making the author of a PR not an approver if it modifies EIP-1?

[MicahZoltu]

That functionally just gets us to 2 editor auto-merge since changes to EIP-1 are largely done by editors, which isn't enough in most cases IMO.

[JEAlfonsoP]

I was rethinking it and looking into the code.

There is another possible solution:

This runs in main, testFile(file):

if (editorApprovals.length < EIP1_REQUIRED_EDITOR_APPROVALS) {
return multiLineString(" ")(
Changes to EIP 1 require at least ${EIP1_REQUIRED_EDITOR_APPROVALS},
unique approvals from editors; there's currently ${editorApprovals.length} approvals;,
the remaining editors are ${editors .filter((editor) => !editorApprovals.includes(editor)) .join(", ")}
);
} else return;
};

actually, EIP1_REQUIRED_EDITOR_APPROVALS = 2;

option 1: forbid EIP-BOT automerge EIP 1 (strict rule)

option 2: set a higher value for EIP1_REQUIRED_EDITOR_APPROVALS.

[MicahZoltu]

Ah, that seems easy then. Just submit a PR that sets EIP1_REQUIRED_EDITOR_APPROVALS to 5. That will functionally disable auto-merge unless we have 100% participation (which never happens).

[JEAlfonsoP]

Done.

[MicahZoltu]

@JEAlfonsoP Can you link to the PR that this is done in?

[JEAlfonsoP]

https://github.com/ethereum/EIP-Bot/pull/81/commits/c3effe75a3a7151fb61ceba029f90c4efbb43760
c3effe7

[MicahZoltu]

There should be a Pull Request for that specific change, isolated from the rest of the changes. I recommend reading up on Git branching, as it appears all of your changes are happening in a single branch which is making it difficult to review them in isolation.

[JEAlfonsoP]

I will delete the FORK and will create a new one with a Branch and PR for each Issue.

[JEAlfonsoP]

as requested:

#85

[MicahZoltu]

There will also need to be a PR to update the bot used by the EIP repository to the latest version. I don't know how to do that, maybe @alita-moore does?

[Pandapip1]

I know how to, so I can make that PR.

[Pandapip1]

Fixed in #85