search

futurelearn / squiddy

Github Actions bot
1 stars 1 forks source link

Teach Squiddy how to check CI status #14

Closed elenatanasoiu closed 3 years ago

elenatanasoiu commented 3 years ago

It's been requested multiple times that we teach @squiddy-bot how to check CI status before it goes ahead with a merge.

This PR:

Tested successfully on this PR: https://github.com/futurelearn/test-github-actions/pull/51#issuecomment-750825979. (I recycled an existing PR for this so I've just linked to where we started using this PR for bubble merging).

We've tested the following states:

Since this PR has gotten quite big I'm going to defer other improvements to subsequent PRs. Some things I'd like to improve further are:

  1. Move rebase action into squiddy. This would allow us to:
    • Have more descriptive error messages on rebase fail. At the moment we just offer a generic comment: "Rebase was not possible. Please rebase manually and try again". Moving the rebase action into squiddy would give us better control over the errors reported from the github API.
    • I can see that the custom rebase action also starts up its own docker container 😱 so moving this into squiddy would speed up the entire process.
  2. Move "fixup!" commit check into squiddy. (maybe)
  3. Wait a bit longer after a successful merge to delete a branch. At the moment if a merge succeeds, we immediately delete the branch. Sometimes Github takes its time closing the PR, so if the branch has already been deleted, it winds up closing the PR with a red "CLOSED" status rather than "MERGED".
urbanautomaton commented 3 years ago

Thanks Elena, this is exciting!

I'll have a proper read through (though I might not finish by 3pm), but just so I understand: does this change mean it now rebases, pushes and then waits for CI again? Or is it that it now checks the PR status before doing its rebase-and-merge?

elenatanasoiu commented 3 years ago

does this change mean it now rebases, pushes and then waits for CI again?

From what I've seen on the test PR there's two scenarios:

  1. If the PR needs rebasing, it will rebase and trigger the CI checks which will prevent the merge from happening
  2. If the PR doesn't need rebasing, it won't trigger CI again so merges can go ahead

Or is it that it now checks the PR status before doing its rebase-and-merge?

The rebase action happens before Squiddy using this action so the CI check happens after it.

From testing, I can see that this doesn't trigger CI on push if the PR is already rebased.

elenatanasoiu commented 3 years ago

Scenario 1 could be further improved by moving the rebase action inside Squiddy because we can then decide earlier to just rebase and exit (because we know we've just triggered CI so we won't be able to merge).

urbanautomaton commented 3 years ago

[weeks pass] Thanks for the clarification, Elena!

If two branches needing a rebase are sent to squiddy for merge at the same time, does it rebase and push both of them simultaneously? I'm just wondering what the implications are of the new ~10 min delay between typing @.squiddy-bot merge and the merge actually occurring.

For example, if three PRs needing rebase get sent to squiddy within a couple of minutes of each other, will we end up with a history like this? (which git log --graph makes a pig's ear of, tbh)

*   c202f29388 - (HEAD -> master) Merge branch 'three' into master (3 seconds ago) <Simon Coffey>
|\
| * a506d7e578 - (three) three (39 seconds ago) <Simon Coffey>
* |   5adaab291a - Merge branch 'two' into master (12 seconds ago) <Simon Coffey>
|\ \
| * | ed75d7c8cb - (two) two (56 seconds ago) <Simon Coffey>
| |/
* |   9b340dc3a8 - Merge branch 'one' into master (25 seconds ago) <Simon Coffey>
|\ \
| |/
|/|
| * f50b58bf73 - (one) one (68 seconds ago) <Simon Coffey>
|/
*
elenatanasoiu commented 3 years ago

If two branches needing a rebase are sent to squiddy for merge at the same time, does it rebase and push both of them simultaneously?

Yes, it will rebase both of them and push them up. HOWEVER, this will trigger pending CI checks which will make squiddy quit. You will then have to comment @.squiddy-bot merge again in order retry the merge. At that point, if a previous PR has been merged, a rebase will be performed again since the entire bubble_merge action will be executed from the beginning, including the rebase check.

I'm just wondering what the implications are of the new ~10 min delay between typing @.squiddy-bot merge and the merge actually occurring.

So because you have to start the whole process over because squiddy exited with a "CI pending" message, it won't be a 10 min delay between the rebase and merge. We'll check again if we need to do a rebase every time you comment.

elenatanasoiu commented 3 years ago

@urbanautomaton I've performed some tests to show the behaviour we have with multiple PRs. Let me know what you think.

Scenario 1: All three PRs need a rebase and change the same file

I created three PRs that needed a rebase (an extra commit was added to master to trigger a rebase). I then told @squiddy-bot to merge all three.

PR#1 - stopped on the rebase action with a message: "Rebase was not possible. Please rebase manually and try again." PR#2 - merged successfully PR#3 - stopped on the merge action with an API Merge Conflict response: Screenshot 2021-01-05 at 10 54 39 (please ignore the format of the error message, I've made it slightly nicer now)

Screenshot 2021-01-05 at 12 21 16

Scenario 2: All three PRs need a rebase and change different files

For this one I called squiddy in quick succession for PRs 4 & 5 (within one second of each other), and waited 5 seconds more before calling squiddy on PR 6.

PR#4 - merged successfully PR#5 - merged successfully PR#6 - stopped on rebase action

I can see that the scenario you were talking about happened for PRs 4&5, but for the PR where a bit of lag was introduced the rebase prevented the merge:

Screenshot 2021-01-05 at 12 22 09

So you're correct in saying that multiple merge API calls in very quick succession will cause this, but I think this was already present before CI checks were introduced so I'm not sure we should be fixing this here.

I'm happy to keep looking at this separately. I'm not quite sure how we could stop two parallel merge calls to the API at the moment.

elenatanasoiu commented 3 years ago

Spoke to Simon offline and we agreed this is basically fine since Squiddy will exit rather than wait for CI checks to finish, so we don't introduce extra waiting time. See this PR for an example of the flow.