#428 Add support for on-demand policies
Merged 4 years ago by gnaponie. Opened 4 years ago by yashn.
yashn/greenwave on_demand_policy  into  master

file modified
+176 -2
@@ -97,7 +97,8 @@ 

  

  

  @pytest.mark.smoke

- def test_cannot_make_decision_without_decision_context(requests_session, greenwave_server):

+ def test_cannot_make_decision_without_decision_context_and_user_policies(

+         requests_session, greenwave_server):

      data = {

          'product_version': 'fedora-26',

          'subject_type': 'bodhi_update',
@@ -107,7 +108,7 @@ 

                                headers={'Content-Type': 'application/json'},

                                data=json.dumps(data))

      assert r.status_code == 400

-     assert 'Missing required decision context' == r.json()['message']

+     assert 'Either decision_context or rules is required.' == r.json()['message']

  

  

  @pytest.mark.smoke
@@ -1408,3 +1409,176 @@ 

      res_data = r.json()

  

      assert len(res_data['results']) == 2

+ 

+ 

+ @pytest.mark.smoke

+ def test_cannot_make_decision_with_both_decision_context_and_user_policies(

+         requests_session, greenwave_server):

+     data = {

+         'product_version': 'fedora-26',

+         'subject_type': 'bodhi_update',

+         'subject_identifier': 'FEDORA-2018-ec7cb4d5eb',

+         'decision_context': 'koji_build_push_missing_results',

+         'rules': [

+             {

+                 'type': 'PassingTestCaseRule',

+                 'test_case_name': 'osci.brew-build.rpmdeplint.functional'

+             },

+         ],

+     }

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

+                               headers={'Content-Type': 'application/json'},

+                               data=json.dumps(data))

+     assert r.status_code == 400

+     assert ('Cannot have both decision_context and rules') == r.json()['message']

+ 

+ 

+ @pytest.mark.smoke

+ def test_cannot_make_decision_without_required_rule_type(

+         requests_session, greenwave_server):

+     data = {

+         'product_version': 'fedora-26',

+         'subject_type': 'bodhi_update',

+         'subject_identifier': 'FEDORA-2018-ec7cb4d5eb',

+         'rules': [

+             {

+                 'typo': 'PassingTestCaseRule',

+                 'test_case_name': 'dist.abicheck'

+             },

+             {

+                 'type': 'PassingTestCaseRule',

+                 'test_case_name': 'dist.rpmdeplint'

+             },

+         ],

+     }

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

+                               headers={'Content-Type': 'application/json'},

+                               data=json.dumps(data))

+     assert r.status_code == 400

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

+ 

+ 

+ @pytest.mark.smoke

+ def test_cannot_make_decision_without_required_rule_testcase_name(

+         requests_session, greenwave_server):

+     data = {

+         'product_version': 'fedora-26',

+         'subject_type': 'bodhi_update',

+         'subject_identifier': 'FEDORA-2018-ec7cb4d5eb',

+         'rules': [

+             {

+                 'type': 'PassingTestCaseRule',

+                 'test_case_name': 'dist.abicheck'

+             },

+             {

+                 'type': 'PassingTestCaseRule'

+             },

+         ],

+     }

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

+                               headers={'Content-Type': 'application/json'},

+                               data=json.dumps(data))

+     assert r.status_code == 400

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

+ 

+ 

+ def test_make_a_decision_with_verbose_flag_on_demand_policy(

+         requests_session, greenwave_server, testdatabuilder):

+     nvr = testdatabuilder.unique_nvr()

+     results = []

+     expected_waivers = []

+     # First one failed but was waived

+     results.append(testdatabuilder.create_result(item=nvr,

+                                                  testcase_name=TASKTRON_RELEASE_CRITICAL_TASKS[0],

+                                                  outcome='FAILED'))

+     expected_waivers.append(

+         testdatabuilder.create_waiver(nvr=nvr,

+                                       product_version='fedora-31',

+                                       testcase_name=TASKTRON_RELEASE_CRITICAL_TASKS[0],

+                                       comment='This is fine'))

