Add gitlab_merge_request_rule, create Gitlab Merge Request Rule (Enterprise/Ultimate Only) #10077

mil1i · 2025-04-28T17:02:39Z

SUMMARY

Adding a new Gitlab module named gitlab_merge_request_rule. This is for managing (creating, updating and deleting) merge request rules for those with Gitlab Enterprise/Ultimate.

It can take in a group name or id, as well as usernames or user id, perform the lookup (assumes the authenticated token/user has the proper access) to properly generate the request.

Has failure messages builtin for unable to find a user or a group. If the specified protected branch is not found, it defaults back to "All Branches".

If applies_to_all_protected_branches option is set to true it ignores protected_branch if set.

This should idempotently detect changes (additions or removals) for all settings.

Could probably use some additional tweaking to reduce complexity ;d but threw this together quickly for work, thought I should share it back.

ISSUE TYPE

New Module/Plugin Pull Request

COMPONENT NAME

gitlab_merge_request_rule

ADDITIONAL INFORMATION

Example task:

- name: Create merge request rule 'first-rule' on main and develop
  community.general.gitlab_merge_request_rule:
    api_url: "https://gitlab.company.com"
    api_token: "{{ api_token }}"
    validate_certs: false
    state: present
    project_group: 'group/subgroup'
    project_name: 'repo-name'
    rule_name: 'first-rule'
    approvals_required: 3
    applies_to_all_protected_branches: false
    protected_branch: main  # API states it can take a list, but it actually only allows a single branch, exception is not passing a branch name which sets to "All branches" or setting the above flag to true for all "protected_branches".
    usernames:
        - my_username
    user_ids:
        - 290
    groups:
        - 743  # developers group

ansibullbot · 2025-04-28T17:04:04Z

cc @Lunik @Shaps @lgatellier @marwatk @metanovii @nejch @scodeman @sh0shin @suukit @waheedi @zanssa
click here for bot help

felixfontein · 2025-04-29T21:18:09Z

Thanks for your contribution! As a first quick comment, please take a look at https://github.com/ansible-collections/community.general/blob/main/CONTRIBUTING.md#creating-new-modules-or-plugins if you haven't.

felixfontein · 2025-05-04T19:50:10Z

plugins/modules/gitlab_merge_request_rule.py

