Skip to content
Snippets Groups Projects

Allow self-managed to prevent auto disabling hooks

Merged Luke Duncalfe requested to merge 390157-auto-disabling-webhooks-ops-flag into master
All threads resolved!

What does this MR do and why?

This merge request adds an ops flag to make auto-disabling hooks an opt-in feature. It will allow self-managed to prevent the auto-disabling webhooks feature. By default the feature will be disabled for self-managed #390157 (closed).

Refactoring

The auto-disabling code has undergone a bit of refactoring in this change. The reason is that we previously had two modules:

  • WebHooks::Autodisabling
  • WebHooks::Unstoppable

A webhook model (one that inherits from WebHook) would mix in one of those modules to determine whether auto-disabling applied to the webhook or not.

We're now introducing the idea of a webhook that supports the auto-disabling feature, still not auto-disabling if the ops flag is disabled.

To avoid the codebase getting quite confusing, this change consolidates the two modules into one, with that one module deciding whether to apply the auto-disabling behaviour or not. For auto-disabling to be enabled, support for hook type needs to be defined in a const, and the flag must be toggled on.

It also moves some auto-disabling methods out of WebHook and into the one module, again, mostly to simplify the ability to rationalise the feature and discover the code.

Follow-up merge requests

Auto-disabling webhook behaviour is quite intricate and has been the source of a number of bugs in the past when the behaviour is not 100% correct.

I'm proposing that we merge the change in separate pieces.

  1. This change
  2. Related constants in WebHook !113479 (comment 1308304278).
  3. Related tests of methods being removed from WebHook !113479 (comment 1308319756).

The idea is to allow us at every step to have a good degree of confidence that the behaviours of the feature are unchanged.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #390157 (closed)

Edited by Luke Duncalfe

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Contributor
    2 Warnings
    :warning: This MR changes code in ee/, but its Changelog commit is missing the EE: true trailer. Consider adding it to your Changelog commits.
    :warning:

    featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.

    For more information, see:

    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
    backend Max Fan current availability (@mfangitlab) (UTC-7, 20 hours behind @.luke) John Mason current availability (@john-mason) (UTC-4, 17 hours behind @.luke)

    To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. 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

  • mentioned in issue #390157 (closed)

  • Contributor

    Allure report

    allure-report-publisher generated test report!

    e2e-package-and-test: :exclamation: test report for 4a3ac051

    expand test summary
    +-----------------------------------------------------------------------+
    |                            suites summary                             |
    +------------------+--------+--------+---------+-------+-------+--------+
    |                  | passed | failed | skipped | flaky | total | result |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Manage           | 132    | 0      | 6       | 56    | 138   | ❗     |
    | Create           | 322    | 0      | 14      | 110   | 336   | ❗     |
    | Fulfillment      | 4      | 0      | 44      | 0     | 48    | ✅     |
    | Package          | 0      | 0      | 6       | 0     | 6     | ➖     |
    | ModelOps         | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Verify           | 100    | 0      | 6       | 98    | 106   | ❗     |
    | Govern           | 88     | 0      | 2       | 88    | 90    | ❗     |
    | Configure        | 0      | 0      | 6       | 0     | 6     | ➖     |
    | Plan             | 120    | 0      | 0       | 46    | 120   | ❗     |
    | Secure           | 14     | 0      | 2       | 14    | 16    | ❗     |
    | Release          | 12     | 0      | 0       | 10    | 12    | ❗     |
    | Growth           | 0      | 0      | 4       | 0     | 4     | ➖     |
    | Monitor          | 16     | 0      | 0       | 16    | 16    | ❗     |
    | Framework sanity | 0      | 0      | 2       | 0     | 2     | ➖     |
    | Analytics        | 4      | 0      | 0       | 4     | 4     | ❗     |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Total            | 812    | 0      | 94      | 442   | 906   | ❗     |
    +------------------+--------+--------+---------+-------+-------+--------+
  • Luke Duncalfe added 270 commits

    added 270 commits

    Compare with previous version

  • Luke Duncalfe added 1 commit

    added 1 commit

    • 0871035f - Allow self-managed to prevent auto disabling hooks

    Compare with previous version

  • Luke Duncalfe
  • Luke Duncalfe
  • Luke Duncalfe marked the checklist item Documentation. as completed

    marked the checklist item Documentation. as completed

  • Luke Duncalfe added 929 commits

    added 929 commits

    Compare with previous version

  • A deleted user added documentation label

    added documentation label

  • Luke Duncalfe added 1 commit

    added 1 commit

    Compare with previous version

  • Luke Duncalfe added 1 commit

    added 1 commit

    • cfdaaa40 - Allow self-managed to prevent auto disabling hooks

    Compare with previous version

  • A deleted user added documentation label

    added documentation label

  • Luke Duncalfe changed the description

    changed the description

  • Luke Duncalfe
  • Luke Duncalfe requested review from @bmarjanovic

    requested review from @bmarjanovic

  • Luke Duncalfe requested review from @ashrafkhamis

    requested review from @ashrafkhamis

  • Luke Duncalfe changed the description

    changed the description

  • Author Maintainer

    I've added a Changelog entry, which might seem weird because it's a feature that already works both on GitLab.com and self-managed, but I figure we do want to generate a changelog for the featureaddition that self-managed now can prevent the behaviour if they wish to.

  • Luke Duncalfe added 1 commit

    added 1 commit

    Compare with previous version

  • Luke Duncalfe added 1 commit

    added 1 commit

    Compare with previous version

  • Luke Duncalfe removed review request for @ashrafkhamis

    removed review request for @ashrafkhamis

  • Luke Duncalfe added 1 commit

    added 1 commit

    Compare with previous version

  • mentioned in issue #385902 (closed)

  • Luke Duncalfe added 1 commit

    added 1 commit

    Compare with previous version

  • Luke Duncalfe
  • Bojan Marjanovic requested review from @dbalexandre and removed review request for @bmarjanovic

    requested review from @dbalexandre and removed review request for @bmarjanovic

  • Bojan Marjanovic approved this merge request

    approved this merge request

  • :wave: @bmarjanovic, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.

    For more info, please refer to the following links:

  • resolved all threads

  • Douglas Barbosa Alexandre approved this merge request

    approved this merge request

  • Douglas Barbosa Alexandre enabled an automatic merge when the pipeline for a1aef503 succeeds

    enabled an automatic merge when the pipeline for a1aef503 succeeds

  • mentioned in commit 7c2c3fb3

  • Luke Duncalfe mentioned in merge request !114508 (merged)

    mentioned in merge request !114508 (merged)

  • Keelan Lang mentioned in epic &8083

    mentioned in epic &8083

  • Luke Duncalfe mentioned in commit 3c5a0b3a

    mentioned in commit 3c5a0b3a

  • Luke Duncalfe mentioned in merge request !114657 (merged)

    mentioned in merge request !114657 (merged)

  • Luke Duncalfe mentioned in commit f0a79694

    mentioned in commit f0a79694

  • mentioned in issue #513253

  • Please register or sign in to reply
    Loading