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

Fix EIP Bot #5400

Merged
merged 28 commits into from
Aug 10, 2022
Merged

Fix EIP Bot #5400

merged 28 commits into from
Aug 10, 2022

Conversation

Pandapip1
Copy link
Member

@Pandapip1 Pandapip1 commented Aug 3, 2022

Will need a manual merge, and the following branch protection settings will need to be updated:

  • Enable "Require Review from Code Owners"
  • Enable "Dismiss Stale PR Reviews"
  • Remove "Auto-Merge Bot / EIP Auto-Merge Bot" from required checks

Do these only AFTER this PR is merged.

NEEDS TESTING, BE READY TO REVERT THIS PR IF NEEDED This has been successfully E2E tested.

Here's how it works:

  1. The original triggering event triggers the trigger workflow.
  2. The trigger job runs, adding the PR Number as an artifact.
  3. The trigger workflow completes, triggering the actual workflow.
  4. EIP-Bot runs
  5. If EIP-Bot passes, then auto-merge is enabled and @eth-bot submits an approval, passing the CODEOWNERS review.
  6. Automerge merges the PR

@eth-bot eth-bot enabled auto-merge (squash) August 3, 2022 14:13
@eth-bot
Copy link
Collaborator

eth-bot commented Aug 3, 2022

A critical and unhandled exception has occurred:
Message: not found
Data: (there is no data for this error)(cc @alita-moore, @mryalamanchi)

auto-merge was automatically disabled August 3, 2022 14:17

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) August 3, 2022 14:17
@Pandapip1
Copy link
Member Author

Pandapip1 commented Aug 3, 2022

Unfortunately, it's hard to know if the changes will work or not in advance. I think they will, but it really is hard to know. They work https://github.com/Pandapip1/EIPs-clone/pull/1.

auto-merge was automatically disabled August 3, 2022 14:27

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) August 3, 2022 14:27
auto-merge was automatically disabled August 3, 2022 14:28

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) August 3, 2022 14:29
auto-merge was automatically disabled August 3, 2022 14:45

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) August 3, 2022 14:46
auto-merge was automatically disabled August 3, 2022 14:58

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) August 3, 2022 14:58
Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Left a few comments, but I have no idea what's going on here 🤣

.github/workflows/auto-review-bot.yml Outdated Show resolved Hide resolved
.github/workflows/auto-review-bot.yml Outdated Show resolved Hide resolved
.github/workflows/auto-review-trigger.yml Show resolved Hide resolved
auto-merge was automatically disabled August 3, 2022 15:53

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) August 3, 2022 15:53
@Pandapip1 Pandapip1 requested a review from SamWilsn August 3, 2022 16:10
@MicahZoltu
Copy link
Contributor

Unfortunately, it's hard to know if the changes will work or not in advance. I think they will, but it really is hard to know.

It can (and should) be tested against a fork of the EIPs repository. This is how alita did all of their testing before we merged bot changes previously.

Out of curiosity, does anyone know why we don't just rely on GitHub's built in automerge feature entirely? It requires all checks pass I believe, so as long there is a bot to check for appropriate approvals (fails other ise) it feels like that should be good enough?

@JEAlfonsoP
Copy link

Unfortunately, it's hard to know if the changes will work or not in advance. I think they will, but it really is hard to know.

It can (and should) be tested against a fork of the EIPs repository. This is how alita did (I do not want to said something different but when I contacted her, She suggested to run a yarn test (I presume manually) ) all of their testing before we merged bot changes previously.

There are some changes needed to be done in yours EIP fork: .github/workflows
We have managed to get the Bot (auto merge bot) running, still working in the test cases for the PRs.

Out of curiosity, does anyone know why we don't just rely on GitHub's built in automerge feature entirely? It requires all checks pass I believe, so as long there is a bot to check for appropriate approvals (fails other ise) it feels like that should be good enough?

I believe that automerge is not an option if You the Editors do not feel 100% comfortable with BOTs and/or Validators. Thats why I imaging there are so many open issues and, why we have been working to get an automatized working environment.

@JEAlfonsoP
Copy link

Unfortunately, it's hard to know if the changes will work or not in advance. I think they will, but it really is hard to know.

We can test it (or at least try to).

@Pandapip1
Copy link
Member Author

Out of curiosity, does anyone know why we don't just rely on GitHub's built in automerge feature entirely? It requires all checks pass I believe, so as long there is a bot to check for appropriate approvals (fails other ise) it feels like that should be good enough?

That is exactly what we're doing right now.

auto-merge was automatically disabled August 8, 2022 19:10

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) August 8, 2022 19:10
auto-merge was automatically disabled August 8, 2022 19:10

Head branch was pushed to by a user without write access

auto-merge was automatically disabled August 10, 2022 13:53

Head branch was pushed to by a user without write access

@eth-bot eth-bot enabled auto-merge (squash) August 10, 2022 13:53
@lightclient lightclient merged commit 0b07ac5 into ethereum:master Aug 10, 2022
@Pandapip1 Pandapip1 deleted the fix-auto-merge-bot branch August 10, 2022 14:29
Pandapip1 added a commit to Pandapip1/EIPs that referenced this pull request Aug 10, 2022
* Fix EIP-Bot CI

* Add newline to CODEOWNERS

* Make changes for testing

* Test if GH Actions is a valid codeowner

* It isn't allowed

* Fix infinite loop

* Do some tricks to avoid unneccesary extra runs

* Fixing bug

* Add Pandapip1-bot

* Fix quotes

* Another fix bites the dust

* Another fix

* Another thing

* Use my testing fork

* More fixes

* Unpin while in dev

* Add testing bot to codeowners

* Is that the bug?

* Try this fix

* Quickfix

* That was an easy fix

* Remove Pandapip1-bot references

* Update diff

* Missed some

* Pin to commit
@MicahZoltu
Copy link
Contributor

This PR added me back as an editor. 😢

@lightclient
Copy link
Member

Sorry about that

@ethereum ethereum deleted a comment Nov 22, 2022
@ethereum ethereum deleted a comment Nov 22, 2022
@ethereum ethereum deleted a comment Nov 22, 2022
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* Fix EIP-Bot CI

* Add newline to CODEOWNERS

* Make changes for testing

* Test if GH Actions is a valid codeowner

* It isn't allowed

* Fix infinite loop

* Do some tricks to avoid unneccesary extra runs

* Fixing bug

* Add Pandapip1-bot

* Fix quotes

* Another fix bites the dust

* Another fix

* Another thing

* Use my testing fork

* More fixes

* Unpin while in dev

* Add testing bot to codeowners

* Is that the bug?

* Try this fix

* Quickfix

* That was an easy fix

* Remove Pandapip1-bot references

* Update diff

* Missed some

* Pin to commit
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.

9 participants