+     for testcase_name in TASKTRON_RELEASE_CRITICAL_TASKS[1:]:

+         results.append(testdatabuilder.create_result(item=nvr,

+                                                      testcase_name=testcase_name,

+                                                      outcome='PASSED'))

+ 

+     data = {

+         'product_version': 'fedora-31',

+         'subject_type': 'koji_build',

+         'subject_identifier': nvr,

+         'verbose': True,

+         'rules': [

+             {

+                 'type': 'PassingTestCaseRule',

+                 'test_case_name': 'dist.abicheck'

+             },

+             {

+                 'type': 'PassingTestCaseRule',

+                 'test_case_name': 'dist.rpmdeplint'

+             },

+             {

+                 'type': 'PassingTestCaseRule',

+                 'test_case_name': 'dist.upgradepath'

+             },

+         ],

+     }

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

+                               headers={'Content-Type': 'application/json'},

+                               data=json.dumps(data))

+     assert r.status_code == 200

+     res_data = r.json()

+ 

+     assert len(res_data['results']) == len(results)

+     assert res_data['results'] == list(reversed(results))

+     assert len(res_data['waivers']) == len(expected_waivers)

+     assert res_data['waivers'] == expected_waivers

+     assert len(res_data['satisfied_requirements']) == len(results)

+     assert len(res_data['unsatisfied_requirements']) == 0

+ 

+ 

+ def test_make_a_decision_on_demand_policy(

+         requests_session, greenwave_server, testdatabuilder):

+     nvr = testdatabuilder.unique_nvr()

+     results = []

+     expected_waivers = []

+     # First one failed but was waived

+     results.append(testdatabuilder.create_result(item=nvr,

+                                                  testcase_name=TASKTRON_RELEASE_CRITICAL_TASKS[0],

+                                                  outcome='FAILED'))

+     expected_waivers.append(

+         testdatabuilder.create_waiver(nvr=nvr,

+                                       product_version='fedora-31',

+                                       testcase_name=TASKTRON_RELEASE_CRITICAL_TASKS[0],

+                                       comment='This is fine'))

+     for testcase_name in TASKTRON_RELEASE_CRITICAL_TASKS[1:]:

+         results.append(testdatabuilder.create_result(item=nvr,

+                                                      testcase_name=testcase_name,

+                                                      outcome='PASSED'))

+ 

+     data = {

+         'id': 'on_demand',

+         'product_version': 'fedora-31',

+         'subject_type': 'koji_build',

+         'subject_identifier': nvr,

+         'rules': [

+             {

+                 'type': 'PassingTestCaseRule',

+                 'test_case_name': 'dist.abicheck'

+             },

+             {

+                 'type': 'PassingTestCaseRule',

+                 'test_case_name': 'dist.rpmdeplint'

+             },

+             {

+                 'type': 'PassingTestCaseRule',

+                 'test_case_name': 'dist.upgradepath'

+             },

+         ],

+     }

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

+                               headers={'Content-Type': 'application/json'},

+                               data=json.dumps(data))

+     assert r.status_code == 200

+     res_data = r.json()

+ 

+     assert len(res_data['satisfied_requirements']) == len(results)

+     assert len(res_data['unsatisfied_requirements']) == 0

file modified
+83 -11
@@ -8,6 +8,7 @@ 

  from greenwave import __version__

  from greenwave.policies import (summarize_answers,

                                  RemotePolicy,

+                                 OnDemandPolicy,

                                  _missing_decision_contexts_in_parent_policies)

  from greenwave.resources import ResultsRetriever, retrieve_waivers

  from greenwave.safe_yaml import SafeYAMLError
@@ -271,10 +272,59 @@ 

             ],

         }

  

+     **Sample On-demand policy request**:

+ 

+     Note: Greenwave would not publish a message on the message bus when an on-demand

+           policy request is received.

+ 

+     .. sourcecode:: http

+ 

+        POST /api/v1.0/decision HTTP/1.1

+        Accept: application/json

+        Content-Type: application/json

+ 

