From acefaaedf14bc235e4eb0d900eff75f3dc4b5b2e Mon Sep 17 00:00:00 2001
From: Lukas Holecek <hluk@email.cz>
Date: Aug 27 2019 14:44:00 +0000
Subject: Use brew_task_id in ResultsDB messages to guess product version


Gets Brew task ID from the UMB message instead of using additional Brew
XMLRPC call.

This also solves an issues with containers which store task ID in
'container_koji_task_id' field (under 'extra' dict) instead of
'task_id'.

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

---

diff --git a/greenwave/consumers/resultsdb.py b/greenwave/consumers/resultsdb.py
index 1b0375d..8f622b1 100644
--- a/greenwave/consumers/resultsdb.py
+++ b/greenwave/consumers/resultsdb.py
@@ -10,69 +10,35 @@ to the message bus about the newly satisfied/unsatisfied policy.
 """
 
 import logging
-import re
 
 from greenwave.consumers.consumer import Consumer
+from greenwave.product_versions import subject_product_version
 
 import xmlrpc.client
 
 log = logging.getLogger(__name__)
 
 
-def _guess_product_version(toparse, koji_build=False):
-    if toparse == 'rawhide' or toparse.startswith('Fedora-Rawhide'):
-        return 'fedora-rawhide'
-
-    product_version = None
-    if toparse.startswith('f') and koji_build:
-        product_version = 'fedora-'
-    elif toparse.startswith('epel'):
-        product_version = 'epel-'
-    elif toparse.startswith('el') and len(toparse) > 2 and toparse[2].isdigit():
-        product_version = 'rhel-'
-    elif toparse.startswith('fc') or toparse.startswith('Fedora'):
-        product_version = 'fedora-'
-
-    if product_version:
-        # seperate the prefix from the number
-        result = list(filter(None, '-'.join(re.split(r'(\d+)', toparse)).split('-')))
-        if len(result) >= 2:
-            try:
-                int(result[1])
-                product_version += result[1]
-                return product_version
-            except ValueError:
-                pass
-
-    log.error("It wasn't possible to guess the product version")
-    return None
-
-
-def _subject_product_version(subject_identifier, subject_type, koji_proxy=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
+def _unpack_value(value):
+    """
+    If value is list with single element, returns the element, otherwise
+    returns the value.
+    """
+    if isinstance(value, list) and len(value) == 1:
+        value = value[0]
+    return value
 
-    if subject_type == "compose":
-        return _guess_product_version(subject_identifier)
 
-    if subject_type == "redhat-module":
-        return "rhel-8"
+def _get_brew_task_id(msg):
+    data = msg.get('data')
+    if not data:
+        return None
 
-    if koji_proxy:
-        try:
-            build = koji_proxy.getBuild(subject_identifier)
-            if build:
-                target = koji_proxy.getTaskRequest(build['task_id'])[1]
-                return _guess_product_version(target, koji_build=True)
-        except KeyError:
-            pass
-        except xmlrpc.client.Fault:
-            pass
+    task_id = _unpack_value(data.get('brew_task_id'))
+    try:
+        return int(task_id)
+    except (ValueError, TypeError):
+        return None
 
 
 class ResultsDBHandler(Consumer):
@@ -112,13 +78,7 @@ class ResultsDBHandler(Consumer):
         except KeyError:
             data = message['msg']['task']  # Old format
 
-        def _decode(value):
-            """ Decode either a string or a list of strings. """
-            if value and len(value) == 1:
-                value = value[0]
-            return value
-
-        _type = _decode(data.get('type'))
+        _type = _unpack_value(data.get('type'))
         # 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
@@ -129,20 +89,20 @@ class ResultsDBHandler(Consumer):
         # 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', _decode(data['productmd.compose.id']))
+            yield ('compose', _unpack_value(data['productmd.compose.id']))
         elif _type == 'compose':
             pass
         elif 'original_spec_nvr' in data:
-            nvr = _decode(data['original_spec_nvr'])
+            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', _decode(data['item']))
+            yield ('koji_build', _unpack_value(data['item']))
         elif 'item' in data and _type:
-            yield (_type, _decode(data['item']))
+            yield (_type, _unpack_value(data['item']))
 
     def _consume_message(self, message):
         msg = message['msg']
@@ -157,11 +117,17 @@ class ResultsDBHandler(Consumer):
         except KeyError:
             submit_time = msg['result']['submit_time']
 
+        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)
+            product_version = subject_product_version(
+                subject_identifier,
+                subject_type,
+                self.koji_proxy,
+                brew_task_id,
+            )
 
             self._publish_decision_change(
                 submit_time=submit_time,
diff --git a/greenwave/product_versions.py b/greenwave/product_versions.py
new file mode 100644
index 0000000..f83dec6
--- /dev/null
+++ b/greenwave/product_versions.py
@@ -0,0 +1,79 @@
+# SPDX-License-Identifier: GPL-2.0+
+"""
+Product version guessing for subject identifiers
+"""
+
+import logging
+import re
+
+import xmlrpc.client
+
+log = logging.getLogger(__name__)
+
+
+def _guess_product_version(toparse, koji_build=False):
+    if toparse == 'rawhide' or toparse.startswith('Fedora-Rawhide'):
+        return 'fedora-rawhide'
+
+    product_version = None
+    if toparse.startswith('f') and koji_build:
+        product_version = 'fedora-'
+    elif toparse.startswith('epel'):
+        product_version = 'epel-'
+    elif toparse.startswith('el') and len(toparse) > 2 and toparse[2].isdigit():
+        product_version = 'rhel-'
+    elif toparse.startswith('fc') or toparse.startswith('Fedora'):
+        product_version = 'fedora-'
+
+    if product_version:
+        # seperate the prefix from the number
+        result = list(filter(None, '-'.join(re.split(r'(\d+)', toparse)).split('-')))
+        if len(result) >= 2:
+            try:
+                int(result[1])
+                product_version += result[1]
+                return product_version
+            except ValueError:
+                pass
+
+    log.error("It wasn't possible to guess the product version")
+    return None
+
+
+def _guess_koji_build_product_version(
+        subject_identifier, koji_proxy, koji_task_id=None):
+    try:
+        if not koji_task_id:
+            build = koji_proxy.getBuild(subject_identifier) or {}
+            koji_task_id = build.get('task_id')
+            if not koji_task_id:
+                return None
+
+        target = koji_proxy.getTaskRequest(koji_task_id)[1]
+        return _guess_product_version(target, koji_build=True)
+    except xmlrpc.client.Fault:
+        pass
+
+
+def subject_product_version(
+        subject_identifier,
+        subject_type,
+        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_type == "compose":
+        return _guess_product_version(subject_identifier)
+
+    if subject_type == "redhat-module":
+        return "rhel-8"
+
+    if koji_proxy:
+        return _guess_koji_build_product_version(
+            subject_identifier, koji_proxy, koji_task_id)
diff --git a/greenwave/tests/test_resultsdb_consumer.py b/greenwave/tests/test_resultsdb_consumer.py
index fe7fdf7..21669ae 100644
--- a/greenwave/tests/test_resultsdb_consumer.py
+++ b/greenwave/tests/test_resultsdb_consumer.py
@@ -7,6 +7,7 @@ from textwrap import dedent
 
 import greenwave.app_factory
 import greenwave.consumers.resultsdb
+from greenwave.product_versions import subject_product_version
 from greenwave.policies import Policy
 
 
@@ -313,7 +314,6 @@ def test_remote_rule_decision_change_not_matching(
 
 
 def test_guess_product_version():
-    # pylint: disable=protected-access
     hub = mock.MagicMock()
     hub.config = {
         'environment': 'environment',
@@ -321,32 +321,39 @@ def test_guess_product_version():
     }
     handler = greenwave.consumers.resultsdb.ResultsDBHandler(hub)
     with handler.flask_app.app_context():
-        product_version = greenwave.consumers.resultsdb._subject_product_version(
+        product_version = subject_product_version(
             'release-e2e-test-1.0.1685-1.el5', 'koji_build')
         assert product_version == 'rhel-5'
 
-        product_version = greenwave.consumers.resultsdb._subject_product_version(
+        product_version = subject_product_version(
             'rust-toolset-rhel8-20181010170614.b09eea91', 'redhat-module')
         assert product_version == 'rhel-8'
 
 
 def test_guess_product_version_with_koji():
-    # pylint: disable=protected-access,unused-variable
-    class DummyKojiProxy():
-        def getBuild(self, subject_identifier):
-            assert 'fake_koji_build' == subject_identifier
-
     koji_proxy = mock.MagicMock()
     koji_proxy.getBuild.return_value = {'task_id': 666}
     koji_proxy.getTaskRequest.return_value = ['git://example.com/project', 'rawhide', {}]
 
-    product_version = greenwave.consumers.resultsdb._subject_product_version(
+    product_version = subject_product_version(
         'fake_koji_build', 'container-build', 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'
 
 
+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)
+
+    koji_proxy.getBuild.assert_called_once_with('fake_koji_build')
+    koji_proxy.getTaskRequest.assert_not_called()
+    assert product_version is None
+
+
 @pytest.mark.parametrize('nvr', (
     'badnvr.elastic-1-228',
     'badnvr-1.2-1.elastic8',
@@ -354,8 +361,7 @@ def test_guess_product_version_with_koji():
     'badnvr-1.2.f30',
 ))
 def test_guess_product_version_failure(nvr):
-    # pylint: disable=protected-access
-    product_version = greenwave.consumers.resultsdb._subject_product_version(nvr, 'koji_build')
+    product_version = subject_product_version(nvr, 'koji_build')
     assert product_version is None
 
 
@@ -594,3 +600,91 @@ def test_real_fedora_messaging_msg(
                 'subject_identifier': 'FEDORA-2019-9244c8b209',
                 'previous': None,
             }
+
+
+@mock.patch('greenwave.resources.ResultsRetriever.retrieve')
+@mock.patch('greenwave.resources.retrieve_decision')
+def test_container_brew_build(
+        mock_retrieve_decision,
+        mock_retrieve_results):
+    message = {
+        'msg': {
+            'submit_time': '2019-08-27T13:57:53.490376',
+            'testcase': {
+                'name': 'example_test',
+            },
+            'data': {
+                'brew_task_id': ['666'],
+                'type': ['brew-build'],
+                'item': ['example-container'],
+            }
+        }
+    }
+
+    policies = dedent("""
+        --- !Policy
+        id: test_policy
+        product_versions: [example_product_version]
+        decision_context: test_context
+        subject_type: koji_build
+        rules:
+          - !PassingTestCaseRule {test_case_name: example_test}
+    """)
+
+    config = 'fedora-messaging'
+    publish = 'greenwave.consumers.consumer.fedora_messaging.api.publish'
+
+    with mock.patch('greenwave.config.Config.MESSAGING', config):
+        with mock.patch(publish) as mock_fedmsg:
+            result = {
+                'id': 1,
+                'testcase': {'name': 'example_test'},
+                'outcome': 'PASSED',
+                'data': {'item': 'example-container', 'type': 'koji_build'},
+                'submit_time': '2019-04-24 13:06:12.135146'
+            }
+            mock_retrieve_results.return_value = [result]
+
+            def retrieve_decision(url, data):
+                #pylint: disable=unused-argument
+                if 'when' in data:
+                    return None
+                return {}
+            mock_retrieve_decision.side_effect = retrieve_decision
+
+            hub = mock.MagicMock()
+            hub.config = {
+                'environment': 'environment',
+                'topic_prefix': 'topic_prefix',
+            }
+            handler = greenwave.consumers.resultsdb.ResultsDBHandler(hub)
+
+            koji_proxy = mock.MagicMock()
+            koji_proxy.getBuild.return_value = None
+            koji_proxy.getTaskRequest.return_value = [
+                'git://example.com/project', 'example_product_version', {}]
+            handler.koji_proxy = koji_proxy
+
+            handler.flask_app.config['policies'] = Policy.safe_load_all(policies)
+            with handler.flask_app.app_context():
+                handler.consume(message)
+
+            koji_proxy.getBuild.assert_not_called()
+            koji_proxy.getTaskRequest.assert_called_once_with(666)
+
+            assert len(mock_fedmsg.mock_calls) == 1
+
+            mock_call = mock_fedmsg.mock_calls[0][1][0]
+            assert mock_call.topic == 'greenwave.decision.update'
+            actual_msgs_sent = mock_call.body
+
+            assert actual_msgs_sent == {
+                'decision_context': 'test_context',
+                'product_version': 'example_product_version',
+                'subject': [
+                    {'item': 'example-container', 'type': 'koji_build'},
+                ],
+                'subject_type': 'koji_build',
+                'subject_identifier': 'example-container',
+                'previous': None,
+            }