greenwave

Clone

#529 Fix loading RemoteRule attribute in on-demand rules
Merged 4 years ago by lholecek. Opened 4 years ago by lholecek.
lholecek/greenwave fix-on-demand-rules  into  master

file modified
+8 -2 
@@ -1387,7 +1387,10 @@ 
 

      }
 

      r = requests_session.post(greenwave_server + 'api/v1.0/decision', json=data)
 

      assert r.status_code == 400
 

-     assert ('Key \'type\' is required for every rule') == r.json()['message']
 

+     assert (
 

+         "Failed to parse on demand policy: Attribute 'rules': "
 

+         "Key 'type' is required for each list item"
 

+     ) == r.json()['message']
 

  

  

  @pytest.mark.smoke
 
@@ -1409,7 +1412,10 @@ 
 

      }
 

      r = requests_session.post(greenwave_server + 'api/v1.0/decision', json=data)
 

      assert r.status_code == 400
 

-     assert ('Key \'test_case_name\' is required if not a RemoteRule') == r.json()['message']
 

+     assert (
 

+         "Failed to parse on demand policy: Attribute 'rules': "
 

+         "Attribute 'test_case_name' is required"
 

+     ) == r.json()['message']
 

  

  

  def test_make_a_decision_with_verbose_flag_on_demand_policy(

file modified
+25 -41 
@@ -413,7 +413,9 @@ 
 

          processed_rules = []
 

          for rule in rules:
 

              if rule['type'] == 'RemoteRule':
 

-                 processed_rules.append(RemoteRule())
 

+                 temp_rule = RemoteRule()
 

+                 temp_rule.required = rule.get('required', False)
 

+                 processed_rules.append(temp_rule)
 

              elif rule['type'] == 'PassingTestCaseRule':
 

                  temp_rule = PassingTestCaseRule()
 

                  temp_rule.test_case_name = rule['test_case_name']
 
@@ -665,12 +667,15 @@ 
 

          if product_version and not self.matches_product_version(product_version):
 

              return False
 

  

-         subject = attributes.get('subject')
 

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

+         if not self.matches_subject_type(**attributes):
 

              return False
 

  

          return not self.rules or any(rule.matches(self, **attributes) for rule in self.rules)
 

  

+     def matches_subject_type(self, **attributes):
 

+         subject = attributes.get('subject')
 

+         return not subject or subject.type == self.subject_type
 

+ 

      @remove_duplicates
 

      def check(
 

              self,
 
@@ -711,46 +716,25 @@ 
 

  

  

  class OnDemandPolicy(Policy):
 

-     root_yaml_tag = '!Policy'
 

-     safe_yaml_attributes = {}
 

- 

-     def __init__(self):
 

-         self.id = None
 

-         self.product_versions = None
 

-         self.subject_type = None
 

-         self.rules = None
 

-         self.blacklist = None
 

-         self.excluded_packages = None
 

-         self.packages = None
 

-         self.relevance_key = None
 

+     root_yaml_tag = None
 

  

      @classmethod
 

-     def create_from_json(cls, data_dict):
 

-         policy = cls()
 

-         policy.id = data_dict.get('id')
 

-         policy.product_versions = [data_dict['product_version']]
 

-         policy.subject_type = data_dict['subject_type']
 

-         policy.rules = Rule.process_on_demand_rules(data_dict['rules'])
 

-         policy.blacklist = data_dict.get('blacklist', [])
 

-         policy.excluded_packages = data_dict.get('excluded_packages', [])
 

-         policy.packages = data_dict.get('packages', [])
 

-         policy.relevance_key = data_dict.get('relevance_key')
 

- 

-         # Validate the data before processing.
 

-         policy.__validate_attributes()  # pylint: disable=protected-access
 

-         return policy
 

- 

-     def __validate_attributes(self):
 

-         """ Validates types of the attributes. """
 

-         list_attributes = ['product_versions', 'rules', 'excluded_packages', 'packages']
 

-         for attribute in self.__dict__.keys():
 

-             if attribute in list_attributes and not isinstance(
 

-                     getattr(self, attribute, None), list):
 

-                 raise TypeError('{} should be a list.'.format(attribute))
 

-             elif attribute not in list_attributes:
 

-                 if getattr(self, attribute, None) and not isinstance(
 

-                         getattr(self, attribute, None), str):
 

-                     raise TypeError('{} should be a string.'.format(attribute))
 

+     def create_from_json(cls, data):
 

+         try:
 

+             data2 = {
 

+                 'id': 'on-demand-policy',
 

+                 'product_versions': [data['product_version']],
 

+                 'decision_context': 'on-demand-policy',
 

+                 'subject_type': 'unused',
 

+             }
 

+             data2.update(data)
 

+             result = cls.from_value(data2)
 

+             return result
 

+         except SafeYAMLError as e:
 

+             raise BadRequest('Failed to parse on demand policy: {}'.format(e))
 

+ 

+     def matches_subject_type(self, **attributes):
 

+         return True
 

  

  

  class RemotePolicy(Policy):

file modified
+70 -6 
@@ -4,6 +4,8 @@ 
 

  """
 

  import yaml
 

  

+ safe_yaml_tag_to_class = {}
 

+ 

  

  class SafeYAMLError(RuntimeError):
 

      """
 
@@ -22,6 +24,9 @@ 
 

      def from_yaml(self, loader, node):
 

          raise NotImplementedError()
 

  

+     def from_value(self, value):
 

+         raise NotImplementedError()
 

+ 

      def to_json(self, value):
 

          raise NotImplementedError()
 

  
@@ -39,6 +44,9 @@ 
 

  

      def from_yaml(self, loader, node):
 

          value = loader.construct_mapping(node)
 

+         return self.from_value(value)
 

+ 

+     def from_value(self, value):
 

          if isinstance(value, dict):
 

              return value
 

  
@@ -63,6 +71,9 @@ 
 

      def from_yaml(self, loader, node):
 

          value = loader.construct_scalar(node)
 

          value = yaml.safe_load(value)
 

+         return self.from_value(value)
 

+ 

+     def from_value(self, value):
 

          if isinstance(value, bool):
 

              return value
 

  
@@ -86,6 +97,9 @@ 
 

  

      def from_yaml(self, loader, node):
 

          value = loader.construct_scalar(node)
 

+         return self.from_value(value)
 

+ 

+     def from_value(self, value):
 

          return str(value)
 

  

      def to_json(self, value):
 
@@ -109,6 +123,29 @@ 
 

  

      def from_yaml(self, loader, node):
 

          values = loader.construct_sequence(node)
 

+         return self._from_value(values)
 

+ 

+     def from_value(self, value):
 

+         results = []
 

+         for item in value:
 

+             if not isinstance(item, dict):
 

+                 results.append(item)
 

+                 continue
 

+ 

+             item_type = item.get('type')
 

+             if not item_type:
 

+                 raise SafeYAMLError("Key 'type' is required for each list item")
 

+ 

+             cls = safe_yaml_tag_to_class.get(item_type)
 

+             if cls is None:
 

+                 raise SafeYAMLError(
 

+                     "Key 'type' for an list item is not valid: {}".format(item_type))
 

+ 

+             results.append(cls.from_value(item))
 

+ 

+         return self._from_value(results)
 

+ 

+     def _from_value(self, values):
 

          for value in values:
 

              if not isinstance(value, self.item_type):
 

                  raise SafeYAMLError(
 
@@ -137,23 +174,28 @@ 
 

      def __init__(cls, name, bases, kwds):
 

          super().__init__(name, bases, kwds)
 

  

-         tag = getattr(cls, 'root_yaml_tag', None)
 

-         if tag:
 

+         root_yaml_tag = getattr(cls, 'root_yaml_tag', None)
 

+         if root_yaml_tag:
 

              class Loader(cls.yaml_loader):
 

                  def get_node(self):
 

                      node = super().get_node()
 

  

-                     if node.tag != tag:
 

-                         raise SafeYAMLError('Missing {} tag'.format(tag))
 

+                     if node.tag != root_yaml_tag:
 

+                         raise SafeYAMLError('Missing {} tag'.format(root_yaml_tag))
 

  

                      if not isinstance(node, yaml.MappingNode):
 

-                         raise SafeYAMLError('Expected mapping for {} tagged object'.format(tag))
 

+                         raise SafeYAMLError(
 

+                             'Expected mapping for {} tagged object'.format(root_yaml_tag))
 

  

                      return node
 

  

-             Loader.add_constructor(tag, cls.from_yaml)
 

+             Loader.add_constructor(root_yaml_tag, cls.from_yaml)
 

              cls.yaml_loader = Loader
 

  

+         yaml_tag = getattr(cls, 'yaml_tag', None)
 

+         if yaml_tag:
 

+             safe_yaml_tag_to_class[yaml_tag.lstrip('!')] = cls
 

+ 

  

  class SafeYAMLObject(yaml.YAMLObject, metaclass=SafeYAMLObjectMetaclass):
 

      """
 
@@ -228,6 +270,28 @@ 
 

  

          return values
 

  

+     @classmethod
 

+     def from_value(cls, data):
 

+         result = cls()
 

+ 

+         for attribute_name, yaml_attribute in cls.safe_yaml_attributes.items():
 

+             value = data.get(attribute_name)
 

+             if value is None:
 

+                 if not yaml_attribute.optional:
 

+                     msg = 'Attribute {!r} is required'.format(attribute_name)
 

+                     raise SafeYAMLError(msg)
 

+                 value = yaml_attribute.default_value
 

+             else:
 

+                 try:
 

+                     value = yaml_attribute.from_value(value)
 

+                 except SafeYAMLError as e:
 

+                     msg = 'Attribute {!r}: {}'.format(attribute_name, str(e))
 

+                     raise SafeYAMLError(msg)
 

+ 

+             setattr(result, attribute_name, value)
 

+ 

+         return result
 

+ 

      @property
 

      def safe_yaml_label(self):
 

          return 'YAML object {}'.format(self.yaml_tag)

file modified
+40 
@@ -12,6 +12,7 @@ 
 

      summarize_answers,
 

      Policy,
 

      RemotePolicy,
 

+     RemoteRule,
 

      RuleSatisfied,
 

      TestResultMissing,
 

      TestResultFailed,
 
@@ -1074,6 +1075,45 @@ 
 

                  assert isinstance(decision[0], RuleSatisfied)
 

  

  

+ def test_remote_rule_policy_on_demand_policy_required():
 

+     """ Testing the RemoteRule with the koji interaction when on_demand policy is given.
 

+     In this case we are just mocking koji """
 

+ 

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

+     subject = create_test_subject('koji_build', nvr)
 

+ 

+     serverside_json = {
 

+         'product_version': 'fedora-26',
 

+         'id': 'taskotron_release_critical_tasks_with_remoterule',
 

+         'subject': [{'item': nvr, 'type': 'koji_build'}],
 

+         'rules': [
 

+             {
 

+                 'type': 'RemoteRule',
 

+                 'required': True
 

+             },
 

+         ],
 

+     }
 

+ 

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

+     with app.app_context():
 

+         with mock.patch('greenwave.resources.retrieve_scm_from_koji') as scm:
 

+             scm.return_value = ('rpms', 'nethack', 'c3c47a08a66451cb9686c49f040776ed35a0d1bb')
 

+             with mock.patch('greenwave.resources.retrieve_yaml_remote_rule') as f:
 

+                 f.return_value = None
 

+ 

+                 policy = OnDemandPolicy.create_from_json(serverside_json)
 

+                 assert len(policy.rules) == 1
 

+                 assert isinstance(policy.rules[0], RemoteRule)
 

+                 assert policy.rules[0].required
 

+ 

+                 results = DummyResultsRetriever()
 

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

+                 assert len(decision) == 1
 

+                 assert isinstance(decision[0], MissingGatingYaml)
 

+                 assert not decision[0].is_satisfied
 

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

+ 

+ 

  def test_two_rules_no_duplicate(tmpdir):
 

      nvr = 'nethack-1.2.3-1.el9000'
 

      subject = create_test_subject('koji_build', nvr)

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

The way the on-demand policy loading is implemented is bad. The code needs to be updated if we add a new attribute to existing rule or policy class, or add a new rule -- and there is no code that checks consistency or if the loaded attributes are valid.

1 new commit added

  • Make on-demand policies loading more generic
4 years ago

2 new commits added

  • Make on-demand policies loading more generic
  • Fix loading RemoteRule attribute in on-demand rules
4 years ago

I've changed to code to be more generic (in the second commit).

could you please rename this var, so there's no trailing underscore

+1 after fixing minor comment above

could you please rename this var, so there's no trailing underscore

type is function in Python. Adding underscore suffix is a standard way to avoid using conflicting names (type_ is used on couple of other places in code already).

type is function in Python. Adding underscore suffix is a standard way to avoid using conflicting names (type_ is used on couple of other places in code already).

I know that. I mean that you can rename it to something more meaningful and more descriptive like xyz_type

rebased onto 069c5c7
4 years ago

type is function in Python. Adding underscore suffix is a standard way to avoid using conflicting names (type_ is used on couple of other places in code already).

I know that. I mean that you can rename it to something more meaningful and more descriptive like xyz_type

Renamed to item_type.

Pull-Request has been merged by lholecek
4 years ago
Metadata
None
No Tags
Changes Summary 4
+8 -2
file changed
functional-tests/test_api_v1.py
+25 -41
file changed
greenwave/policies.py
+70 -6
file changed
greenwave/safe_yaml.py
+40 -0
file changed
greenwave/tests/test_policies.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.