#489 Refactor subject type and item usage
Closed 4 years ago by lholecek. Opened 4 years ago by lholecek.
lholecek/greenwave refactor-subjects  into  master

Refactor subject type and item usage
Lukas Holecek • 4 years ago  
file modified
+21 -41
@@ -15,6 +15,7 @@ 

  from greenwave.safe_yaml import SafeYAMLError

  from greenwave.utils import insert_headers, jsonp

  from greenwave.waivers import waive_answers

+ from greenwave.subjects.factory import create_subject, subject_type_and_id_from_dict

  from greenwave.monitor import (

      registry,

      decision_exception_counter,
@@ -27,22 +28,13 @@ 

  

  

  def _decision_subject(subject):

-     subject_type = subject.get('type')

-     subject_identifier = subject.get('item')

+     subject_type, subject_identifier = subject_type_and_id_from_dict(subject)

  

-     if 'productmd.compose.id' in subject:

-         return ('compose', subject['productmd.compose.id'])

+     if subject_type is None or subject_identifier is None:

+         log.info('Couldn\'t detect subject_identifier.')

Use doublequotes to avoid escapes

Use doublequotes to avoid escapes

This is just a moved code, but I'll change it as suggested.

Use doublequotes to avoid escapes

This is just a moved code, but I'll change it as suggested.

Actually, let's use "Could not" instead.

+         raise BadRequest('Couldn\'t detect subject_identifier.')

  

-     if 'original_spec_nvr' in subject:

-         return ('koji_build', subject['original_spec_nvr'])

- 

-     if subject_identifier:

-         if subject_type == 'brew-build':

-             return ('koji_build', subject_identifier)

-         return (subject_type, subject_identifier)

- 

-     log.info('Couldn\'t detect subject_identifier.')

-     raise BadRequest('Couldn\'t detect subject_identifier.')

+     return create_subject(subject_type, subject_identifier)

  

  

  def _decision_subjects_for_request(data):
@@ -72,18 +64,7 @@ 

              log.error('Missing required "subject_identifier" parameter')

              raise BadRequest('Missing required "subject_identifier" parameter')

  

-         yield data['subject_type'], data['subject_identifier']

- 

- 

- def subject_type_identifier_to_list(subject_type, subject_identifier):

-     """

-     Inverse of the above function.

-     This is for backwards compatibility in emitted messages.

-     """

-     if subject_type == 'compose':

-         return [{'productmd.compose.id': subject_identifier}]

-     else:

-         return [{'type': subject_type, 'item': subject_identifier}]

+         yield create_subject(data['subject_type'], data['subject_identifier'])

  

  

  @api.route('/version', methods=['GET'])
@@ -413,9 +394,9 @@ 

      on_demand_policies = []

      if rules:

          request_data = {key: data[key] for key in data if key not in ('subject', 'subject_type')}

-         for subject_type, subject_identifier in _decision_subjects_for_request(data):

-             request_data['subject_type'] = subject_type

-             request_data['subject_identifier'] = subject_identifier

+         for subject in _decision_subjects_for_request(data):

+             request_data['subject_type'] = subject.type

+             request_data['subject_identifier'] = subject.identifier

              on_demand_policy = OnDemandPolicy.create_from_json(request_data)

              on_demand_policies.append(on_demand_policy)

  
@@ -451,34 +432,33 @@ 

      waiver_filters = []

  

      policies = on_demand_policies or current_app.config['policies']

-     for subject_type, subject_identifier in _decision_subjects_for_request(data):

+     for subject in _decision_subjects_for_request(data):

          subject_policies = [

              policy for policy in policies

              if policy.matches(

                  decision_context=decision_context,

                  product_version=product_version,

-                 subject_type=subject_type)

+                 subject=subject)

          ]

  

          if not subject_policies:

              # Ignore non-existent policy for Bodhi updates.

-             if subject_type == 'bodhi_update':

+             if subject.type == 'bodhi_update':

                  continue

  

              log.error(

                  'Cannot find any applicable policies for %s subjects at gating point %s in %s',

-                 subject_type, decision_context, product_version)

+                 subject.type, decision_context, product_version)

              raise NotFound(

                  'Cannot find any applicable policies for %s subjects at gating point %s in %s' % (

-                     subject_type, decision_context, product_version))

+                     subject.type, decision_context, product_version))

  

          if verbose:

              # Retrieve test results and waivers for all items when verbose output is requested.

-             verbose_results.extend(

-                 results_retriever.retrieve(subject_type, subject_identifier))

+             verbose_results.extend(results_retriever.retrieve(subject))

              waiver_filters.append(dict(

-                 subject_type=subject_type,

-                 subject_identifier=subject_identifier,

+                 subject_type=subject.type,

+                 subject_identifier=subject.identifier,

                  product_version=product_version,

              ))

  
@@ -486,7 +466,7 @@ 

              answers.extend(

                  policy.check(

                      product_version,

-                     subject_identifier,

+                     subject,

                      results_retriever))

  

          applicable_policies.extend(subject_policies)
@@ -495,8 +475,8 @@ 

          for answer in answers:

              if not answer.is_satisfied:

                  waiver_filters.append(dict(

-                     subject_type=answer.subject_type,

-                     subject_identifier=answer.subject_identifier,

+                     subject_type=answer.subject.type,

+                     subject_identifier=answer.subject.identifier,

                      product_version=product_version,

                      testcase=answer.test_case_name,

                  ))

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

  import fedmsg.consumers

  

  import greenwave.app_factory

- from greenwave.api_v1 import subject_type_identifier_to_list

  from greenwave.monitor import (

      publish_decision_exceptions_result_counter,

      messaging_tx_to_send_counter, messaging_tx_stopped_counter,
@@ -157,15 +156,13 @@ 

      def _publish_decision_change(

              self,

              submit_time,

-             subject_type,

-             subject_identifier,

+             subject,

              testcase,

              product_version,

              publish_testcase):

  

          policy_attributes = dict(

-             subject_type=subject_type,

-             subject_identifier=subject_identifier,

+             subject=subject,

              testcase=testcase,

          )

  
@@ -185,8 +182,8 @@ 

                  submit_time,

                  decision_context=decision_context,

                  product_version=product_version,

-                 subject_type=subject_type,

-                 subject_identifier=subject_identifier,

+                 subject_type=subject.type,

+                 subject_identifier=subject.identifier,

              )

              if decision is None:

                  continue
