PR#444: Retrieve waivers only when needed - greenwave

greenwave

#444 Retrieve waivers only when needed

Merged 4 years ago by lholecek. Opened 4 years ago by lholecek.

lholecek/greenwave retrieve-waivers-when-needed into master

Lukas Holecek • 4 years ago

aa26996

greenwave/api_v1.py

file modified

+40 -16

		`@@ -10,9 +10,10 @@`
		`RemotePolicy,`
		`OnDemandPolicy,`
		`_missing_decision_contexts_in_parent_policies)`
		`- from greenwave.resources import ResultsRetriever, retrieve_waivers`
		`+ from greenwave.resources import ResultsRetriever, WaiversRetriever`
		`from greenwave.safe_yaml import SafeYAMLError`
		`from greenwave.utils import insert_headers, jsonp`
		`+ from greenwave.waivers import waive_answers`
		`from greenwave.monitor import (`
		`registry,`
		`decision_exception_counter,`
		`@@ -396,14 +397,20 @@`

		`answers = []`
		`verbose_results = []`
		`- verbose_waivers = []`
		`applicable_policies = []`
		`- results_retriever = ResultsRetriever(`
		`- ignore_results=ignore_results,`
		`+ retriever_args = dict(`
		`when=when,`
		`timeout=current_app.config['REQUESTS_TIMEOUT'],`
		`- verify=current_app.config['REQUESTS_VERIFY'],`
		`- url=current_app.config['RESULTSDB_API_URL'])`
		`+ verify=current_app.config['REQUESTS_VERIFY'])`
		`+ results_retriever = ResultsRetriever(`
		`+ ignore_ids=ignore_results,`
		`+ url=current_app.config['RESULTSDB_API_URL'],`
		`+ **retriever_args)`
		`+ waivers_retriever = WaiversRetriever(`
		`+ ignore_ids=ignore_waivers,`
		`+ url=current_app.config['WAIVERDB_API_URL'],`
		`+ **retriever_args)`
		`+ waiver_filters = []`

		`policies = on_demand_policies or current_app.config['policies']`
		`for subject_type, subject_identifier in _decision_subjects_for_request(data):`
		`@@ -427,22 +434,40 @@`
		`'Cannot find any applicable policies for %s subjects at gating point %s in %s' % (`
		`subject_type, decision_context, product_version))`

		`- waivers = retrieve_waivers(`
		`- product_version, subject_type, [subject_identifier], when)`
		`- if ignore_waivers:`
		`- waivers = [w for w in waivers if w['id'] not in ignore_waivers]`
		`-`
		`for policy in subject_policies:`
		`answers.extend(`
		`- policy.check(product_version, subject_identifier, results_retriever, waivers))`
		`+ policy.check(`
		`+ product_version,`
		`+ subject_identifier,`
		`+ results_retriever))`

		`applicable_policies.extend(subject_policies)`

		`if verbose:`
		`- # Retrieve test results for all items when verbose output is requested.`
		`+ # Retrieve test results and waivers for all items when verbose output is requested.`
		`verbose_results.extend(`
		`results_retriever.retrieve(subject_type, subject_identifier))`
		`- verbose_waivers.extend(waivers)`
		`+ waiver_filters.append(dict(`
		`+ subject_type=subject_type,`
		`+ subject_identifier=subject_identifier,`
		`+ product_version=product_version,`
		`+ ))`
		`+`
		`+ if not verbose:`
		`+ for answer in answers:`
		`+ if not answer.is_satisfied:`
		`+ waiver_filters.append(dict(`
		`+ subject_type=answer.subject_type,`
		`+ subject_identifier=answer.subject_identifier,`
		`+ product_version=product_version,`
		`+ testcase=answer.test_case_name,`
		`+ ))`
		`+`
		`+ if waiver_filters:`
		`+ waivers = waivers_retriever.retrieve(waiver_filters)`
		`+ else:`
		`+ waivers = []`
		`+ answers = waive_answers(answers, waivers)`

		`response = {`
		`'policies_satisfied': all(answer.is_satisfied for answer in answers),`
		`@@ -458,10 +483,9 @@`
		`response.update({'applicable_policies': [policy.id for policy in applicable_policies]})`

		`if verbose:`
		`- # removing duplicated elements...`
		`response.update({`
		`'results': list({result['id']: result for result in verbose_results}.values()),`
		`- 'waivers': list({waiver['id']: waiver for waiver in verbose_waivers}.values()),`
		`+ 'waivers': list({waiver['id']: waiver for waiver in waivers}.values()),`
		`})`

		`log.debug('Response: %s', response)`

greenwave/policies.py

file modified