+        {

+            "subject_identifier": "cross-gcc-7.0.1-0.3.el8",

+            "verbose": false,

+            "subject_type": "koji_build",

+            "rules": [

+                {

+                    "type": "PassingTestCaseRule",

+                    "test_case_name": "fake.testcase.tier0.validation"

+                }

+            ],

+            "product_version": ["rhel-8"],

+            "excluded_packages": ["python2-*"]

+        }

+ 

+ 

+ 

+     **Sample On-demand policy response**:

+ 

+     .. sourcecode:: none

+ 

+        HTTP/1.0 200

+        Content-Length: 228

+        Content-Type: application/json

+ 

+        {

+            "policies_satisfied": True,

+            "satisfied_requirements": [

+                {

+                    "result_id": 7403736,

+                    "testcase": "fake.testcase.tier0.validation",

+                    "type": "test-result-passed"

+                 }

+            ],

+            "summary": "All required tests passed",

+            "unsatisfied_requirements": []

+        }

+ 

      :jsonparam string product_version: The product version string used for querying WaiverDB.

      :jsonparam string decision_context: The decision context string, identified by a

          free-form string label. It is to be named through coordination between policy

          author and calling application, for example ``bodhi_update_push_stable``.

+         Do not use this parameter with `rules`.

      :jsonparam string subject_type: The type of software artefact we are

          making a decision about, for example ``koji_build``.

          See :ref:`subject-types` for a list of possible subject types.
@@ -295,26 +345,44 @@ 

          the decision.

      :jsonparam string when: A date (or datetime) in ISO8601 format. Greenwave will

          take a decision considering only results and waivers from that point in time.

+     :jsonparam list rules: A list of dictionaries containing the 'type' and 'test_case_name'

+         of an individual rule used to specify on-demand policy.

+         For example, [{"type":"PassingTestCaseRule", "test_case_name":"dist.abicheck"},

+                       {"type":"RemoteRule"}]

+         Do not use this parameter along with `decision_context`.

      :statuscode 200: A decision was made.

      :statuscode 400: Invalid data was given.

      """  # noqa: E501

-     if request.get_json():

-         if ('product_version' not in request.get_json() or

-                 not request.get_json()['product_version']):

+     data = request.get_json()

+     if data:

+         if not data.get('product_version'):

              log.error('Missing required product version')

              raise BadRequest('Missing required product version')

-         if ('decision_context' not in request.get_json() or

-                 not request.get_json()['decision_context']):

-             log.error('Missing required decision context')

-             raise BadRequest('Missing required decision context')

+         if not data.get('decision_context') and not data.get('rules'):

+             log.error('Either decision_context or rules is required.')

+             raise BadRequest('Either decision_context or rules is required.')

      else:

          log.error('No JSON payload in request')

          raise UnsupportedMediaType('No JSON payload in request')

  

-     data = request.get_json()

      log.debug('New decision request for data: %s', data)

      product_version = data['product_version']

-     decision_context = data['decision_context']

+ 

+     decision_context = data.get('decision_context', None)

+     rules = data.get('rules', [])

+     if decision_context and rules:

+         log.error('Cannot have both decision_context and rules')

+         raise BadRequest('Cannot have both decision_context and rules')

+ 

+     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

+             on_demand_policy = OnDemandPolicy.create_from_json(request_data)

+             on_demand_policies.append(on_demand_policy)

+ 

      verbose = data.get('verbose', False)

      if not isinstance(verbose, bool):

          log.error('Invalid verbose flag, must be a bool')
@@ -341,9 +409,10 @@ 

          verify=current_app.config['REQUESTS_VERIFY'],

          url=current_app.config['RESULTSDB_API_URL'])

  

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

      for subject_type, subject_identifier in _decision_subjects_for_request(data):

          subject_policies = [

-             policy for policy in current_app.config['policies']

+             policy for policy in policies

              if policy.matches(

                  decision_context=decision_context,

                  product_version=product_version,
@@ -382,13 +451,16 @@ 

      response = {

          'policies_satisfied': all(answer.is_satisfied for answer in answers),

          'summary': summarize_answers(answers),

-         'applicable_policies': [policy.id for policy in applicable_policies],

          'satisfied_requirements':

              [answer.to_json() for answer in answers if answer.is_satisfied],

          'unsatisfied_requirements':

              [answer.to_json() for answer in answers if not answer.is_satisfied],

      }

  

+     # Check if on-demand policy was specified

+     if not rules:

+         response.update({'applicable_policies': [policy.id for policy in applicable_policies]})

+ 

      if verbose:

          # removing duplicated elements...

          response.update({

file modified
+80
@@ -6,6 +6,7 @@ 

  import os

  import re

  import greenwave.resources

+ from werkzeug.exceptions import BadRequest

  from flask import current_app

  

  from greenwave.safe_yaml import (
@@ -305,6 +306,36 @@ 

          """

          return True

  

