#304 Add BaseEvent.is_allowed method to check if the event passes configured whitelist/blacklist.
Merged 10 months ago by jkaluza. Opened 10 months 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

10 months ago

rebased onto 423d6c4f46699b16a514c5675af5b066b6aeb024

10 months 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

10 months ago

Commit 8369cf3 fixes this pull-request

Pull-Request has been merged by jkaluza

10 months ago

Pull-Request has been merged by jkaluza

10 months ago