Support rebasing through GitLab's api - #160
Conversation
The argument parser would end up being called with the `sys.argv` of `py.test`, so the only way in which this test could ever pass was if `py.test` was ran without arguments!
Add the counterpart of Mocklab but for the execution of git commands on the local repository. This means that we no longer need to mock the calls to `Job.update_from_target_branch_and_push` and can actually cover all paths of execution. In particular, we can now run all tests for both `merge` and `rebase`. The mock works by redefining the `Repo.git()` method, so that it understands the set of git commands that marge runs. It keeps an extremely simplified model of a git repository (plus the remote repositories to which it connects) and uses this to implement each command In addition, we use fixtures to try the cross product of parameters (merge/rebase; add-tested yes/no; add-part-of yes/no) which actually found minor errors (fixed in next commit)
In the first version of the code, `branch_rewritten` was meant to be a flag that would indicate whether the code managed to go past the `add_trailers()`, so that in the exception handlers we could know in where the exception came from. But because the name was ambiguous, in f956e70 it was apparently changed to reflect whether the a rewrite was performed at all after the rebase/merge. However, in some places of the error handling logic, it was still being used as the former in some places. So we rename the variables that are meant to be flagposts to have a hopefully more clear meaning, and fix the condition when we check if the branch is protected: we need to compare the final commit to the initial commit, to find out if we were actually trying to push new branch. This bug was found by the refactoring of the single-job tests.
While this was correct, it meant that in the `test_waits_for_approvals()` unit test, we would have to wait put a relatively high number of seconds to ensure we don't race and fail because we don't see the log message. Since we are now checking many more combinations of cases, this started to be noticeable in the speed of the test
They can return a 202 with empty body in a rebase request
It makes the rest call and keeps polling until it gets a result.
At the moment, the only mechanism for synchronization that we have is `git push --force`. But we will soon add a rebase via the api as an alternative mechanism (to be used in specific cases)
We replace the boolean `use_merge_stratategy` by an enum type. We add `gitlab_rebase` as one of the possible enum values. At the moment it does nothing, but we have prepared the code/tests to handle this strategy.
On a transition one can now specify a side-effect that will be run when the endpoint is called. We will need something like that in order to update the `remote_repo` of the mock of git when a rebase is done on the mock of gitlab.
We do the rebase locally and instead of pushing, request a rebase and check that the resulting sha is what we expect. We ignore all commit tagging options, but this needs to be validated on the app
|
This won't fix the batch job working with forks. So won't fix the real problem which is present in #157 |
|
@mpickering IIUC, in batch mode, after a batch passes the tests, it is first pushed to the target branch and then all the PRs are rebased, and because the changes are already there, gitlab notices the PR is a no-op and closes it. If the rebasing of the PR is done through the API I expect the same to happen. I'm probably missing something? |
|
Yes sorry, the problem is that after rebasing My attempted fix is something like - mpickering@c5b0c59 |
|
I see what you mean now! Yes, the batch processing code seems to be assuming that the rebased branch will be already in the repository (which made sense when forks where not handled) and, when using gitlab-rebase, we'd need to fetch the changes beforehand as you did there. @mpickering do you want to add a commit on top of this PR? |
|
Does my change look correct? It felt a bit hacky to me. I'm also unsure how to test this locally, I just deployed my fix and have yet to see if it actually works. |
|
@jcpetruzza Can you please tell me how to run the tests so that I can add a test where a MR comes from a fork? It has been a nightmare trying to get this to work with a 6hr CI turnaround. |
|
I tried just running The same thing happens on this branch. |
|
@mpickering You can do since pylint is super slow (there must be a better way of doing this, but well) |
|
Also, instead of doing the checkout yourself as in your commit, would it work if, whenever the rebase was done by gitlab, you pass the |
|
@jcpetruzza I will try and add some tests tonight and once I have added those, will feel a lot more confident refactoring. |
|
Just to update this. I tried adding some tests but it was too complicated for me and the tests for batch mode are quite lacking to base things from. However, I eventually got things to work properly but you should be aware that there is a race condition present in the rebase API where the API will say that the rebase has concluded but the change is not reflected yet in the remote. I had to add a big sleep (2 minutes) to be sure that this situation didn't happen. You can see the changes I made on my branch if you're interested in merging them into upstream. I'm very reluctant to do any refactoring now though due to the lack of tests. |
|
@mpickering @jcpetruzza @aschmolck I'm trying it out and indeed it seems to hit a race condition. It basically tries to rebase twice, which in our case causes the pipeline to be skipped. That in turns makes it possible to immediately merge the branch before waiting until the tests on the branch actually pass. Please see the screenshot below.
Looks like marge-bot also complains about someone skipping the queue, but no-one was merging any other branches at the same time.
Any ways to work around this? |
|
We are using a fork of marge now (in order to fix the batch merge mode). The only workaround I know is to increase the timeouts. |
|
The problem of race condition is due to Gitlab not returning |
|
#348 adding |
6 participants
Here's an attempt to add support for GitLab's new "merge_request/rebase" endpoint (#157), which should enable using marge in public gitlab instances (#159). It is enabled with
--rebase-remotelyand conflicts with--use-merge-strategyand all theadd-xxxoptions.It works by performing the rebase locally but replacing the
git push --forcestep by a call to this endpoint.Because the change happens inside
MergeJob.update_from_target_branch_and_push(), it wouldn't have been picked by the mocklab tests, where this method was being replaced by a mock. So I went down a bit of a rabbit hole at first and started by adding a mock of git, to fix this. The mock works by hooking into theRepo.git()method and interpreting the git commands that marge emits, using an extremely simplified model of git.Because now these tests can see inside
update_from_target_branch_and_push(), I make it so that they run on (almost) all combinations of fusion-strategies and trailer-adding, which caught a small bug in the handling of protected branches.While the PR is a bit large, the individual commits should be smallish and self-contained..