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

Updated PR Merge Queue #5640

Closed
Pandapip1 opened this issue Sep 10, 2022 · 10 comments
Closed

Updated PR Merge Queue #5640

Pandapip1 opened this issue Sep 10, 2022 · 10 comments
Labels
e-consensus Waiting on editor consensus enhancement r-process Relates to an EIP Process w-stale Waiting on activity

Comments

@Pandapip1
Copy link
Member

Proposed Change

I suggest that instead of the current "EIP Editors review whichever PRs they want", I suggest the following:

  • If there are one or more PRs that have contentious issues (changes to CI, the website, and living and final EIPs would count as contentious until proven otherwise by the PR being merged) and have new activity, review them first, in ascending order of least recent unread message. GitHub will give you that list for you (if we make a contentious issue label): TODO
  • If there are one or more PRs that would need manual merging and have new activity, review them in top-to-down milestone order (new PRs must appear at the bottom). Again, GitHub gives you that list for you: https://github.com/ethereum/EIPs/milestone/1
  • Review all PRs that add a new EIP as Withdrawn, Living, or Final and have new activity in least recently updated order. (All these items have lists that could work. I could even probably make a small app that aggregates these lists).
  • Review all PRs that change the status of an existing EIP to final, living, or withdrawn and have new activity in most recently updated order
  • Review all PRs that change the status of an existing EIP to last call and have new activity in most recently updated order
  • Review all PRs that change the status of an existing EIP to review and have new activity in most recently updated order
  • Review all PRs that add a new EIP and have new activity in order of ascending number of changed lines
  • Review all other PRs that have new activity at random :] (I think I covered all the cases, but this works as a catch-all)
@Pandapip1 Pandapip1 changed the title Proposal: Updated PR Merge Queue Updated PR Merge Queue Sep 10, 2022
@Pandapip1 Pandapip1 added the r-process Relates to an EIP Process label Sep 10, 2022
@SamWilsn
Copy link
Contributor

I don't mean to come across as rude, but I'm going to continue reviewing in the order that works for me:

  • Whatever is added to the EIP Office Hours agendas; and
  • Going in order according to GitHub's Least recently updated sort option.

I'm only able to contribute about 4-5 hours per week on editing EIPs, and I think this is the best use of my time.


I've been trying to stay on top of the manual merge queue, but it has a lot of items in it that I disagree should be manually merged:

@Pandapip1
Copy link
Member Author

Pandapip1 commented Sep 12, 2022

I don't mean to come across as rude, but I'm going to continue reviewing in the order that works for me

I myself am a bad judge of this, so DW.

Will be merged once we have enough approvals, right? Yes, I'm aware enough == all.

@MicahZoltu was the one that suggested this behavior, and I quote him (from ethereum/EIP-Bot#53 (comment)):

image

The point being that EIP-1 changes have to be manually merged.

Has GPL assets.

Has formatting issues that should be fixed before merging.

These are issues that I missed, and I have removed the tag from #5506.

Should be merged once authors approve.

All of these have CI errors that mean that they need manual merging, but it does appear that I missed the fact that author approvals were missing. I'll remedy that.

@Pandapip1
Copy link
Member Author

I'm going to continue reviewing in the order that works for me

Would you mind trying to explain what that order is / why it works for you? Even if this doesn't get implemented, it might still end up being useful to me personally as an editor.

@SamWilsn
Copy link
Contributor

MicahZoltu was the one that suggested this behavior

And of course that thread doesn't have a policy for when to manually merge 🤣

I guess make (or find, if they exist) discussion threads on discord for these issues, and add them to the issues? Then we can ping editors that haven't responded.

I'm going to continue reviewing in the order that works for me

Would you mind trying to explain what that order is / why it works for you? Even if this doesn't get implemented, it might still end up being useful to me personally as an editor.

My order:

  • Whatever is added to the EIP Office Hours agendas; and
  • Going in order according to GitHub's Least recently updated sort option.

The first is every second Tuesday, and the second happens most Fridays.

It works because it's simple. I assume that you're covering the more active EIPs, so I try to help the slower ones along.

@MicahZoltu
Copy link
Contributor

My recommendation was to have changes to non-EIPs require a decreasing number of approvals based on how long they have been open. Something like:
1 week: 5 approvals
2 weeks: 4 approvals
4 weeks: 3 approvals
8 weeks: 2 approvals
16 weeks: 1 approval
and a single rejection would prevent auto-merging.

The idea here is that if there is consensus among all editors then things would go through quickly and smoothly. If, however, there aren't enough actively engaging editors then system doesn't permanently freeze/halt, it just slows down.

Setting it to "require 5 approvals" was just meant as a way to address the immediate problem of non-EIP changes getting instantly merged until such time as a better long term strategy could be established/implemented.

@Pandapip1
Copy link
Member Author

I think we're getting a little off-topic here. I'll submit a new issue.

@Pandapip1 Pandapip1 added the e-consensus Waiting on editor consensus label Sep 15, 2022
@github-actions
Copy link

There has been no activity on this issue for 1 week. It will be closed after 3 months of inactivity.

@github-actions github-actions bot added the w-stale Waiting on activity label Jan 26, 2023
@Pandapip1
Copy link
Member Author

Dismissing stale bot.

@github-actions github-actions bot removed the w-stale Waiting on activity label Jan 27, 2023
@github-actions
Copy link

github-actions bot commented Feb 3, 2023

There has been no activity on this issue for 1 week. It will be closed after 3 months of inactivity.

@github-actions github-actions bot added the w-stale Waiting on activity label Feb 3, 2023
@github-actions
Copy link

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e-consensus Waiting on editor consensus enhancement r-process Relates to an EIP Process w-stale Waiting on activity
Projects
None yet
Development

No branches or pull requests

4 participants
@MicahZoltu @Pandapip1 @SamWilsn and others