@@ -197,11 +194,10 @@ 

                  continue

  

              decision.update({

-                 'subject_type': subject_type,

-                 'subject_identifier': subject_identifier,

+                 'subject_type': subject.type,

+                 'subject_identifier': subject.identifier,

                  # subject is for backwards compatibility only:

-                 'subject': subject_type_identifier_to_list(subject_type,

-                                                            subject_identifier),

+                 'subject': [subject.to_item_dict()],

                  'decision_context': decision_context,

                  'product_version': product_version,

                  'previous': old_decision,

@@ -13,6 +13,7 @@ 

  

  from greenwave.consumers.consumer import Consumer

  from greenwave.product_versions import subject_product_version

+ from greenwave.subjects.factory import create_subject, subject_type_and_id_from_dict

  

  import xmlrpc.client

  
@@ -64,9 +65,9 @@ 

              self.koji_proxy = None

  

      @staticmethod

-     def announcement_subjects(message):

+     def announcement_subject(message):

          """

-         Yields pairs of (subject type, subject identifier) for announcement

+         Returns pairs of (subject type, subject identifier) for announcement

          consideration from the message.

  

          Args:
@@ -78,7 +79,12 @@ 

          except KeyError:

              data = message['msg']['task']  # Old format

  

-         _type = _unpack_value(data.get('type'))

+         unpacked = {}

+         for k, v in data.items():

I feel like this could be carelessly used. Can we just look for the keys we care about?

for k in ('type', 'identifier'):
  v = data.get(k)
  if v:
    unpacked[k] = _unpack_value(v)

UPDATE: Disregard this comment. I see now that the whole point of this refactor is to isolate which keys/attributes we care about.

+             unpacked[k] = _unpack_value(v)

Would rather use comprehension here:

unpacked = {k: _unpack_value(v) for k, v in data.items()}
~

Would rather use comprehension here

Good idea. I'll fix it in the #493 and close this PR.

+ 

+         subject_type, subject_identifier = subject_type_and_id_from_dict(unpacked)

+ 

          # note: it is *intentional* that we do not handle old format

          # compose-type messages, because it is impossible to reliably

          # produce a decision from these. compose decisions can only be
@@ -88,21 +94,10 @@ 

          # https://pagure.io/taskotron/resultsdb/issue/92

          # https://pagure.io/taskotron/resultsdb/pull-request/101

          # https://pagure.io/greenwave/pull-request/262#comment-70350

-         if 'productmd.compose.id' in data:

-             yield ('compose', _unpack_value(data['productmd.compose.id']))

-         elif _type == 'compose':

-             pass

-         elif 'original_spec_nvr' in data:

-             nvr = _unpack_value(data['original_spec_nvr'])

-             # when the pipeline ignores a package, which happens

-             # *a lot*, we get a message with an 'original_spec_nvr'

-             # key with an empty value; let's not try and handle this

-             if nvr:

-                 yield ('koji_build', nvr)

-         elif _type == 'brew-build':

-             yield ('koji_build', _unpack_value(data['item']))

-         elif 'item' in data and _type:

-             yield (_type, _unpack_value(data['item']))

+         if subject_type == 'compose' and 'productmd.compose.id' not in data:

+             return None, None

+ 

+         return subject_type, subject_identifier

  

      def _consume_message(self, message):

          msg = message['msg']
@@ -119,21 +114,24 @@ 

  

          brew_task_id = _get_brew_task_id(msg)

  

-         for subject_type, subject_identifier in self.announcement_subjects(message):

-             log.debug('Considering subject %s: %r', subject_type, subject_identifier)

- 

-             product_version = subject_product_version(

-                 subject_identifier,

-                 subject_type,

-                 self.koji_proxy,

-                 brew_task_id,

-             )

- 

-             self._publish_decision_change(

-                 submit_time=submit_time,

-                 subject_type=subject_type,

-                 subject_identifier=subject_identifier,

-                 testcase=testcase,

-                 product_version=product_version,

-                 publish_testcase=False,

-             )

+         subject_type, subject_identifier = self.announcement_subject(message)

+         if subject_type is None:

+             return

+ 

+         log.debug('Considering subject %s: %r', subject_type, subject_identifier)

+ 

+         subject = create_subject(subject_type, subject_identifier)

+ 

+         product_version = subject_product_version(

+             subject,

+             self.koji_proxy,

+             brew_task_id,

+         )

+ 

+         self._publish_decision_change(

+             submit_time=submit_time,

+             subject=subject,

+             testcase=testcase,

+             product_version=product_version,

+             publish_testcase=False,

+         )

@@ -10,6 +10,7 @@ 

  """

  

  from greenwave.consumers.consumer import Consumer

+ from greenwave.subjects.factory import create_subject

  

  

  class WaiverDBHandler(Consumer):
@@ -30,14 +31,12 @@ 

  

          product_version = msg['product_version']

          testcase = msg['testcase']

-         subject_type = msg['subject_type']

-         subject_identifier = msg['subject_identifier']

+         subject = create_subject(msg['subject_type'], msg['subject_identifier'])

          submit_time = msg['timestamp']

  

          self._publish_decision_change(

              submit_time=submit_time,

-             subject_type=subject_type,

-             subject_identifier=subject_identifier,

+             subject=subject,

              testcase=testcase,

              product_version=product_version,

              publish_testcase=True,

file modified
+64 -115
@@ -9,6 +9,8 @@ 

  from werkzeug.exceptions import BadRequest

  from flask import current_app

  

+ from greenwave.subjects.factory import remote_rule_subject_types

+ 

  from greenwave.safe_yaml import (

      SafeYAMLBool,

      SafeYAMLChoice,
@@ -42,17 +44,6 @@ 

      pass

  

  

- def subject_type_identifier_to_item(subject_type, subject_identifier):

-     """

-     Greenwave < 0.8 included an "item" key in the "unsatisfied_requirements".

-     This returns a suitable value for that key, for backwards compatibility.

-     """

-     if subject_type == 'compose':

-         return {'productmd.compose.id': subject_identifier}

-     else:

-         return {'type': subject_type, 'item': subject_identifier}

- 

- 

  class Answer(object):

      """

      Represents the result of evaluating a policy rule against a particular
@@ -107,9 +98,8 @@ 

      ResultsDB with a matching item and test case name).

      """

  

-     def __init__(self, subject_type, subject_identifier, test_case_name, scenario):

-         self.subject_type = subject_type

-         self.subject_identifier = subject_identifier

+     def __init__(self, subject, test_case_name, scenario):

+         self.subject = subject

          self.test_case_name = test_case_name

          self.scenario = scenario

  
@@ -117,17 +107,16 @@ 

          return {

              'type': 'test-result-missing',

              'testcase': self.test_case_name,

-             'subject_type': self.subject_type,

-             'subject_identifier': self.subject_identifier,

+             'subject_type': self.subject.type,

+             'subject_identifier': self.subject.identifier,

              'scenario': self.scenario,

              # For backwards compatibility only:

-             'item': subject_type_identifier_to_item(self.subject_type, self.subject_identifier),

+             'item': self.subject.to_item_dict()

          }

  

      def to_waived(self):

          return TestResultMissingWaived(

-             self.subject_type,

-             self.subject_identifier,

+             self.subject,

              self.test_case_name,

              self.scenario)

  
@@ -136,9 +125,8 @@ 

      """

      Same as TestResultMissing but the result was waived.

      """

-     def __init__(self, subject_type, subject_identifier, test_case_name, scenario):

-         self.subject_type = subject_type

-         self.subject_identifier = subject_identifier

+     def __init__(self, subject, test_case_name, scenario):

+         self.subject = subject

          self.test_case_name = test_case_name

          self.scenario = scenario

  
@@ -146,8 +134,8 @@ 

          return {

              'type': 'test-result-missing-waived',

              'testcase': self.test_case_name,

-             'subject_type': self.subject_type,

-             'subject_identifier': self.subject_identifier,

+             'subject_type': self.subject.type,

+             'subject_identifier': self.subject.identifier,

              'scenario': self.scenario,

          }

  
@@ -158,9 +146,8 @@ 

      not ``PASSED`` or ``INFO``) and no corresponding waiver was found.

      """

  

-     def __init__(self, subject_type, subject_identifier, test_case_name, scenario, result_id):

-         self.subject_type = subject_type

-         self.subject_identifier = subject_identifier

+     def __init__(self, subject, test_case_name, scenario, result_id):

+         self.subject = subject

          self.test_case_name = test_case_name

          self.scenario = scenario

          self.result_id = result_id
@@ -173,7 +160,7 @@ 

              # These are for backwards compatibility only

              # (the values are already visible in the result data itself, the

              # caller shouldn't need them repeated here):

-             'item': subject_type_identifier_to_item(self.subject_type, self.subject_identifier),

+             'item': self.subject.to_item_dict(),

              'scenario': self.scenario,

          }

  
@@ -186,9 +173,8 @@ 

      Remote policy parsing failed.

      """

  

-     def __init__(self, subject_type, subject_identifier, test_case_name, details):

-         self.subject_type = subject_type

-         self.subject_identifier = subject_identifier

+     def __init__(self, subject, test_case_name, details):

+         self.subject = subject

          self.test_case_name = test_case_name

          self.details = details

  
@@ -196,8 +182,8 @@ 

          return {

              'type': 'invalid-gating-yaml',

              'testcase': self.test_case_name,

-             'subject_type': self.subject_type,

-             'subject_identifier': self.subject_identifier,

+             'subject_type': self.subject.type,

+             'subject_identifier': self.subject.identifier,

              'details': self.details

          }

  
@@ -212,16 +198,15 @@ 

  

      test_case_name = 'missing-gating-yaml'

  

-     def __init__(self, subject_type, subject_identifier):

-         self.subject_type = subject_type

-         self.subject_identifier = subject_identifier

+     def __init__(self, subject):

+         self.subject = subject

  

      def to_json(self):

          return {

              'type': 'missing-gating-yaml',

              'testcase': self.test_case_name,

-             'subject_type': self.subject_type,

-             'subject_identifier': self.subject_identifier,

+             'subject_type': self.subject.type,

+             'subject_identifier': self.subject.identifier,

          }

  

      def to_waived(self):
@@ -322,7 +307,7 @@ 

              self,

              policy,

              product_version,

-             subject_identifier,

+             subject,

              results_retriever):

          """

          Evaluate this policy rule for the given item.
@@ -330,7 +315,7 @@ 

          Args:

              policy (Policy): Parent policy of the rule

              product_version (str): Product version we are making a decision about

-             subject_identifier (str): Item we are making a decision about (for

+             subject (Subject): Item we are making a decision about (for

                  example, Koji build NVR, Bodhi update id, ...)

              results_retriever (ResultsRetriever): Object for retrieving data

                  from ResultsDB.
@@ -391,12 +376,12 @@ 

          'required': SafeYAMLBool(optional=True, default=False),

      }

  

-     def _get_sub_policies(self, policy, subject_identifier):

-         if policy.subject_type not in ['koji_build', 'redhat-module']:

+     def _get_sub_policies(self, policy, subject):

+         if subject.type not in remote_rule_subject_types():

              return []

  

          pkg_namespace, pkg_name, rev = greenwave.resources.retrieve_scm_from_koji(

-             subject_identifier)

+             subject.identifier)

          # if the element is actually a container and not a pkg there will be a "-container"

          # string at the end of the "pkg_name" and it will not match with the one in the

          # gating.yaml URL
@@ -424,26 +409,25 @@ 

              self,

              policy,

              product_version,

-             subject_identifier,

+             subject,

              results_retriever):

          try:

-             policies = self._get_sub_policies(policy, subject_identifier)

+             policies = self._get_sub_policies(policy, subject)

          except SafeYAMLError as e:

              return [

-                 InvalidGatingYaml(

-                     policy.subject_type, subject_identifier, 'invalid-gating-yaml', str(e))

+                 InvalidGatingYaml(subject, 'invalid-gating-yaml', str(e))

              ]

  

          if policies is None:

              if self.required:

-                 return [MissingGatingYaml(policy.subject_type, subject_identifier)]

+                 return [MissingGatingYaml(subject)]

              return []

  

          answers = []

          for remote_policy in policies:

              if remote_policy.matches_product_version(product_version):

                  response = remote_policy.check(

-                     product_version, subject_identifier, results_retriever)

+                     product_version, subject, results_retriever)

  

                  if isinstance(response, list):

                      answers.extend(response)
@@ -454,22 +438,21 @@ 

  

      def matches(self, policy, **attributes):

          #pylint: disable=broad-except

-         subject_identifier = attributes.get('subject_identifier')

-         if not subject_identifier:

+         subject = attributes.get('subject')

+         if not subject:

              return True

  

          sub_policies = []

          try:

-             sub_policies = self._get_sub_policies(policy, subject_identifier)

+             sub_policies = self._get_sub_policies(policy, subject)

          except SafeYAMLError:

-             logging.exception(

-                 'Failed to parse policies for %r', subject_identifier)

+             logging.exception('Failed to parse policies for %r', subject)

          except Exception:

-             logging.exception(

-                 'Failed to retrieve policies for %r', subject_identifier)

+             logging.exception('Failed to retrieve policies for %r', subject)

  

          if sub_policies is None:

-             return False

+             # RemoteRule matches if remote policy file is missing.

+             return True

  

          return any(sub_policy.matches(**attributes) for sub_policy in sub_policies)

  
@@ -495,10 +478,9 @@ 

              self,

              policy,

              product_version,

-             subject_identifier,

+             subject,

              results_retriever):

-         matching_results = results_retriever.retrieve(

-             policy.subject_type, subject_identifier, self.test_case_name)

+         matching_results = results_retriever.retrieve(subject, self.test_case_name)

  

          if self.scenario is not None:

              matching_results = [
@@ -507,43 +489,15 @@ 

  

          # Investigate the absence of result first.

          if not matching_results:

-             return TestResultMissing(

-                 policy.subject_type, subject_identifier, self.test_case_name, self.scenario)

- 

-         # For compose make decisions based on all architectures and variants.

-         if policy.subject_type == 'compose':

-             visited_arch_variants = set()

-             answers = []

-             for result in matching_results:

-                 result_data = result['data']

- 

-                 # Items under test result "data" are lists which are unhashable

-                 # types in Python. This converts anything that is stored there

-                 # to a string so we don't have to care about the stored value.

-                 arch_variant = (

-                     str(result_data.get('system_architecture')),

-                     str(result_data.get('system_variant')))

- 

-                 if arch_variant not in visited_arch_variants:

-                     visited_arch_variants.add(arch_variant)

-                     answer = self._answer_for_result(

-                         result,

-                         policy.subject_type,

-                         subject_identifier)

-                     answers.append(answer)

- 

-             return answers

+             return TestResultMissing(subject, self.test_case_name, self.scenario)

  

          # If we find multiple matching results, we always use the first one which

          # will be the latest chronologically, because ResultsDB always returns

          # results ordered by `submit_time` descending.

-         answers = []

-         for result in matching_results:

-             answers.append(self._answer_for_result(

-                 result,

-                 policy.subject_type,

-                 subject_identifier))

-         return answers

+         return [

+             self._answer_for_result(result, subject)

+             for result in subject.get_latest_results(matching_results)

+         ]

  

      def matches(self, policy, **attributes):

          testcase = attributes.get('testcase')
@@ -556,8 +510,7 @@ 

              'scenario': self.scenario,

          }

  

-     def _answer_for_result(

-             self, result, subject_type, subject_identifier):

+     def _answer_for_result(self, result, subject):

          if result['outcome'] in ('PASSED', 'INFO'):

              log.debug('Test result passed for the result_id %s and testcase %s,'

                        ' because the outcome is %s', result['id'], self.test_case_name,
@@ -565,17 +518,14 @@ 

              return TestResultPassed(self.test_case_name, result['id'])

  

          if result['outcome'] in ('QUEUED', 'RUNNING'):

-             log.debug('Test result MISSING for the subject_type %s, subject_identifier %s and '

-                       'testcase %s, because the outcome is %s', subject_type, subject_identifier,

+             log.debug('Test result MISSING for the %s and '

+                       'testcase %s, because the outcome is %s', subject,

                        self.test_case_name, result['outcome'])

-             return TestResultMissing(subject_type, subject_identifier, self.test_case_name,

-                                      self.scenario)

-         log.debug('Test result failed for the subject_type %s, subject_identifier %s and '

+             return TestResultMissing(subject, self.test_case_name, self.scenario)

+         log.debug('Test result failed for the %s and '

                    'testcase %s, because the outcome is %s and it didn\'t match any of the '

-                   'previous cases', subject_type, subject_identifier,

-                   self.test_case_name, result['outcome'])

-         return TestResultFailed(subject_type, subject_identifier, self.test_case_name,

-                                 self.scenario, result['id'])

+                   'previous cases', subject, self.test_case_name, result['outcome'])

+         return TestResultFailed(subject, self.test_case_name, self.scenario, result['id'])

  

  

  class ObsoleteRule(Rule):
@@ -594,7 +544,7 @@ 

              self,

              policy,

              product_version,

-             subject_identifier,

+             subject,

              results_retriever):

          raise ValueError('This rule is obsolete and can\'t be checked')

  
@@ -615,7 +565,6 @@ 

          'id': SafeYAMLString(),

          'product_versions': SafeYAMLList(str),

          'decision_context': SafeYAMLString(),

-         # TODO: Handle brew-build value better.

          'subject_type': SafeYAMLString(),

          'rules': SafeYAMLList(Rule),

          'blacklist': SafeYAMLList(str, optional=True),
@@ -643,8 +592,8 @@ 

          if product_version and not self.matches_product_version(product_version):

              return False

  

-         subject_type = attributes.get('subject_type')

-         if subject_type and subject_type != self.subject_type:

+         subject = attributes.get('subject')

+         if subject and subject.type != self.subject_type:

              return False

  

          return not self.rules or any(rule.matches(self, **attributes) for rule in self.rules)
@@ -652,16 +601,16 @@ 

      def check(

              self,

              product_version,

-             subject_identifier,

+             subject,

              results_retriever):

          # If an item is about a package and it is in the blacklist, return RuleSatisfied()

-         if self.subject_type == 'koji_build':

-             name = subject_identifier.rsplit('-', 2)[0]

+         name = subject.package_name

+         if name:

              if name in self.blacklist:

-                 return [BlacklistedInPolicy(subject_identifier) for rule in self.rules]

+                 return [BlacklistedInPolicy(subject.identifier) for rule in self.rules]

              for exclude in self.excluded_packages:

                  if fnmatch(name, exclude):

-                     return [ExcludedInPolicy(subject_identifier) for rule in self.rules]

+                     return [ExcludedInPolicy(subject.identifier) for rule in self.rules]

              if self.packages and not any(fnmatch(name, package) for package in self.packages):

                  # If the `packages` whitelist is set and this package isn't in the

                  # `packages` whitelist, then the policy doesn't apply to it
@@ -671,7 +620,7 @@ 

              response = rule.check(

                  self,

                  product_version,

-                 subject_identifier,

+                 subject,

                  results_retriever)

              if isinstance(response, list):

                  answers.extend(response)
@@ -736,7 +685,7 @@ 

      safe_yaml_attributes = {

          'id': SafeYAMLString(optional=True),

          'product_versions': SafeYAMLList(str),

-         'subject_type': SafeYAMLChoice('koji_build', 'redhat-module', optional=True),

+         'subject_type': SafeYAMLChoice(*remote_rule_subject_types(), optional=True),

          'decision_context': SafeYAMLString(),

          'rules': SafeYAMLList(Rule),

          'blacklist': SafeYAMLList(str, optional=True),

file modified
+8 -17
@@ -56,24 +56,15 @@ 

  

  

  def subject_product_version(

-         subject_identifier,

-         subject_type,

+         subject,

          koji_proxy=None,

          koji_task_id=None):

-     if subject_type == 'koji_build':

-         try:

-             _, _, release = subject_identifier.rsplit('-', 2)

-             _, short_prod_version = release.rsplit('.', 1)

-             return _guess_product_version(short_prod_version, koji_build=True)

-         except (KeyError, ValueError):

-             pass

+     if subject.short_product_version:

+         product_version = _guess_product_version(

+             subject.short_product_version, koji_build=subject.is_koji_build)

+         if product_version:

+             return product_version

  

-     if subject_type == "compose":

-         return _guess_product_version(subject_identifier)

- 

-     if subject_type == "redhat-module":

-         return "rhel-8"

- 

-     if koji_proxy:

+     if koji_proxy and subject.is_koji_build:

          return _guess_koji_build_product_version(

-             subject_identifier, koji_proxy, koji_task_id)

+             subject.identifier, koji_proxy, koji_task_id)

file modified
+6 -19
@@ -54,10 +54,10 @@ 

          super().__init__(**args)

          self.cache = {}

  

-     def _retrieve_all(self, subject_type, subject_identifier, testcase=None, scenarios=None):

+     def _retrieve_all(self, subject, testcase=None, scenarios=None):

          # Get test case result from cache if all test case results were already

-         # retrieved for given subject type/ID.

-         cache_key = (subject_type, subject_identifier, scenarios)

+         # retrieved for given Subject.

+         cache_key = (subject.type, subject.identifier, scenarios)

          if testcase and cache_key in self.cache:

              for result in self.cache[cache_key]:

                  if result['testcase']['name'] == testcase:
@@ -75,22 +75,9 @@ 

              params.update({'scenario': ','.join(scenarios)})

  

          results = []

-         if subject_type == 'koji_build':

-             params['type'] = 'koji_build,brew-build'

-             params['item'] = subject_identifier

-             results = self._retrieve_data(params)

- 

-             del params['type']

-             del params['item']

-             params['original_spec_nvr'] = subject_identifier

-             results.extend(self._retrieve_data(params))

-         elif subject_type == 'compose':

-             params['productmd.compose.id'] = subject_identifier

-             results = self._retrieve_data(params)

-         else:

-             params['type'] = subject_type

-             params['item'] = subject_identifier

-             results = self._retrieve_data(params)

+         for query in subject.result_queries():

+             query.update(params)

+             results.extend(self._retrieve_data(query))

  

          if not testcase:

              self.cache[cache_key] = results

@@ -0,0 +1,41 @@ 

+ # SPDX-License-Identifier: GPL-2.0+

+ 

+ from .subject import Subject

+ 

+ 

+ def _get_latest_results(results):

+     """

+     Yields only the latest results for unique architecture and variant pairs.

+ 

+     The input results are sorted from latest to oldest.

+     """

+     visited_arch_variants = set()

+     for result in results:

+         result_data = result["data"]

+ 

+         # Items under test result "data" are lists which are unhashable

+         # types in Python. This converts anything that is stored there

+         # to a string so we don't have to care about the stored value.

+         arch_variant = (

+             str(result_data.get("system_architecture")),

+             str(result_data.get("system_variant")),

+         )

+ 

+         if arch_variant not in visited_arch_variants:

+             visited_arch_variants.add(arch_variant)

+             yield result

+ 

+ 

+ class Compose(Subject):

+     type_ = "compose"

+ 

+     @property

+     def short_product_version(self):

+         # FIXME: Is this really correct?

+         return self.item

+ 

+     def to_item_dict(self):

+         return {"productmd.compose.id": self.item}

+ 

+     def get_latest_results(self, results):

+         return list(_get_latest_results(results))

@@ -0,0 +1,49 @@ 

+ # SPDX-License-Identifier: GPL-2.0+

+ """

+ Factory functions and helper functions to create subject object.

+ """

+ 

+ from .compose import Compose

+ from .generic import GenericSubject

+ from .koji_build import KojiBuild

+ from .redhat_module import RedHatModule

+ 

+ # Subject subclasses (except GenericSubject)

+ # TODO: This could be moved to configuration as list of strings.

+ SUBJECT_CLASSES = (

+     Compose,

+     KojiBuild,

+     RedHatModule,

+ )

+ 

+ 

+ def remote_rule_subject_types():

+     return [

+         cls.type_

+         for cls in SUBJECT_CLASSES

+         if cls.supports_remote_rule

+     ]

+ 

+ 

+ def subject_type_and_id_from_dict(data):

+     compose = data.get("productmd.compose.id")

+     if compose:

+         return Compose.type_, compose

+ 

+     nvr = data.get("original_spec_nvr")

+     if nvr:

+         return KojiBuild.type_, nvr

+ 

+     return data.get("type"), data.get("item")

+ 

+ 

+ def create_subject(type_, identifier):

+     for cls in SUBJECT_CLASSES:

+         if type_ in cls.type_:

+             return cls(identifier)

+ 

+     # Special case for backwards compatibility.

+     if type_ == "brew-build":

+         return KojiBuild(identifier)

+ 

+     return GenericSubject(type_, identifier)

@@ -0,0 +1,21 @@ 

+ # SPDX-License-Identifier: GPL-2.0+

+ 

+ from .subject import Subject

+ 

+ 

+ class GenericSubject(Subject):

+     """

+     Subject class for a generic subject.

+ 

+     This is used when subject type does not require specialized functionality.

+     """

+ 

+     # Set in constructor.

+     type_ = None

+ 

+     # Can be potentially Koji/Brew build.

+     is_koji_build = True

+ 

+     def __init__(self, type_, item):

+         self.type_ = type_

+         self.item = item

@@ -0,0 +1,26 @@ 

+ # SPDX-License-Identifier: GPL-2.0+

+ 

+ from .subject import Subject

+ 

+ 

+ class KojiBuild(Subject):

+     type_ = "koji_build"

+     supports_remote_rule = True

+     is_koji_build = True

+ 

+     @property

+     def package_name(self):

+         return self.item.rsplit("-", 2)[0]

+ 

+     @property

+     def short_product_version(self):

+         try:

+             _, _, release = self.identifier.rsplit("-", 2)

+             _, short_prod_version = release.rsplit(".", 1)

+             return short_prod_version

+         except (KeyError, ValueError):

+             return None

+ 

+     def result_queries(self):

+         yield {"type": "koji_build,brew-build", "item": self.item}

+         yield {"original_spec_nvr": self.item}

@@ -0,0 +1,16 @@ 

+ # SPDX-License-Identifier: GPL-2.0+

+ 

+ from .subject import Subject

+ 

+ 

+ class RedHatModule(Subject):

+     type_ = "redhat-module"

+     supports_remote_rule = True

+ 

+     # TODO: It could be possible get product version from Koji/Brew instead of

+     #       hard-coding it.

+     # is_koji_build = True

+ 

+     @property

+     def short_product_version(self):

+         return "el8"

@@ -0,0 +1,87 @@ 

+ # SPDX-License-Identifier: GPL-2.0+

+ 

+ 

+ class Subject:

+     """

+     Base class for decision subjects.

+ 

+     Main properties are type and item/identifier.

+ 

+     Type can be, for example, Koji build, compose, module or other. It must be

+     uniquely defined in subclasses.

+ 

+     Item or identifier should uniquely identify the artefact (test subject).

+     """

+ 

+     """

+     Subject type string.

+ 

+     Override in subclass.

+ 

+     Factory function uses this to determine which subclass to use for

+     instantiation.

+     """

+     type_ = None

+ 

+     """

+     Subject can be used with RemoteRule only if value is True.

+     """

+     supports_remote_rule = False

+ 

+     """

+     A build for subject identifier can be found on Koji/Brew.

+     """

+     is_koji_build = False

+ 

+     def __init__(self, item):

+         self.item = item

+ 

+     @property

+     def type(self):

+         """Subject type string."""

+         return self.type_

+ 

+     @property

+     def identifier(self):

+         """Alias for item property."""

+         return self.item

+ 

+     @property

+     def package_name(self):

+         """Package name of the subject or None."""

+         return None

+ 

+     @property

+     def short_product_version(self):

+         """Get short product version of the subject (guess from identifier) or None."""

+         return None

+ 

+     def to_item_dict(self):

+         """

+         Greenwave < 0.8 included an "item" key in the "unsatisfied_requirements".

+         This returns a suitable value for that key, for backwards compatibility.

+         """

+         return {"type": self.type, "item": self.item}

+ 

+     def result_queries(self):

+         """

+         Yields parameters for RestulsDB queries.

+ 

+         For example, one set of parameters could have "type=koji_build" for one

+         query and "type=brew-build" for another.

+         """

+         yield self.to_item_dict()

+ 

+     def get_latest_results(self, results):

+         """

+         Filters out results to get only the latest relevant ones.

+ 

+         The input results are sorted from latest to oldest.

+         """

+         return results

+ 

+     # TODO: Test __str__ and __repr__.

+     def __str__(self):

+         return "subject_type {!r}, subject_identifier {!r}".format(

+             self.type_, self.item

+         )

@@ -22,33 +22,33 @@ 

  )

  from greenwave.resources import ResultsRetriever

  from greenwave.safe_yaml import SafeYAMLError

+ from greenwave.subjects.generic import GenericSubject

+ from greenwave.subjects.koji_build import KojiBuild

+ from greenwave.subjects.redhat_module import RedHatModule

  from greenwave.waivers import waive_answers

  

  

  class DummyResultsRetriever(ResultsRetriever):

-     def __init__(

-             self, subject_identifier=None, testcase=None, outcome='PASSED',

-             subject_type='koji_build'):

+     def __init__(self, subject=None, testcase=None, outcome='PASSED'):

          super(DummyResultsRetriever, self).__init__(

              ignore_ids=[],

              when='',

              timeout=0,

              verify=False,

              url='')

-         self.subject_identifier = subject_identifier

-         self.subject_type = subject_type

+         self.subject = subject

          self.testcase = testcase

          self.outcome = outcome

  

      def _retrieve_data(self, params):

-         if (params.get('item') == self.subject_identifier and

-                 ('type' not in params or self.subject_type in params['type'].split(',')) and

+         if (self.subject and params.get('item') == self.subject.identifier and

+                 ('type' not in params or self.subject.type in params['type'].split(',')) and

                  params.get('testcases') == self.testcase):

              return [{

                  'id': 123,

                  'data': {

-                     'item': [self.subject_identifier],

-                     'type': [self.subject_type],

+                     'item': [self.subject.identifier],

+                     'type': [self.subject.type],

                  },

                  'testcase': {'name': self.testcase},

                  'outcome': self.outcome,
@@ -57,21 +57,22 @@ 

  

  

  def test_summarize_answers():

+     subject = KojiBuild('nvr')

      assert summarize_answers([RuleSatisfied()]) == \

          'All required tests passed'

-     assert summarize_answers([TestResultFailed('koji_build', 'nvr', 'test', None, 'id'),

+     assert summarize_answers([TestResultFailed(subject, 'test', None, 'id'),

                                RuleSatisfied()]) == \

          '1 of 2 required tests failed'

-     assert summarize_answers([TestResultMissing('koji_build', 'nvr', 'test', None)]) == \

+     assert summarize_answers([TestResultMissing(subject, 'test', None)]) == \

          '1 of 1 required test results missing'

-     assert summarize_answers([TestResultMissing('koji_build', 'nvr', 'test', None),

-                               TestResultFailed('koji_build', 'nvr', 'test', None, 'id')]) == \

+     assert summarize_answers([TestResultMissing(subject, 'test', None),

+                               TestResultFailed(subject, 'test', None, 'id')]) == \

          '1 of 2 required tests failed, 1 result missing'

-     assert summarize_answers([TestResultMissing('koji_build', 'nvr', 'testa', None),

-                               TestResultMissing('koji_build', 'nvr', 'testb', None),

-                               TestResultFailed('koji_build', 'nvr', 'test', None, 'id')]) == \

+     assert summarize_answers([TestResultMissing(subject, 'testa', None),

+                               TestResultMissing(subject, 'testb', None),

+                               TestResultFailed(subject, 'test', None, 'id')]) == \

          '1 of 3 required tests failed, 2 results missing'

-     assert summarize_answers([TestResultMissing('koji_build', 'nvr', 'test', None),

+     assert summarize_answers([TestResultMissing(subject, 'test', None),

                               RuleSatisfied()]) == \

          '1 of 2 required test results missing'

  
@@ -92,10 +93,10 @@ 

      policy = policies[0]

  

      results = DummyResultsRetriever()

-     subject_identifier = 'some_nevr'

+     subject = KojiBuild('some_nevr')

  

      # Ensure that absence of a result is failure.

-     decision = policy.check('fedora-rawhide', subject_identifier, results)

+     decision = policy.check('fedora-rawhide', subject, results)

      assert len(decision) == 1

      assert isinstance(decision[0], TestResultMissing)

  
@@ -122,9 +123,10 @@ 

      policy = policies[0]

  

      item = 'some_nevr'

-     results = DummyResultsRetriever(item, 'sometest', 'FAILED', 'brew-build')

+     subject = KojiBuild(item)

+     results = DummyResultsRetriever(subject, 'sometest', 'FAILED')

  

-     decision = policy.check('fedora-rawhide', item, results)

+     decision = policy.check('fedora-rawhide', subject, results)

      assert len(decision) == 1

      assert isinstance(decision[0], TestResultFailed)

  
@@ -136,7 +138,7 @@ 

          'product_version': 'fedora-rawhide',

          'waived': True,

      }]

-     decision = policy.check('fedora-rawhide', item, results)

+     decision = policy.check('fedora-rawhide', subject, results)

      decision = waive_answers(decision, waivers)

      assert len(decision) == 1

      assert isinstance(decision[0], RuleSatisfied)
@@ -164,9 +166,10 @@ 

      policy = policies[0]

  

      item = 'some_bodhi_update'

-     results = DummyResultsRetriever(item, 'sometest', 'FAILED', 'bodhi_update')

+     subject = GenericSubject('bodhi_update', item)

+     results = DummyResultsRetriever(subject, 'sometest', 'FAILED')

  

-     decision = policy.check('fedora-rawhide', item, results)

+     decision = policy.check('fedora-rawhide', subject, results)

      assert len(decision) == 1

      assert isinstance(decision[0], TestResultFailed)

  
@@ -178,7 +181,7 @@ 

          'product_version': 'fedora-rawhide',

          'waived': True,

      }]

-     decision = policy.check('fedora-rawhide', item, results)

+     decision = policy.check('fedora-rawhide', subject, results)

      decision = waive_answers(decision, waivers)

      assert len(decision) == 1

      assert isinstance(decision[0], RuleSatisfied)
@@ -280,7 +283,7 @@ 

      """ Testing the RemoteRule with the koji interaction.

      In this case we are just mocking koji """

  

-     nvr = 'nethack-1.2.3-1.el9000'

+     subject = KojiBuild('nethack-1.2.3-1.el9000')

  

      serverside_fragment = dedent("""

          --- !Policy
@@ -315,20 +318,20 @@ 

                  policy = policies[0]

  

                  # Ensure that presence of a result is success.

-                 results = DummyResultsRetriever(nvr, 'dist.upgradepath')

-                 decision = policy.check('fedora-26', nvr, results)

+                 results = DummyResultsRetriever(subject, 'dist.upgradepath')

+                 decision = policy.check('fedora-26', subject, results)

                  assert len(decision) == 1

                  assert isinstance(decision[0], RuleSatisfied)

  

                  # Ensure that absence of a result is failure.

                  results = DummyResultsRetriever()

-                 decision = policy.check('fedora-26', nvr, results)

+                 decision = policy.check('fedora-26', subject, results)

                  assert len(decision) == 1

                  assert isinstance(decision[0], TestResultMissing)

  

                  # And that a result with a failure, is a failure.

-                 results = DummyResultsRetriever(nvr, 'dist.upgradepath', 'FAILED')

-                 decision = policy.check('fedora-26', nvr, results)

+                 results = DummyResultsRetriever(subject, 'dist.upgradepath', 'FAILED')

+                 decision = policy.check('fedora-26', subject, results)

                  assert len(decision) == 1

                  assert isinstance(decision[0], TestResultFailed)

  
@@ -339,6 +342,7 @@ 

      In this case we are just mocking koji """

  

      nvr = '389-ds-1.4-820181127205924.9edba152'

+     subject = RedHatModule(nvr)

  

      serverside_fragment = dedent("""

          --- !Policy
@@ -374,28 +378,27 @@ 

                  policy = policies[0]

  

                  # Ensure that presence of a result is success.

-                 results = DummyResultsRetriever(nvr, 'baseos-ci.redhat-module.tier0.functional',

-                                                 subject_type='redhat-module')

-                 decision = policy.check('rhel-8', nvr, results)

+                 results = DummyResultsRetriever(subject, 'baseos-ci.redhat-module.tier0.functional')

+                 decision = policy.check('rhel-8', subject, results)

                  assert len(decision) == 1

                  assert isinstance(decision[0], RuleSatisfied)

  

                  # Ensure that absence of a result is failure.

-                 results = DummyResultsRetriever(subject_type='redhat-module')

-                 decision = policy.check('rhel-8', nvr, results)

+                 results = DummyResultsRetriever(subject)

+                 decision = policy.check('rhel-8', subject, results)

                  assert len(decision) == 1

                  assert isinstance(decision[0], TestResultMissing)

  

                  # And that a result with a failure, is a failure.

-                 results = DummyResultsRetriever(nvr, 'baseos-ci.redhat-module.tier0.functional',

-                                                 'FAILED', subject_type='redhat-module')

-                 decision = policy.check('rhel-8', nvr, results)

+                 results = DummyResultsRetriever(

+                     subject, 'baseos-ci.redhat-module.tier0.functional', 'FAILED')

+                 decision = policy.check('rhel-8', subject, results)

                  assert len(decision) == 1

                  assert isinstance(decision[0], TestResultFailed)

  

  

  def test_remote_rule_policy_optional_id(tmpdir):

-     nvr = 'nethack-1.2.3-1.el9000'

+     subject = KojiBuild('nethack-1.2.3-1.el9000')

  

      serverside_fragment = dedent("""

          --- !Policy
@@ -428,7 +431,7 @@ 

  

                  results = DummyResultsRetriever()

                  expected_details = "Policy 'untitled': Attribute 'product_versions' is required"

-                 decision = policy.check('fedora-26', nvr, results)

+                 decision = policy.check('fedora-26', subject, results)

                  assert len(decision) == 1

                  assert isinstance(decision[0], InvalidGatingYaml)

                  assert decision[0].is_satisfied is False
@@ -438,7 +441,7 @@ 

  def test_remote_rule_malformed_yaml(tmpdir):

      """ Testing the RemoteRule with a malformed gating.yaml file """

  

-     nvr = 'nethack-1.2.3-1.el9000'

+     subject = KojiBuild('nethack-1.2.3-1.el9000')

  

      serverside_fragment = dedent("""

          --- !Policy
@@ -483,7 +486,7 @@ 

                      policy = policies[0]

  

                      results = DummyResultsRetriever()

-                     decision = policy.check('fedora-26', nvr, results)

+                     decision = policy.check('fedora-26', subject, results)

                      assert len(decision) == 1

                      assert isinstance(decision[0], InvalidGatingYaml)

                      assert decision[0].is_satisfied is False
@@ -493,7 +496,7 @@ 

      """ Testing the RemoteRule with a malformed gating.yaml file

      But this time waiving the error """

  

-     nvr = 'nethack-1.2.3-1.el9000'

+     subject = KojiBuild('nethack-1.2.3-1.el9000')

  

      serverside_fragment = dedent("""

          --- !Policy
@@ -548,14 +551,14 @@ 

                          'comment': 'Waiving the invalid gating.yaml file',

                          'waived': True,

                      }]

-                     decision = policy.check('fedora-26', nvr, results)

+                     decision = policy.check('fedora-26', subject, results)

                      decision = waive_answers(decision, waivers)

                      assert len(decision) == 0

  

  

  def test_remote_rule_required():

      """ Testing the RemoteRule with required flag set """

-     nvr = 'nethack-1.2.3-1.el9000'

+     subject = KojiBuild('nethack-1.2.3-1.el9000')

      app = create_app('greenwave.config.TestingConfig')

      with app.app_context():

          with mock.patch('greenwave.resources.retrieve_scm_from_koji') as scm:
@@ -573,11 +576,11 @@ 

                  """))

                  policy = policies[0]

                  results = DummyResultsRetriever()

-                 decision = policy.check('fedora-rawhide', nvr, results)

+                 decision = policy.check('fedora-rawhide', subject, results)

                  assert len(decision) == 1

                  assert isinstance(decision[0], MissingGatingYaml)

                  assert not decision[0].is_satisfied

-                 assert decision[0].subject_identifier == nvr

+                 assert decision[0].subject.identifier == subject.identifier

  

  

  def test_parse_policies_missing_tag():
@@ -653,8 +656,9 @@ 

      policies = load_policies(tmpdir.strpath)

      policy = policies[0]

  

-     results = DummyResultsRetriever('nethack-1.2.3-1.el9000', 'sometest', 'PASSED', 'kind-of-magic')

-     decision = policy.check('rhel-9000', 'nethack-1.2.3-1.el9000', results)

+     subject = GenericSubject('kind-of-magic', 'nethack-1.2.3-1.el9000')

+     results = DummyResultsRetriever(subject, 'sometest', 'PASSED')

+     decision = policy.check('rhel-9000', subject, results)

      assert len(decision) == 1

      assert isinstance(decision[0], TestResultPassed)

  
@@ -681,8 +685,9 @@ 

      policies = load_policies(tmpdir.strpath)

      policy = policies[0]

  

-     results = DummyResultsRetriever('nethack-1.2.3-1.el9000', 'sometest', 'PASSED', 'koji_build')

-     decision = policy.check('rhel-9000', 'nethack-1.2.3-1.el9000', results)

+     subject = KojiBuild('nethack-1.2.3-1.el9000')

+     results = DummyResultsRetriever(subject, 'sometest', 'PASSED')

+     decision = policy.check('rhel-9000', subject, results)

      assert len(decision) == num_decisions

      if num_decisions:

          assert isinstance(decision[0], TestResultPassed)
@@ -861,6 +866,7 @@ 

  

  def test_policy_with_subject_type_component_version(tmpdir):

      nv = '389-ds-base-1.4.0.10'

+     subject = GenericSubject('component-version', nv)

      p = tmpdir.join('fedora.yaml')

      p.write(dedent("""

          --- !Policy
@@ -875,15 +881,15 @@ 

          """))

      policies = load_policies(tmpdir.strpath)

      policy = policies[0]

-     results = DummyResultsRetriever(nv, 'test_for_new_type', 'PASSED',

-                                     'component-version')

-     decision = policy.check('fedora-29', nv, results)

+     results = DummyResultsRetriever(subject, 'test_for_new_type', 'PASSED')

+     decision = policy.check('fedora-29', subject, results)

      assert len(decision) == 1

      assert isinstance(decision[0], RuleSatisfied)

  

  

  def test_policy_with_subject_type_redhat_module(tmpdir):

      nsvc = 'httpd:2.4:20181018085700:9edba152'

+     subject = RedHatModule(nsvc)

      p = tmpdir.join('fedora.yaml')

      p.write(dedent("""

          --- !Policy
@@ -898,9 +904,8 @@ 

          """))

      policies = load_policies(tmpdir.strpath)

      policy = policies[0]

-     results = DummyResultsRetriever(nsvc, 'test_for_redhat_module_type', 'PASSED',

-                                     'redhat-module')

-     decision = policy.check('fedora-29', nsvc, results)

+     results = DummyResultsRetriever(subject, 'test_for_redhat_module_type', 'PASSED')

+     decision = policy.check('fedora-29', subject, results)

      assert len(decision) == 1

      assert isinstance(decision[0], RuleSatisfied)

  
@@ -911,6 +916,7 @@ 

      In this case we are just mocking koji """

  

      nvr = 'nethack-1.2.3-1.el9000'

+     subject = KojiBuild(nvr)

  

      serverside_json = {

          'product_version': 'fedora-26',
@@ -943,19 +949,19 @@ 

                  policy = OnDemandPolicy.create_from_json(serverside_json)

  

                  # Ensure that presence of a result is success.

-                 results = DummyResultsRetriever(nvr, 'dist.upgradepath')

-                 decision = policy.check('fedora-26', nvr, results)

+                 results = DummyResultsRetriever(subject, 'dist.upgradepath')

+                 decision = policy.check('fedora-26', subject, results)

                  assert len(decision) == 1

                  assert isinstance(decision[0], RuleSatisfied)

  

                  # Ensure that absence of a result is failure.

                  results = DummyResultsRetriever()

-                 decision = policy.check('fedora-26', nvr, results)

+                 decision = policy.check('fedora-26', subject, results)

                  assert len(decision) == 1

                  assert isinstance(decision[0], TestResultMissing)

  

                  # And that a result with a failure, is a failure.

-                 results = DummyResultsRetriever(nvr, 'dist.upgradepath', 'FAILED')

-                 decision = policy.check('fedora-26', nvr, results)

+                 results = DummyResultsRetriever(subject, 'dist.upgradepath', 'FAILED')

+                 decision = policy.check('fedora-26', subject, results)

                  assert len(decision) == 1

                  assert isinstance(decision[0], TestResultFailed)

@@ -9,6 +9,7 @@ 

  import greenwave.consumers.resultsdb

  from greenwave.product_versions import subject_product_version

  from greenwave.policies import Policy

+ from greenwave.subjects.factory import create_subject

  

  

  def test_announcement_keys_decode_with_list():
@@ -16,9 +17,8 @@ 

      message = {'msg': {'data': {

          'original_spec_nvr': ['glibc-1.0-1.fc27'],

      }}}

-     subjects = list(cls.announcement_subjects(message))

  

-     assert subjects == [('koji_build', 'glibc-1.0-1.fc27')]

+     assert cls.announcement_subject(message) == ('koji_build', 'glibc-1.0-1.fc27')

  

  

  def test_no_announcement_subjects_for_empty_nvr():
@@ -32,9 +32,8 @@ 

      message = {'msg': {'data': {

          'original_spec_nvr': [""],

      }}}

-     subjects = list(cls.announcement_subjects(message))

  

-     assert subjects == []

+     assert cls.announcement_subject(message) == (None, None)

  

  

  def test_announcement_subjects_for_brew_build():
@@ -45,9 +44,8 @@ 

          'type': 'brew-build',

          'item': ['glibc-1.0-3.fc27'],

      }}}

-     subjects = list(cls.announcement_subjects(message))

  

-     assert subjects == [('koji_build', 'glibc-1.0-3.fc27')]

+     assert cls.announcement_subject(message) == ('brew-build', 'glibc-1.0-3.fc27')

  

  

  def test_announcement_subjects_for_new_compose_message():
@@ -78,9 +76,8 @@ 

              }

          }

      }

-     subjects = list(cls.announcement_subjects(message))

  

-     assert subjects == [('compose', 'Fedora-Rawhide-20181205.n.0')]

+     assert cls.announcement_subject(message) == ('compose', 'Fedora-Rawhide-20181205.n.0')

  

  

  def test_no_announcement_subjects_for_old_compose_message():
@@ -106,9 +103,8 @@ 

              }

          }

      }

-     subjects = list(cls.announcement_subjects(message))

  

-     assert subjects == []

+     assert cls.announcement_subject(message) == (None, None)

  

  

  parameters = [
@@ -321,12 +317,12 @@ 

      }

      handler = greenwave.consumers.resultsdb.ResultsDBHandler(hub)

      with handler.flask_app.app_context():

-         product_version = subject_product_version(

-             'release-e2e-test-1.0.1685-1.el5', 'koji_build')

+         subject = create_subject('koji_build', 'release-e2e-test-1.0.1685-1.el5')

+         product_version = subject_product_version(subject)

          assert product_version == 'rhel-5'

  

-         product_version = subject_product_version(

-             'rust-toolset-rhel8-20181010170614.b09eea91', 'redhat-module')

+         subject = create_subject('redhat-module', 'rust-toolset-rhel8-20181010170614.b09eea91')

+         product_version = subject_product_version(subject)

          assert product_version == 'rhel-8'

  

  
@@ -335,8 +331,8 @@ 

      koji_proxy.getBuild.return_value = {'task_id': 666}

      koji_proxy.getTaskRequest.return_value = ['git://example.com/project', 'rawhide', {}]

  

-     product_version = subject_product_version(

-         'fake_koji_build', 'container-build', koji_proxy)

+     subject = create_subject('container-build', 'fake_koji_build')

+     product_version = subject_product_version(subject, koji_proxy)

      koji_proxy.getBuild.assert_called_once_with('fake_koji_build')

      koji_proxy.getTaskRequest.assert_called_once_with(666)

      assert product_version == 'fedora-rawhide'
@@ -346,8 +342,8 @@ 

      koji_proxy = mock.MagicMock()

      koji_proxy.getBuild.return_value = {'task_id': None}

  

-     product_version = subject_product_version(

-         'fake_koji_build', 'container-build', koji_proxy)

+     subject = create_subject('container-build', 'fake_koji_build')

+     product_version = subject_product_version(subject, koji_proxy)

  

      koji_proxy.getBuild.assert_called_once_with('fake_koji_build')

      koji_proxy.getTaskRequest.assert_not_called()
@@ -361,7 +357,8 @@ 

      'badnvr-1.2.f30',

  ))

  def test_guess_product_version_failure(nvr):

-     product_version = subject_product_version(nvr, 'koji_build')

+     subject = create_subject('koji_build', nvr)

+     product_version = subject_product_version(subject)

      assert product_version is None

  

  

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

  from greenwave.app_factory import create_app

  from greenwave.policies import Policy, RemoteRule

  from greenwave.safe_yaml import SafeYAMLError

+ from greenwave.subjects.koji_build import KojiBuild

  

  

  def test_match_passing_test_case_rule():
@@ -50,6 +51,7 @@ 

            - !PassingTestCaseRule {test_case_name: some_test_case}

      """)

      nvr = 'nethack-1.2.3-1.el9000'

+     subject = KojiBuild(nvr)

      mock_retrieve_scm_from_koji.return_value = ('rpms', nvr, '123')

  

      app = create_app('greenwave.config.TestingConfig')
@@ -62,9 +64,9 @@ 

  

          rule = policy.rules[0]

          assert rule.matches(policy)

-         assert rule.matches(policy, subject_identifier=nvr)

-         assert rule.matches(policy, subject_identifier=nvr, testcase='some_test_case')

-         assert not rule.matches(policy, subject_identifier=nvr, testcase='other_test_case')

+         assert rule.matches(policy, subject=subject)

+         assert rule.matches(policy, subject=subject, testcase='some_test_case')

+         assert not rule.matches(policy, subject=subject, testcase='other_test_case')

  

          # Simulate invalid gating.yaml file.

          def raiseYamlError(*args):
@@ -73,9 +75,9 @@ 

          mock_retrieve_yaml_remote_rule.side_effect = raiseYamlError

  

          assert rule.matches(policy)

-         assert not rule.matches(policy, subject_identifier=nvr)

-         assert not rule.matches(policy, subject_identifier=nvr, testcase='some_test_case')

-         assert not rule.matches(policy, subject_identifier=nvr, testcase='other_test_case')

+         assert not rule.matches(policy, subject=subject)

+         assert not rule.matches(policy, subject=subject, testcase='some_test_case')

+         assert not rule.matches(policy, subject=subject, testcase='other_test_case')

  

  

  @pytest.mark.parametrize(('required_flag', 'required_value'), (

@@ -8,16 +8,19 @@ 

      InvalidGatingYaml,

  )

  

+ from greenwave.subjects.koji_build import KojiBuild

  

+ 

+ testSubject = KojiBuild('nethack-1.2.3-1.el9000')

  testResultPassed = RuleSatisfied()

  testResultFailed = TestResultFailed(

-     'koji_build', 'nethack-1.2.3-1.el9000', 'test', None, 1)

+     testSubject, 'test', None, 1)

  testResultMissing = TestResultMissing(

-     'koji_build', 'nethack-1.2.3-1.el9000', 'test', None)

+     testSubject, 'test', None)

  testResultMissingWaived = TestResultMissingWaived(

-     'koji_build', 'nethack-1.2.3-1.el9000', 'test', None)

