gitlabform: MR approvers not working in some cases

I am beginning working on this feature. For a start I would like to get feedback on a configuration syntax proposal.

The current syntax, f.e.:

    merge_requests:
      approvals:
        approvals_before_merge: 2
        reset_approvals_on_push: false
        disable_overriding_approvers_per_merge_request: true
        merge_requests_author_approval: false
      approvers:
        - user1
        - user2
      approver_groups:
        - my-group
        - my-group1/subgroup
        - my-group2/subgroup/subsubgroup
      remove_other_approval_rules: false

…would remain working for users who don’t need more than 1 approval rule and related syntax complexity. The single approval rule managed this way would keep its current name - “Approvers (configured using GitLabForm)”.

But the remove_other_approval_rules key would be deprecated and a more standard enforce: true key would replace it.

So the new syntax would look like this:

    merge_requests:
      approvals:
        # (no changes here)
        reset_approvals_on_push: false
        disable_overriding_approvers_per_merge_request: true
        merge_requests_author_approval: false
      approval_rules:
      	standard:
          approvals_required: 2
          name: "Regular approvers"
          groups:
            - developers
      	security:
          approvals_required: 2
          name: "Extra Security Team approval for selected branches"
          users:
            - secmember1
            - secmember2
          protected_branches:
            - sensitive-branch 
        enforce: true 

…where:

• “standard” and “security” are just labels,
• the values under these keys are like in https://docs.gitlab.com/ee/api/merge_request_approvals.html#create-project-level-rule but:
  • instead of “group_ids” there would be “groups” (names)
  • instead of “protected_branch_ids” there would be “protected_branches” (names)
  • instead of “user_ids” there would be “users” (names)

So to achieve what you wanted, @weakcamel, we would need to use the following config syntax:

    merge_requests:
      # (...)
      approval_rules:
      	default:
          approvals_required: 1
          name: "All Members"
          rule_type: any_approver
        enforce: true 

Opinions?

cc @jimisola @amimas

Please try out v3.4.0rc1 with this feature and a syntax working like described above, @weakcamel. Let me know what you think!

What @zanella is tackling in #423 made me realize that from the implementation point of view it would be easier to treat managing Merge Requests approvals split into:

• approvals settings - with a SingleEntityProcessor,
• approval rules - with a MultipleEntitiesProcessor,

The simplest way to do it would be to change the new syntax to this (note that merge_requests_approvals and merge_requests_approval_rules are new sections just below group/project):

    merge_requests_approvals:
      # (no changes here)
      reset_approvals_on_push: false
      disable_overriding_approvers_per_merge_request: true
      merge_requests_author_approval: false
    merge_requests_approval_rules:
      standard:
        approvals_required: 2
        name: "Regular approvers"
        groups:
          - developers
      security:
        approvals_required: 2
        name: "Extra Security Team approval for selected branches"
        users:
          - secmember1
          - secmember2
        protected_branches:
          - sensitive-branch 
      enforce: true 

(Of course in v3.x the old syntax would still be supported, but with a deprecation warning recommending to update to a new syntax.)

I am happy to report that the v3.4.0 with this feature has finally been released as a new stable release. 🥳

The docs are available at https://gitlabform.github.io/gitlabform/reference/merge_requests/.

Big thanks to everyone for your cooperation on this!

Please open new issues if you find any bugs in the new features.

Thanks @weakcamel! I plan to implement this feature over the next few weeks and will release a RC version and I will appreciate testing it. 😃

The proposal looks good to me @gdubicki .

For the new syntax of setting custom approval rules:

the values under these keys are like in https://docs.gitlab.com/ee/api/merge_request_approvals.html#create-project-level-rule but:

instead of “group_ids” there would be “groups” (names) instead of “protected_branch_ids” there would be “protected_branches” (names) instead of “user_ids” there would be “users” (names)

Is it more work to support/remove the usage of ids? Personally I don’t think I would use the ids, but we seem to support that for advanced branch protection settings (i.e. allowed to push, allowed to merge, etc.).

https://gitlabform.github.io/gitlabform/reference/protected_branches/#premium-only-features

So, having ids would make it consistent across gitlabform syntax. That could be useful if someone is modularizing their config using yaml anchors or other setup. Or, maybe the usage of ids in branch protection gets deprecated/removed in v4.

Also, I assume this is being planned for v3. Perhaps we create a separate issue for updating the existing approval rule syntax in v4. For example, instead of the following:

      approvers:
        - user1
        - user2
      approver_groups:
        - my-group
        - my-group1/subgroup
        - my-group2/subgroup/subsubgroup

This is probably more intuitive and again keeps the overall syntax consistent with other areas of gitlabform (i.e. branch protection, members, etc.)

      approvers:
        users:
          - user1
          - user2
        groups:
          - my-group
          - my-group1/subgroup
          - my-group2/subgroup/subsubgroup

If anyone wants to test what will most probably be in v3.4.0 final, please check out v3.4.0rc4 released a moment ago.

Please don’t hesitate with sharing any issues you may find - I will release a final release asap after the weekend if no one will report any issue.

Big thanks for the tests @Siythrun and @weakcamel!

I did also notice that transforming from the old syntax is not working like it should too - the groups seem to not be transformed - so I definitely have some fixes to do before this goes final.

When running if i have

projects_and_groups:
  "*":
    merge_requests_approval_rules:
      standard:
        approvals_required: 1
        name: "All eligible users"
      enforce: true 

  "config/kustomize/*":
    merge_requests_approval_rules:
      dev-uat:
        approvals_required: 1
        name: "Dev Code Review - UAT"
        groups:
          - developers
          - insights
        protected_branches:
          - uat
      devops-uat:
        approvals_required: 1
        name: "DevOps Code Review - UAT"
        groups:
          - devops
        protected_branches:
          - uat
      inf-pp:
        approvals_required: 1
        name: "Inf/DevOps Approval - PP"
        groups:
          - devops
          - infrastructure
        protected_branches:
          - pp
      inf-prod:
        approvals_required: 1
        name: "Inf/DevOps Approval - Prod"
        groups:
          - devops
          - infrastructure
        protected_branches:
          - prod
      enforce: true

The rules are created but non of the groups have been added

but removing the merge_requests_approval_rules from the top level into the sub-level it works fine

I would have to check what are the options to restore the default „any eligible dev” approval rule with the API and we should come up with a proposal for config syntax that would result in it. Any suggestions about the latter, @weakcamel?