+DOCUMENTATION = r"""
+module: gitlab_merge_request_rule
+short_description: Manage Gitlab Project merge request rules
+version_added: 3.4.0


The next feature release will be:

Suggested change

version_added: 3.4.0

version_added: 10.7.0

felixfontein · 2025-05-04T19:51:14Z

plugins/modules/gitlab_merge_request_rule.py

+short_description: Manage Gitlab Project merge request rules
+version_added: 3.4.0
+description:
+  - Managing Gitlab Project merge request rules (requires Enterprise/Ultimate Edition).


Suggested change

- Managing Gitlab Project merge request rules (requires Enterprise/Ultimate Edition).

- Manage Gitlab Project merge request rules.

- This currently requires an Enterprise/Ultimate Edition.

felixfontein · 2025-05-04T19:51:43Z

plugins/modules/gitlab_merge_request_rule.py

+description:
+  - Managing Gitlab Project merge request rules (requires Enterprise/Ultimate Edition).
+author:
+  - mil1i (!UNKNOWN) [[email protected]]


Suggested change

- mil1i (!UNKNOWN) [liebhaber_sgl@gwto.me]

- mil1i (@mil1i) <liebhaber_sgl@gwto.me>

felixfontein · 2025-05-04T19:52:09Z

plugins/modules/gitlab_merge_request_rule.py

+    elements: str
+  user_ids:
+    description:
+      - List of user_ids of who can approve merge requests for this rule.


Suggested change

- List of user_ids of who can approve merge requests for this rule.

- List of user ID of who can approve merge requests for this rule.

felixfontein · 2025-05-04T19:52:31Z

plugins/modules/gitlab_merge_request_rule.py

+    elements: int
+  protected_branch:
+    description:
+      - A protected branch name or branch id for this rule to apply to.


Suggested change

- A protected branch name or branch id for this rule to apply to.

- A protected branch name or branch ID for this rule to apply to.

felixfontein · 2025-05-04T19:53:30Z

plugins/modules/gitlab_merge_request_rule.py

+  applies_to_all_protected_branches:
+    description:
+      - Flag to enable applying this rule to ALL protected branches.
+      - Ignores "protected_branches" if this flag is set to True.


Suggested change

- Ignores "protected_branches" if this flag is set to True.

- Ignores O(protected_branches) if this flag is set to V(true).

Why does protected_branches have the special value none (unset) with the same meaning? I would remove that, and make protected_branches required unless this parameter is set to true. (And if this parameter is set to true, disallow specifying protected_branches.)

felixfontein · 2025-05-04T19:53:48Z

plugins/modules/gitlab_merge_request_rule.py

+    applies_to_all_protected_branches: false
+    protected_branch: main
+    usernames:
+        - someonesusername


Please also adjust the other YAML indentations.

Suggested change

- someonesusername

- someonesusername

russoz

Hi @mil1i thanks for the cobritbution!

Some initial comments. Overall, the module is over-engineered - multiple checks, including for things that were checked before, others that could be avoided entirely. Please take a look at the comments.

russoz · 2025-04-30T22:11:16Z

plugins/modules/gitlab_merge_request_rule.py

+from ansible_collections.community.general.plugins.module_utils.version import (
+    LooseVersion,
+)


Simpler as:

Suggested change

from ansible_collections.community.general.plugins.module_utils.version import (

LooseVersion,

)

from ansible_collections.community.general.plugins.module_utils.version import LooseVersion

russoz · 2025-04-30T22:25:45Z

plugins/modules/gitlab_merge_request_rule.py

+        self.rule_usernames = (
+            self._string_to_set(rule_usernames) if rule_usernames is not None else []
+        )


It might be a matter of personal preference, but I find this abundance of brackets harder to read than without them. Why not just:

Suggested change

self.rule_usernames = (

self._string_to_set(rule_usernames) if rule_usernames is not None else []

)

self.rule_usernames = self._string_to_set(rule_usernames) if rule_usernames is not None else []

russoz · 2025-05-06T04:36:32Z

plugins/modules/gitlab_merge_request_rule.py

+    if not gmmr.rule_exists:
+        if module.params["state"] == "absent":
+            module.exit_json(
+                changed=False,
+                msg="Merge Request Rule ({}) does not exist for {}, no changes made.".format(
+                    module.params["rule_name"], module.params["project_name"]
+                ),
+            )
+
+        elif module.params["state"] == "present":
+            gmmr.create_approval_rule()
+            gmmr.rules = gmmr.list_approval_rules()
+            gmmr.rule_exists, new_mmr = gmmr.check_rule_exists(gmmr.rule_name)
+            module.exit_json(
+                changed=True,
+                msg="Merge Request Rule ({}) has been successfully created.".format(
+                    module.params["rule_name"]
+                ),
+                merge_request_rule=new_mmr.asdict(),
+            )
+
+    elif gmmr.rule_exists:


No need for this to be an elif, it could be a plain else. That being said, I would place the if gmmr.rule_exists block first, then the "not exists", as else second, for the sake of readability.

russoz · 2025-05-06T04:54:50Z

plugins/modules/gitlab_merge_request_rule.py

+        project_name=dict(type="str", required=True),
+        rule_name=dict(type="str", required=True),
+        approvals_required=dict(type="int", required=True),
+        usernames=dict(type="list", elements="str", default=None, required=False),


The required=false is redundant, here and in other parameters below.

As of the default, None is redundant (is already going to be None if not passed). However, the first thing the code does with that value, inside the GitlabMergeRequestRule is to check whether it is None and then transform it to a list. Therefore, I would recommend making default=[] here and removing those verifications from the class. That might apply to other parameters as well.

Suggested change

usernames=dict(type="list", elements="str", default=None, required=False),

usernames=dict(type="list", elements="str", default=[]),

russoz · 2025-05-06T04:58:58Z

plugins/modules/gitlab_merge_request_rule.py

+            else []
+        )
+        self.rule_usernames = (
+            self._string_to_set(rule_usernames) if rule_usernames is not None else []


I understand it is going to work as-is, but... the fact that this can be either a set or a list does not help the readability of this code.

Suggested change

self._string_to_set(rule_usernames) if rule_usernames is not None else []

self._string_to_set(rule_usernames) if rule_usernames is not None else set()

russoz · 2025-05-06T05:01:04Z

plugins/modules/gitlab_merge_request_rule.py

+        rule_approvals_required=0,
+        rule_groups=None,
+        rule_user_ids=None,
+        rule_usernames=None,
+        rule_branches=None,
+        rule_all_protected_branches=False,


This class is not used outside this module, and the only one place that creates a new instance provides all the parameters. The default values in this constructor are redundant.

Suggested change

rule_approvals_required=0,

rule_groups=None,

rule_user_ids=None,

rule_usernames=None,

rule_branches=None,

rule_all_protected_branches=False,

rule_approvals_required,

rule_groups,

rule_user_ids,

rule_usernames,

rule_branches,

rule_all_protected_branches,

russoz · 2025-05-06T06:23:29Z

plugins/modules/gitlab_merge_request_rule.py

+        _protected_branch_ids = (
+            list(self.rule_branches) if self.rule_branches is not None else []
+        )


This (probably the lines below as well) are overkill: the code already guarantees that self.rule_branches is not going to be None, in lines 167-171.

mil1i · 2025-05-06T14:48:55Z

Hey thank you. I will take a look over the comments and adjust when I get a moment.

At least with the comment on the extra brackets, I had wrote it the way you suggested. However my editors formatter altered it to the way it is. Assuming due to line length.

felixfontein · 2025-05-06T20:06:22Z

You probably have some auto-formatting enabled in your editor (the extra brackets are something that black or ruff tend to insert to shorten long lines). It's probably best to disable the formatter.

felixfontein · 2025-05-24T13:21:49Z

PIng @mil1i

Add gitlab_merge_request_rule.py

09fc96f

ansibullbot added module module new_contributor Help guide this first time contributor plugins plugin (any type) labels Apr 28, 2025

This comment was marked as outdated.

Sign in to view

ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed ci_verified Push fixes to PR branch to re-run CI labels Apr 28, 2025

This comment was marked as outdated.

Sign in to view

ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Apr 28, 2025

mil1i force-pushed the gitlab-merge-request-rule branch from 9c8a4b6 to cda37cb Compare April 28, 2025 19:25

ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Apr 28, 2025

mil1i force-pushed the gitlab-merge-request-rule branch from cda37cb to 3db57b4 Compare April 28, 2025 19:29

This comment was marked as outdated.

Sign in to view

ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Apr 28, 2025

mil1i force-pushed the gitlab-merge-request-rule branch from 3db57b4 to cc4fa6c Compare April 28, 2025 23:02

ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Apr 28, 2025

This comment was marked as outdated.

Sign in to view

ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Apr 28, 2025

mil1i force-pushed the gitlab-merge-request-rule branch from cc4fa6c to e4fd556 Compare April 28, 2025 23:27

ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Apr 28, 2025

mil1i force-pushed the gitlab-merge-request-rule branch from e4fd556 to 52fec9b Compare April 28, 2025 23:35

This comment was marked as outdated.

Sign in to view

ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Apr 28, 2025

mil1i force-pushed the gitlab-merge-request-rule branch from 52fec9b to 8b22741 Compare April 28, 2025 23:45

ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Apr 28, 2025

mil1i force-pushed the gitlab-merge-request-rule branch from 8b22741 to 0afbd0f Compare April 28, 2025 23:51

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Apr 29, 2025

Formatting changes

491a4b2

mil1i force-pushed the gitlab-merge-request-rule branch from 0afbd0f to 491a4b2 Compare April 29, 2025 00:13

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Apr 29, 2025

felixfontein reviewed May 4, 2025

View reviewed changes

russoz reviewed May 6, 2025

View reviewed changes

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label May 14, 2025

felixfontein removed the backport-10 Automatically create a backport for the stable-10 branch label May 24, 2025

	- Managing Gitlab Project merge request rules (requires Enterprise/Ultimate Edition).
	- Manage Gitlab Project merge request rules.
	- This currently requires an Enterprise/Ultimate Edition.

	- mil1i (!UNKNOWN) [liebhaber_sgl@gwto.me]
	- mil1i (@mil1i) <liebhaber_sgl@gwto.me>

	- List of user_ids of who can approve merge requests for this rule.
	- List of user ID of who can approve merge requests for this rule.

	- A protected branch name or branch id for this rule to apply to.
	- A protected branch name or branch ID for this rule to apply to.

	- Ignores "protected_branches" if this flag is set to True.
	- Ignores O(protected_branches) if this flag is set to V(true).

	usernames=dict(type="list", elements="str", default=None, required=False),
	usernames=dict(type="list", elements="str", default=[]),

	self._string_to_set(rule_usernames) if rule_usernames is not None else []
	self._string_to_set(rule_usernames) if rule_usernames is not None else set()

Add gitlab_merge_request_rule, create Gitlab Merge Request Rule (Enterprise/Ultimate Only) #10077

Are you sure you want to change the base?

Add gitlab_merge_request_rule, create Gitlab Merge Request Rule (Enterprise/Ultimate Only) #10077

Uh oh!

Conversation

mil1i commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Uh oh!

ansibullbot commented Apr 28, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

felixfontein commented Apr 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

russoz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mil1i commented May 6, 2025

Uh oh!

felixfontein commented May 6, 2025

Uh oh!

felixfontein commented May 24, 2025

Uh oh!

Uh oh!

mil1i commented Apr 28, 2025 •

edited

Loading