launchpad-reviewers team mailing list archive

[Colin Watson <mp+428785@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:remove-code-webhook-feature-rules into launchpad:master.

Commit message:
Remove code.*.webhooks.enabled feature rules

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/428785

They've all been enabled everywhere for some time.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:remove-code-webhook-feature-rules into launchpad:master.

diff --git a/lib/lp/code/interfaces/branchmergeproposal.py b/lib/lp/code/interfaces/branchmergeproposal.py
index a49bff1..1da9194 100644
--- a/lib/lp/code/interfaces/branchmergeproposal.py
+++ b/lib/lp/code/interfaces/branchmergeproposal.py
@@ -6,7 +6,6 @@
 __all__ = [
     "BRANCH_MERGE_PROPOSAL_FINAL_STATES",
     "BRANCH_MERGE_PROPOSAL_OBSOLETE_STATES",
-    "BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG",
     "IBranchMergeProposal",
     "IBranchMergeProposalGetter",
     "IBranchMergeProposalJob",
@@ -87,11 +86,6 @@ BRANCH_MERGE_PROPOSAL_OBSOLETE_STATES = (
 )
 
 
-BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG = (
-    "code.merge_proposals.webhooks.enabled"
-)
-
-
 class IBranchMergeProposalPublic(IPrivacy):
 
     id = Int(
diff --git a/lib/lp/code/model/gitjob.py b/lib/lp/code/model/gitjob.py
index 4fecf0e..f37b9d5 100644
--- a/lib/lp/code/model/gitjob.py
+++ b/lib/lp/code/model/gitjob.py
@@ -42,7 +42,6 @@ from lp.services.database.locking import (
     try_advisory_lock,
 )
 from lp.services.database.stormbase import StormBase
-from lp.services.features import getFeatureFlag
 from lp.services.helpers import english_list
 from lp.services.job.model.job import EnumeratedSubclass, Job
 from lp.services.job.runner import BaseRunnableJob
@@ -248,16 +247,15 @@ class GitRefScanJob(GitJobDerived):
                     ref.path: ref.commit_sha1 for ref in self.repository.refs
                 }
                 upserted_refs, removed_refs = self.repository.scan(log=log)
-                if getFeatureFlag("code.git.webhooks.enabled"):
-                    payload = self.composeWebhookPayload(
-                        self.repository,
-                        old_refs_commits,
-                        upserted_refs,
-                        removed_refs,
-                    )
-                    getUtility(IWebhookSet).trigger(
-                        self.repository, "git:push:0.1", payload
-                    )
+                payload = self.composeWebhookPayload(
+                    self.repository,
+                    old_refs_commits,
+                    upserted_refs,
+                    removed_refs,
+                )
+                getUtility(IWebhookSet).trigger(
+                    self.repository, "git:push:0.1", payload
+                )
         except LostObjectError:
             log.info(
                 "Skipping repository %s because it has been deleted."
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index cf5cb2b..f71aabe 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -54,7 +54,6 @@ from lp.code.interfaces.branchmergeproposal import (
     BRANCH_MERGE_PROPOSAL_OBSOLETE_STATES as OBSOLETE_STATES,
 )
 from lp.code.interfaces.branchmergeproposal import (
-    BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG,
     IBranchMergeProposal,
     IBranchMergeProposalGetter,
     IBranchMergeProposalJobSource,
@@ -80,7 +79,6 @@ from lp.registry.interfaces.product import IProductSet
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.sqlobject import SQLObjectNotFound
-from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.webapp import canonical_url
 from lp.services.webhooks.testing import LogsScheduledWebhooks
@@ -1196,12 +1194,6 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
-    def setUp(self):
-        super().setUp()
-        self.useFixture(
-            FeatureFixture({BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG: "on"})
-        )
-
     def getWebhookTarget(self, branch):
         if self.git:
             return branch.repository
diff --git a/lib/lp/code/model/tests/test_branchmergeproposaljobs.py b/lib/lp/code/model/tests/test_branchmergeproposaljobs.py
index 807fc0a..872eb9f 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposaljobs.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposaljobs.py
@@ -26,7 +26,6 @@ from zope.security.proxy import removeSecurityProxy
 from lp.code.adapters.branch import BranchMergeProposalNoPreviewDiffDelta
 from lp.code.enums import BranchMergeProposalStatus
 from lp.code.interfaces.branchmergeproposal import (
-    BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG,
     IBranchMergeProposalJob,
     IBranchMergeProposalJobSource,
     ICodeReviewCommentEmailJob,
@@ -404,9 +403,6 @@ class TestUpdatePreviewDiffJob(DiffTestCase):
         )
 
     def test_triggers_webhooks_bzr(self):
-        self.useFixture(
-            FeatureFixture({BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG: "on"})
-        )
         logger = self.useFixture(FakeLogger())
         bmp = self.createExampleBzrMerge()[0]
         hook = self.factory.makeWebhook(
@@ -422,9 +418,6 @@ class TestUpdatePreviewDiffJob(DiffTestCase):
         )
 
     def test_triggers_webhooks_git(self):
-        self.useFixture(
-            FeatureFixture({BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG: "on"})
-        )
         logger = self.useFixture(FakeLogger())
         bmp = self.createExampleGitMerge()[0]
         hook = self.factory.makeWebhook(
diff --git a/lib/lp/code/model/tests/test_gitjob.py b/lib/lp/code/model/tests/test_gitjob.py
index 2b4b333..81ab40f 100644
--- a/lib/lp/code/model/tests/test_gitjob.py
+++ b/lib/lp/code/model/tests/test_gitjob.py
@@ -23,9 +23,6 @@ from zope.security.proxy import removeSecurityProxy
 
 from lp.code.adapters.gitrepository import GitRepositoryDelta
 from lp.code.enums import GitGranteeType, GitObjectType
-from lp.code.interfaces.branchmergeproposal import (
-    BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG,
-)
 from lp.code.interfaces.gitjob import (
     IGitJob,
     IGitRefScanJob,
@@ -42,7 +39,6 @@ from lp.code.model.gitjob import (
 from lp.code.tests.helpers import GitHostingFixture
 from lp.services.config import config
 from lp.services.database.constants import UTC_NOW
-from lp.services.features.testing import FeatureFixture
 from lp.services.job.runner import JobRunner
 from lp.services.utils import seconds_since_epoch
 from lp.services.webapp import canonical_url
@@ -183,7 +179,6 @@ class TestGitRefScanJob(TestCaseWithFactory):
 
     def test_triggers_webhooks(self):
         # Jobs trigger any relevant webhooks when they're enabled.
-        self.useFixture(FeatureFixture({"code.git.webhooks.enabled": "on"}))
         logger = self.useFixture(FakeLogger())
         repository = self.factory.makeGitRepository()
         self.factory.makeGitRefs(
@@ -238,7 +233,6 @@ class TestGitRefScanJob(TestCaseWithFactory):
 
     def test_triggers_webhooks_with_oci_project_as_repository_target(self):
         # Jobs trigger any relevant webhooks when they're enabled.
-        self.useFixture(FeatureFixture({"code.git.webhooks.enabled": "on"}))
         logger = self.useFixture(FakeLogger())
         oci_project = self.factory.makeOCIProject()
         repository = self.factory.makeGitRepository(target=oci_project)
@@ -293,9 +287,6 @@ class TestGitRefScanJob(TestCaseWithFactory):
             )
 
     def test_merge_detection_triggers_webhooks(self):
-        self.useFixture(
-            FeatureFixture({BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG: "on"})
-        )
         logger = self.useFixture(FakeLogger())
         repository = self.factory.makeGitRepository()
         target, source = self.factory.makeGitRefs(
diff --git a/lib/lp/code/subscribers/branchmergeproposal.py b/lib/lp/code/subscribers/branchmergeproposal.py
index 40e67cc..4eb94dc 100644
--- a/lib/lp/code/subscribers/branchmergeproposal.py
+++ b/lib/lp/code/subscribers/branchmergeproposal.py
@@ -9,7 +9,6 @@ from zope.principalregistry.principalregistry import UnauthenticatedPrincipal
 from lp.code.adapters.branch import BranchMergeProposalNoPreviewDiffDelta
 from lp.code.enums import BranchMergeProposalStatus
 from lp.code.interfaces.branchmergeproposal import (
-    BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG,
     IBranchMergeProposal,
     IMergeProposalNeedsReviewEmailJobSource,
     IMergeProposalUpdatedEmailJobSource,
@@ -17,7 +16,6 @@ from lp.code.interfaces.branchmergeproposal import (
     IUpdatePreviewDiffJobSource,
 )
 from lp.registry.interfaces.person import IPerson
-from lp.services.features import getFeatureFlag
 from lp.services.utils import text_delta
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webhooks.interfaces import IWebhookSet
@@ -67,12 +65,11 @@ def merge_proposal_created(merge_proposal, event):
     Create a job to update the diff for the merge proposal; trigger webhooks.
     """
     getUtility(IUpdatePreviewDiffJobSource).create(merge_proposal)
-    if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
-        payload = {
-            "action": "created",
-            "new": _compose_merge_proposal_webhook_payload(merge_proposal),
-        }
-        _trigger_webhook(merge_proposal, payload)
+    payload = {
+        "action": "created",
+        "new": _compose_merge_proposal_webhook_payload(merge_proposal),
+    }
+    _trigger_webhook(merge_proposal, payload)
 
 
 def merge_proposal_needs_review(merge_proposal, event):
@@ -121,20 +118,19 @@ def merge_proposal_modified(merge_proposal, event):
             getUtility(IMergeProposalUpdatedEmailJobSource).create(
                 merge_proposal, changes, from_person
             )
-    if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
-        payload = {
-            "action": "modified",
-            "old": _compose_merge_proposal_webhook_payload(
-                event.object_before_modification
-            ),
-            "new": _compose_merge_proposal_webhook_payload(merge_proposal),
-        }
-        # Some fields may not be in the before-modification snapshot; take
-        # values for these from the new object instead.
-        for field in payload["old"]:
-            if not hasattr(event.object_before_modification, field):
-                payload["old"][field] = payload["new"][field]
-        _trigger_webhook(merge_proposal, payload)
+    payload = {
+        "action": "modified",
+        "old": _compose_merge_proposal_webhook_payload(
+            event.object_before_modification
+        ),
+        "new": _compose_merge_proposal_webhook_payload(merge_proposal),
+    }
+    # Some fields may not be in the before-modification snapshot; take
+    # values for these from the new object instead.
+    for field in payload["old"]:
+        if not hasattr(event.object_before_modification, field):
+            payload["old"][field] = payload["new"][field]
+    _trigger_webhook(merge_proposal, payload)
 
 
 def review_requested(vote_reference, event):
@@ -147,12 +143,11 @@ def review_requested(vote_reference, event):
 
 def merge_proposal_deleted(merge_proposal, event):
     """A merge proposal has been deleted."""
-    if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
-        # The merge proposal link will be invalid by the time the webhook is
-        # delivered, but this may still be useful for endpoints that might
-        # e.g. want to cancel CI jobs in flight.
-        payload = {
-            "action": "deleted",
-            "old": _compose_merge_proposal_webhook_payload(merge_proposal),
-        }
-        _trigger_webhook(merge_proposal, payload)
+    # The merge proposal link will be invalid by the time the webhook is
+    # delivered, but this may still be useful for endpoints that might e.g.
+    # want to cancel CI jobs in flight.
+    payload = {
+        "action": "deleted",
+        "old": _compose_merge_proposal_webhook_payload(merge_proposal),
+    }
+    _trigger_webhook(merge_proposal, payload)
diff --git a/lib/lp/codehosting/scanner/bzrsync.py b/lib/lp/codehosting/scanner/bzrsync.py
index 637b007..08243fc 100755
--- a/lib/lp/codehosting/scanner/bzrsync.py
+++ b/lib/lp/codehosting/scanner/bzrsync.py
@@ -29,7 +29,6 @@ from lp.code.model.branchrevision import BranchRevision
 from lp.code.model.revision import Revision
 from lp.codehosting.scanner import events
 from lp.services.config import config
-from lp.services.features import getFeatureFlag
 from lp.services.utils import iter_chunks
 from lp.services.webhooks.interfaces import IWebhookSet
 from lp.translations.interfaces.translationtemplatesbuild import (
@@ -368,7 +367,7 @@ def update_snaps(tip_changed):
 def trigger_webhooks(tip_changed):
     old_revid = tip_changed.old_tip_revision_id
     new_revid = tip_changed.new_tip_revision_id
-    if getFeatureFlag("code.bzr.webhooks.enabled") and old_revid != new_revid:
+    if old_revid != new_revid:
         payload = tip_changed.composeWebhookPayload(
             tip_changed.db_branch, old_revid, new_revid
         )
diff --git a/lib/lp/codehosting/scanner/tests/test_bzrsync.py b/lib/lp/codehosting/scanner/tests/test_bzrsync.py
index 7616e38..290a304 100644
--- a/lib/lp/codehosting/scanner/tests/test_bzrsync.py
+++ b/lib/lp/codehosting/scanner/tests/test_bzrsync.py
@@ -869,7 +869,6 @@ class TestTriggerWebhooks(BzrSyncTestCase):
 
     def test_triggers_webhooks(self):
         # On tip change, any relevant webhooks are triggered.
-        self.useFixture(FeatureFixture({"code.bzr.webhooks.enabled": "on"}))
         logger = self.useFixture(FakeLogger())
         self.syncAndCount()
         old_revid = self.db_branch.last_scanned_id