+64 -39

		`@@ -93,6 +93,12 @@`
		`def to_json(self):`
		`raise NotImplementedError()`

		`+ def to_waived(self):`
		`+ """`
		`+ Transform unsatisfied answer to waived one.`
		`+ """`
		`+ raise NotImplementedError()`
		`+`

		`class TestResultMissing(RuleNotSatisfied):`
		`"""`
		`@@ -117,6 +123,13 @@`
		`'item': subject_type_identifier_to_item(self.subject_type, self.subject_identifier),`
		`}`

		`+ def to_waived(self):`
		`+ return TestResultMissingWaived(`
		`+ self.subject_type,`
		`+ self.subject_identifier,`
		`+ self.test_case_name,`
		`+ self.scenario)`
		`+`

		`class TestResultMissingWaived(RuleSatisfied):`
		`"""`
		`@@ -163,6 +176,9 @@`
		`'scenario': self.scenario,`
		`}`

		`+ def to_waived(self):`
		`+ return TestResultPassed(self.test_case_name, self.result_id)`
		`+`

		`class InvalidGatingYaml(RuleNotSatisfied):`
		`"""`
		`@@ -184,6 +200,9 @@`
		`'details': self.details`
		`}`

		`+ def to_waived(self):`
		`+ return None`
		`+`

		`class TestResultPassed(RuleSatisfied):`
		`"""`
		`@@ -275,7 +294,12 @@`

		`This base class is not used directly.`
		`"""`
		`- def check(self, policy, product_version, subject_identifier, results_retriever, waivers):`
		`+ def check(`
		`+ self,`
		`+ policy,`
		`+ product_version,`
		`+ subject_identifier,`
		`+ results_retriever):`
		`"""`
		`Evaluate this policy rule for the given item.`

		`@@ -286,7 +310,6 @@`
		`example, Koji build NVR, Bodhi update id, ...)`
		`results_retriever (ResultsRetriever): Object for retrieving data`
		`from ResultsDB.`
		`- 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.
		`@@ -337,12 +360,6 @@`
		`return processed_rules`


		`- def waives_invalid_gating_yaml(waiver, subject_type, subject_identifier):`
		`- return (waiver['testcase'] == 'invalid-gating-yaml' and`
		`- waiver['subject']['type'] == subject_type and`
		`- waiver['subject']['item'] == subject_identifier)`
		`-`
		`-`
		`class RemoteRule(Rule):`
		`yaml_tag = '!RemoteRule'`
		`safe_yaml_attributes = {}`
		`@@ -376,14 +393,15 @@`
		`if sub_policy.decision_context == policy.decision_context`
		`]`

		`- def check(self, policy, product_version, subject_identifier, results_retriever, waivers):`
		`-`
		`+ def check(`
		`+ self,`
		`+ policy,`
		`+ product_version,`
		`+ subject_identifier,`
		`+ results_retriever):`
		`try:`
		`policies = self._get_sub_policies(policy, subject_identifier)`
		`except SafeYAMLError as e:`
		`- if any(waives_invalid_gating_yaml(waiver, policy.subject_type, subject_identifier)`
gnaponie commented 4 years ago Why removing this? Isn't this here because of this? https://docs.pagure.org/greenwave/package-specific-policies.html#tolerate-an-invalid-gating-yaml-file
lholecek commented 4 years ago I only moved the code for waiving out of the policies submodule. Waiving invalid `gating.yaml` still works.
		`- for waiver in waivers):`
		`- return []`
		`return [`
		`InvalidGatingYaml(`
		`policy.subject_type, subject_identifier, 'invalid-gating-yaml', str(e))`
		`@@ -393,7 +411,7 @@`
		`for remote_policy in policies:`
		`if remote_policy.matches_product_version(product_version):`
		`response = remote_policy.check(`
		`- product_version, subject_identifier, results_retriever, waivers)`
		`+ product_version, subject_identifier, results_retriever)`

		`if isinstance(response, list):`
		`answers.extend(response)`
		`@@ -438,11 +456,14 @@`
		`'scenario': SafeYAMLString(optional=True),`
		`}`

		`- def check(self, policy, product_version, subject_identifier, results_retriever, waivers):`
		`+ def check(`
		`+ self,`
		`+ policy,`
		`+ product_version,`
		`+ subject_identifier,`
		`+ results_retriever):`
		`matching_results = results_retriever.retrieve(`
		`policy.subject_type, subject_identifier, self.test_case_name)`
		`- matching_waivers = [`
		`- w for w in waivers if (w['testcase'] == self.test_case_name and w['waived'] is True)]`

		`if self.scenario is not None:`
		`matching_results = [`
		`@@ -451,10 +472,7 @@`

		`# Investigate the absence of result first.`
		`if not matching_results:`
		`- if not matching_waivers:`
		`- return TestResultMissing(`
		`- policy.subject_type, subject_identifier, self.test_case_name, self.scenario)`
		`- return TestResultMissingWaived(`
		`+ return TestResultMissing(`
		`policy.subject_type, subject_identifier, self.test_case_name, self.scenario)`

		`# For compose make decisions based on all architectures and variants.`
		`@@ -474,7 +492,10 @@`
		`if arch_variant not in visited_arch_variants:`
		`visited_arch_variants.add(arch_variant)`
		`answer = self._answer_for_result(`
		`- result, waivers, policy.subject_type, subject_identifier)`
		`+ result,`
		`+ product_version,`
		`+ policy.subject_type,`
		`+ subject_identifier)`
		`answers.append(answer)`

		`return answers`
		`@@ -485,7 +506,10 @@`
		`answers = []`
		`for result in matching_results:`
		`answers.append(self._answer_for_result(`
		`- result, waivers, policy.subject_type, subject_identifier))`
		`+ result,`
		`+ product_version,`
		`+ policy.subject_type,`
		`+ subject_identifier))`
		`return answers`

		`def matches(self, policy, **attributes):`
		`@@ -499,25 +523,14 @@`
		`'scenario': self.scenario,`
		`}`

		`- def _answer_for_result(self, result, waivers, subject_type, subject_identifier):`
		`+ def _answer_for_result(`
		`+ self, result, product_version, subject_type, subject_identifier):`
		`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,`
		`result['outcome'])`
		`return TestResultPassed(self.test_case_name, result['id'])`

		`- # TODO limit who is allowed to waive`
		`-`
		`- matching_waivers = [w for w in waivers if (`
		`- w['subject_type'] == subject_type and`
		`- w['subject_identifier'] == result['data']['item'][0] and`
		`- w['testcase'] == result['testcase']['name'] and`
		`- w['waived'] is True`
		`- )]`
		`- if matching_waivers:`
		`- log.debug('Found matching waivers for the result_id %s and the testcase %s,'`
		`- ' so the Test result is PASSED', result['id'], self.test_case_name)`
		`- 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,`
		`@@ -544,7 +557,12 @@`
		`tag = self.yaml_tag or '!' + type(self).__name__`
		`raise SafeYAMLError('{} is obsolete. {}'.format(tag, self.advice))`

		`- def check(self, policy, product_version, subject_identifier, results_retriever, waivers):`
		`+ def check(`
		`+ self,`
		`+ policy,`
		`+ product_version,`
		`+ subject_identifier,`
		`+ results_retriever):`
		`raise ValueError('This rule is obsolete and can\'t be checked')`


		`@@ -598,7 +616,11 @@`

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

		`- def check(self, product_version, subject_identifier, results_retriever, waivers):`
		`+ def check(`
		`+ self,`
		`+ product_version,`
		`+ subject_identifier,`
		`+ 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]`
		`@@ -614,7 +636,10 @@`
		`answers = []`
		`for rule in self.rules:`
		`response = rule.check(`
		`- self, product_version, subject_identifier, results_retriever, waivers)`
		`+ self,`
		`+ product_version,`
		`+ subject_identifier,`
		`+ results_retriever)`
		`if isinstance(response, list):`
		`answers.extend(response)`
		`else:`

greenwave/resources.py

file modified

+57 -55

		`@@ -26,59 +26,88 @@`
		`requests_session = get_requests_session()`


		`- class ResultsRetriever(object):`
		`- """`
		`- Retrieves results from ResultsDB.`
		`- """`
		`- def __init__(self, ignore_results, when, timeout, verify, url):`
		`- self.ignore_results = ignore_results`
		`- self.when = when`
		`+ class BaseRetriever:`
		`+ def __init__(self, ignore_ids, when, timeout, verify, url):`
		`+ self.ignore_ids = ignore_ids`
		`self.timeout = timeout`
		`self.verify = verify`
		`self.url = url`

		`- def retrieve(self, subject_type, subject_identifier, testcase=None, scenarios=None):`
		`- """`
		`- Return generator over results.`
		`- """`
		`- params = {}`
		`- if self.when:`
		`- params.update({'since': '1900-01-01T00:00:00.000000,{}'.format(self.when)})`
		`+ if when:`
		`+ self.since = '1900-01-01T00:00:00.000000,{}'.format(when)`
		`+ else:`
		`+ self.since = None`
		`+`
		`+ def retrieve(self, args, *kwargs):`
		`+ items = self._retrieve_all(args, *kwargs)`
		`+ return [item for item in items if item['id'] not in self.ignore_ids]`
		`+`
		`+ def _retrieve_data(self, params):`
		`+ response = self._make_request(params, verify=self.verify, timeout=self.timeout)`
		`+ response.raise_for_status()`
		`+ return response.json()['data']`
		`+`
		`+`
		`+ class ResultsRetriever(BaseRetriever):`
		`+ """`
		`+ Retrieves results from cache or ResultsDB.`
		`+ """`
		`+ def _retrieve_all(self, subject_type, subject_identifier, testcase=None, scenarios=None):`
		`+ params = {`
		`+ '_distinct_on': 'scenario,system_architecture'`
		`+ }`
		`+ if self.since:`
		`+ params.update({'since': self.since})`
		`if testcase:`
		`params.update({'testcases': testcase})`
		`if scenarios:`
		`params.update({'scenario': ','.join(scenarios)})`
		`- return self._retrieve_helper(params, subject_type, subject_identifier)`
		`-`
		`- def _make_request(self, params):`
		`- params['_distinct_on'] = 'scenario,system_architecture'`
		`- response = requests_session.get(`
		`- self.url + '/results/latest', params=params, verify=self.verify, timeout=self.timeout)`
		`- response.raise_for_status()`
		`- return response.json()['data']`

		`- def _retrieve_helper(self, params, subject_type, subject_identifier):`
		`results = []`
		`if subject_type == 'koji_build':`
		`params['type'] = 'koji_build,brew-build'`
		`params['item'] = subject_identifier`
		`- results = self._make_request(params=params)`
		`+ results = self._retrieve_data(params)`

		`del params['type']`
		`del params['item']`
		`params['original_spec_nvr'] = subject_identifier`
		`- results.extend(self._make_request(params=params))`
		`+ results.extend(self._retrieve_data(params))`
		`elif subject_type == 'compose':`
		`params['productmd.compose.id'] = subject_identifier`
		`- results = self._make_request(params=params)`
		`+ results = self._retrieve_data(params)`
		`else:`
		`params['type'] = subject_type`
		`params['item'] = subject_identifier`
		`- results = self._make_request(params=params)`
		`+ results = self._retrieve_data(params)`

		`- results = [r for r in results if r['id'] not in self.ignore_results]`
		`return results`

		`+ def _make_request(self, params, **request_args):`
		`+ return requests_session.get(`
		`+ self.url + '/results/latest',`
		`+ params=params,`
		`+ **request_args)`
		`+`
		`+`
		`+ class WaiversRetriever(BaseRetriever):`
		`+ """`
		`+ Retrieves waivers from WaiverDB.`
		`+ """`
		`+ def _retrieve_all(self, filters):`
		`+ if self.since:`
		`+ for filter_ in filters:`
		`+ filter_.update({'since': self.since})`
		`+ waivers = self._retrieve_data(filters)`
		`+ return [waiver for waiver in waivers if waiver['waived']]`
		`+`
		`+ def _make_request(self, params, **request_args):`
		`+ return requests_session.post(`
		`+ self.url + '/waivers/+filtered',`
		`+ headers={'Content-Type': 'application/json'},`
		`+ data=json.dumps({'filters': params}),`
		`+ **request_args)`
		`+`

		`@cached`
		`def retrieve_scm_from_koji(nvr):`
		`@@ -192,33 +221,6 @@`
		`return gating_yaml`


		`- # NOTE - not cached, for now.`
		`- def retrieve_waivers(product_version, subject_type, subject_identifiers, when):`
		`- if not subject_identifiers:`
		`- return []`
		`-`
		`- timeout = current_app.config['REQUESTS_TIMEOUT']`
		`- verify = current_app.config['REQUESTS_VERIFY']`
		`- filters = []`
		`- for subject_identifier in subject_identifiers:`
		`- d = {`
		`- 'product_version': product_version,`
		`- 'subject_type': subject_type,`
		`- 'subject_identifier': subject_identifier`
		`- }`
		`- if when:`
		`- d['since'] = '1900-01-01T00:00:00.000000,{}'.format(when)`
		`- filters.append(d)`
		`- response = requests_session.post(`
		`- current_app.config['WAIVERDB_API_URL'] + '/waivers/+filtered',`
		`- headers={'Content-Type': 'application/json'},`
		`- data=json.dumps({'filters': filters}),`
		`- verify=verify,`
		`- timeout=timeout)`
		`- response.raise_for_status()`
		`- return response.json()['data']`
		`-`
		`-`
		`# NOTE - not cached.`
		`def retrieve_decision(greenwave_url, data):`
		`timeout = current_app.config['REQUESTS_TIMEOUT']`

greenwave/tests/test_api_v1.py

file added

+92

		`@@ -0,0 +1,92 @@`
		`+ # SPDX-License-Identifier: GPL-2.0+`
		`+`
		`+ import mock`
		`+ import pytest`
		`+`
		`+ from textwrap import dedent`
		`+`
		`+ from greenwave.app_factory import create_app`
		`+ from greenwave.policies import Policy`
		`+`
		`+ DEFAULT_DECISION_DATA = dict(`
		`+ decision_context='test_policies',`
		`+ product_version='fedora-rawhide',`
		`+ subject_type='koji_build',`
		`+ subject_identifier='nethack-1.2.3-1.f31',`
		`+ )`
		`+`
		`+ DEFAULT_DECISION_POLICIES = """`
		`+ --- !Policy`
		`+ id: "test_policy"`
		`+ product_versions:`
		`+ - fedora-rawhide`
		`+ decision_context: test_policies`
		`+ subject_type: koji_build`
		`+ rules:`
		`+ - !PassingTestCaseRule {test_case_name: sometest}`
		`+ """`
		`+`
		`+`
		`+ def make_result(outcome):`
		`+ return {`
		`+ 'id': 123,`
		`+ 'data': {`
		`+ 'item': [DEFAULT_DECISION_DATA['subject_identifier']],`
		`+ 'type': [DEFAULT_DECISION_DATA['subject_type']],`
		`+ },`
		`+ 'testcase': {'name': 'sometest'},`
		`+ 'outcome': outcome,`
		`+ }`
		`+`
		`+`
		`+ @pytest.fixture`
		`+ def mock_results():`
		`+ with mock.patch('greenwave.resources.ResultsRetriever.retrieve') as mocked:`
		`+ mocked.return_value = []`
		`+ yield mocked`
		`+`
		`+`
		`+ @pytest.fixture`
		`+ def mock_waivers():`
		`+ with mock.patch('greenwave.resources.WaiversRetriever.retrieve') as mocked:`
		`+ mocked.return_value = []`
		`+ yield mocked`
		`+`
		`+`
		`+ def make_decision(**kwargs):`
		`+ app = create_app('greenwave.config.TestingConfig')`
		`+ app.config['policies'] = Policy.safe_load_all(dedent(DEFAULT_DECISION_POLICIES))`
		`+ client = app.test_client()`
		`+ data = DEFAULT_DECISION_DATA.copy()`
		`+ data.update(kwargs)`
		`+ return client.post('/api/v1.0/decision', json=data)`
		`+`
		`+`
		`+ def test_make_decision_retrieves_waivers_on_missing(mock_results, mock_waivers):`
		`+ response = make_decision()`
		`+ assert 200 == response.status_code`
		`+ assert '1 of 1 required test results missing' == response.json['summary']`
		`+ mock_waivers.assert_called_once()`
		`+`
		`+`
		`+ def test_make_decision_retrieves_waivers_on_failed(mock_results, mock_waivers):`
		`+ mock_results.return_value = [make_result(outcome='FAILED')]`
		`+ response = make_decision()`
		`+ assert 200 == response.status_code`
		`+ assert '1 of 1 required tests failed' == response.json['summary']`
		`+ mock_waivers.assert_called_once()`
		`+`
		`+`
		`+ def test_make_decision_retrieves_waivers_omitted_on_passed(mock_results, mock_waivers):`
		`+ mock_results.return_value = [make_result(outcome='PASSED')]`
		`+ response = make_decision()`
		`+ assert 200 == response.status_code`
		`+ assert 'All required tests passed' == response.json['summary']`
		`+ mock_waivers.assert_not_called()`
		`+`
		`+`
		`+ def test_make_decision_retrieves_waivers_once_on_verbose_and_missing(mock_results, mock_waivers):`
		`+ response = make_decision(verbose=True)`
		`+ assert 200 == response.status_code`
		`+ assert '1 of 1 required test results missing' == response.json['summary']`
		`+ mock_waivers.assert_called_once()`

greenwave/tests/test_policies.py

file modified

+55 -70

		`@@ -21,6 +21,7 @@`
		`)`
		`from greenwave.resources import ResultsRetriever`
		`from greenwave.safe_yaml import SafeYAMLError`
		`+ from greenwave.waivers import waive_answers`


		`class DummyResultsRetriever(ResultsRetriever):`
		`@@ -28,7 +29,7 @@`
		`self, subject_identifier=None, testcase=None, outcome='PASSED',`
		`subject_type='koji_build'):`
		`super(DummyResultsRetriever, self).__init__(`
		`- ignore_results=[],`
		`+ ignore_ids=[],`
		`when='',`
		`timeout=0,`
		`verify=False,`
		`@@ -38,7 +39,7 @@`
		`self.testcase = testcase`
		`self.outcome = outcome`

		`- def _make_request(self, params):`
		`+ 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`
		`params.get('testcases') == self.testcase):`
		`@@ -74,7 +75,7 @@`
		`'1 of 2 required test results missing'`


		`- def test_waive_absence_of_result(tmpdir):`
		`+ def test_decision_with_missing_result(tmpdir):`
		`p = tmpdir.join('fedora.yaml')`
		`p.write(dedent("""`
		`--- !Policy`
		`@@ -89,18 +90,14 @@`
		`policies = load_policies(tmpdir.strpath)`
		`policy = policies[0]`

		`+ results = DummyResultsRetriever()`
		`+ subject_identifier = 'some_nevr'`
		`+`
		`# Ensure that absence of a result is failure.`
		`- item, results, waivers = {}, DummyResultsRetriever(), []`
		`- decision = policy.check('fedora-rawhide', item, results, waivers)`
		`+ decision = policy.check('fedora-rawhide', subject_identifier, results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], TestResultMissing)`

		`- # But also that waiving the absence works.`
		`- waivers = [{'testcase': 'sometest', 'waived': True}]`
		`- decision = policy.check('fedora-rawhide', item, results, waivers)`
		`- assert len(decision) == 1`
		`- assert isinstance(decision[0], RuleSatisfied)`
		`-`

		`def test_waive_brew_koji_mismatch(tmpdir):`
		`""" Ensure that a koji_build waiver can match a brew-build result`
		`@@ -123,29 +120,26 @@`
		`policies = load_policies(tmpdir.strpath)`
		`policy = policies[0]`

		`- results = DummyResultsRetriever('some_nevr', 'sometest', 'FAILED', 'brew-build')`
		`- waiver = {`
		`- u'subject_identifier': u'some_nevr',`
		`- u'subject_type': u'koji_build',`
		`- u'testcase': u'sometest',`
		`- u'waived': True,`
		`- }`
		`+ item = 'some_nevr'`
		`+ results = DummyResultsRetriever(item, 'sometest', 'FAILED', 'brew-build')`

		`- item, waivers = 'some_nevr', [waiver]`
		`- decision = policy.check('fedora-rawhide', item, results, [])`
		`+ decision = policy.check('fedora-rawhide', item, results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], TestResultFailed)`

		`- decision = policy.check('fedora-rawhide', item, results, waivers)`
		`+ waivers = [{`
		`+ 'id': 1,`
		`+ 'subject_identifier': item,`
		`+ 'subject_type': 'koji_build',`
		`+ 'testcase': 'sometest',`
		`+ 'product_version': 'fedora-rawhide',`
		`+ 'waived': True,`
		`+ }]`
		`+ decision = policy.check('fedora-rawhide', item, results)`
		`+ decision = waive_answers(decision, waivers)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], RuleSatisfied)`

		`- # Also, be sure that negative waivers work.`
		`- waivers[0]['waived'] = False`
		`- decision = policy.check('fedora-rawhide', item, results, waivers)`
		`- assert len(decision) == 1`
		`- assert isinstance(decision[0], TestResultFailed)`
		`-`

		`def test_waive_bodhi_update(tmpdir):`
		`""" Ensure that a koji_build waiver can match a brew-build result`
		`@@ -170,27 +164,24 @@`

		`item = 'some_bodhi_update'`
		`results = DummyResultsRetriever(item, 'sometest', 'FAILED', 'bodhi_update')`
		`- waivers = [{`
		`- u'subject_identifier': item,`
		`- u'subject_type': 'bodhi_update',`
		`- u'testcase': 'sometest',`
		`- u'waived': True,`
		`- }]`

		`- decision = policy.check('fedora-rawhide', item, results, [])`
		`+ decision = policy.check('fedora-rawhide', item, results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], TestResultFailed)`

		`- decision = policy.check('fedora-rawhide', item, results, waivers)`
		`+ waivers = [{`
		`+ 'id': 1,`
		`+ 'subject_identifier': item,`
		`+ 'subject_type': 'bodhi_update',`
		`+ 'testcase': 'sometest',`
		`+ 'product_version': 'fedora-rawhide',`
		`+ 'waived': True,`
		`+ }]`
		`+ decision = policy.check('fedora-rawhide', item, results)`
		`+ decision = waive_answers(decision, waivers)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], RuleSatisfied)`

		`- # Also, be sure that negative waivers work.`
		`- waivers[0]['waived'] = False`
		`- decision = policy.check('fedora-rawhide', item, results, waivers)`
		`- assert len(decision) == 1`
		`- assert isinstance(decision[0], TestResultFailed)`
		`-`

		`def test_load_policies():`
		`app = create_app('greenwave.config.TestingConfig')`
		`@@ -322,23 +313,21 @@`
		`policies = load_policies(tmpdir.strpath)`
		`policy = policies[0]`

		`- waivers = []`
		`-`
		`# Ensure that presence of a result is success.`
		`results = DummyResultsRetriever(nvr, 'dist.upgradepath')`
		`- decision = policy.check('fedora-26', nvr, results, waivers)`
		`+ decision = policy.check('fedora-26', nvr, 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, waivers)`
		`+ decision = policy.check('fedora-26', nvr, 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, waivers)`
		`+ decision = policy.check('fedora-26', nvr, results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], TestResultFailed)`

		`@@ -383,25 +372,23 @@`
		`policies = load_policies(tmpdir.strpath)`
		`policy = policies[0]`

		`- waivers = []`
		`-`
		`# 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, waivers)`
		`+ decision = policy.check('rhel-8', nvr, 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, waivers)`
		`+ decision = policy.check('rhel-8', nvr, 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, waivers)`
		`+ decision = policy.check('rhel-8', nvr, results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], TestResultFailed)`

		`@@ -438,9 +425,9 @@`
		`policies = load_policies(tmpdir.strpath)`
		`policy = policies[0]`

		`- results, waivers = [], []`
		`+ results = DummyResultsRetriever()`
		`expected_details = "Policy 'untitled': Attribute 'product_versions' is required"`
		`- decision = policy.check('fedora-26', nvr, results, waivers)`
		`+ decision = policy.check('fedora-26', nvr, results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], InvalidGatingYaml)`
		`assert decision[0].is_satisfied is False`
		`@@ -494,8 +481,8 @@`
		`policies = load_policies(tmpdir.strpath)`
		`policy = policies[0]`

		`- results, waivers = [], []`
		`- decision = policy.check('fedora-26', nvr, results, waivers)`
		`+ results = DummyResultsRetriever()`
		`+ decision = policy.check('fedora-26', nvr, results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], InvalidGatingYaml)`
		`assert decision[0].is_satisfied is False`
		`@@ -549,16 +536,19 @@`
		`policies = load_policies(tmpdir.strpath)`
		`policy = policies[0]`

		`- results = []`
		`+ results = DummyResultsRetriever()`
		`waivers = [{`
		`+ 'id': 1,`
		`'subject_type': 'koji_build',`
		`'subject_identifier': 'nethack-1.2.3-1.el9000',`
		`'subject': {'type': 'koji_build', 'item': 'nethack-1.2.3-1.el9000'},`
		`'testcase': 'invalid-gating-yaml',`
		`'product_version': 'fedora-26',`
		`- 'comment': 'Waiving the invalig gating.yaml file'`
		`+ 'comment': 'Waiving the invalid gating.yaml file',`
		`+ 'waived': True,`
		`}]`
		`- decision = policy.check('fedora-26', nvr, results, waivers)`
		`+ decision = policy.check('fedora-26', nvr, results)`
		`+ decision = waive_answers(decision, waivers)`
		`assert len(decision) == 0`


		`@@ -635,9 +625,8 @@`
		`policies = load_policies(tmpdir.strpath)`
		`policy = policies[0]`

		`- waivers = []`
		`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, waivers)`
		`+ decision = policy.check('rhel-9000', 'nethack-1.2.3-1.el9000', results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], TestResultPassed)`

		`@@ -664,9 +653,8 @@`
		`policies = load_policies(tmpdir.strpath)`
		`policy = policies[0]`

		`- waivers = []`
		`results = DummyResultsRetriever('nethack-1.2.3-1.el9000', 'sometest', 'PASSED', 'koji_build')`
		`- decision = policy.check('rhel-9000', 'nethack-1.2.3-1.el9000', results, waivers)`
		`+ decision = policy.check('rhel-9000', 'nethack-1.2.3-1.el9000', results)`
		`assert len(decision) == num_decisions`
		`if num_decisions:`
		`assert isinstance(decision[0], TestResultPassed)`
		`@@ -861,8 +849,7 @@`
		`policy = policies[0]`
		`results = DummyResultsRetriever(nv, 'test_for_new_type', 'PASSED',`
		`'component-version')`
		`- waivers = []`
		`- decision = policy.check('fedora-29', nv, results, waivers)`
		`+ decision = policy.check('fedora-29', nv, results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], RuleSatisfied)`

		`@@ -885,8 +872,7 @@`
		`policy = policies[0]`
		`results = DummyResultsRetriever(nsvc, 'test_for_redhat_module_type', 'PASSED',`
		`'redhat-module')`
		`- waivers = []`
		`- decision = policy.check('fedora-29', nsvc, results, waivers)`
		`+ decision = policy.check('fedora-29', nsvc, results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], RuleSatisfied)`

		`@@ -927,22 +913,21 @@`
		`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)`
		`+ decision = policy.check('fedora-26', nvr, 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, waivers)`
		`+ decision = policy.check('fedora-26', nvr, 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, waivers)`
		`+ decision = policy.check('fedora-26', nvr, results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], TestResultFailed)`

greenwave/tests/test_waive.py

file added

+98

		`@@ -0,0 +1,98 @@`
		`+ # SPDX-License-Identifier: GPL-2.0+`
		`+`
		`+ from greenwave.policies import (`
		`+ InvalidGatingYaml,`
		`+ TestResultMissing,`
		`+ TestResultFailed,`
		`+ )`
		`+ 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',`
		`+ test_case_name='test1',`
		`+ scenario='scenario1',`
		`+ result_id=99,`
		`+ )`
		`+ ]`
		`+`
		`+ waived = waive_answers(answers, [])`
		`+ assert answers == waived`
		`+`
		`+ waivers = [`
		`+ dict(`
		`+ subject_type='koji-build',`
		`+ subject_identifier='nethack-1.2.3-1.rawhide',`
		`+ product_version='rawhide',`
		`+ testcase='test1',`
		`+ )`
		`+ ]`
		`+ waived = waive_answers(answers, waivers)`
		`+ expected_json = dict(`
		`+ type='test-result-passed',`
		`+ testcase='test1',`
		`+ result_id=99,`
		`+ )`
		`+ assert 1 == len(waived)`
		`+ assert expected_json == waived[0].to_json()`
		`+`
		`+`
		`+ def test_waive_missing_result():`
		`+ answers = [`
		`+ TestResultMissing(`
		`+ subject_type='koji-build',`
		`+ subject_identifier='nethack-1.2.3-1.rawhide',`
		`+ test_case_name='test1',`
		`+ scenario='scenario1',`
		`+ )`
		`+ ]`
		`+`
		`+ waived = waive_answers(answers, [])`
		`+ assert answers == waived`
		`+`
		`+ waivers = [`
		`+ dict(`
		`+ subject_type='koji-build',`
		`+ subject_identifier='nethack-1.2.3-1.rawhide',`
		`+ product_version='rawhide',`
		`+ testcase='test1',`
		`+ )`
		`+ ]`
		`+ waived = waive_answers(answers, waivers)`
		`+ expected_json = dict(`
		`+ type='test-result-missing-waived',`
		`+ testcase='test1',`
		`+ subject_type='koji-build',`
		`+ subject_identifier='nethack-1.2.3-1.rawhide',`
		`+ scenario='scenario1',`
		`+ )`
		`+ assert 1 == len(waived)`
		`+ assert expected_json == waived[0].to_json()`
		`+`
		`+`
		`+ def test_waive_invalid_gatin_yaml():`
		`+ answers = [`
		`+ InvalidGatingYaml(`
		`+ subject_type='koji-build',`
		`+ subject_identifier='nethack-1.2.3-1.rawhide',`
		`+ test_case_name='invalid-gating-yaml',`
		`+ details='',`
		`+ )`
		`+ ]`
		`+`
		`+ waived = waive_answers(answers, [])`
		`+ assert answers == waived`
		`+`
		`+ waivers = [`
		`+ dict(`
		`+ subject_type='koji-build',`
		`+ subject_identifier='nethack-1.2.3-1.rawhide',`
		`+ product_version='rawhide',`
		`+ testcase='invalid-gating-yaml',`
		`+ )`
		`+ ]`
		`+ waived = waive_answers(answers, waivers)`
		`+ assert [] == waived`

greenwave/tests/test_waivers_retriever.py

file added

+62

		`@@ -0,0 +1,62 @@`
		`+ # SPDX-License-Identifier: GPL-2.0+`
		`+`
		`+ import mock`
		`+`
		`+ from greenwave.resources import WaiversRetriever`
		`+`
		`+ _DUMMY_RETRIEVER_ARGUMENTS = dict(`
		`+ ignore_ids=[],`
		`+ when=None,`
		`+ timeout=None,`
		`+ verify=None,`
		`+ url=None,`
		`+ )`
		`+`
		`+ _DUMMY_FILTERS = ['dummy_filter']`
		`+`
		`+`
		`+ def test_waivers_retriever_retrieves_not_ignored_ids():`
		`+ retriever = WaiversRetriever(**_DUMMY_RETRIEVER_ARGUMENTS)`
		`+ retriever.ignore_ids = [100]`
		`+ waiver = dict(`
		`+ id=99,`
		`+ subject_type='koji-build',`
		`+ subject_identifier='nethack-1.2.3-1.rawhide',`
		`+ product_version='rawhide',`
		`+ testcase='test1',`
		`+ waived=True,`
		`+ )`
		`+ retriever._retrieve_data = mock.MagicMock(return_value=[waiver])`
		`+ waivers = retriever.retrieve(_DUMMY_FILTERS)`
		`+ assert [waiver] == waivers`
		`+`
		`+`
		`+ def test_waivers_retriever_ignores_ids():`
		`+ retriever = WaiversRetriever(**_DUMMY_RETRIEVER_ARGUMENTS)`
		`+ retriever.ignore_ids = [99]`
		`+ waiver = dict(`
		`+ id=99,`
		`+ subject_type='koji-build',`
		`+ subject_identifier='nethack-1.2.3-1.rawhide',`
		`+ product_version='rawhide',`
		`+ testcase='test1',`
		`+ waived=True,`
		`+ )`
		`+ retriever._retrieve_data = mock.MagicMock(return_value=[waiver])`
		`+ waivers = retriever.retrieve(_DUMMY_FILTERS)`
		`+ assert [] == waivers`
		`+`
		`+`
		`+ def test_waivers_retriever_ignores_no_waived():`
		`+ retriever = WaiversRetriever(**_DUMMY_RETRIEVER_ARGUMENTS)`
		`+ waiver = dict(`
		`+ id=99,`
		`+ subject_type='koji-build',`
		`+ subject_identifier='nethack-1.2.3-1.rawhide',`
		`+ product_version='rawhide',`
		`+ testcase='test1',`
		`+ waived=False,`
		`+ )`
		`+ retriever._retrieve_data = mock.MagicMock(return_value=[waiver])`
		`+ waivers = retriever.retrieve(_DUMMY_FILTERS)`
		`+ assert [] == waivers`

greenwave/waivers.py

file added

+32

		`@@ -0,0 +1,32 @@`
		`+ # SPDX-License-Identifier: GPL-2.0+`
		`+`
		`+`
		`+ def _is_waived(answer, waivers):`
		`+ """`
		`+ 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['testcase'] == answer.test_case_name`
		`+ for waiver in waivers)`
		`+`
		`+`
		`+ def _maybe_waive(answer, waivers):`
		`+ """`
		`+ Returns waived answer if it's unsatisfied there is a matching waiver,`
		`+ otherwise returns unchanged answer.`
		`+ """`
		`+ if not answer.is_satisfied and _is_waived(answer, waivers):`
		`+ return answer.to_waived()`
		`+ return answer`
		`+`
		`+`
		`+ def waive_answers(answers, waivers):`
		`+ """`
		`+ Returns answers with unsatisfied answers waived`
		+ (`RuleNotSatisfied.to_waived()`) if there is a matching waiver.
		`+ """`
		`+ waived_answers = [_maybe_waive(answer, waivers) for answer in answers]`
		`+ waived_answers = [answer for answer in waived_answers if answer is not None]`
		`+ return waived_answers`

lholecek commented 4 years ago

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

gnaponie commented 4 years ago

Did you check pylint? It gives me 2 errors (that don't seem to be related to your changes though... but just to be sure).

gnaponie commented on line 45 of greenwave/api_v1.py 4 years ago

This line is too long.
E501 line too long (104 > 100 characters)

Edited 4 years ago by gnaponie

gnaponie commented on line 6 of greenwave/policies.py 4 years ago

greenwave/policies.py|278 col 101| E501 line too long (103 > 100 characters)

gnaponie commented on line 39 of greenwave/policies.py 4 years ago

greenwave/policies.py|374 col 101| E501 line too long (103 > 100 characters)

gnaponie commented on line 68 of greenwave/policies.py 4 years ago

greenwave/policies.py|439 col 101| E501 line too long (103 > 100 characters)

gnaponie commented on line 95 of greenwave/policies.py 4 years ago

greenwave/policies.py|478 col 101| E501 line too long (108 > 100 characters)

gnaponie commented on line 104 of greenwave/policies.py 4 years ago

greenwave/policies.py|489 col 101| E501 line too long (101 > 100 characters)

lholecek commented 4 years ago

Checked pylint and except those "line too long" messages didn't see anything else. I ignored those because I didn't think we use pylint. Let me fix it.

gnaponie commented on line 35 of greenwave/tests/test_policies.py 4 years ago

Is this method actually used somewhere?

rebased onto b85dd6001a09459d4ff6054b7a25d3a83f089e17

4 years ago

gnaponie commented 4 years ago

We should keep pylint because it is necessary to the release. It is in the Jenkins job, if it doesn't work, the jenkins job fails and the release is blocked.
But we can discuss about the actual benefit of pylint and consider if we should remove it or keep it.

gnaponie commented on line 72 of greenwave/tests/test_policies.py 4 years ago

nvr?

gnaponie commented on line 240 of greenwave/tests/test_policies.py 4 years ago

invalid? It was there before, but let's change it..

gnaponie commented 4 years ago

I don't exactly understand where is the place that is getting the waivers only if actually needed.
Can you point me at it?

And can you provide a test for this case? For example: if I have only PASSED results, I don't need to ask for waivers. Additionally: I would maybe somewhere write (either in the doc, or either in the decision response) that waivers are not retrieved if it's not really needed (if the decision is already positive considering only the results).

lholecek commented on line 72 of greenwave/tests/test_policies.py 4 years ago

nvr?

I just copied this from other test.

lholecek commented on line 35 of greenwave/tests/test_policies.py 4 years ago

The retriever instances call _retrieve_data() to get the "data" field value from restulsdb/waiverdb requests. This is just overridden for tests.

rebased onto df5355fc92cda943ac0005d4504e7390a41102fb

4 years ago

lholecek commented 4 years ago

I don't exactly understand where is the place that is getting the waivers only if actually needed.
Can you point me at it?
And can you provide a test for this case? For example: if I have only PASSED results, I don't need to ask for waivers. Additionally: I would maybe somewhere write (either in the doc, or either in the decision response) that waivers are not retrieved if it's not really needed (if the decision is already positive considering only the results).

Previously, all waivers for given product version and subjects were retrieved. Now, waivers are retrieved for specific test case only when the test fails or is missing.

Let me try to write a test and update docs.

rebased onto c33dabd18f48db72282bd108c993278a6eb474e5

4 years ago

lholecek commented 4 years ago

We should keep pylint because it is necessary to the release. It is in the Jenkins job, if it doesn't work, the jenkins job fails and the release is blocked.
But we can discuss about the actual benefit of pylint and consider if we should remove it or keep it.

Let's keep pylint - it can find interesting errors (it's better in this than flake8).

gnaponie commented 4 years ago

nvr?

I just copied this from other test.

I wouldn't carry typos over.. I understand if you don't want to change the old test, it's not the scope of this PR, but let's not introduce the typo also in the new tests.

lholecek commented 4 years ago

I got an idea how to improve this even further.

GW can make only single waiverdb request after it checks all the policies. Checking policies could give "result missing" or "results failed" answer - for those GW can create single query to waiverdb.

This will also decouple waiving results from policies - policies will only know how to retrieve results, waiving failed and missing results can be done separately.

lholecek commented 4 years ago

nvr?
I just copied this from other test.

I wouldn't carry typos over.. I understand if you don't want to change the old test, it's not the scope of this PR, but let's not introduce the typo also in the new tests.

Is that typo? Doesn't E in NEVR mean "epoch"?

Edited 4 years ago by lholecek

gnaponie commented 4 years ago

nvr?
I just copied this from other test.
I wouldn't carry typos over.. I understand if you don't want to change the old test, it's not the scope of this PR, but let's not introduce the typo also in the new tests.

Is that typo? Doesn't E in NEVR mean "epoch"?

I don't recall this type. If you are aware of it, please just ignore my comment.

Edited 4 years ago by gnaponie

gnaponie commented 4 years ago

I got an idea how to improve this even further.
GW can make only single waiverdb request after it checks all the policies. Checking policies could give "result missing" or "results failed" answer - for those GW can create single query to waiverdb.

please do. That was what I was hoping for in this change.

This will also decouple waiving results from policies - policies will only know how to retrieve results, waiving failed and missing results can be done separately.

rebased onto c12b3acb5245d29cea4325f80ccf2f900c6e7f8d

4 years ago

gnaponie commented 4 years ago

@lholecek can you remove the conflicts here? I'll make another review then. Thank you.

rebased onto db1cc5eb76941fcf77495fbb737912427d46b498

4 years ago

gnaponie commented on line 5 of docs/release-notes.rst 4 years ago

We decided not to go with this approach, but I really appreciate this section since we still don't have a solution! Thank you for posting that.

gnaponie commented on line 72 of greenwave/api_v1.py 4 years ago

mmm shouldn't that be done only if verbose == False? Otherwise (if verbose == True) you've already added these. Right?

gnaponie commented on line 98 of greenwave/policies.py 4 years ago

Why removing this? Isn't this here because of this?
https://docs.pagure.org/greenwave/package-specific-policies.html#tolerate-an-invalid-gating-yaml-file

lholecek commented on line 5 of docs/release-notes.rst 4 years ago

OK, let me remove the changelog entry.

There is probably no need to document any of this - how waivers are retrieved is an implementation detail.

lholecek commented on line 72 of greenwave/api_v1.py 4 years ago

mmm shouldn't that be done only if verbose == False? Otherwise (if verbose == True) you've already added these. Right?

Hmm, possibly. Let me try to omit this when verbose is set.

lholecek commented on line 98 of greenwave/policies.py 4 years ago

I only moved the code for waiving out of the policies submodule. Waiving invalid gating.yaml still works.

rebased onto debf7c1ddae9a68be924888a64033d5f8a21ff8a

4 years ago

gnaponie commented 4 years ago

OK, let me remove the changelog entry.
There is probably no need to document any of this - how waivers are retrieved is an implementation detail.

I actually liked that! But you are right, this change maybe doesn't apply so much. We don't need to put in the changelog.

gnaponie commented 4 years ago

I only moved the code for waiving out of the policies submodule. Waiving invalid gating.yaml still works.

Sounds good!

gnaponie commented on line 80 of greenwave/api_v1.py 4 years ago

In case we have verbose == False and all results satisfied, is this going to make a request?
If not: I'm confused about where this gets checked.
If yes: shouldn't we check if it is actually needed?

gnaponie commented on line 13 of greenwave/waivers.py 4 years ago

Can you provide some description to "_is_waived" and "_maybe_waive"? The names are a bit strange to me.

gnaponie commented on line 21 of greenwave/waivers.py 4 years ago

What do you mean by "unsatisfied waived"?

lholecek commented on line 80 of greenwave/api_v1.py 4 years ago

In case we have verbose == False and all results satisfied, is this going to make a request?
If not: I'm confused about where this gets checked.

It just returns empty list, see WaiversRetriever._retrieve_all().

lholecek commented on line 21 of greenwave/waivers.py 4 years ago

What do you mean by "unsatisfied waived"?

Ah, I meant:

Returns answers with unsatisfied answers waived (RuleNotSatisfied.to_waived()) if there is matching waiver.

Fixing.

rebased onto 6f28201ba30e93f7aaf31b451d78810d9f75940d

4 years ago

lholecek commented on line 13 of greenwave/waivers.py 4 years ago

Can you provide some description to "_is_waived" and "_maybe_waive"? The names are a bit strange to me.

Done.

gnaponie commented 4 years ago

Last request, I promise :) Can you add one or two tests (maybe functional test)? One should check that no call to waiverdb is made when all results are satisfied and the other one should check that one call is made to waiverdb when one result is either missing or failed. You can use maybe https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_called_once

rebased onto 6f39656d119c4cc880a62208b0e8f0870dbb9dfa

4 years ago

rebased onto e0bc8e935ecea5a9a31b302979ab865a13e5d03e

4 years ago

lholecek commented 4 years ago

I've added the tests.