From 4ad3ea900b3a19c6922f10142d4c474fa558352f Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Aug 22 2017 01:08:21 +0000 Subject: Special relevance. I'm not totally happy with this approach. First, I'll describe the bug, then the solution, then what I'm not happy with. First, the bug is that when we changed the format of the `subject` to a list, we didn't think about how policies would map to subjects of different types in a single query. Bodhi would like to make a single query with all the artifacts it care about, and have greenwave do the heavy lifting. It will provide a query with a ``koji_build``, and a ``bodhi_update`` alias, and a new ``original_spec_nvr`` nvr from the atomic ci pipeline. The problem is that all of our policies apply indiscriminately to all of those subjects - which can't make sense. There is no `rpmlint` run on an "update". The approach taken here is to introduce new attributes to our policies that let them define what subjects they are relevant to. They can specify a relevance_key or a relevance_value which makes them only narrowly applicable to certain subjects. I don't like that we have to have relevance_key *and* relevance_value (in order to handle the strange looking ``original_spec_nvr``). I also don't like that "is relevance to" and "is applicable to" mean almost exactly the same thing in english, but here in greenwave they are defined at different scopes of the query. "applicable" applies to the query as a whole while "relevant" applies to each item in the query's subject. Perhaps this can be rethought and thrown out.. but I wanted to post something to have something to argue about. I'll also apply this to the greenwave "stg" instance so we can poke at it there. Lastly, this is on top of the `disjunction`, branch from #62. --- diff --git a/conf/policies/fedora.yaml b/conf/policies/fedora.yaml index 59309eb..ce25a44 100644 --- a/conf/policies/fedora.yaml +++ b/conf/policies/fedora.yaml @@ -5,6 +5,8 @@ id: "taskotron_release_critical_tasks" product_versions: - fedora-26 decision_context: bodhi_update_push_stable +# This policy is applicable to fedora-26, but is only relevant to koji_builds +relevance_value: koji_build rules: - !PassingTestCaseRule {test_case_name: dist.abicheck} - !PassingTestCaseRule {test_case_name: dist.rpmdeplint} @@ -16,5 +18,7 @@ id: "atomic_ci_pipeline_results" product_versions: - fedora-26 decision_context: bodhi_update_push_stable +# This policy is applicable to fedora-26, but is only relevant to original_spec_nvrs. +relevance_key: original_spec_nvr rules: - !Any { test_case_names: [org.centos.prod.ci.pipeline.package.complete, org.centos.prod.ci.pipeline.package.ignored] } diff --git a/functional-tests/test_api_v1.py b/functional-tests/test_api_v1.py index 5cbec4f..e008d17 100644 --- a/functional-tests/test_api_v1.py +++ b/functional-tests/test_api_v1.py @@ -354,7 +354,7 @@ def test_multiple_results_in_a_subject( 'taskotron_release_critical_tasks', 'atomic_ci_pipeline_results', ] - assert res_data['summary'] == '1 of 4 required tests failed' + assert res_data['summary'] == '1 of 3 required tests failed' expected_unsatisfied_requirements = [ { 'item': {'item': nvr, 'type': 'koji_build'}, @@ -381,7 +381,50 @@ def test_failing_any_rule( data = { 'decision_context': 'bodhi_update_push_stable', 'product_version': 'fedora-26', - 'subject': [{'item': nvr, 'type': 'koji_build'}] + 'subject': [{'original_spec_nvr': nvr}], + } + r = requests_session.post(greenwave_server.url + 'api/v1.0/decision', + headers={'Content-Type': 'application/json'}, + data=json.dumps(data)) + assert r.status_code == 200 + res_data = r.json() + # The failed result should be taken into account. + assert res_data['policies_satisified'] is False + assert res_data['applicable_policies'] == [ + 'taskotron_release_critical_tasks', + 'atomic_ci_pipeline_results', + ] + assert res_data['summary'] == 'no test results found' + expected_unsatisfied_requirements = [ + { + 'item': {'original_spec_nvr': nvr}, + 'testcase': 'org.centos.prod.ci.pipeline.package.complete', + 'type': 'test-result-missing' + }, + ] + assert res_data['unsatisfied_requirements'] == expected_unsatisfied_requirements + + +def test_combining_types_in_a_query( + requests_session, greenwave_server, testdatabuilder): + """ + This make sure that Greenwave returns a meaningful response when more than + one type of item is specified in a request. + """ + nvr = testdatabuilder.unique_nvr() + # All passing here.. but no atomic_ci results. + for testcase_name in TASKTRON_RELEASE_CRITICAL_TASKS: + testdatabuilder.create_result(item=nvr, + testcase_name=testcase_name, + outcome='PASSED') + data = { + 'decision_context': 'bodhi_update_push_stable', + 'product_version': 'fedora-26', + 'subject': [ + {'item': nvr, 'type': 'koji_build'}, + {'original_spec_nvr': nvr}, + {'item': 'FEDORA-2017-ABCDEFG', 'type': 'bodhi_update'}, + ] } r = requests_session.post(greenwave_server.url + 'api/v1.0/decision', headers={'Content-Type': 'application/json'}, @@ -397,7 +440,7 @@ def test_failing_any_rule( assert res_data['summary'] == '1 of 4 required tests not found' expected_unsatisfied_requirements = [ { - 'item': {'item': nvr, 'type': 'koji_build'}, + 'item': {'original_spec_nvr': nvr}, 'testcase': 'org.centos.prod.ci.pipeline.package.complete', 'type': 'test-result-missing' }, diff --git a/greenwave/api_v1.py b/greenwave/api_v1.py index 650bf17..e8fd71d 100644 --- a/greenwave/api_v1.py +++ b/greenwave/api_v1.py @@ -121,7 +121,9 @@ def make_decision(): waivers = response.json()['data'] else: waivers = [] - for policy in applicable_policies: + relevant_policies = [policy for policy in applicable_policies + if policy.is_relevant_to(item)] + for policy in relevant_policies: answers.extend(policy.check(item, results, waivers)) res = { 'policies_satisified': all(answer.is_satisfied for answer in answers), diff --git a/greenwave/policies.py b/greenwave/policies.py index 55bc4ea..46cf797 100644 --- a/greenwave/policies.py +++ b/greenwave/policies.py @@ -204,10 +204,23 @@ class Policy(yaml.YAMLObject): return (decision_context == self.decision_context and product_version in self.product_versions) + def is_relevant_to(self, item): + relevance_key = getattr(self, 'relevance_key', None) + relevance_value = getattr(self, 'relevance_value', None) + if relevance_key and relevance_value: + return item.get(relevance_key) == relevance_value + if relevance_key: + return relevance_key in item + if relevance_value: + return relevance_value in item.values() + return True + def check(self, item, results, waivers): return [rule.check(item, results, waivers) for rule in self.rules] def __repr__(self): - return "%s(id=%r, product_versions=%r, decision_context=%r, rules=%r)" % ( - self.__class__.__name__, self.id, self.product_versions, self.decision_context, - self.rules) + return ("%s(id=%r, product_versions=%r, decision_context=%r, rules=%r," + " relevance_key=%r relevance_value=%r)" % ( + self.__class__.__name__, self.id, self.product_versions, + self.decision_context, self.rules, + self.relevance_key, self.relevance_value))