+     testSubject, 'test', None)

  testInvalidGatingYaml = InvalidGatingYaml(

-     'koji_build', 'nethack-1.2.3-1.el9000', 'test', 'Missing !Policy tag')

+     testSubject, 'test', 'Missing !Policy tag')

  

  

  def test_summary_passed():

file modified
+8 -10
@@ -5,14 +5,14 @@ 

      TestResultMissing,

      TestResultFailed,

  )

+ from greenwave.subjects.koji_build import KojiBuild

  from greenwave.waivers import waive_answers

  

  

  def test_waive_failed_result():

      answers = [

          TestResultFailed(

-             subject_type='koji-build',

-             subject_identifier='nethack-1.2.3-1.rawhide',

+             subject=KojiBuild('nethack-1.2.3-1.rawhide'),

              test_case_name='test1',

              scenario='scenario1',

              result_id=99,
@@ -24,7 +24,7 @@ 

  

      waivers = [

          dict(

-             subject_type='koji-build',

+             subject_type='koji_build',

              subject_identifier='nethack-1.2.3-1.rawhide',

              product_version='rawhide',

              testcase='test1',
@@ -43,8 +43,7 @@ 

  def test_waive_missing_result():

      answers = [

          TestResultMissing(

-             subject_type='koji-build',

-             subject_identifier='nethack-1.2.3-1.rawhide',

+             subject=KojiBuild('nethack-1.2.3-1.rawhide'),

              test_case_name='test1',

              scenario='scenario1',

          )
@@ -55,7 +54,7 @@ 

  

      waivers = [

          dict(

-             subject_type='koji-build',

+             subject_type='koji_build',

              subject_identifier='nethack-1.2.3-1.rawhide',

              product_version='rawhide',

              testcase='test1',
@@ -65,7 +64,7 @@ 

      expected_json = dict(

          type='test-result-missing-waived',

          testcase='test1',

-         subject_type='koji-build',

+         subject_type='koji_build',

          subject_identifier='nethack-1.2.3-1.rawhide',

          scenario='scenario1',

      )
@@ -76,8 +75,7 @@ 

  def test_waive_invalid_gatin_yaml():

      answers = [

          InvalidGatingYaml(

-             subject_type='koji-build',

-             subject_identifier='nethack-1.2.3-1.rawhide',

+             subject=KojiBuild('nethack-1.2.3-1.rawhide'),

              test_case_name='invalid-gating-yaml',

              details='',

          )
@@ -88,7 +86,7 @@ 

  

      waivers = [

          dict(

-             subject_type='koji-build',

+             subject_type='koji_build',

              subject_identifier='nethack-1.2.3-1.rawhide',

              product_version='rawhide',

              testcase='invalid-gating-yaml',

@@ -21,7 +21,7 @@ 

      retriever.ignore_ids = [100]

      waiver = dict(

          id=99,

-         subject_type='koji-build',

+         subject_type='koji_build',

          subject_identifier='nethack-1.2.3-1.rawhide',

          product_version='rawhide',

          testcase='test1',
@@ -38,7 +38,7 @@ 

      retriever.ignore_ids = [99]

      waiver = dict(

          id=99,

-         subject_type='koji-build',

+         subject_type='koji_build',

          subject_identifier='nethack-1.2.3-1.rawhide',

          product_version='rawhide',

          testcase='test1',
@@ -54,7 +54,7 @@ 

      retriever = WaiversRetriever(**_DUMMY_RETRIEVER_ARGUMENTS)

      waiver = dict(

          id=99,

-         subject_type='koji-build',

+         subject_type='koji_build',

          subject_identifier='nethack-1.2.3-1.rawhide',

          product_version='rawhide',

          testcase='test1',

file modified
+2 -2
@@ -6,8 +6,8 @@ 

      Returns true only if there is a matching waiver for given answer.

      """

      return any(

-         waiver['subject_type'] == answer.subject_type and

-         waiver['subject_identifier'] == answer.subject_identifier and

+         waiver['subject_type'] == answer.subject.type and

+         waiver['subject_identifier'] == answer.subject.identifier and

          waiver['testcase'] == answer.test_case_name

          for waiver in waivers)

  

Refactor subject type and item usage

Adds a base class for decision subject. Each subclass has subject type
and item/identifier property. Type is static for the specialized classes
and dynamically set on instantiation of the generic subject
(GenericSubject).

This makes the code more modular and allows to easily add and specialize
subject types and, eventually, disable or remove specializations.

Fixes "koji-build" in tests to "koji_build".

Signed-off-by: Lukas Holecek hluk@email.cz

rebased onto 2a9e3da

4 years ago

Mmm I don't like that. We did an effort in the past to remove hard-coded subject types in Greenwave and make it more easily to have "whatever" subject type. I thought it was a great thing and it was sad that we couldn't remove them even more.
This looks like going backwards to me...

This does not hard-code anything. It just moves the specialized functionality of subject types to separate submodules so it's easier to create another special subject types.

Having to handle subject types in various places in code doesn't scale well.

It creates a whole file and class for these subject types, it is hard-coded IMHO.

It creates a whole file and class for these subject types, it is hard-coded IMHO.

That doesn't sound like hard-coding. Not sure what you mean by it. Can you explain?

Currently, if you want to create a special handling for a subject type you probably need to change behavior in policies and rules (well, and couple of other places as well). But why should the policies and rules know how to handle special subject type? This basically means having if subject_type == 'koji_build' and other branches everywhere.

The change is to move the logic to a single submodule (subjects) so the other code can just directly query the specialized subject objects.

Later this would allow us to handle the specialized classes as plugins, e.g. Fedora instances wouldn't require to load RedHatModule (this is something we had in PDC as well).

That doesn't sound like hard-coding. Not sure what you mean by it. Can you explain?
You even have a variable with this: is_koji_build
From wikipedia: Hard coding is the software development practice of embedding data directly into the source code. It seems hard-coded to me.

At the moment you can have any kind of subject type. The problem is that we have some special cases that have to be handled in the code due to the fact that before the subject types were hard-coded and we had to keep backward compatibility.
I'd like to remove completely special cases, and not standardize them on a class and file.
For example, if the user uses "brew-build" instead of "koji_build" I think it's their own problem. Greenwave shouldn't care that brew == koji. This was done for backward compatibility
Maybe in next major version we should just announce this is not going to be supported any more and drop it.
Remote rules accepted subject types should be in a variable in the configuration.

OK, but this is not only about brew-build ~ koji_build (thought with this change it will be easier to remove it). This change replaces on various places: the subject item/type serialization, creating resultsdb query, getting product version (unfortunately, not completely) and getting the name to check blacklist/excluded_packages.

Remote rules accepted subject types should be in a variable in the configuration.

Hmm, maybe we can have everything in configuration, but not sure how simple it would be.

I feel like this could be carelessly used. Can we just look for the keys we care about?

for k in ('type', 'identifier'):
  v = data.get(k)
  if v:
    unpacked[k] = _unpack_value(v)

UPDATE: Disregard this comment. I see now that the whole point of this refactor is to isolate which keys/attributes we care about.

PR version 2 with subject type configuration in YAML: #493

Use doublequotes to avoid escapes

Would rather use comprehension here:

unpacked = {k: _unpack_value(v) for k, v in data.items()}
~

Would rather use comprehension here

Good idea. I'll fix it in the #493 and close this PR.

Pull-Request has been closed by lholecek

4 years ago

Use doublequotes to avoid escapes

This is just a moved code, but I'll change it as suggested.

Use doublequotes to avoid escapes

This is just a moved code, but I'll change it as suggested.

Actually, let's use "Could not" instead.