+     @staticmethod

+     def process_on_demand_rules(rules):

+         """

+         Validates rules and creates objects for them.

+ 

+         Args:

+             rules (json): User specified rules

+ 

+         Returns:

+             list: Returns a list of appropriate objects

+         """

+         if not all([rule.get('type') for rule in rules]):

+             raise BadRequest('Key \'type\' is required for every rule')

+         if not all([rule.get('test_case_name') for rule in rules if rule['type'] != 'RemoteRule']):

+             raise BadRequest('Key \'test_case_name\' is required if not a RemoteRule')

+ 

+         processed_rules = []

+         for rule in rules:

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

+                 processed_rules.append(RemoteRule())

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

+                 temp_rule = PassingTestCaseRule()

+                 temp_rule.test_case_name = rule['test_case_name']  # pylint: disable=W0201

+                 temp_rule.scenario = rule.get('scenario')  # pylint: disable=W0201

+                 processed_rules.append(temp_rule)

+             else:

+                 raise BadRequest('Invalid rule type {}'.format(rule['type']))

+ 

+         return processed_rules

+ 

  

  def waives_invalid_gating_yaml(waiver, subject_type, subject_identifier):

      return (waiver['testcase'] == 'invalid-gating-yaml' and
@@ -334,6 +365,12 @@ 

              return []

  

          policies = RemotePolicy.safe_load_all(response)

+         if isinstance(policy, OnDemandPolicy):

+             return [

+                 sub_policy for sub_policy in policies

+                 if set(sub_policy.product_versions) == set(policy.product_versions)

+             ]

+ 

          return [

              sub_policy for sub_policy in policies

              if sub_policy.decision_context == policy.decision_context
@@ -592,6 +629,49 @@ 

          return 'Policy {!r}'.format(self.id or 'untitled')

  

  

+ 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

+ 

+     @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=W0212

+         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))

+ 

+ 

  class RemotePolicy(Policy):

      root_yaml_tag = '!Policy'

  

@@ -16,7 +16,8 @@ 

      TestResultMissing,

      TestResultFailed,

      TestResultPassed,

-     InvalidGatingYaml

+     InvalidGatingYaml,

+     OnDemandPolicy

  )

  from greenwave.resources import ResultsRetriever

  from greenwave.safe_yaml import SafeYAMLError
@@ -889,3 +890,60 @@ 

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

      assert len(decision) == 1

      assert isinstance(decision[0], RuleSatisfied)

+ 

+ 

+ @pytest.mark.parametrize('namespace', ["rpms", ""])

+ def test_remote_rule_policy_on_demand_policy(namespace):

+     """ 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'

+ 

+     serverside_json = {

+         'product_version': 'fedora-26',

+         'id': 'taskotron_release_critical_tasks_with_remoterule',

+         'subject_type': 'koji_build',

+         'subject_identifier': nvr,

+         'rules': [

+             {

+                 'type': 'RemoteRule'

+             },

+         ],

+     }

+ 

+     remote_fragment = dedent("""

+         --- !Policy

+         id: "some-policy-from-a-random-packager"

+         product_versions:

+           - fedora-26

+         decision_context: bodhi_update_push_stable_with_remoterule

+         rules:

+         - !PassingTestCaseRule {test_case_name: dist.upgradepath}

