Allow self-managed to prevent auto disabling hooks
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.
- This change
- Related constants in
WebHook
!113479 (comment 1308304278). - 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.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #390157 (closed)
Merge request reports
Activity
changed milestone to %15.10
assigned to @.luke
- A deleted user
added feature flag label
- Resolved by Luke Duncalfe
2 Warnings This MR changes code in ee/
, but its Changelog commit is missing theEE: true
trailer. Consider adding it to your Changelog commits.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:
- The Handbook page on merge request types.
- The definition of done documentation.
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 (
@mfangitlab
) (UTC-7, 20 hours behind@.luke
)John Mason (
@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
danger-review
job that generated this comment.Generated by
Dangermentioned in commit gitlab-org-sandbox/gitlab-jh-validation@e2f9ea48
mentioned in issue #390157 (closed)
Allure report
allure-report-publisher
generated test report!e2e-package-and-test:
test report for 4a3ac051expand 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 | ❗ | +------------------+--------+--------+---------+-------+-------+--------+
added 270 commits
-
685006cb...54411bc4 - 269 commits from branch
master
- 15adf1a1 - Allow self-managed to prevent auto disabling hooks
-
685006cb...54411bc4 - 269 commits from branch
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@bdf7f644
added 1 commit
- 0871035f - Allow self-managed to prevent auto disabling hooks
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@f6a5d507
- Resolved by Luke Duncalfe
- Resolved by Douglas Barbosa Alexandre
added 929 commits
-
0871035f...baeb0ac0 - 927 commits from branch
master
- f227e26a - Allow self-managed to prevent auto disabling hooks
- 821ffd3c - Add feature docs
-
0871035f...baeb0ac0 - 927 commits from branch
- A deleted user
added documentation label
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@9b09c9f0
removed documentation label
added 1 commit
- cfdaaa40 - Allow self-managed to prevent auto disabling hooks
- A deleted user
added documentation label
removed documentation label
- Resolved by Douglas Barbosa Alexandre
- Resolved by Douglas Barbosa Alexandre
Hi @bmarjanovic! I was hoping you could take a look at this change. The description outlines the reason for the changing of the modules, and also my attempt to merge the changes in a few steps in order for us to have confidence the behaviours are in-tact. Let me know your thoughts! Thank you!
requested review from @bmarjanovic
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@87d06727
- Resolved by Ashraf Khamis
Hi @ashrafkhamis! Could you please give this merge request its technical writing review? Thank you!Edited by Luke Duncalfe
requested review from @ashrafkhamis
removed Deliverable backend-weight3 labels
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.removed review request for @ashrafkhamis
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@6a371d12
added Technical Writing label
- Resolved by Bojan Marjanovic
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@1d37de28
mentioned in issue #385902 (closed)
- Resolved by Douglas Barbosa Alexandre
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@458daf2a
requested review from @dbalexandre and removed review request for @bmarjanovic
@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:
added pipeline:mr-approved label
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@f523d1bb
enabled an automatic merge when the pipeline for a1aef503 succeeds
mentioned in commit gitlab-org-sandbox/gitlab-jh-validation@a1aef503
mentioned in commit 7c2c3fb3
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
mentioned in merge request !114508 (merged)
added self-managed workflowstaging labels and removed self-managed workflowcanary labels
added self-managed workflowproduction labels and removed self-managed workflowstaging labels
mentioned in epic &8083
mentioned in commit 3c5a0b3a
mentioned in merge request !114657 (merged)
mentioned in commit f0a79694
added releasedcandidate label
added releasedpublished label and removed releasedcandidate label
mentioned in merge request kubitus-project/kubitus-installer!1990 (merged)
added groupimport and integrate label and removed groupintegrations [DEPRECATED] label
mentioned in issue bmarjanovic/growth-and-development#1
mentioned in issue gitlab-org/quality/triage-reports#20593 (closed)
mentioned in issue gitlab-org/quality/triage-reports#20982 (closed)
mentioned in issue gitlab-org/quality/triage-reports#21523 (closed)
mentioned in issue #513253
mentioned in issue gitlab-org/quality/triage-reports#22009