[BUGFIX] Force pushing do not work with protected branches #124
Conversation
Error: "I couldn't merge this branch: failed to push merged changes, check my logs!" `GitLab: You are not allowed to force push code to a protected branch on this project.` As mentioned here: https://about.gitlab.com/2014/11/26/keeping-your-code-protected/ `Note that even Maintainer is not able to force push to or delete a protected branch.`
|
Another reason for using a merge strategy? |
|
Marge only force pushes to feature branches. Do you have a reason for keeping these protected? Usually you would only protect your master branch. |
|
@JaimeLennox quiet the contrary, why would you not use marge to merge to your master branch? This heavily limits the usability and forces avoidable manual intervention in the CI/CD. @Jellby What do you mean? it happens on fast-forward and merge commits, tried both. |
|
@grebois Of course, the idea is to use marge to merge to the master branch. But for merging to So a force-push is only used when rebasing the changes in the merge-request branch. And the question would be why to protect those branches since they are ephemeral. |
|
@jcpetruzza in my particular use case, im using the environment branches approach: https://docs.gitlab.com/ee/workflow/gitlab_flow.html#environment-branches-with-gitlab-flow In this case, all release branches, like pre-production and production are protected:
So I would like to have Marge merge this for me, but she cant, am I missing something? |
|
@grebois So, if I understand correctly, you are asking marge to merge a request from Pre-production (protected) to Production, and by the time she tries to do this, Production has moved and is ahead of Pre-production and you get a failure when she tries to rebase I'm not sure why your Production branch is ahead of Pre-production in the first-place, though. Maybe you are squashing the commits when moving from Pre-production to Production? What would you want marge to do in that scenario? I mean, even if you were to turn off push-force, it seems to me that moving commits from Production to Pre-production goes against the workflow you are trying to implement |
|
@jcpetruzza let me show you, I have master (protected) and production (protected) both up-to-date:
Now I do a simple commit to master, which makes production behind by one:
I create a merge request:
and assign it to marge:
Minutes later I got,
So I go to the logs and I get: @Jellby So I go and do the same with use-merge-strategy:
Same error, and in the logs: Marge is a maintainer of the project:
and I am doing Merge Commits, it also happens with fast-forward
not sure what I'm doing wrong if this is not the expected behavior. |
|
@grebois Ok, I think this makes sense. So, your master branch should by construction never be behind production, which means that when marge rebases master onto production, nothing should change. It then does a push-force regardless since, assuming the branch is not protected, that will be a no-op anyway. For a rebase-based workflow, maybe the right thing to do would be not to do the push-force if after rebasing the head of the source branch hasn't moved (i.e., make the no-op explicit) since that will then also work on protected branches. However. this may not won't work for a merge-based workflow, since after the merge, a merge commit will have been created anyway, so pushing is necessary (am I right here, @Jellby?) So in my opinion a more robust solution could perhaps be:
|
|
@grebois In any case, the reason you are using marge for this is for uniformity since you use it for the previous stages of development, right? I mean, otherwise, you could just accept the merge request directly, or am I missing something? |
I'm not sure what happens with a "merge commit", what I'd recommend is the "fast-forward" option in GitLab, and the
I still think there should be a prior check as in #112: If there is no need, don't try pushing at all. Then, if a push is needed, I'd rather check first if it requires -force or not. |
|
I agree we should have a more appropriate error. @grebois The force push is needed here. Marge has the ability to add trailers (as explained in https://github.com/smarkets/marge-bot#adding-reviewed-by-tested-and-part-of-to-commit-messages), and the process of doing this rewrites the commits; hence the force push is required to update the repo with the rewritten commits. I can see from the logs you posted that Marge is trying to add the "Reviewed-By" trailer; if you would prefer not to have a force push, then I think not setting any options for trailers should solve this. Can you try running Marge like this and see if this is still an issue? |
|
@JaimeLennox still with fast-forward and
I get: it always force pushes :/ |
|
It always force-pushes indeed. In my tests I couldn't get a failure when manually trying to force-push a no-op to a protected branch. But then again, I didn't get a failure when trying to push a no-op to a branch where I have no push permission. However, the latter failed for me with marge-bot, so I guess the former is failing for you. It must be something related to the git settings... |
|
The force push is a no-op at this point. I don't see the error from GitLab anymore, so I think this is a separate issue - and probably related to the fact that you are merging from master to somewhere else (which is a case we don't consider that often). It seems we assume that if you're merging from master, then you are also merging from a forked repo. This is obviously not always the case, and should be fixed, but it's unrelated to the force pushing (which is fine at this stage). |
|
@JaimeLennox I get the same error between two randomly protected branches. @Jellby You were right, it was related to git setting, now it works without "Reject unsigned commits" which was for some reason enabled, any way to make Marge sign the commits? |
Or at least do a final check before failing at the end: Is the commit I want to push identical to the current remote one? Yes: issue a warning. No: fail |
|
I've double checked that force pushing to a protected branch is possible without errors as long as it's a fast forward. @grebois in the workflow you describe, this would therefore not be an issue, provided each successive branch is simply behind the one prior. Of course, if you do cherry-picks across them then this would not work, as a rebase would be required and Marge would then need to force push. I've therefore added a separate issue (#128) in favour of this to respond with a more appropriate message for protected branches, and will close this MR as I don't see the solution here as relevant to the issue at hand. Int terms of signing commits, it would be relatively easy to get Marge to do so (by providing a GPG key), but I'm not sure this makes sense. If Marge doesn't need to rewrite commits, then there isn't an issue, as the commits will stay signed; if Marge does need to rewrite commits, then she would have to use her own key, which of course means the commits are no longer signed by the original authors - to me, this defeats the purpose of doing so. |
Fixes an issue uncovered by #124 where Marge throws an assertion error if the source/target branches are from the same project and the source branch is master. We assume that merges will always be into master, which is obviously not always the case; you may have a workflow requiring you to merge from master into another branch (e.g. a staging/production branch).
|
@JaimeLennox @Jellby thanks a lot for your overall effort on this project and your constant and insightful follow up on this particular topic, great work! |
Fixes an issue uncovered by #124 where Marge throws an assertion error if the source/target branches are from the same project and the source branch is master. We assume that merges will always be into master, which is obviously not always the case; you may have a workflow requiring you to merge from master into another branch (e.g. a staging/production branch).
Error: "I couldn't merge this branch: failed to push merged changes, check my logs!"
GitLab: You are not allowed to force push code to a protected branch on this project.As mentioned here: https://about.gitlab.com/2014/11/26/keeping-your-code-protected/
Note that even Maintainer is not able to force push to or delete a protected branch.