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 guarantee final pipeline #351

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

qusielle
Copy link

@qusielle qusielle commented Sep 30, 2022

Background:
There are cases when a project needs to have a final run of the CI pipeline before the merge.
For example, if a team practices fixup-driven code review process in a project, the fixup commits must be auto-squashed prior to a merge. This can be done in CI check by having a job that performs the check only when Marge-bot is assigned. Gitlab CI is not triggered automatically on actions like assigning, so that Marge-bot --guarantee-final-pipeline option is what is exactly needed in this case: it is supposed to trigger a pipeline right before the merge.

Problem and cause:
--guarantee-final-pipeline does not trigger Gitlab CI at all. Only a comment jenkins retry appears in the review. Looking to the code, it is visible that no more than additional 30 seconds of waiting is implemented.

Measure:

  • When --guarantee-final-pipeline is specified, change the Marge-bot behavior to triggering a merge request pipeline (falling back to a regular branch pipeline if MR workflow is not set up) and waiting for it to pass.
  • Check the solution in Marge-bot tests.

Background:
There are cases when a project needs to have a final run of the CI
pipeline before the merge.
For example, if a team practices fixup-driven code review process in a
project, the fixup commits must be auto-squashed prior to a merge. This
can be done in CI check by having a job that performs the check only
when Marge-bot is assigned. Gitlab CI is not triggered automatically on
actions like assigning, so that Marge-bot `--guarantee-final-pipeline`
option is what is exactly needed in this case: it is supposed to trigger
a pipeline right before the merge.

Problem and cause:
`--guarantee-final-pipeline` does not trigger Gitlab CI at all. Only a
comment `jenkins retry` appears in the review. Looking to the code, it
is visible that no more than additional 30 seconds of waiting is
implemented.

Measure:
- When `--guarantee-final-pipeline` is specified, change the Marge-bot
  behavior to triggering a merge request pipeline (falling back to a
  regular branch pipeline if [MR workflow][0] is not set up) and waiting
  for it to pass.
- Check the solution in Marge-bot tests.

[0]: https://docs.gitlab.com/ee/ci/pipelines/merge_request_pipelines.html#prerequisites
`--guarantee-final-pipeline` flag is a bit hidden now from a potential
user who has no Merge-bot installed yet because it is not mentioned in
the readme file.

Add the output from `--help`.
@@ -64,8 +64,9 @@ def update_merge_request_and_accept(self, approvals):

if _updated_sha == actual_sha and self._options.guarantee_final_pipeline:
Copy link

@d3m3vilurr d3m3vilurr Mar 14, 2023

Choose a reason for hiding this comment

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

i'm not sure that this condition is right or not.
maybe it could be if _updated_sha != actual_sha && self._options.guarantee_final_pipeline

in my test with this patch, marge bot is still unwaited the final testing result.

I found more good point
Line 88 to be

if target_project.only_allow_merge_if_pipeline_succeeds or self._options.guarantee_final_pipeline:

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your interest in the PR, @d3m3vilurr.

in my test with this patch, marge bot is still unwaited the final testing result.

Do you have the "Pipelines must succeed" option checked in Gitlab project settings? In my understanding, the main point of the --guarantee-final-pipeline switch is to enforce the mentioned Gitlab merge strategy in the case when no actual changes happened to the MR commits, so the pipeline is triggered for the last time, as it must succeed.

Choose a reason for hiding this comment

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

you are right. we didn't mark the pipeline must succeed in gitlab project setting.
because we want to push & merge without margebot in emergency time.

in my case, a bot started with guarantee_final_pipeline, when reassigned to a bot, a branch of MR was rebased with a target branch by a bot.
(a target branch has another commits, so technically, MR was changed)
but if a project doesn't have pipeline must succeed option, the last checking CI would be always failed cuz bot didn't wait for this CI before merging.

So, I want to wait for CI result like another bot (such as bors-ng)

@felh
Copy link

felh commented Apr 13, 2023

@qusielle @d3m3vilurr ping.
This feature would be very interesting to us as well but my company only accepts tagged/released versions. Would be great to have this feature merged soon :)

@qusielle
Copy link
Author

@qusielle @d3m3vilurr ping. This feature would be very interesting to us as well but my company only accepts tagged/released versions. Would be great to have this feature merged soon :)

Hello @felh,
I would be happy to get this merged but I am afraid the repo is abandoned by its maintainers, as there are more people with active PRs and no response.

@JohnVillalovos
Copy link

@qusielle @d3m3vilurr ping. This feature would be very interesting to us as well but my company only accepts tagged/released versions. Would be great to have this feature merged soon :)

Hello @felh,
I would be happy to get this merged but I am afraid the repo is abandoned by its maintainers, as there are more people with active PRs and no response.

There are a group of people who have recently forked the project at https://gitlab.com/marge-org/marge-bot

@nejch
Copy link
Contributor

nejch commented Apr 16, 2023

@qusielle @d3m3vilurr ping. This feature would be very interesting to us as well but my company only accepts tagged/released versions. Would be great to have this feature merged soon :)

Hello @felh,
I would be happy to get this merged but I am afraid the repo is abandoned by its maintainers, as there are more people with active PRs and no response.

There are a group of people who have recently forked the project at https://gitlab.com/marge-org/marge-bot

Hey folks, as John said, this pull request has been imported into the community fork:

https://gitlab.com/marge-org/marge-bot/-/merge_requests/351

If you'd like to see it merged, please join over there. @qusielle do you have a GitLab.com account?

@qusielle
Copy link
Author

Great to hear it was forked into something maintainable!

If you'd like to see it merged, please join over there. @qusielle do you have a GitLab.com account?

@nejch, yes, I do. It is the same name there. I will get into it a bit later this week.

@d3m3vilurr
Copy link

d3m3vilurr commented Apr 20, 2023

@qusielle @d3m3vilurr ping. This feature would be very interesting to us as well but my company only accepts tagged/released versions. Would be great to have this feature merged soon :)

Hello @felh,
I would be happy to get this merged but I am afraid the repo is abandoned by its maintainers, as there are more people with active PRs and no response.

There are a group of people who have recently forked the project at https://gitlab.com/marge-org/marge-bot

Hey folks, as John said, this pull request has been imported into the community fork:

https://gitlab.com/marge-org/marge-bot/-/merge_requests/351

If you'd like to see it merged, please join over there. @qusielle do you have a GitLab.com account?

couple weeks ago, I also got a email from gitlab.org about import an issue.
I'm glad to see a maintainable fork :)

now I've reopened a thread of my comment in gitlab, and copied thread comments of this to there.

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.

5 participants