From 47e60d8e5e288eb10c6fb956fb70a87820e5793f Mon Sep 17 00:00:00 2001 From: Dan Callaghan Date: Jul 14 2017 05:37:33 +0000 Subject: replace Answer types with exceptions --- diff --git a/greenwave/api_v1.py b/greenwave/api_v1.py index f04d342..3ffc90f 100644 --- a/greenwave/api_v1.py +++ b/greenwave/api_v1.py @@ -3,7 +3,7 @@ import requests from flask import Blueprint, request, current_app, jsonify from werkzeug.exceptions import BadRequest, NotFound, UnsupportedMediaType -from greenwave.policies import summarize_answers +from greenwave.policies import summarize api = (Blueprint('api_v1', __name__)) @@ -45,7 +45,7 @@ def make_decision(): if not applicable_policies: raise NotFound('Cannot find any applicable policies for %s' % product_version) subjects = [item.strip() for item in request.get_json()['subject'] if item] - answers = [] + unsatisfied_requirements = [] timeout = current_app.config['REQUESTS_TIMEOUT'] for item in subjects: # XXX make this more efficient than just fetching everything @@ -65,12 +65,12 @@ def make_decision(): else: waivers = [] for policy in applicable_policies: - answers.extend(policy.check(item, results, waivers)) + unsatisfied_requirements.extend(policy.check(item, results, waivers)) res = { - 'policies_satisified': all(answer.is_satisfied for answer in answers), - 'summary': summarize_answers(answers), + 'policies_satisified': len(unsatisfied_requirements) == 0, + 'summary': summarize(subjects, applicable_policies, unsatisfied_requirements), 'applicable_policies': [policy.id for policy in applicable_policies], - 'unsatisfied_requirements': [answer.to_json() for answer in answers - if not answer.is_satisfied], + 'unsatisfied_requirements': [unsatisfied_requirement.to_json() + for unsatisfied_requirement in unsatisfied_requirements], } return jsonify(res), 200 diff --git a/greenwave/policies.py b/greenwave/policies.py index 129ee12..e96b870 100644 --- a/greenwave/policies.py +++ b/greenwave/policies.py @@ -3,37 +3,14 @@ import yaml -class Answer(object): - """ - Represents the result of evaluating a policy rule against a particular - item. But we call it an "answer" because the word "result" is a bit - overloaded in here. :-) - - This base class is not used directly -- each answer is an instance of - a subclass, depending on what the answer was. - """ - - pass - - -class RuleSatisfied(Answer): - """ - The rule's requirements are satisfied for this item. - """ - - is_satisfied = True - - -class RuleNotSatisfied(Answer): +class RuleNotSatisfied(Exception): """ The rule's requirements are not satisfied for this item. - Not used directly -- the answer is an instance of a subclass, specifying - exactly what was not satisfied. + Not raised directly -- one of the more specific subclasses will be raised + instead, specifying exactly what was not satisfied. """ - is_satisfied = False - def to_json(self): """ Returns a machine-readable description of the problem for API responses. @@ -79,36 +56,40 @@ class TestResultFailed(RuleNotSatisfied): } -def summarize_answers(answers): +def summarize(subjects, applicable_policies, unsatisfied_requirements): """ Produces a one-sentence human-readable summary of the result of evaluating a policy. Args: - answers (list): List of :py:class:`Answers ` from evaluating a policy. + subjects (list): List of items we are evaluating the policies for. + applicable_policies (list): List of policies which were evaluated. + unsatisfied_requirements (list): List of :py:class:`RuleNotSatisfied` exception + instances from evaluating a policy. Returns: str: Human-readable summary. """ - if len(answers) == 0: + rules_count = len(subjects) * sum(len(policy.rules) for policy in applicable_policies) + if rules_count == 0: return 'no tests are required' - if all(answer.is_satisfied for answer in answers): + if not unsatisfied_requirements: return 'all required tests passed' - failure_count = len([answer for answer in answers if isinstance(answer, TestResultFailed)]) + failure_count = len([e for e in unsatisfied_requirements if isinstance(e, TestResultFailed)]) if failure_count: - return ('{} of {} required tests failed'.format(failure_count, len(answers))) - missing_count = len([answer for answer in answers if isinstance(answer, TestResultMissing)]) - if missing_count == len(answers): + return ('{} of {} required tests failed'.format(failure_count, rules_count)) + missing_count = len([e for e in unsatisfied_requirements if isinstance(e, TestResultMissing)]) + if missing_count == rules_count: return 'no test results found' elif missing_count: - return '{} of {} required tests not found'.format(missing_count, len(answers)) + return '{} of {} required tests not found'.format(missing_count, rules_count) return 'inexplicable result' class Rule(yaml.YAMLObject): """ An individual rule within a policy. A policy consists of multiple rules. - When the policy is evaluated, each rule returns an answer - (instance of :py:class:`Answer`). + When the policy is evaluated, each rule raises an instance of + :py:class:`RuleNotSatisfied` if it is not satisfied. This base class is not used directly. """ @@ -121,9 +102,6 @@ class Rule(yaml.YAMLObject): for example a build NVR). results (list): List of result objects looked up in ResultsDB for this item. waivers (list): List of waiver objects looked up in WaiverDB for the results. - - Returns: - Answer: An instance of a subclass of :py:class:`Answer` describing the result. """ raise NotImplementedError() @@ -141,15 +119,15 @@ class PassingTestCaseRule(Rule): def check(self, item, results, waivers): matching_results = [r for r in results if r['testcase']['name'] == self.test_case_name] if not matching_results: - return TestResultMissing(item, self.test_case_name) + raise TestResultMissing(item, self.test_case_name) # XXX need to handle multiple results (take the latest) matching_result = matching_results[0] if matching_result['outcome'] in ['PASSED', 'INFO']: - return RuleSatisfied() + return # XXX limit who is allowed to waive if any(w['result_id'] == matching_result['id'] and w['waived'] for w in waivers): - return RuleSatisfied() - return TestResultFailed(item, self.test_case_name, matching_result['id']) + return + raise TestResultFailed(item, self.test_case_name, matching_result['id']) def __repr__(self): return "%s(test_case_name=%r)" % (self.__class__.__name__, self.test_case_name) @@ -169,7 +147,26 @@ class Policy(yaml.YAMLObject): product_version in self.product_versions) def check(self, item, results, waivers): - return [rule.check(item, results, waivers) for rule in self.rules] + """ + Evaluate this policy for the given item. + + Args: + item (str): The item we are evaluating ('item' key in ResultsDB, + for example a build NVR). + results (list): List of result objects looked up in ResultsDB for this item. + waivers (list): List of waiver objects looked up in WaiverDB for the results. + + Returns: + list: list of :py:class:`RuleNotSatisfied` instances raised while evaluating the + rules in this policy. + """ + result = [] + for rule in self.rules: + try: + rule.check(item, results, waivers) + except RuleNotSatisfied as e: + result.append(e) + return result def __repr__(self): return "%s(id=%r, product_versions=%r, decision_context=%r, rules=%r)" % ( diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py index afafbbd..a1a130f 100644 --- a/greenwave/tests/test_policies.py +++ b/greenwave/tests/test_policies.py @@ -2,20 +2,26 @@ # SPDX-License-Identifier: GPL-2.0+ from greenwave.app_factory import create_app -from greenwave.policies import summarize_answers, RuleSatisfied, TestResultMissing, TestResultFailed +from greenwave.policies import summarize, Policy, PassingTestCaseRule, \ + TestResultMissing, TestResultFailed -def test_summarize_answers(): - assert summarize_answers([RuleSatisfied()]) == \ +def test_summarize(): + subjects = ['item'] + policy = Policy('', [], '', [PassingTestCaseRule('test'), PassingTestCaseRule('test2')]) + assert summarize(subjects, [policy], []) == \ 'all required tests passed' - assert summarize_answers([TestResultFailed('item', 'test', 'id'), RuleSatisfied()]) == \ + assert summarize(subjects, [policy], [TestResultFailed('item', 'test', 'id')]) == \ '1 of 2 required tests failed' - assert summarize_answers([TestResultMissing('item', 'test')]) == \ + assert summarize(subjects, [policy], + [TestResultMissing('item', 'test'), + TestResultMissing('item', 'test2')]) == \ 'no test results found' - assert summarize_answers([TestResultMissing('item', 'test'), - TestResultFailed('item', 'test', 'id')]) == \ + assert summarize(subjects, [policy], + [TestResultMissing('item', 'test'), + TestResultFailed('item', 'test2', 'id')]) == \ '1 of 2 required tests failed' - assert summarize_answers([TestResultMissing('item', 'test'), RuleSatisfied()]) == \ + assert summarize(subjects, [policy], [TestResultMissing('item', 'test')]) == \ '1 of 2 required tests not found'