From 2a9e3da7ab1ac7b266f3d838d6b414191df90e91 Mon Sep 17 00:00:00 2001 From: Lukas Holecek Date: Oct 01 2019 08:23:28 +0000 Subject: Refactor subject type and item usage Adds a base class for decision subject. Each subclass has subject type and item/identifier property. Type is static for the specialized classes and dynamically set on instantiation of the generic subject (GenericSubject). This makes the code more modular and allows to easily add and specialize subject types and, eventually, disable or remove specializations. Fixes "koji-build" in tests to "koji_build". Signed-off-by: Lukas Holecek --- diff --git a/greenwave/api_v1.py b/greenwave/api_v1.py index 0900e63..dc1d696 100644 --- a/greenwave/api_v1.py +++ b/greenwave/api_v1.py @@ -15,6 +15,7 @@ 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.subjects.factory import create_subject, subject_type_and_id_from_dict from greenwave.monitor import ( registry, decision_exception_counter, @@ -27,22 +28,13 @@ log = logging.getLogger(__name__) def _decision_subject(subject): - subject_type = subject.get('type') - subject_identifier = subject.get('item') + subject_type, subject_identifier = subject_type_and_id_from_dict(subject) - if 'productmd.compose.id' in subject: - return ('compose', subject['productmd.compose.id']) + if subject_type is None or subject_identifier is None: + log.info('Couldn\'t detect subject_identifier.') + raise BadRequest('Couldn\'t detect subject_identifier.') - if 'original_spec_nvr' in subject: - return ('koji_build', subject['original_spec_nvr']) - - if subject_identifier: - if subject_type == 'brew-build': - return ('koji_build', subject_identifier) - return (subject_type, subject_identifier) - - log.info('Couldn\'t detect subject_identifier.') - raise BadRequest('Couldn\'t detect subject_identifier.') + return create_subject(subject_type, subject_identifier) def _decision_subjects_for_request(data): @@ -72,18 +64,7 @@ def _decision_subjects_for_request(data): log.error('Missing required "subject_identifier" parameter') raise BadRequest('Missing required "subject_identifier" parameter') - yield data['subject_type'], data['subject_identifier'] - - -def subject_type_identifier_to_list(subject_type, subject_identifier): - """ - Inverse of the above function. - This is for backwards compatibility in emitted messages. - """ - if subject_type == 'compose': - return [{'productmd.compose.id': subject_identifier}] - else: - return [{'type': subject_type, 'item': subject_identifier}] + yield create_subject(data['subject_type'], data['subject_identifier']) @api.route('/version', methods=['GET']) @@ -413,9 +394,9 @@ def make_decision(): 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 + for subject 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) @@ -451,34 +432,33 @@ def make_decision(): waiver_filters = [] policies = on_demand_policies or current_app.config['policies'] - for subject_type, subject_identifier in _decision_subjects_for_request(data): + for subject in _decision_subjects_for_request(data): subject_policies = [ policy for policy in policies if policy.matches( decision_context=decision_context, product_version=product_version, - subject_type=subject_type) + subject=subject) ] if not subject_policies: # Ignore non-existent policy for Bodhi updates. - if subject_type == 'bodhi_update': + if subject.type == 'bodhi_update': continue log.error( 'Cannot find any applicable policies for %s subjects at gating point %s in %s', - subject_type, decision_context, product_version) + subject.type, decision_context, product_version) raise NotFound( 'Cannot find any applicable policies for %s subjects at gating point %s in %s' % ( - subject_type, decision_context, product_version)) + subject.type, decision_context, product_version)) if verbose: # Retrieve test results and waivers for all items when verbose output is requested. - verbose_results.extend( - results_retriever.retrieve(subject_type, subject_identifier)) + verbose_results.extend(results_retriever.retrieve(subject)) waiver_filters.append(dict( - subject_type=subject_type, - subject_identifier=subject_identifier, + subject_type=subject.type, + subject_identifier=subject.identifier, product_version=product_version, )) @@ -486,7 +466,7 @@ def make_decision(): answers.extend( policy.check( product_version, - subject_identifier, + subject, results_retriever)) applicable_policies.extend(subject_policies) @@ -495,8 +475,8 @@ def make_decision(): for answer in answers: if not answer.is_satisfied: waiver_filters.append(dict( - subject_type=answer.subject_type, - subject_identifier=answer.subject_identifier, + subject_type=answer.subject.type, + subject_identifier=answer.subject.identifier, product_version=product_version, testcase=answer.test_case_name, )) diff --git a/greenwave/consumers/consumer.py b/greenwave/consumers/consumer.py index 75f0247..4e0572e 100644 --- a/greenwave/consumers/consumer.py +++ b/greenwave/consumers/consumer.py @@ -6,7 +6,6 @@ import requests import fedmsg.consumers import greenwave.app_factory -from greenwave.api_v1 import subject_type_identifier_to_list from greenwave.monitor import ( publish_decision_exceptions_result_counter, messaging_tx_to_send_counter, messaging_tx_stopped_counter, @@ -157,15 +156,13 @@ class Consumer(fedmsg.consumers.FedmsgConsumer): def _publish_decision_change( self, submit_time, - subject_type, - subject_identifier, + subject, testcase, product_version, publish_testcase): policy_attributes = dict( - subject_type=subject_type, - subject_identifier=subject_identifier, + subject=subject, testcase=testcase, ) @@ -185,8 +182,8 @@ class Consumer(fedmsg.consumers.FedmsgConsumer): submit_time, decision_context=decision_context, product_version=product_version, - subject_type=subject_type, - subject_identifier=subject_identifier, + subject_type=subject.type, + subject_identifier=subject.identifier, ) if decision is None: continue @@ -197,11 +194,10 @@ class Consumer(fedmsg.consumers.FedmsgConsumer): continue decision.update({ - 'subject_type': subject_type, - 'subject_identifier': subject_identifier, + 'subject_type': subject.type, + 'subject_identifier': subject.identifier, # subject is for backwards compatibility only: - 'subject': subject_type_identifier_to_list(subject_type, - subject_identifier), + 'subject': [subject.to_item_dict()], 'decision_context': decision_context, 'product_version': product_version, 'previous': old_decision, diff --git a/greenwave/consumers/resultsdb.py b/greenwave/consumers/resultsdb.py index 8f622b1..1ee1509 100644 --- a/greenwave/consumers/resultsdb.py +++ b/greenwave/consumers/resultsdb.py @@ -13,6 +13,7 @@ import logging from greenwave.consumers.consumer import Consumer from greenwave.product_versions import subject_product_version +from greenwave.subjects.factory import create_subject, subject_type_and_id_from_dict import xmlrpc.client @@ -64,9 +65,9 @@ class ResultsDBHandler(Consumer): self.koji_proxy = None @staticmethod - def announcement_subjects(message): + def announcement_subject(message): """ - Yields pairs of (subject type, subject identifier) for announcement + Returns pairs of (subject type, subject identifier) for announcement consideration from the message. Args: @@ -78,7 +79,12 @@ class ResultsDBHandler(Consumer): except KeyError: data = message['msg']['task'] # Old format - _type = _unpack_value(data.get('type')) + unpacked = {} + for k, v in data.items(): + unpacked[k] = _unpack_value(v) + + subject_type, subject_identifier = subject_type_and_id_from_dict(unpacked) + # note: it is *intentional* that we do not handle old format # compose-type messages, because it is impossible to reliably # produce a decision from these. compose decisions can only be @@ -88,21 +94,10 @@ class ResultsDBHandler(Consumer): # https://pagure.io/taskotron/resultsdb/issue/92 # https://pagure.io/taskotron/resultsdb/pull-request/101 # https://pagure.io/greenwave/pull-request/262#comment-70350 - if 'productmd.compose.id' in data: - yield ('compose', _unpack_value(data['productmd.compose.id'])) - elif _type == 'compose': - pass - elif 'original_spec_nvr' in data: - nvr = _unpack_value(data['original_spec_nvr']) - # when the pipeline ignores a package, which happens - # *a lot*, we get a message with an 'original_spec_nvr' - # key with an empty value; let's not try and handle this - if nvr: - yield ('koji_build', nvr) - elif _type == 'brew-build': - yield ('koji_build', _unpack_value(data['item'])) - elif 'item' in data and _type: - yield (_type, _unpack_value(data['item'])) + if subject_type == 'compose' and 'productmd.compose.id' not in data: + return None, None + + return subject_type, subject_identifier def _consume_message(self, message): msg = message['msg'] @@ -119,21 +114,24 @@ class ResultsDBHandler(Consumer): brew_task_id = _get_brew_task_id(msg) - for subject_type, subject_identifier in self.announcement_subjects(message): - log.debug('Considering subject %s: %r', subject_type, subject_identifier) - - product_version = subject_product_version( - subject_identifier, - subject_type, - self.koji_proxy, - brew_task_id, - ) - - self._publish_decision_change( - submit_time=submit_time, - subject_type=subject_type, - subject_identifier=subject_identifier, - testcase=testcase, - product_version=product_version, - publish_testcase=False, - ) + subject_type, subject_identifier = self.announcement_subject(message) + if subject_type is None: + return + + log.debug('Considering subject %s: %r', subject_type, subject_identifier) + + subject = create_subject(subject_type, subject_identifier) + + product_version = subject_product_version( + subject, + self.koji_proxy, + brew_task_id, + ) + + self._publish_decision_change( + submit_time=submit_time, + subject=subject, + testcase=testcase, + product_version=product_version, + publish_testcase=False, + ) diff --git a/greenwave/consumers/waiverdb.py b/greenwave/consumers/waiverdb.py index d022787..16fa68a 100644 --- a/greenwave/consumers/waiverdb.py +++ b/greenwave/consumers/waiverdb.py @@ -10,6 +10,7 @@ to the message bus about the newly satisfied/unsatisfied policy. """ from greenwave.consumers.consumer import Consumer +from greenwave.subjects.factory import create_subject class WaiverDBHandler(Consumer): @@ -30,14 +31,12 @@ class WaiverDBHandler(Consumer): product_version = msg['product_version'] testcase = msg['testcase'] - subject_type = msg['subject_type'] - subject_identifier = msg['subject_identifier'] + subject = create_subject(msg['subject_type'], msg['subject_identifier']) submit_time = msg['timestamp'] self._publish_decision_change( submit_time=submit_time, - subject_type=subject_type, - subject_identifier=subject_identifier, + subject=subject, testcase=testcase, product_version=product_version, publish_testcase=True, diff --git a/greenwave/policies.py b/greenwave/policies.py index cdb0ddf..c4ba403 100644 --- a/greenwave/policies.py +++ b/greenwave/policies.py @@ -9,6 +9,8 @@ import greenwave.resources from werkzeug.exceptions import BadRequest from flask import current_app +from greenwave.subjects.factory import remote_rule_subject_types + from greenwave.safe_yaml import ( SafeYAMLBool, SafeYAMLChoice, @@ -42,17 +44,6 @@ class DisallowedRuleError(RuntimeError): pass -def subject_type_identifier_to_item(subject_type, subject_identifier): - """ - Greenwave < 0.8 included an "item" key in the "unsatisfied_requirements". - This returns a suitable value for that key, for backwards compatibility. - """ - if subject_type == 'compose': - return {'productmd.compose.id': subject_identifier} - else: - return {'type': subject_type, 'item': subject_identifier} - - class Answer(object): """ Represents the result of evaluating a policy rule against a particular @@ -107,9 +98,8 @@ class TestResultMissing(RuleNotSatisfied): ResultsDB with a matching item and test case name). """ - def __init__(self, subject_type, subject_identifier, test_case_name, scenario): - self.subject_type = subject_type - self.subject_identifier = subject_identifier + def __init__(self, subject, test_case_name, scenario): + self.subject = subject self.test_case_name = test_case_name self.scenario = scenario @@ -117,17 +107,16 @@ class TestResultMissing(RuleNotSatisfied): return { 'type': 'test-result-missing', 'testcase': self.test_case_name, - 'subject_type': self.subject_type, - 'subject_identifier': self.subject_identifier, + 'subject_type': self.subject.type, + 'subject_identifier': self.subject.identifier, 'scenario': self.scenario, # For backwards compatibility only: - 'item': subject_type_identifier_to_item(self.subject_type, self.subject_identifier), + 'item': self.subject.to_item_dict() } def to_waived(self): return TestResultMissingWaived( - self.subject_type, - self.subject_identifier, + self.subject, self.test_case_name, self.scenario) @@ -136,9 +125,8 @@ class TestResultMissingWaived(RuleSatisfied): """ Same as TestResultMissing but the result was waived. """ - def __init__(self, subject_type, subject_identifier, test_case_name, scenario): - self.subject_type = subject_type - self.subject_identifier = subject_identifier + def __init__(self, subject, test_case_name, scenario): + self.subject = subject self.test_case_name = test_case_name self.scenario = scenario @@ -146,8 +134,8 @@ class TestResultMissingWaived(RuleSatisfied): return { 'type': 'test-result-missing-waived', 'testcase': self.test_case_name, - 'subject_type': self.subject_type, - 'subject_identifier': self.subject_identifier, + 'subject_type': self.subject.type, + 'subject_identifier': self.subject.identifier, 'scenario': self.scenario, } @@ -158,9 +146,8 @@ class TestResultFailed(RuleNotSatisfied): not ``PASSED`` or ``INFO``) and no corresponding waiver was found. """ - def __init__(self, subject_type, subject_identifier, test_case_name, scenario, result_id): - self.subject_type = subject_type - self.subject_identifier = subject_identifier + def __init__(self, subject, test_case_name, scenario, result_id): + self.subject = subject self.test_case_name = test_case_name self.scenario = scenario self.result_id = result_id @@ -173,7 +160,7 @@ class TestResultFailed(RuleNotSatisfied): # These are for backwards compatibility only # (the values are already visible in the result data itself, the # caller shouldn't need them repeated here): - 'item': subject_type_identifier_to_item(self.subject_type, self.subject_identifier), + 'item': self.subject.to_item_dict(), 'scenario': self.scenario, } @@ -186,9 +173,8 @@ class InvalidGatingYaml(RuleNotSatisfied): Remote policy parsing failed. """ - def __init__(self, subject_type, subject_identifier, test_case_name, details): - self.subject_type = subject_type - self.subject_identifier = subject_identifier + def __init__(self, subject, test_case_name, details): + self.subject = subject self.test_case_name = test_case_name self.details = details @@ -196,8 +182,8 @@ class InvalidGatingYaml(RuleNotSatisfied): return { 'type': 'invalid-gating-yaml', 'testcase': self.test_case_name, - 'subject_type': self.subject_type, - 'subject_identifier': self.subject_identifier, + 'subject_type': self.subject.type, + 'subject_identifier': self.subject.identifier, 'details': self.details } @@ -212,16 +198,15 @@ class MissingGatingYaml(RuleNotSatisfied): test_case_name = 'missing-gating-yaml' - def __init__(self, subject_type, subject_identifier): - self.subject_type = subject_type - self.subject_identifier = subject_identifier + def __init__(self, subject): + self.subject = subject def to_json(self): return { 'type': 'missing-gating-yaml', 'testcase': self.test_case_name, - 'subject_type': self.subject_type, - 'subject_identifier': self.subject_identifier, + 'subject_type': self.subject.type, + 'subject_identifier': self.subject.identifier, } def to_waived(self): @@ -322,7 +307,7 @@ class Rule(SafeYAMLObject): self, policy, product_version, - subject_identifier, + subject, results_retriever): """ Evaluate this policy rule for the given item. @@ -330,7 +315,7 @@ class Rule(SafeYAMLObject): Args: policy (Policy): Parent policy of the rule product_version (str): Product version we are making a decision about - subject_identifier (str): Item we are making a decision about (for + subject (Subject): Item we are making a decision about (for example, Koji build NVR, Bodhi update id, ...) results_retriever (ResultsRetriever): Object for retrieving data from ResultsDB. @@ -391,12 +376,12 @@ class RemoteRule(Rule): 'required': SafeYAMLBool(optional=True, default=False), } - def _get_sub_policies(self, policy, subject_identifier): - if policy.subject_type not in ['koji_build', 'redhat-module']: + def _get_sub_policies(self, policy, subject): + if subject.type not in remote_rule_subject_types(): return [] pkg_namespace, pkg_name, rev = greenwave.resources.retrieve_scm_from_koji( - subject_identifier) + subject.identifier) # if the element is actually a container and not a pkg there will be a "-container" # string at the end of the "pkg_name" and it will not match with the one in the # gating.yaml URL @@ -424,26 +409,25 @@ class RemoteRule(Rule): self, policy, product_version, - subject_identifier, + subject, results_retriever): try: - policies = self._get_sub_policies(policy, subject_identifier) + policies = self._get_sub_policies(policy, subject) except SafeYAMLError as e: return [ - InvalidGatingYaml( - policy.subject_type, subject_identifier, 'invalid-gating-yaml', str(e)) + InvalidGatingYaml(subject, 'invalid-gating-yaml', str(e)) ] if policies is None: if self.required: - return [MissingGatingYaml(policy.subject_type, subject_identifier)] + return [MissingGatingYaml(subject)] return [] answers = [] for remote_policy in policies: if remote_policy.matches_product_version(product_version): response = remote_policy.check( - product_version, subject_identifier, results_retriever) + product_version, subject, results_retriever) if isinstance(response, list): answers.extend(response) @@ -454,22 +438,21 @@ class RemoteRule(Rule): def matches(self, policy, **attributes): #pylint: disable=broad-except - subject_identifier = attributes.get('subject_identifier') - if not subject_identifier: + subject = attributes.get('subject') + if not subject: return True sub_policies = [] try: - sub_policies = self._get_sub_policies(policy, subject_identifier) + sub_policies = self._get_sub_policies(policy, subject) except SafeYAMLError: - logging.exception( - 'Failed to parse policies for %r', subject_identifier) + logging.exception('Failed to parse policies for %r', subject) except Exception: - logging.exception( - 'Failed to retrieve policies for %r', subject_identifier) + logging.exception('Failed to retrieve policies for %r', subject) if sub_policies is None: - return False + # RemoteRule matches if remote policy file is missing. + return True return any(sub_policy.matches(**attributes) for sub_policy in sub_policies) @@ -495,10 +478,9 @@ class PassingTestCaseRule(Rule): self, policy, product_version, - subject_identifier, + subject, results_retriever): - matching_results = results_retriever.retrieve( - policy.subject_type, subject_identifier, self.test_case_name) + matching_results = results_retriever.retrieve(subject, self.test_case_name) if self.scenario is not None: matching_results = [ @@ -507,43 +489,15 @@ class PassingTestCaseRule(Rule): # Investigate the absence of result first. if not matching_results: - return TestResultMissing( - policy.subject_type, subject_identifier, self.test_case_name, self.scenario) - - # For compose make decisions based on all architectures and variants. - if policy.subject_type == 'compose': - visited_arch_variants = set() - answers = [] - for result in matching_results: - result_data = result['data'] - - # Items under test result "data" are lists which are unhashable - # types in Python. This converts anything that is stored there - # to a string so we don't have to care about the stored value. - arch_variant = ( - str(result_data.get('system_architecture')), - str(result_data.get('system_variant'))) - - if arch_variant not in visited_arch_variants: - visited_arch_variants.add(arch_variant) - answer = self._answer_for_result( - result, - policy.subject_type, - subject_identifier) - answers.append(answer) - - return answers + return TestResultMissing(subject, self.test_case_name, self.scenario) # If we find multiple matching results, we always use the first one which # will be the latest chronologically, because ResultsDB always returns # results ordered by `submit_time` descending. - answers = [] - for result in matching_results: - answers.append(self._answer_for_result( - result, - policy.subject_type, - subject_identifier)) - return answers + return [ + self._answer_for_result(result, subject) + for result in subject.get_latest_results(matching_results) + ] def matches(self, policy, **attributes): testcase = attributes.get('testcase') @@ -556,8 +510,7 @@ class PassingTestCaseRule(Rule): 'scenario': self.scenario, } - def _answer_for_result( - self, result, subject_type, subject_identifier): + def _answer_for_result(self, result, subject): 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, @@ -565,17 +518,14 @@ class PassingTestCaseRule(Rule): 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, + log.debug('Test result MISSING for the %s and ' + 'testcase %s, because the outcome is %s', subject, self.test_case_name, result['outcome']) - return TestResultMissing(subject_type, subject_identifier, self.test_case_name, - self.scenario) - log.debug('Test result failed for the subject_type %s, subject_identifier %s and ' + return TestResultMissing(subject, self.test_case_name, self.scenario) + log.debug('Test result failed for the %s and ' 'testcase %s, because the outcome is %s and it didn\'t match any of the ' - 'previous cases', subject_type, subject_identifier, - self.test_case_name, result['outcome']) - return TestResultFailed(subject_type, subject_identifier, self.test_case_name, - self.scenario, result['id']) + 'previous cases', subject, self.test_case_name, result['outcome']) + return TestResultFailed(subject, self.test_case_name, self.scenario, result['id']) class ObsoleteRule(Rule): @@ -594,7 +544,7 @@ class ObsoleteRule(Rule): self, policy, product_version, - subject_identifier, + subject, results_retriever): raise ValueError('This rule is obsolete and can\'t be checked') @@ -615,7 +565,6 @@ class Policy(SafeYAMLObject): 'id': SafeYAMLString(), 'product_versions': SafeYAMLList(str), 'decision_context': SafeYAMLString(), - # TODO: Handle brew-build value better. 'subject_type': SafeYAMLString(), 'rules': SafeYAMLList(Rule), 'blacklist': SafeYAMLList(str, optional=True), @@ -643,8 +592,8 @@ class Policy(SafeYAMLObject): if product_version and not self.matches_product_version(product_version): return False - subject_type = attributes.get('subject_type') - if subject_type and subject_type != self.subject_type: + subject = attributes.get('subject') + if subject and subject.type != self.subject_type: return False return not self.rules or any(rule.matches(self, **attributes) for rule in self.rules) @@ -652,16 +601,16 @@ class Policy(SafeYAMLObject): def check( self, product_version, - subject_identifier, + subject, 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] + name = subject.package_name + if name: if name in self.blacklist: - return [BlacklistedInPolicy(subject_identifier) for rule in self.rules] + return [BlacklistedInPolicy(subject.identifier) for rule in self.rules] for exclude in self.excluded_packages: if fnmatch(name, exclude): - return [ExcludedInPolicy(subject_identifier) for rule in self.rules] + return [ExcludedInPolicy(subject.identifier) for rule in self.rules] if self.packages and not any(fnmatch(name, package) for package in self.packages): # If the `packages` whitelist is set and this package isn't in the # `packages` whitelist, then the policy doesn't apply to it @@ -671,7 +620,7 @@ class Policy(SafeYAMLObject): response = rule.check( self, product_version, - subject_identifier, + subject, results_retriever) if isinstance(response, list): answers.extend(response) @@ -736,7 +685,7 @@ class RemotePolicy(Policy): safe_yaml_attributes = { 'id': SafeYAMLString(optional=True), 'product_versions': SafeYAMLList(str), - 'subject_type': SafeYAMLChoice('koji_build', 'redhat-module', optional=True), + 'subject_type': SafeYAMLChoice(*remote_rule_subject_types(), optional=True), 'decision_context': SafeYAMLString(), 'rules': SafeYAMLList(Rule), 'blacklist': SafeYAMLList(str, optional=True), diff --git a/greenwave/product_versions.py b/greenwave/product_versions.py index f83dec6..8c17e0e 100644 --- a/greenwave/product_versions.py +++ b/greenwave/product_versions.py @@ -56,24 +56,15 @@ def _guess_koji_build_product_version( def subject_product_version( - subject_identifier, - subject_type, + subject, koji_proxy=None, koji_task_id=None): - if subject_type == 'koji_build': - try: - _, _, release = subject_identifier.rsplit('-', 2) - _, short_prod_version = release.rsplit('.', 1) - return _guess_product_version(short_prod_version, koji_build=True) - except (KeyError, ValueError): - pass + if subject.short_product_version: + product_version = _guess_product_version( + subject.short_product_version, koji_build=subject.is_koji_build) + if product_version: + return product_version - if subject_type == "compose": - return _guess_product_version(subject_identifier) - - if subject_type == "redhat-module": - return "rhel-8" - - if koji_proxy: + if koji_proxy and subject.is_koji_build: return _guess_koji_build_product_version( - subject_identifier, koji_proxy, koji_task_id) + subject.identifier, koji_proxy, koji_task_id) diff --git a/greenwave/resources.py b/greenwave/resources.py index eda8380..6789041 100644 --- a/greenwave/resources.py +++ b/greenwave/resources.py @@ -54,10 +54,10 @@ class ResultsRetriever(BaseRetriever): super().__init__(**args) self.cache = {} - def _retrieve_all(self, subject_type, subject_identifier, testcase=None, scenarios=None): + def _retrieve_all(self, subject, testcase=None, scenarios=None): # Get test case result from cache if all test case results were already - # retrieved for given subject type/ID. - cache_key = (subject_type, subject_identifier, scenarios) + # retrieved for given Subject. + cache_key = (subject.type, subject.identifier, scenarios) if testcase and cache_key in self.cache: for result in self.cache[cache_key]: if result['testcase']['name'] == testcase: @@ -75,22 +75,9 @@ class ResultsRetriever(BaseRetriever): params.update({'scenario': ','.join(scenarios)}) results = [] - if subject_type == 'koji_build': - params['type'] = 'koji_build,brew-build' - params['item'] = subject_identifier - results = self._retrieve_data(params) - - del params['type'] - del params['item'] - params['original_spec_nvr'] = subject_identifier - results.extend(self._retrieve_data(params)) - elif subject_type == 'compose': - params['productmd.compose.id'] = subject_identifier - results = self._retrieve_data(params) - else: - params['type'] = subject_type - params['item'] = subject_identifier - results = self._retrieve_data(params) + for query in subject.result_queries(): + query.update(params) + results.extend(self._retrieve_data(query)) if not testcase: self.cache[cache_key] = results diff --git a/greenwave/subjects/compose.py b/greenwave/subjects/compose.py new file mode 100644 index 0000000..dc416b7 --- /dev/null +++ b/greenwave/subjects/compose.py @@ -0,0 +1,41 @@ +# SPDX-License-Identifier: GPL-2.0+ + +from .subject import Subject + + +def _get_latest_results(results): + """ + Yields only the latest results for unique architecture and variant pairs. + + The input results are sorted from latest to oldest. + """ + visited_arch_variants = set() + for result in results: + result_data = result["data"] + + # Items under test result "data" are lists which are unhashable + # types in Python. This converts anything that is stored there + # to a string so we don't have to care about the stored value. + arch_variant = ( + str(result_data.get("system_architecture")), + str(result_data.get("system_variant")), + ) + + if arch_variant not in visited_arch_variants: + visited_arch_variants.add(arch_variant) + yield result + + +class Compose(Subject): + type_ = "compose" + + @property + def short_product_version(self): + # FIXME: Is this really correct? + return self.item + + def to_item_dict(self): + return {"productmd.compose.id": self.item} + + def get_latest_results(self, results): + return list(_get_latest_results(results)) diff --git a/greenwave/subjects/factory.py b/greenwave/subjects/factory.py new file mode 100644 index 0000000..9edce90 --- /dev/null +++ b/greenwave/subjects/factory.py @@ -0,0 +1,49 @@ +# SPDX-License-Identifier: GPL-2.0+ +""" +Factory functions and helper functions to create subject object. +""" + +from .compose import Compose +from .generic import GenericSubject +from .koji_build import KojiBuild +from .redhat_module import RedHatModule + +# Subject subclasses (except GenericSubject) +# TODO: This could be moved to configuration as list of strings. +SUBJECT_CLASSES = ( + Compose, + KojiBuild, + RedHatModule, +) + + +def remote_rule_subject_types(): + return [ + cls.type_ + for cls in SUBJECT_CLASSES + if cls.supports_remote_rule + ] + + +def subject_type_and_id_from_dict(data): + compose = data.get("productmd.compose.id") + if compose: + return Compose.type_, compose + + nvr = data.get("original_spec_nvr") + if nvr: + return KojiBuild.type_, nvr + + return data.get("type"), data.get("item") + + +def create_subject(type_, identifier): + for cls in SUBJECT_CLASSES: + if type_ in cls.type_: + return cls(identifier) + + # Special case for backwards compatibility. + if type_ == "brew-build": + return KojiBuild(identifier) + + return GenericSubject(type_, identifier) diff --git a/greenwave/subjects/generic.py b/greenwave/subjects/generic.py new file mode 100644 index 0000000..ac62539 --- /dev/null +++ b/greenwave/subjects/generic.py @@ -0,0 +1,21 @@ +# SPDX-License-Identifier: GPL-2.0+ + +from .subject import Subject + + +class GenericSubject(Subject): + """ + Subject class for a generic subject. + + This is used when subject type does not require specialized functionality. + """ + + # Set in constructor. + type_ = None + + # Can be potentially Koji/Brew build. + is_koji_build = True + + def __init__(self, type_, item): + self.type_ = type_ + self.item = item diff --git a/greenwave/subjects/koji_build.py b/greenwave/subjects/koji_build.py new file mode 100644 index 0000000..855697e --- /dev/null +++ b/greenwave/subjects/koji_build.py @@ -0,0 +1,26 @@ +# SPDX-License-Identifier: GPL-2.0+ + +from .subject import Subject + + +class KojiBuild(Subject): + type_ = "koji_build" + supports_remote_rule = True + is_koji_build = True + + @property + def package_name(self): + return self.item.rsplit("-", 2)[0] + + @property + def short_product_version(self): + try: + _, _, release = self.identifier.rsplit("-", 2) + _, short_prod_version = release.rsplit(".", 1) + return short_prod_version + except (KeyError, ValueError): + return None + + def result_queries(self): + yield {"type": "koji_build,brew-build", "item": self.item} + yield {"original_spec_nvr": self.item} diff --git a/greenwave/subjects/redhat_module.py b/greenwave/subjects/redhat_module.py new file mode 100644 index 0000000..4b49024 --- /dev/null +++ b/greenwave/subjects/redhat_module.py @@ -0,0 +1,16 @@ +# SPDX-License-Identifier: GPL-2.0+ + +from .subject import Subject + + +class RedHatModule(Subject): + type_ = "redhat-module" + supports_remote_rule = True + + # TODO: It could be possible get product version from Koji/Brew instead of + # hard-coding it. + # is_koji_build = True + + @property + def short_product_version(self): + return "el8" diff --git a/greenwave/subjects/subject.py b/greenwave/subjects/subject.py new file mode 100644 index 0000000..8a2462c --- /dev/null +++ b/greenwave/subjects/subject.py @@ -0,0 +1,87 @@ +# SPDX-License-Identifier: GPL-2.0+ + + +class Subject: + """ + Base class for decision subjects. + + Main properties are type and item/identifier. + + Type can be, for example, Koji build, compose, module or other. It must be + uniquely defined in subclasses. + + Item or identifier should uniquely identify the artefact (test subject). + """ + + """ + Subject type string. + + Override in subclass. + + Factory function uses this to determine which subclass to use for + instantiation. + """ + type_ = None + + """ + Subject can be used with RemoteRule only if value is True. + """ + supports_remote_rule = False + + """ + A build for subject identifier can be found on Koji/Brew. + """ + is_koji_build = False + + def __init__(self, item): + self.item = item + + @property + def type(self): + """Subject type string.""" + return self.type_ + + @property + def identifier(self): + """Alias for item property.""" + return self.item + + @property + def package_name(self): + """Package name of the subject or None.""" + return None + + @property + def short_product_version(self): + """Get short product version of the subject (guess from identifier) or None.""" + return None + + def to_item_dict(self): + """ + Greenwave < 0.8 included an "item" key in the "unsatisfied_requirements". + This returns a suitable value for that key, for backwards compatibility. + """ + return {"type": self.type, "item": self.item} + + def result_queries(self): + """ + Yields parameters for RestulsDB queries. + + For example, one set of parameters could have "type=koji_build" for one + query and "type=brew-build" for another. + """ + yield self.to_item_dict() + + def get_latest_results(self, results): + """ + Filters out results to get only the latest relevant ones. + + The input results are sorted from latest to oldest. + """ + return results + + # TODO: Test __str__ and __repr__. + def __str__(self): + return "subject_type {!r}, subject_identifier {!r}".format( + self.type_, self.item + ) diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py index 4759d18..0e0152f 100644 --- a/greenwave/tests/test_policies.py +++ b/greenwave/tests/test_policies.py @@ -22,33 +22,33 @@ from greenwave.policies import ( ) from greenwave.resources import ResultsRetriever from greenwave.safe_yaml import SafeYAMLError +from greenwave.subjects.generic import GenericSubject +from greenwave.subjects.koji_build import KojiBuild +from greenwave.subjects.redhat_module import RedHatModule from greenwave.waivers import waive_answers class DummyResultsRetriever(ResultsRetriever): - def __init__( - self, subject_identifier=None, testcase=None, outcome='PASSED', - subject_type='koji_build'): + def __init__(self, subject=None, testcase=None, outcome='PASSED'): super(DummyResultsRetriever, self).__init__( ignore_ids=[], when='', timeout=0, verify=False, url='') - self.subject_identifier = subject_identifier - self.subject_type = subject_type + self.subject = subject self.testcase = testcase self.outcome = outcome 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 + if (self.subject and 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): return [{ 'id': 123, 'data': { - 'item': [self.subject_identifier], - 'type': [self.subject_type], + 'item': [self.subject.identifier], + 'type': [self.subject.type], }, 'testcase': {'name': self.testcase}, 'outcome': self.outcome, @@ -57,21 +57,22 @@ class DummyResultsRetriever(ResultsRetriever): def test_summarize_answers(): + subject = KojiBuild('nvr') assert summarize_answers([RuleSatisfied()]) == \ 'All required tests passed' - assert summarize_answers([TestResultFailed('koji_build', 'nvr', 'test', None, 'id'), + assert summarize_answers([TestResultFailed(subject, 'test', None, 'id'), RuleSatisfied()]) == \ '1 of 2 required tests failed' - assert summarize_answers([TestResultMissing('koji_build', 'nvr', 'test', None)]) == \ + assert summarize_answers([TestResultMissing(subject, 'test', None)]) == \ '1 of 1 required test results missing' - assert summarize_answers([TestResultMissing('koji_build', 'nvr', 'test', None), - TestResultFailed('koji_build', 'nvr', 'test', None, 'id')]) == \ + assert summarize_answers([TestResultMissing(subject, 'test', None), + TestResultFailed(subject, 'test', None, 'id')]) == \ '1 of 2 required tests failed, 1 result missing' - assert summarize_answers([TestResultMissing('koji_build', 'nvr', 'testa', None), - TestResultMissing('koji_build', 'nvr', 'testb', None), - TestResultFailed('koji_build', 'nvr', 'test', None, 'id')]) == \ + assert summarize_answers([TestResultMissing(subject, 'testa', None), + TestResultMissing(subject, 'testb', None), + TestResultFailed(subject, 'test', None, 'id')]) == \ '1 of 3 required tests failed, 2 results missing' - assert summarize_answers([TestResultMissing('koji_build', 'nvr', 'test', None), + assert summarize_answers([TestResultMissing(subject, 'test', None), RuleSatisfied()]) == \ '1 of 2 required test results missing' @@ -92,10 +93,10 @@ def test_decision_with_missing_result(tmpdir): policy = policies[0] results = DummyResultsRetriever() - subject_identifier = 'some_nevr' + subject = KojiBuild('some_nevr') # Ensure that absence of a result is failure. - decision = policy.check('fedora-rawhide', subject_identifier, results) + decision = policy.check('fedora-rawhide', subject, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultMissing) @@ -122,9 +123,10 @@ def test_waive_brew_koji_mismatch(tmpdir): policy = policies[0] item = 'some_nevr' - results = DummyResultsRetriever(item, 'sometest', 'FAILED', 'brew-build') + subject = KojiBuild(item) + results = DummyResultsRetriever(subject, 'sometest', 'FAILED') - decision = policy.check('fedora-rawhide', item, results) + decision = policy.check('fedora-rawhide', subject, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultFailed) @@ -136,7 +138,7 @@ def test_waive_brew_koji_mismatch(tmpdir): 'product_version': 'fedora-rawhide', 'waived': True, }] - decision = policy.check('fedora-rawhide', item, results) + decision = policy.check('fedora-rawhide', subject, results) decision = waive_answers(decision, waivers) assert len(decision) == 1 assert isinstance(decision[0], RuleSatisfied) @@ -164,9 +166,10 @@ def test_waive_bodhi_update(tmpdir): policy = policies[0] item = 'some_bodhi_update' - results = DummyResultsRetriever(item, 'sometest', 'FAILED', 'bodhi_update') + subject = GenericSubject('bodhi_update', item) + results = DummyResultsRetriever(subject, 'sometest', 'FAILED') - decision = policy.check('fedora-rawhide', item, results) + decision = policy.check('fedora-rawhide', subject, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultFailed) @@ -178,7 +181,7 @@ def test_waive_bodhi_update(tmpdir): 'product_version': 'fedora-rawhide', 'waived': True, }] - decision = policy.check('fedora-rawhide', item, results) + decision = policy.check('fedora-rawhide', subject, results) decision = waive_answers(decision, waivers) assert len(decision) == 1 assert isinstance(decision[0], RuleSatisfied) @@ -280,7 +283,7 @@ def test_remote_rule_policy(tmpdir, namespace): """ Testing the RemoteRule with the koji interaction. In this case we are just mocking koji """ - nvr = 'nethack-1.2.3-1.el9000' + subject = KojiBuild('nethack-1.2.3-1.el9000') serverside_fragment = dedent(""" --- !Policy @@ -315,20 +318,20 @@ def test_remote_rule_policy(tmpdir, namespace): policy = policies[0] # Ensure that presence of a result is success. - results = DummyResultsRetriever(nvr, 'dist.upgradepath') - decision = policy.check('fedora-26', nvr, results) + results = DummyResultsRetriever(subject, 'dist.upgradepath') + decision = policy.check('fedora-26', subject, 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) + decision = policy.check('fedora-26', subject, 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) + results = DummyResultsRetriever(subject, 'dist.upgradepath', 'FAILED') + decision = policy.check('fedora-26', subject, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultFailed) @@ -339,6 +342,7 @@ def test_remote_rule_policy_redhat_module(tmpdir, namespace): In this case we are just mocking koji """ nvr = '389-ds-1.4-820181127205924.9edba152' + subject = RedHatModule(nvr) serverside_fragment = dedent(""" --- !Policy @@ -374,28 +378,27 @@ def test_remote_rule_policy_redhat_module(tmpdir, namespace): policy = policies[0] # 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) + results = DummyResultsRetriever(subject, 'baseos-ci.redhat-module.tier0.functional') + decision = policy.check('rhel-8', subject, 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) + results = DummyResultsRetriever(subject) + decision = policy.check('rhel-8', subject, 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) + results = DummyResultsRetriever( + subject, 'baseos-ci.redhat-module.tier0.functional', 'FAILED') + decision = policy.check('rhel-8', subject, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultFailed) def test_remote_rule_policy_optional_id(tmpdir): - nvr = 'nethack-1.2.3-1.el9000' + subject = KojiBuild('nethack-1.2.3-1.el9000') serverside_fragment = dedent(""" --- !Policy @@ -428,7 +431,7 @@ def test_remote_rule_policy_optional_id(tmpdir): results = DummyResultsRetriever() expected_details = "Policy 'untitled': Attribute 'product_versions' is required" - decision = policy.check('fedora-26', nvr, results) + decision = policy.check('fedora-26', subject, results) assert len(decision) == 1 assert isinstance(decision[0], InvalidGatingYaml) assert decision[0].is_satisfied is False @@ -438,7 +441,7 @@ def test_remote_rule_policy_optional_id(tmpdir): def test_remote_rule_malformed_yaml(tmpdir): """ Testing the RemoteRule with a malformed gating.yaml file """ - nvr = 'nethack-1.2.3-1.el9000' + subject = KojiBuild('nethack-1.2.3-1.el9000') serverside_fragment = dedent(""" --- !Policy @@ -483,7 +486,7 @@ def test_remote_rule_malformed_yaml(tmpdir): policy = policies[0] results = DummyResultsRetriever() - decision = policy.check('fedora-26', nvr, results) + decision = policy.check('fedora-26', subject, results) assert len(decision) == 1 assert isinstance(decision[0], InvalidGatingYaml) assert decision[0].is_satisfied is False @@ -493,7 +496,7 @@ def test_remote_rule_malformed_yaml_with_waiver(tmpdir): """ Testing the RemoteRule with a malformed gating.yaml file But this time waiving the error """ - nvr = 'nethack-1.2.3-1.el9000' + subject = KojiBuild('nethack-1.2.3-1.el9000') serverside_fragment = dedent(""" --- !Policy @@ -548,14 +551,14 @@ def test_remote_rule_malformed_yaml_with_waiver(tmpdir): 'comment': 'Waiving the invalid gating.yaml file', 'waived': True, }] - decision = policy.check('fedora-26', nvr, results) + decision = policy.check('fedora-26', subject, results) decision = waive_answers(decision, waivers) assert len(decision) == 0 def test_remote_rule_required(): """ Testing the RemoteRule with required flag set """ - nvr = 'nethack-1.2.3-1.el9000' + subject = KojiBuild('nethack-1.2.3-1.el9000') app = create_app('greenwave.config.TestingConfig') with app.app_context(): with mock.patch('greenwave.resources.retrieve_scm_from_koji') as scm: @@ -573,11 +576,11 @@ def test_remote_rule_required(): """)) policy = policies[0] results = DummyResultsRetriever() - decision = policy.check('fedora-rawhide', nvr, results) + decision = policy.check('fedora-rawhide', subject, results) assert len(decision) == 1 assert isinstance(decision[0], MissingGatingYaml) assert not decision[0].is_satisfied - assert decision[0].subject_identifier == nvr + assert decision[0].subject.identifier == subject.identifier def test_parse_policies_missing_tag(): @@ -653,8 +656,9 @@ def test_policy_with_arbitrary_subject_type(tmpdir): policies = load_policies(tmpdir.strpath) policy = policies[0] - 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) + subject = GenericSubject('kind-of-magic', 'nethack-1.2.3-1.el9000') + results = DummyResultsRetriever(subject, 'sometest', 'PASSED') + decision = policy.check('rhel-9000', subject, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultPassed) @@ -681,8 +685,9 @@ def test_policy_with_packages_whitelist(tmpdir, package, num_decisions): policies = load_policies(tmpdir.strpath) policy = policies[0] - results = DummyResultsRetriever('nethack-1.2.3-1.el9000', 'sometest', 'PASSED', 'koji_build') - decision = policy.check('rhel-9000', 'nethack-1.2.3-1.el9000', results) + subject = KojiBuild('nethack-1.2.3-1.el9000') + results = DummyResultsRetriever(subject, 'sometest', 'PASSED') + decision = policy.check('rhel-9000', subject, results) assert len(decision) == num_decisions if num_decisions: assert isinstance(decision[0], TestResultPassed) @@ -861,6 +866,7 @@ def test_policies_to_json(): def test_policy_with_subject_type_component_version(tmpdir): nv = '389-ds-base-1.4.0.10' + subject = GenericSubject('component-version', nv) p = tmpdir.join('fedora.yaml') p.write(dedent(""" --- !Policy @@ -875,15 +881,15 @@ def test_policy_with_subject_type_component_version(tmpdir): """)) policies = load_policies(tmpdir.strpath) policy = policies[0] - results = DummyResultsRetriever(nv, 'test_for_new_type', 'PASSED', - 'component-version') - decision = policy.check('fedora-29', nv, results) + results = DummyResultsRetriever(subject, 'test_for_new_type', 'PASSED') + decision = policy.check('fedora-29', subject, results) assert len(decision) == 1 assert isinstance(decision[0], RuleSatisfied) def test_policy_with_subject_type_redhat_module(tmpdir): nsvc = 'httpd:2.4:20181018085700:9edba152' + subject = RedHatModule(nsvc) p = tmpdir.join('fedora.yaml') p.write(dedent(""" --- !Policy @@ -898,9 +904,8 @@ def test_policy_with_subject_type_redhat_module(tmpdir): """)) policies = load_policies(tmpdir.strpath) policy = policies[0] - results = DummyResultsRetriever(nsvc, 'test_for_redhat_module_type', 'PASSED', - 'redhat-module') - decision = policy.check('fedora-29', nsvc, results) + results = DummyResultsRetriever(subject, 'test_for_redhat_module_type', 'PASSED') + decision = policy.check('fedora-29', subject, results) assert len(decision) == 1 assert isinstance(decision[0], RuleSatisfied) @@ -911,6 +916,7 @@ def test_remote_rule_policy_on_demand_policy(namespace): In this case we are just mocking koji """ nvr = 'nethack-1.2.3-1.el9000' + subject = KojiBuild(nvr) serverside_json = { 'product_version': 'fedora-26', @@ -943,19 +949,19 @@ def test_remote_rule_policy_on_demand_policy(namespace): policy = OnDemandPolicy.create_from_json(serverside_json) # Ensure that presence of a result is success. - results = DummyResultsRetriever(nvr, 'dist.upgradepath') - decision = policy.check('fedora-26', nvr, results) + results = DummyResultsRetriever(subject, 'dist.upgradepath') + decision = policy.check('fedora-26', subject, 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) + decision = policy.check('fedora-26', subject, 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) + results = DummyResultsRetriever(subject, 'dist.upgradepath', 'FAILED') + decision = policy.check('fedora-26', subject, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultFailed) diff --git a/greenwave/tests/test_resultsdb_consumer.py b/greenwave/tests/test_resultsdb_consumer.py index 21669ae..e8ffd7f 100644 --- a/greenwave/tests/test_resultsdb_consumer.py +++ b/greenwave/tests/test_resultsdb_consumer.py @@ -9,6 +9,7 @@ import greenwave.app_factory import greenwave.consumers.resultsdb from greenwave.product_versions import subject_product_version from greenwave.policies import Policy +from greenwave.subjects.factory import create_subject def test_announcement_keys_decode_with_list(): @@ -16,9 +17,8 @@ def test_announcement_keys_decode_with_list(): message = {'msg': {'data': { 'original_spec_nvr': ['glibc-1.0-1.fc27'], }}} - subjects = list(cls.announcement_subjects(message)) - assert subjects == [('koji_build', 'glibc-1.0-1.fc27')] + assert cls.announcement_subject(message) == ('koji_build', 'glibc-1.0-1.fc27') def test_no_announcement_subjects_for_empty_nvr(): @@ -32,9 +32,8 @@ def test_no_announcement_subjects_for_empty_nvr(): message = {'msg': {'data': { 'original_spec_nvr': [""], }}} - subjects = list(cls.announcement_subjects(message)) - assert subjects == [] + assert cls.announcement_subject(message) == (None, None) def test_announcement_subjects_for_brew_build(): @@ -45,9 +44,8 @@ def test_announcement_subjects_for_brew_build(): 'type': 'brew-build', 'item': ['glibc-1.0-3.fc27'], }}} - subjects = list(cls.announcement_subjects(message)) - assert subjects == [('koji_build', 'glibc-1.0-3.fc27')] + assert cls.announcement_subject(message) == ('brew-build', 'glibc-1.0-3.fc27') def test_announcement_subjects_for_new_compose_message(): @@ -78,9 +76,8 @@ def test_announcement_subjects_for_new_compose_message(): } } } - subjects = list(cls.announcement_subjects(message)) - assert subjects == [('compose', 'Fedora-Rawhide-20181205.n.0')] + assert cls.announcement_subject(message) == ('compose', 'Fedora-Rawhide-20181205.n.0') def test_no_announcement_subjects_for_old_compose_message(): @@ -106,9 +103,8 @@ def test_no_announcement_subjects_for_old_compose_message(): } } } - subjects = list(cls.announcement_subjects(message)) - assert subjects == [] + assert cls.announcement_subject(message) == (None, None) parameters = [ @@ -321,12 +317,12 @@ def test_guess_product_version(): } handler = greenwave.consumers.resultsdb.ResultsDBHandler(hub) with handler.flask_app.app_context(): - product_version = subject_product_version( - 'release-e2e-test-1.0.1685-1.el5', 'koji_build') + subject = create_subject('koji_build', 'release-e2e-test-1.0.1685-1.el5') + product_version = subject_product_version(subject) assert product_version == 'rhel-5' - product_version = subject_product_version( - 'rust-toolset-rhel8-20181010170614.b09eea91', 'redhat-module') + subject = create_subject('redhat-module', 'rust-toolset-rhel8-20181010170614.b09eea91') + product_version = subject_product_version(subject) assert product_version == 'rhel-8' @@ -335,8 +331,8 @@ def test_guess_product_version_with_koji(): koji_proxy.getBuild.return_value = {'task_id': 666} koji_proxy.getTaskRequest.return_value = ['git://example.com/project', 'rawhide', {}] - product_version = subject_product_version( - 'fake_koji_build', 'container-build', koji_proxy) + subject = create_subject('container-build', 'fake_koji_build') + product_version = subject_product_version(subject, koji_proxy) koji_proxy.getBuild.assert_called_once_with('fake_koji_build') koji_proxy.getTaskRequest.assert_called_once_with(666) assert product_version == 'fedora-rawhide' @@ -346,8 +342,8 @@ def test_guess_product_version_with_koji_without_task_id(): koji_proxy = mock.MagicMock() koji_proxy.getBuild.return_value = {'task_id': None} - product_version = subject_product_version( - 'fake_koji_build', 'container-build', koji_proxy) + subject = create_subject('container-build', 'fake_koji_build') + product_version = subject_product_version(subject, koji_proxy) koji_proxy.getBuild.assert_called_once_with('fake_koji_build') koji_proxy.getTaskRequest.assert_not_called() @@ -361,7 +357,8 @@ def test_guess_product_version_with_koji_without_task_id(): 'badnvr-1.2.f30', )) def test_guess_product_version_failure(nvr): - product_version = subject_product_version(nvr, 'koji_build') + subject = create_subject('koji_build', nvr) + product_version = subject_product_version(subject) assert product_version is None diff --git a/greenwave/tests/test_rules.py b/greenwave/tests/test_rules.py index cb5c2d8..82d6b41 100644 --- a/greenwave/tests/test_rules.py +++ b/greenwave/tests/test_rules.py @@ -6,6 +6,7 @@ from textwrap import dedent from greenwave.app_factory import create_app from greenwave.policies import Policy, RemoteRule from greenwave.safe_yaml import SafeYAMLError +from greenwave.subjects.koji_build import KojiBuild def test_match_passing_test_case_rule(): @@ -50,6 +51,7 @@ def test_match_remote_rule(mock_retrieve_scm_from_koji, mock_retrieve_yaml_remot - !PassingTestCaseRule {test_case_name: some_test_case} """) nvr = 'nethack-1.2.3-1.el9000' + subject = KojiBuild(nvr) mock_retrieve_scm_from_koji.return_value = ('rpms', nvr, '123') app = create_app('greenwave.config.TestingConfig') @@ -62,9 +64,9 @@ def test_match_remote_rule(mock_retrieve_scm_from_koji, mock_retrieve_yaml_remot rule = policy.rules[0] assert rule.matches(policy) - assert rule.matches(policy, subject_identifier=nvr) - assert rule.matches(policy, subject_identifier=nvr, testcase='some_test_case') - assert not rule.matches(policy, subject_identifier=nvr, testcase='other_test_case') + assert rule.matches(policy, subject=subject) + assert rule.matches(policy, subject=subject, testcase='some_test_case') + assert not rule.matches(policy, subject=subject, testcase='other_test_case') # Simulate invalid gating.yaml file. def raiseYamlError(*args): @@ -73,9 +75,9 @@ def test_match_remote_rule(mock_retrieve_scm_from_koji, mock_retrieve_yaml_remot mock_retrieve_yaml_remote_rule.side_effect = raiseYamlError assert rule.matches(policy) - assert not rule.matches(policy, subject_identifier=nvr) - assert not rule.matches(policy, subject_identifier=nvr, testcase='some_test_case') - assert not rule.matches(policy, subject_identifier=nvr, testcase='other_test_case') + assert not rule.matches(policy, subject=subject) + assert not rule.matches(policy, subject=subject, testcase='some_test_case') + assert not rule.matches(policy, subject=subject, testcase='other_test_case') @pytest.mark.parametrize(('required_flag', 'required_value'), ( diff --git a/greenwave/tests/test_summary.py b/greenwave/tests/test_summary.py index 199d311..1d1628e 100644 --- a/greenwave/tests/test_summary.py +++ b/greenwave/tests/test_summary.py @@ -8,16 +8,19 @@ from greenwave.policies import ( InvalidGatingYaml, ) +from greenwave.subjects.koji_build import KojiBuild + +testSubject = KojiBuild('nethack-1.2.3-1.el9000') testResultPassed = RuleSatisfied() testResultFailed = TestResultFailed( - 'koji_build', 'nethack-1.2.3-1.el9000', 'test', None, 1) + testSubject, 'test', None, 1) testResultMissing = TestResultMissing( - 'koji_build', 'nethack-1.2.3-1.el9000', 'test', None) + testSubject, 'test', None) testResultMissingWaived = TestResultMissingWaived( - 'koji_build', 'nethack-1.2.3-1.el9000', 'test', None) + testSubject, 'test', None) testInvalidGatingYaml = InvalidGatingYaml( - 'koji_build', 'nethack-1.2.3-1.el9000', 'test', 'Missing !Policy tag') + testSubject, 'test', 'Missing !Policy tag') def test_summary_passed(): diff --git a/greenwave/tests/test_waive.py b/greenwave/tests/test_waive.py index 036ab6a..3ecb00b 100644 --- a/greenwave/tests/test_waive.py +++ b/greenwave/tests/test_waive.py @@ -5,14 +5,14 @@ from greenwave.policies import ( TestResultMissing, TestResultFailed, ) +from greenwave.subjects.koji_build import KojiBuild 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', + subject=KojiBuild('nethack-1.2.3-1.rawhide'), test_case_name='test1', scenario='scenario1', result_id=99, @@ -24,7 +24,7 @@ def test_waive_failed_result(): waivers = [ dict( - subject_type='koji-build', + subject_type='koji_build', subject_identifier='nethack-1.2.3-1.rawhide', product_version='rawhide', testcase='test1', @@ -43,8 +43,7 @@ def test_waive_failed_result(): def test_waive_missing_result(): answers = [ TestResultMissing( - subject_type='koji-build', - subject_identifier='nethack-1.2.3-1.rawhide', + subject=KojiBuild('nethack-1.2.3-1.rawhide'), test_case_name='test1', scenario='scenario1', ) @@ -55,7 +54,7 @@ def test_waive_missing_result(): waivers = [ dict( - subject_type='koji-build', + subject_type='koji_build', subject_identifier='nethack-1.2.3-1.rawhide', product_version='rawhide', testcase='test1', @@ -65,7 +64,7 @@ def test_waive_missing_result(): expected_json = dict( type='test-result-missing-waived', testcase='test1', - subject_type='koji-build', + subject_type='koji_build', subject_identifier='nethack-1.2.3-1.rawhide', scenario='scenario1', ) @@ -76,8 +75,7 @@ def test_waive_missing_result(): def test_waive_invalid_gatin_yaml(): answers = [ InvalidGatingYaml( - subject_type='koji-build', - subject_identifier='nethack-1.2.3-1.rawhide', + subject=KojiBuild('nethack-1.2.3-1.rawhide'), test_case_name='invalid-gating-yaml', details='', ) @@ -88,7 +86,7 @@ def test_waive_invalid_gatin_yaml(): waivers = [ dict( - subject_type='koji-build', + subject_type='koji_build', subject_identifier='nethack-1.2.3-1.rawhide', product_version='rawhide', testcase='invalid-gating-yaml', diff --git a/greenwave/tests/test_waivers_retriever.py b/greenwave/tests/test_waivers_retriever.py index ff6b8e6..f1c2766 100644 --- a/greenwave/tests/test_waivers_retriever.py +++ b/greenwave/tests/test_waivers_retriever.py @@ -21,7 +21,7 @@ def test_waivers_retriever_retrieves_not_ignored_ids(): retriever.ignore_ids = [100] waiver = dict( id=99, - subject_type='koji-build', + subject_type='koji_build', subject_identifier='nethack-1.2.3-1.rawhide', product_version='rawhide', testcase='test1', @@ -38,7 +38,7 @@ def test_waivers_retriever_ignores_ids(): retriever.ignore_ids = [99] waiver = dict( id=99, - subject_type='koji-build', + subject_type='koji_build', subject_identifier='nethack-1.2.3-1.rawhide', product_version='rawhide', testcase='test1', @@ -54,7 +54,7 @@ def test_waivers_retriever_ignores_no_waived(): retriever = WaiversRetriever(**_DUMMY_RETRIEVER_ARGUMENTS) waiver = dict( id=99, - subject_type='koji-build', + subject_type='koji_build', subject_identifier='nethack-1.2.3-1.rawhide', product_version='rawhide', testcase='test1', diff --git a/greenwave/waivers.py b/greenwave/waivers.py index 4e43590..45d79ce 100644 --- a/greenwave/waivers.py +++ b/greenwave/waivers.py @@ -6,8 +6,8 @@ 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['subject_type'] == answer.subject.type and + waiver['subject_identifier'] == answer.subject.identifier and waiver['testcase'] == answer.test_case_name for waiver in waivers)