+         """)

+ 

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

+     with app.app_context():

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

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

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

+                 f.return_value = remote_fragment

+                 policy = OnDemandPolicy.create_from_json(serverside_json)  # pylint: disable=W0212

+                 waivers = []

+ 

+                 # Ensure that presence of a result is success.

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

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

+                 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, waivers)

+                 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, waivers)

+                 assert len(decision) == 1

+                 assert isinstance(decision[0], TestResultFailed)

This change is regarding enhancing the /decision endpoint API to allow a new parameter "rules" that will allow the user to pass some rules. These rules will be immediately processed by Greenwave that will, “on demand”, check the decision (as usually querying ResultsDB and WaiverDB) for those rules and return a response.

This feels awkward. It would be preferable to construct the PassingTestCaseRule object as a local variable first, and then append it to processed_rules once it's fully created.

Consider using isinstance(), to handle potential subclasses of OnDemandPolicy in the future.

This feels like it should be a @classmethod that returns a new OnDemandPolicy instance.

See other comment, it feels like _create_on_demand_policy() should be a @classmethod that returns the fully populated OnDemandPolicy instance.

Assuming that the request comes directly from the user input. If i would but True as the 'decision_context' value would not it brake the intended logic?

Same here as above for the 'rules'.

So i was looking how this method is used, and it is always used on the same place when the actual object is created. Can we use the contstructor instead? Or is there some other reason that i am not seeing?

Wouldn't it be better to have different types of tests (success, failure etc.) in different tests?

So i was looking how this method is used, and it is always used on the same place when the actual object is created. Can we use the contstructor instead? Or is there some other reason that i am not seeing?

As we discussed on the call, I tried it using in the constructor but the __new__ method has been overridden for parent classes and it breaks the current workflow if we pass a param in the constructor (as we would have to do in order to put the logic in the constructor).

Assuming that the request comes directly from the user input. If i would but True as the 'decision_context' value would not it brake the intended logic?

Do you mean when decision_context is also specified along with rules? That's being handled later in the method on line 36 in api_v1.py

This feels awkward. It would be preferable to construct the PassingTestCaseRule object as a local variable first, and then append it to processed_rules once it's fully created.

Agreed, but if there are multiple rules of the same object, assigning it to a local variable won't work as it would end up altering the same object in every iteration.

rebased onto 359b8e67edf6015343e7b0141867f697c0ca21b9

4 years ago

rebased onto 45272e94b7470d06b63d037bfd7c092b7db061e0

4 years ago

Could you simplify this if statement to:

req_json = requests.get_json()
if not req_json.get('decision_context') and not req_json.get('rules'):

How about?

log.error('Either decision_context or rules is required.')

Optional: I'd remove the 'Invalid request. part of the error message. The status code tells the user it's an invalid request.

rebased onto 1853a25857f42baae05b9b125a1cbddf7da07275

4 years ago

If you made this elif rule['type'] == 'PassingTestCaseRule':, then you could bring your raise BadRequest... line to an else. That might be a little cleaner.

Why are you filtering on product_versions instead of decision_context like it is below?

Maybe you should name it create_from_json or something similar?

So falsy values are allowed?

This feels awkward. I would assign PassingTestCaseRule() to a separate variable, set the properties, and then append it.

Why is this part of the response for requests with on-demand policies, but it doesn't seem to be for normal requests?

Why are you filtering on product_versions instead of decision_context like it is below?

Because the decision_context parameter will not be specified in an on-demand policy request. So , we would not be able to filter by that.

Why is this part of the response for requests with on-demand policies, but it doesn't seem to be for normal requests?

This part was mentioned in the design doc example. I don't know the reason why it is not present for normal requests already.

So falsy values are allowed?

Yes, for optional params like blacklist, excluded_packages, packages and relevance_key.

rebased onto 7cc363dc84105be563913afed1b1cbcca30a689f

4 years ago

Probably OK for now, but it won't scale well. Something involving Rule.__subclasses__ could work well but it will probably need some code changes to initialize and validate arguments for the rule.

Why is this part of the response for requests with on-demand policies, but it doesn't seem to be for normal requests?

This part was mentioned in the design doc example. I don't know the reason why it is not present for normal requests already.

@gnaponie or @lholecek, should this be added to normal request responses as well for consistency?

@mprahl @yashn I think is just an error in the document. Sorry about that. I don't think is needed. Can we remove it? I'll update the doc too.

Those information are usually in the message, but they are not needed in the API response. We can remove them.

this gives me flake8 error: functional-tests/test_api_v1.py|1482 col 54| W292 no newline at end of file

This gives me flake8 error: greenwave/api_v1.py|8 col 1| F401 'greenwave.policies.Rule' imported but unused

I get several pylint errors. Could you check them?


(env_resultsdb) ~/proj/greenwave (pr428)$ pylint --reports=n greenwave/
*** Module greenwave.policies
greenwave/policies.py:327:16: W0201: Attribute 'test_case_name' defined outside init (attribute-defined-outside-init)
greenwave/policies.py:328:16: W0201: Attribute 'scenario' defined outside init (attribute-defined-outside-init)
greenwave/policies.py:654:8: W0212: Access to a protected member __validate_attributes of a client class (protected-access)
** Module greenwave.api_v1
greenwave/api_v1.py:327:31: W0212: Access to a protected member _create_on_demand_policy of a client class (protected-access)
greenwave/api_v1.py:8:0: W0611: Unused Rule imported from greenwave.policies (unused-import)
**** Module greenwave.tests.test_policies
greenwave/tests/test_policies.py:929:25: W0212: Access to a protected member _create_on_demand_policy of a client class (protected-access)
greenwave/tests/test_policies.py:895:45: W0613: Unused argument 'tmpdir' (unused-argument)


Your code has been rated at 9.96/10 (previous run: 10.00/10, -0.04)

We could just remove in the response the "applicable_policies", since it doesn't make more sense to include them in this case.

If I make a request with parameter rules:
[{'typo': 'PassingTestCaseRule', 'test_case_name': 'something'}]
With 'typo' instead of 'type' I get a traceback. We should return some error like "type" key is required.

Same thing happens when I put "type": "PassingTestCaseRule" correctly, but then I put a typo in "test_case_name" key.

@yashn Great job!! This new feature is so cool I think none will ever use the old way anymore :D

We might want to make the decision_context in the gating.yaml file optional at this point... Otherwise it might be confusing for the user.. what should they put there? But... this would need some changes in the RemoteRule part... Other opinions? @yashn @mprahl @lucarval

Could you add a test to check if the user is putting the right keys (type and test_case_name) in the rules? Once you handle possible typos or absence of the keys.

rebased onto ad30d380cb73e82b275e80ab8cc627b97c8d3b55

4 years ago

We might want to make the decision_context in the gating.yaml file optional at this point... Otherwise it might be confusing for the user.. what should they put there? But... this would need some changes in the RemoteRule part... Other opinions? @yashn @mprahl @lucarval

I agree. We can do that. @gnaponie My only question is whether we can rely on product_version here instead of the decision_context for RemotePolicy as well? I am unsure if any complications can arise if we use product_version here.

@yashn Great job!! This new feature is so cool I think none will ever use the old way anymore :D

ahahaha, thanks for the review @gnaponie!

I have made the changes you asked. Could you please take a look again?

I agree. We can do that. @gnaponie My only question is whether we can rely on product_version here instead of the decision_context for RemotePolicy as well? I am unsure if any complications can arise if we use product_version here.

@yashn can you make an example of complications about the product_version? Nothing comes to my mind...

That's not true for "RemoteRule".
I should be able to make a request with rules like these:
{'type': 'PassingTestCaseRule', 'test_case_name': 'baseos-ci.redhat-module.tier0.functional'}, {'type': "RemoteRule"}

So we need the check on "type" always. And the check on "test_case_name" only for "PassingTestCaseRule".
Sorry, I should have been clearer in the beginning.

You might change it, but it gives me flake8 error:
functional-tests/test_api_v1.py|1401 col 101| E501 line too long (102 > 100 characters)

Beside those small comments it looks fine!
Then I think we can merge it and worry about the "decision_context" optional in the gating.yaml as future improvement. I don't think people will immediately start using the RemoteRule in this feature, and even if, it's not a bug, just a clearer thing.

Optional: Assigning this message to a variable would be nice so the string isn't duplicated

So we need the check on "type" always. And the check on "test_case_name" only for "PassingTestCaseRule".
Sorry, I should have been clearer in the beginning.

Ah, Sorry about that. I should have been more careful. :(

For compatibility, does it make sense to have applicable_policies default to an empty list? I'm not quite sure if that's an improvement over the implementation in this PR. Thoughts?

getattr(self, attribute) should be getattr(self, attribute, None). If you don't add the third argument, then an AttributeError exception is raised.

The rules parameter should be documented in the docstring, so that the API docs that get autogenerated include it.

@yashn after the comments are addressed, this is good to merge in my opinion. Nice job!

For compatibility, does it make sense to have applicable_policies default to an empty list? I'm not quite sure if that's an improvement over the implementation in this PR. Thoughts?

It already was defaulting to an empty list before. @gnaponie suggested we should remove it in this case. Hence, we only add the applicable_policies key if rules are not specified.

rebased onto 3f0f828d8921eeae0dbee210ac21abb280c4e165

4 years ago

The rules parameter should be documented in the docstring, so that the API docs that get autogenerated include it.

I am going to put that in a separate commit coming soon, after these changes are approved :- )

rebased onto e455e4ab22245173dbdbbe8047a29c9dd0b1e6a5

4 years ago

rebased onto d3083e9

4 years ago

1 new commit added

  • Add documentation for On-demand policy feature
4 years ago

Did you consider creating new endpoint? This could lead to less error prone and simpler API. Now there is more parameters which are optional or required depending on presence of other parameters.

@lholecek it sounds a valid alternative, but we discussed about it and we preferred just to keep the same endpoint.

Can we add in the documentation that if you use this feature Greenwave won't publish any message about it, but it's only a "on-demand" thing? (maybe a bit better rephrased).

Is the response in the example (in the doc) the result of a request with "verbose" == False? (Because there's no results or waivers...). If yes, I would also put verbose=false in the example request parameter. So that if someone just try to copy-paste the example doesn't get something different.

Beside those 2 minor comments on the doc, IMHO this PR is good to be merged.
Good job, Yash!

2 new commits added

  • Add documentation for On-demand policy feature
  • Add support for on-demand policies
4 years ago

Is the response in the example (in the doc) the result of a request with "verbose" == False? (Because there's no results or waivers...). If yes, I would also put verbose=false in the example request parameter. So that if someone just try to copy-paste the example doesn't get something different.

Aha, Yes, you are absolutely right. Changed it.

Can we add in the documentation that if you use this feature Greenwave won't publish any
message about it, but it's only a "on-demand" thing? (maybe a bit better rephrased).

Yes, nice idea. Added a note.

Beside those 2 minor comments on the doc, IMHO this PR is good to be merged.
Good job, Yash!

Thanks Giulia! I have made those changes you recommended above. :-)

UMB => message bus

UMB is Red Hat specific.

This indentation looks off.

Optional: I believe the term for a dictionary in JSON is an object, so it might be nice to use that terminology in the documentation.

containing => containing the

Optional: Also, I believe lists are called arrays in JSON.

A few minor documentation changes, but otherwise, +1. Nice job!

2 new commits added

  • Add documentation for On-demand policy feature
  • Add support for on-demand policies
4 years ago

Optional: Also, I believe lists are called arrays in JSON.

Optional: I believe the term for a dictionary in JSON is an object, so it might be nice to use that terminology in the documentation.

Docs for the rest of params use list so I think it would be nice to be consistent. Also, an object seems a little vague to me, I thought being specific will avoid confusion. :-)

Made rest of the changes. Thanks Matt!

Commit 4f2e79d fixes this pull-request

Pull-Request has been merged by gnaponie

4 years ago

Pull-Request has been merged by gnaponie

4 years ago