From f55d7e7bb9425d678e5e30e61d71383855a4c3ea Mon Sep 17 00:00:00 2001 From: Qusielle Date: Wed, 28 Sep 2022 19:04:54 +0200 Subject: [PATCH 1/2] Fix `guarantee_final_pipeline` feature 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 --- marge/merge_request.py | 21 ++++++++ marge/single_merge_job.py | 5 +- pylintrc | 2 +- tests/test_merge_request.py | 59 ++++++++++++++++++++++- tests/test_single_job.py | 96 +++++++++++++++++++++++++++++++++++-- 5 files changed, 175 insertions(+), 8 deletions(-) diff --git a/marge/merge_request.py b/marge/merge_request.py index 030b79b8..a00334ef 100644 --- a/marge/merge_request.py +++ b/marge/merge_request.py @@ -235,6 +235,27 @@ def fetch_approvals(self): def fetch_commits(self): return self._api.call(GET('/projects/{0.project_id}/merge_requests/{0.iid}/commits'.format(self))) + def trigger_pipeline(self): + """ + Triggers a pipeline for the merge request. + + At first, try to trigger a merge request pipeline, which is different + from a normal Gitlab CI pipeline and should be configured[0]. + If this fails due to unavailable merge request job definitions, trigger + a normal pipeline for the source branch. + + [0]: https://docs.gitlab.com/ee/ci/pipelines/merge_request_pipelines.html + """ + try: + return self._api.call(POST( + '/projects/{0.project_id}/merge_requests/{0.iid}/pipelines'.format(self))) + except gitlab.BadRequest as exc: + if 'No stages / jobs for this pipeline.' in exc.error_message.get('base', []): + log.info('The pipeline is not configured for MR jobs, triggering a usual pipeline.') + return self._api.call(POST( + '/projects/{0.project_id}/pipeline?ref={0.source_branch}'.format(self))) + raise + class MergeRequestRebaseFailed(Exception): pass diff --git a/marge/single_merge_job.py b/marge/single_merge_job.py index 1f8155ba..eb2c1895 100644 --- a/marge/single_merge_job.py +++ b/marge/single_merge_job.py @@ -64,8 +64,9 @@ def update_merge_request_and_accept(self, approvals): if _updated_sha == actual_sha and self._options.guarantee_final_pipeline: log.info('No commits on target branch to fuse, triggering pipeline...') - merge_request.comment("jenkins retry") - time.sleep(30) + pipeline_info = merge_request.trigger_pipeline() + log.info('Pipeline %s is triggered, waiting for it to finish...', pipeline_info.get('id')) + self.wait_for_ci_to_pass(merge_request, actual_sha) log.info( 'Commit id to merge %r into: %r (updated sha: %r)', diff --git a/pylintrc b/pylintrc index bc5347e1..a2001828 100644 --- a/pylintrc +++ b/pylintrc @@ -30,7 +30,7 @@ max-line-length=110 [DESIGN] max-args=10 max-attributes=15 -max-public-methods=35 +max-public-methods=36 # Maximum number of locals for function / method body max-locals=25 diff --git a/tests/test_merge_request.py b/tests/test_merge_request.py index 7259ea72..eb15c14a 100644 --- a/tests/test_merge_request.py +++ b/tests/test_merge_request.py @@ -2,7 +2,7 @@ import pytest -from marge.gitlab import Api, GET, POST, PUT, Version +from marge.gitlab import Api, BadRequest, GET, POST, PUT, Version from marge.merge_request import MergeRequest, MergeRequestRebaseFailed import marge.user @@ -228,6 +228,63 @@ def test_fetch_assigned_at(self): )) assert result == 1597733578.093 + def test_trigger_pipeline_succeeds(self): + api = self.api + expected_result = object() + + def side_effect(request): + if request.endpoint == '/projects/1234/merge_requests/54/pipelines': + return expected_result + return None + + api.call = Mock(side_effect=side_effect) + + result = self.merge_request.trigger_pipeline() + + assert api.call.call_args_list == [ + call(POST('/projects/1234/merge_requests/54/pipelines')), + ] + + assert result == expected_result + + def test_trigger_pipeline_fallback_succeeds(self): + api = self.api + expected_result = object() + + def side_effect(request): + if request.endpoint == '/projects/1234/merge_requests/54/pipelines': + raise BadRequest(400, {'message': {'base': 'No stages / jobs for this pipeline.'}}) + if request.endpoint == '/projects/1234/pipeline?ref=useless_new_feature': + return expected_result + return None + + api.call = Mock(side_effect=side_effect) + + result = self.merge_request.trigger_pipeline() + + assert api.call.call_args_list == [ + call(POST('/projects/1234/merge_requests/54/pipelines')), + call(POST('/projects/1234/pipeline?ref=useless_new_feature')), + ] + + assert result == expected_result + + def test_trigger_pipeline_fallback_fails(self): + api = self.api + + def side_effect(request): + if request.endpoint == '/projects/1234/merge_requests/54/pipelines': + raise BadRequest(500, {'message': {'base': 'Another error.'}}) + + api.call = Mock(side_effect=side_effect) + + with pytest.raises(BadRequest): + self.merge_request.trigger_pipeline() + + assert api.call.call_args_list == [ + call(POST('/projects/1234/merge_requests/54/pipelines')), + ] + def _load(self, json): old_mock = self.api.call self.api.call = Mock(return_value=json) diff --git a/tests/test_single_job.py b/tests/test_single_job.py index a6ba9c92..70240c54 100644 --- a/tests/test_single_job.py +++ b/tests/test_single_job.py @@ -15,7 +15,7 @@ import marge.project import marge.single_merge_job import marge.user -from marge.gitlab import GET, PUT +from marge.gitlab import GET, POST, PUT from marge.job import Fusion from marge.merge_request import MergeRequest from tests.git_repo_mock import RepoMock @@ -64,6 +64,7 @@ def __init__( fork=False, expect_gitlab_rebase=False, merge_request_options=None, + guarantee_final_pipeline=False, ): super().__init__( initial_master_sha, @@ -100,6 +101,24 @@ def __init__( to_state='pushed', ) + if guarantee_final_pipeline: + # Corresponds to the `merge_request.trigger_pipeline()` call. + api.add_transition( + POST( + '/projects/1234/merge_requests/{}/pipelines'.format( + self.merge_request_info['iid'], + ), + ), + Ok({}), + to_state='final_pipeline_triggered', + ) + # Corresponds to `pipelines_by_branch()` by `wait_for_ci_to_pass`. + api.add_pipelines( + self.merge_request_info['source_project_id'], + _pipeline(sha1=rewritten_sha, status='success', ref=self.merge_request_info['source_branch']), + from_state=['final_pipeline_triggered'], to_state='pushed' + ) + api.add_pipelines( self.merge_request_info['source_project_id'], _pipeline(sha1=rewritten_sha, status='running', ref=self.merge_request_info['source_branch']), @@ -208,14 +227,19 @@ def add_part_of(self, request): def add_reviewers(self, request): return request.param + @pytest.fixture(params=[True, False]) + def guarantee_final_pipeline(self, request): + return request.param + @pytest.fixture() - def options_factory(self, fusion, add_tested, add_reviewers, add_part_of): + def options_factory(self, fusion, add_tested, add_reviewers, add_part_of, guarantee_final_pipeline): def make_options(**kwargs): fixture_opts = { 'fusion': fusion, 'add_tested': add_tested, 'add_part_of': add_part_of, 'add_reviewers': add_reviewers, + 'guarantee_final_pipeline': guarantee_final_pipeline, } assert not set(fixture_opts).intersection(kwargs) kwargs.update(fixture_opts) @@ -255,9 +279,14 @@ def patch_sleep(self): yield @pytest.fixture() - def mocklab_factory(self, fork, fusion): + def mocklab_factory(self, fork, fusion, guarantee_final_pipeline): expect_rebase = fusion is Fusion.gitlab_rebase - return partial(SingleJobMockLab, fork=fork, expect_gitlab_rebase=expect_rebase) + return partial( + SingleJobMockLab, + fork=fork, + expect_gitlab_rebase=expect_rebase, + guarantee_final_pipeline=guarantee_final_pipeline, + ) @pytest.fixture() def mocks_factory(self, mocklab_factory, options_factory, update_sha, rewrite_sha): @@ -352,6 +381,16 @@ def test_succeeds_if_skipped(self, mocks): _pipeline(sha1=mocklab.rewritten_sha, status='skipped'), from_state=['skipped', 'merged'], ) + # `pipelines_by_branch()` by `wait_for_ci_to_pass`. + api.add_pipelines( + mocklab.merge_request_info['source_project_id'], + _pipeline( + sha1=mocklab.rewritten_sha, + status='skipped', + ref=mocklab.merge_request_info['source_branch'], + ), + from_state=['final_pipeline_triggered'], to_state='passed' + ) job.execute() assert api.state == 'merged' @@ -390,6 +429,15 @@ def test_fails_if_ci_fails(self, mocks): _pipeline(sha1=mocklab.rewritten_sha, status='failed'), from_state=['failed'], ) + api.add_pipelines( + mocklab.merge_request_info['source_project_id'], + _pipeline( + sha1=mocklab.rewritten_sha, + status='failed', + ref=mocklab.merge_request_info['source_branch'], + ), + from_state=['final_pipeline_triggered'], to_state='failed', + ) with mocklab.expected_failure("CI failed!"): job.execute() @@ -408,6 +456,15 @@ def test_fails_if_ci_canceled(self, mocks): _pipeline(sha1=mocklab.rewritten_sha, status='canceled'), from_state=['canceled'], ) + api.add_pipelines( + mocklab.merge_request_info['source_project_id'], + _pipeline( + sha1=mocklab.rewritten_sha, + status='canceled', + ref=mocklab.merge_request_info['source_branch'], + ), + from_state=['final_pipeline_triggered'], to_state='canceled', + ) with mocklab.expected_failure("Someone canceled the CI."): job.execute() @@ -426,6 +483,16 @@ def test_fails_on_not_acceptable_if_master_did_not_move(self, mocks): Ok({'commit': _commit(commit_id=new_branch_head_sha, status='success')}), from_state='pushed', to_state='pushed_but_head_changed' ) + api.add_pipelines( + mocklab.merge_request_info['source_project_id'], + _pipeline( + sha1=new_branch_head_sha, + status='success', + ref=mocklab.merge_request_info['source_branch'], + ), + from_state=['pushed_but_head_changed'] + ) + with mocklab.expected_failure("Someone pushed to branch while we were trying to merge"): job.execute() @@ -541,6 +608,27 @@ def push_effects(remote_url, remote_branch, old_sha, new_sha): Ok({'commit': _commit(commit_id=moved_master_sha, status='success')}), from_state='merge_rejected' ) + # Overwrite original `guarantee_final_pipeline` transition: no need in + # the state changing here. + api.add_transition( + POST( + '/projects/1234/merge_requests/{}/pipelines'.format( + mocklab.merge_request_info['iid'], + ), + ), + Ok({}), + ) + # Register additional pipeline check introduced by + # `guarantee_final_pipeline`. + api.add_pipelines( + mocklab.merge_request_info['source_project_id'], + _pipeline( + sha1=first_rewritten_sha, + status='success', + ref=mocklab.merge_request_info['source_branch'] + ), + from_state=['pushed_but_master_moved', 'merge_rejected'], + ) if fusion is Fusion.gitlab_rebase: rebase_url = '/projects/{project_id}/merge_requests/{iid}/rebase'.format( project_id=mocklab.merge_request_info['project_id'], From e815d6d74c241a3f58bd832b908ef97c002270e6 Mon Sep 17 00:00:00 2001 From: Qusielle Date: Fri, 30 Sep 2022 21:02:06 +0200 Subject: [PATCH 2/2] Mention `--guarantee-final-pipeline` in readme `--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`. --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index 541e0d6b..77c54f1c 100644 --- a/README.md +++ b/README.md @@ -141,6 +141,9 @@ optional arguments: Use merge commit when creating batches, so that the commits in the batch MR will be the same with in individual MRs. Requires sudo scope in the access token. [env var: MARGE_USE_MERGE_COMMIT_BATCHES] (default: False) --skip-ci-batches Skip CI when updating individual MRs when using batches [env var: MARGE_SKIP_CI_BATCHES] (default: False) + --guarantee-final-pipeline + Guaranteed final pipeline when assigned to marge-bot + [env var: MARGE_GUARANTEE_FINAL_PIPELINE] (default: False) ``` Here is a config file example ```yaml