#304 Add BaseEvent.is_allowed method to check if the event passes configured whitelist/blacklist.
Merged a year ago by jkaluza. Opened a year ago by jkaluza.
jkaluza/freshmaker base-event-is-allowed  into  master

file modified
+27

@@ -24,6 +24,7 @@ 

  import itertools

  

  from freshmaker import conf

+ from freshmaker.types import ArtifactType

  

  try:

      from inspect import signature

@@ -134,6 +135,21 @@ 

          """

          return self.msg_id

  

+     def is_allowed(self, handler, artifact_type, **kwargs):

+         """

+         Returns True if whitelist/blacklist allows handling this event.

+         Calls `handler.allow_build()` to find the answer.

+ 

+         :param BaseHandler handler: Handler currently handling the event.

+         :param ArtifactType artifact_type: Type of artifact to build as part

+             of event.

+         :param args: Extra args to be passed to `handler.allow_build()`.

+         :param kwargs: Extra kwargs to be passed to `handler.allow_build()`.

+         """

+         return handler.allow_build(

+             artifact_type, dry_run=self.dry_run,

+             manual=self.manual, **kwargs)

+ 

  

  class MBSModuleStateChangeEvent(BaseEvent):

      """ A class that inherits from BaseEvent to provide an event

@@ -269,6 +285,17 @@ 

      def search_key(self):

          return str(self.advisory.errata_id)

  

+     def is_allowed(self, handler, **kwargs):

+         return super(ErrataBaseEvent, self).is_allowed(

+             handler, ArtifactType.IMAGE,

+             advisory_state=self.advisory.state,

+             advisory_name=self.advisory.name,

+             advisory_security_impact=self.advisory.security_impact,

+             advisory_highest_cve_severity=self.advisory.highest_cve_severity,

+             advisory_product_short_name=self.advisory.product_short_name,

+             advisory_has_hightouch_bug=self.advisory.has_hightouch_bug,

+             **kwargs)

+ 

  

  class ErrataAdvisoryStateChangedEvent(ErrataBaseEvent):

      """

@@ -92,14 +92,7 @@ 

          self.set_context(db_event)

  

          # Check if we are allowed to build this advisory.

-         if not self.allow_build(

-                 ArtifactType.IMAGE,

-                 advisory_name=event.advisory.name,

-                 advisory_security_impact=event.advisory.security_impact,

-                 advisory_highest_cve_severity=event.advisory.highest_cve_severity,

-                 advisory_product_short_name=event.advisory.product_short_name,

-                 advisory_has_hightouch_bug=event.advisory.has_hightouch_bug,

-                 dry_run=self.dry_run):

+         if not self.event.is_allowed(self):

              msg = ("Errata advisory {0} is not allowed by internal policy "

                     "to trigger rebuilds.".format(event.advisory.errata_id))

              db_event.transition(EventState.SKIPPED, msg)

@@ -618,8 +611,7 @@ 

  

          image_name = koji.parse_NVR(image["brew"]["build"])['name']

  

-         if not self.allow_build(

-                 ArtifactType.IMAGE, image_name=image_name):

+         if not self.event.is_allowed(self, image_name=image_name):

              self.log_info("Skipping rebuild of image %s, not allowed by "

                            "configuration", image_name)

              return True

@@ -659,13 +651,7 @@ 

  

          # Check if we are allowed to rebuild unpublished images and clear

          # published and release_category if so.

-         if self.allow_build(

-                 ArtifactType.IMAGE, advisory_name=self.event.advisory.name,

-                 advisory_security_impact=self.event.advisory.security_impact,

-                 advisory_highest_cve_severity=self.event.advisory.highest_cve_severity,

-                 advisory_product_short_name=self.event.advisory.product_short_name,

-                 advisory_has_hightouch_bug=self.event.advisory.has_hightouch_bug,

-                 published=True, dry_run=self.dry_run):

+         if self.event.is_allowed(self, published=True):

              published = True

              release_category = "Generally Available"

          else:

@@ -25,7 +25,7 @@ 

  from freshmaker.models import Event, EVENT_TYPES

  from freshmaker.handlers import BaseHandler, fail_event_on_handler_exception

  from freshmaker.errata import Errata

- from freshmaker.types import EventState, ArtifactType

+ from freshmaker.types import EventState

  

  

  class ErrataAdvisoryStateChangedHandler(BaseHandler):

@@ -113,8 +113,7 @@ 

  

          extra_events = []

  

-         if (event.manual or

-                 self.allow_build(ArtifactType.IMAGE, advisory_state=event.advisory.state)):

+         if event.is_allowed(self):

              extra_events += self.rebuild_if_not_exists(event, errata_id)

  

          if state == "SHIPPED_LIVE":

@@ -640,6 +640,13 @@ 

              self.assertEqual(len(ret), 0)

  

      @patch('freshmaker.errata.Errata.advisories_from_event')

+     @patch.object(conf, 'handler_build_whitelist', new={

+         'ErrataAdvisoryStateChangedHandler': {

+             'image': {

+                 'advisory_state': '.*',

+             }

+         }

+     })

      def test_rebuild_if_not_exists_already_exists(

              self, advisories_from_event):

          handler = ErrataAdvisoryStateChangedHandler()

@@ -767,7 +774,7 @@ 

      @patch.object(conf, 'handler_build_whitelist', new={

          'ErrataAdvisoryStateChangedHandler': {

              'image': {

-                 'advisory_state': r'REL_PREP',

+                 'advisory_state': r'SHIPPED_LIVE',

              }

          }

      })

We use multiple BaseHandler.allow_build(...) calls to check whether the advisory/image is allowed by whitelist/blacklist. This is necessary, because at first we only check whether the advisory is allowed to trigger the rebuild - this is the first call. Later we call BaseHandler.allow_build(...) again but also with particular image name to find out whether particular image can be rebuilt - this is second call. There are also other cases similar to this one.

The issue is that every call of allow_build(...) must contain all of the Event's metadata and since we always constructed allow_build arguments from scratch for every call on various places, it was hard to maintain it on multiple places. We for example added advisory_has_hightouch_bug key, but filled it only on single place where allow_build is called for Errata advisory.

This PR fixes that by introducing BaseEvent.is_allowed(...) method which calls BaseHandler.allow_build() with all the attributes particular Event provides. Therefore the list of attributes are in single file in the particular Event's class.

rebased onto df8aeaa43e55454eca56a2be1fae67321cd95a08

a year ago

rebased onto 423d6c4f46699b16a514c5675af5b066b6aeb024

a year ago

Do you really need *args here? **kwargs, yes... but it looks like *args may always be empty. Can it be dropped for clarity?

:+1: in general. I left one comment - take it or leave it. :)

rebased onto 8743b70

a year ago

Commit 8369cf3 fixes this pull-request

Pull-Request has been merged by jkaluza

a year ago

Pull-Request has been merged by jkaluza

a year ago