Skip to content
Snippets Groups Projects

Prevent modification of MR-level approval rules after MR is merged

What does this MR do and why?

We need to prevent editing and deleting of merge request rules after a merge request is merged.

To Test:

  • Find a merged MR with approval rules (MergeRequest.merged.last.approval_rules)
  • Try to delete/edit/create a rule through the UI
  • Try to delete/edit/create a rule through the API
  • Try to delete/edit/create a rule through graphql ??
curl -X PUT -d 'approvals_required=1' -H "PRIVATE-TOKEN: TOKEN" "http://localhost:3000/api/v4/projects/PROJECT_ID/merge_requests/MR_IID/approval_rules/APPROVAL_RULE_ID"

curl -X DELETE -H "PRIVATE-TOKEN: TOKEN" "http://localhost:3000/api/v4/projects/PROJECT_ID/merge_requests/MR_IID/approval_rules/APPROVAL_RULE_ID"

curl -X POST -d 'name=a' -d 'approvals_required=1' -H "PRIVATE-TOKEN: TOKEN" "http://localhost:3000/api/v4/projects/PROJECT_ID/merge_requests/MR_IID/approval_rules"

Query Postgres.ai

https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/24524/commands/78234

Related to #432961 (closed)

Edited by Marc Shaw

Merge request reports

Merged results pipeline #1122136661 passed

Pipeline: GitLab

#1122138519

    Pipeline: TRIGGERED_EE_PIPELINE

    #1122138755

      Pipeline: E2E GDK

      #1122140393

        +1

        Merged results pipeline passed for 8426a139

        Test coverage 82.56% (9.06%) from 2 jobs
        Approval is optional
        Ready to merge by members who can write to the target branch.

        Merge details

        • 1 commit and 1 merge commit will be added to master (squashes 1 commit).
        • Source branch will be deleted.

        Activity

        Filter activity
        • Approvals
        • Assignees & reviewers
        • Comments (from bots)
        • Comments (from users)
        • Commits & branches
        • Edits
        • Labels
        • Lock status
        • Mentions
        • Merge request status
        • Tracking
      • 2 Messages
        :book: CHANGELOG missing:

        If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog.

        If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

        :book: This merge request adds or changes files that require a review from the Database team.

        This merge request requires a database review. To make sure these changes are reviewed, take the following steps:

        1. Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
        2. Prepare your MR for database review according to the docs.
        3. Assign and mention the database reviewer suggested by Reviewer Roulette.

        If you no longer require a database review, you can remove this suggestion by removing the database label and re-running the danger-review job.

        Reviewer roulette

        Changes that require review have been detected!

        Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

        Category Reviewer Maintainer
        analytics instrumentation No engineer is available for automated assignment, please reach out to the #g_analyze_analytics_instrumentation Slack channel or mention @gitlab-org/analytics-section/analytics-instrumentation/engineers for assistance. Maintainer review is optional for analytics instrumentation
        backend @akotte profile link current availability (UTC+5.5, 4.5 hours ahead of author) @jessieay profile link current availability (UTC+9, 8 hours ahead of author)

        Please check reviewer's status!

        • available Reviewer is available!
        • unavailable Reviewer is unavailable!

        Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

        To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

        Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

        If needed, you can retry the :repeat: danger-review job that generated this comment.

        Generated by :no_entry_sign: Danger

        Edited by Ghost User
        • Resolved by Rutger Wessels

          E2E Test Result Summary

          allure-report-publisher generated test report!

          e2e-test-on-gdk: :white_check_mark: test report for 991dbb1d

          expand test summary
          +------------------------------------------------------------------+
          |                          suites summary                          |
          +-------------+--------+--------+---------+-------+-------+--------+
          |             | passed | failed | skipped | flaky | total | result |
          +-------------+--------+--------+---------+-------+-------+--------+
          | Plan        | 55     | 0      | 0       | 0     | 55    | ✅     |
          | Verify      | 31     | 0      | 0       | 0     | 31    | ✅     |
          | Data Stores | 23     | 0      | 0       | 0     | 23    | ✅     |
          | Govern      | 72     | 0      | 0       | 0     | 72    | ✅     |
          | Release     | 5      | 0      | 0       | 0     | 5     | ✅     |
          | Create      | 54     | 0      | 7       | 0     | 61    | ✅     |
          | Manage      | 0      | 0      | 1       | 0     | 1     | ➖     |
          | Package     | 15     | 0      | 1       | 0     | 16    | ✅     |
          | Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
          | Monitor     | 7      | 0      | 0       | 0     | 7     | ✅     |
          +-------------+--------+--------+---------+-------+-------+--------+
          | Total       | 264    | 0      | 9       | 0     | 273   | ✅     |
          +-------------+--------+--------+---------+-------+-------+--------+

          e2e-package-and-test: :white_check_mark: test report for 991dbb1d

          expand test summary
          +------------------------------------------------------------------+
          |                          suites summary                          |
          +-------------+--------+--------+---------+-------+-------+--------+
          |             | passed | failed | skipped | flaky | total | result |
          +-------------+--------+--------+---------+-------+-------+--------+
          | Create      | 550    | 0      | 52      | 8     | 602   | ✅     |
          | Govern      | 6      | 0      | 0       | 0     | 6     | ✅     |
          | Data Stores | 4      | 0      | 0       | 0     | 4     | ✅     |
          | Plan        | 8      | 0      | 0       | 0     | 8     | ✅     |
          | Package     | 0      | 0      | 2       | 0     | 2     | ➖     |
          | Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
          +-------------+--------+--------+---------+-------+-------+--------+
          | Total       | 576    | 0      | 54      | 8     | 630   | ✅     |
          +-------------+--------+--------+---------+-------+-------+--------+
          Edited by Ghost User
      • Marc Shaw added 1 commit
      • Marc Shaw added 1 commit
      • Marc Shaw mentioned in merge request !133665 (merged)

        mentioned in merge request !133665 (merged)

      • mentioned in issue #432961 (closed)

      • Marc Shaw mentioned in merge request !138466 (merged)

        mentioned in merge request !138466 (merged)

      • Marc Shaw added 654 commits

        added 654 commits

        Compare with previous version

      • Marc Shaw added 1 commit

        added 1 commit

        • 1308151e - Prevent moditfication of MR approval rules after merge

        Compare with previous version

      • Marc Shaw
      • Marc Shaw
      • Marc Shaw added 1 commit

        added 1 commit

        • cc99095d - Prevent moditfication of MR approval rules after merge

        Compare with previous version

      • Marc Shaw changed the description

        changed the description

      • Marc Shaw marked this merge request as ready

        marked this merge request as ready

      • Loading
      • Loading
      • Loading
      • Loading
      • Loading
      • Loading
      • Loading
      • Loading
      • Loading
      • Loading
      • Please register or sign in to reply
        Loading