Repository: cekit/cekit

Issue:
Failed OSBS build should fail execution
Currently the return code is `0`, we should throw an exception.

Discussion:

Patch:
diff --git a/cekit/builders/osbs.py b/cekit/builders/osbs.py
index 252c446..38f2b1d 100644
--- a/cekit/builders/osbs.py
+++ b/cekit/builders/osbs.py
@@ -1,11 +1,18 @@
 import glob
+import json
 import logging
 import os
 import shutil
 import subprocess
 import sys
+import time
 import yaml
 
+try:
+    from urllib.parse import urlparse
+except ImportError:
+    from urlparse import urlparse
+
 from cekit import tools
 from cekit.config import Config
 from cekit.builder import Builder
@@ -34,11 +41,17 @@ class OSBSBuilder(Builder):
 
         if params.get('redhat'):
             if params.get('stage'):
-                self._rhpkg = 'rhpkg-stage'
+                self._fedpkg = 'rhpkg-stage'
+                self._koji = 'brew-stage'
+                self._koji_url = 'https://brewweb.stage.engineering.redhat.com/brew'
             else:
-                self._rhpkg = 'rhpkg'
+                self._fedpkg = 'rhpkg'
+                self._koji = 'brew'
+                self._koji_url = 'https://brewweb.engineering.redhat.com/brew'
         else:
-            self._rhpkg = 'fedpkg'
+            self._fedpkg = 'fedpkg'
+            self._koji = 'koji'
+            self._koji_url = 'https://koji.fedoraproject.org/koji'
 
         super(OSBSBuilder, self).__init__(build_engine, target, params={})
 
@@ -48,15 +61,26 @@ class OSBSBuilder(Builder):
 
         if config.cfg['common'].get('redhat'):
             if config.cfg['common'].get('stage'):
-                package = 'rhpkg-stage'
+                fedpkg = 'rhpkg-stage'
+                koji = 'brewkoji-stage'
+                koji_executable = 'brew-stage'
             else:
-                package = 'rhpkg'
+                fedpkg = 'rhpkg'
+                koji = 'brewkoji'
+                koji_executable = 'brew'
         else:
-            package = 'fedpkg'
+            fedpkg = 'fedpkg'
+            koji = 'koji'
+            koji_executable = 'koji'
+
+        deps[fedpkg] = {
+            'package': fedpkg,
+            'executable': fedpkg
+        }
 
-        deps[package] = {
-            'package': package,
-            'executable': package
+        deps[koji] = {
+            'package': koji,
+            'executable': koji_executable
         }
 
         return deps
@@ -154,11 +178,50 @@ class OSBSBuilder(Builder):
         with open(dest, 'w') as _file:
             yaml.dump(target, _file, default_flow_style=False)
 
+    def _wait_for_osbs_task(self, task_id, current_time=0, timeout=7200):
+        """ Default timeout is 2hrs """
+
+        logger.debug("Checking if task {} is finished...".format(task_id))
+
+        # Time between subsequent querying the API
+        sleep_time = 20
+
+        if current_time > timeout:
+            raise CekitError(
+                "Timed out while waiting for the task {} to finish, please check the task logs!".format(task_id))
+
+        # Definition of task states
+        states = {'free': 0, 'open': 1, 'closed': 2, 'cancelled': 3, 'assigned': 4, 'failed': 5}
+
+        # Get information about the task
+        try:
+            json_info = subprocess.check_output(
+                ["brew", "call", "--json-output", "getTaskInfo", task_id]).strip().decode("utf8")
+        except subprocess.CalledProcessError as ex:
+            raise CekitError("Could not check the task {} result".format(task_id), ex)
+
+        # Parse the returned JSON
+        info = json.loads(json_info)
+
+        # Task is closed which means that it was successfully finished
+        if info['state'] == states['closed']:
+            return True
+
+        # Task is in progress
+        if info['state'] == states['free'] or info['state'] == states['open'] or info['state'] == states['assigned']:
+            # It's not necessary to query the API so often
+            time.sleep(sleep_time)
+            return self._wait_for_osbs_task(task_id, current_time+sleep_time, timeout)
+
+        # In all other cases (failed, cancelled) task did not finish successfully
+        raise CekitError(
+            "Task {} did not finish successfully, please check the task logs!".format(task_id))
+
     def update_lookaside_cache(self):
         logger.info("Updating lookaside cache...")
         if not self.artifacts:
             return
-        cmd = [self._rhpkg]
+        cmd = [self._fedpkg]
         if self._user:
             cmd += ['--user', self._user]
         cmd += ["new-sources"] + self.artifacts
@@ -174,24 +237,12 @@ class OSBSBuilder(Builder):
         logger.info("Update finished.")
 
     def build(self):
-        cmd = [self._rhpkg]
+        cmd = [self._koji]
 
         if self._user:
             cmd += ['--user', self._user]
-        cmd.append("container-build")
 
-        if self._target:
-            cmd += ['--target', self._target]
-
-        if self._nowait:
-            cmd += ['--nowait']
-
-        if self._rhpkg_set_url_repos:
-            cmd += ['--repo-url']
-            cmd += self._rhpkg_set_url_repos
-
-        if not self._release:
-            cmd.append("--scratch")
+        cmd += ['call', '--python', 'buildContainer', '--kwargs']
 
         with Chdir(self.dist_git_dir):
             self.dist_git.add()
@@ -203,12 +254,50 @@ class OSBSBuilder(Builder):
             else:
                 logger.info("No changes made to the code, committing skipped")
 
+            # Get the url of the repository
+            url = subprocess.check_output(
+                ["git", "remote", "get-url", "origin"]).strip().decode("utf8")
+            # Get the latest commit hash
+            commit = subprocess.check_output(["git", "rev-parse", "HEAD"]).strip().decode("utf8")
+            # Parse the dist-git repository url
+            url = urlparse(url)
+            # Construct the url again, with a hash and removed username and password, if any
+            src = "git://{}{}#{}".format(url.hostname, url.path, commit)
+
+            if not self._target:
+                self._target = "{}-containers-candidate".format(self.dist_git.branch)
+
+            scratch = True
+
+            if self._release:
+                scratch = False
+
+            kwargs = "{{'src': '{}', 'target': '{}', 'opts': {{'scratch': {}, 'git_branch': '{}', 'yum_repourls': {}}}}}".format(
+                src, self._target, scratch, self.dist_git.branch, self._rhpkg_set_url_repos)
+
+            cmd.append(kwargs)
+
             logger.info("About to execute '%s'." % ' '.join(cmd))
+
             if tools.decision("Do you want to build the image in OSBS?"):
                 build_type = "release" if self._release else "scratch"
                 logger.info("Executing %s container build in OSBS..." % build_type)
 
-                subprocess.check_call(cmd)
+                try:
+                    task_id = subprocess.check_output(cmd).strip().decode("utf8")
+
+                except subprocess.CalledProcessError as ex:
+                    raise CekitError("Building conainer image in OSBS failed", ex)
+
+                logger.info("Task {} was submitted, you can watch the progress here: {}/taskinfo?taskID={}".format(
+                    task_id, self._koji_url, task_id))
+
+                if self._nowait:
+                    return
+
+                self._wait_for_osbs_task(task_id)
+
+                logger.info("Image was built successfully in OSBS!")
 
 
 class DistGit(object):
@@ -217,8 +306,7 @@ class DistGit(object):
     def repo_info(path):
 
         with Chdir(path):
-            if subprocess.check_output(["git", "rev-parse", "--is-inside-work-tree"]) \
-                    .strip().decode("utf8") != "true":
+            if subprocess.check_output(["git", "rev-parse", "--is-inside-work-tree"]).strip().decode("utf8") != "true":
 
                 raise Exception("Directory %s doesn't seem to be a git repository. "
                                 "Please make sure you specified correct path." % path)
@@ -321,14 +409,14 @@ class DistGit(object):
 
         if untracked:
             logger.warning("There are following untracked files: %s. Please review your commit."
-                        % ", ".join(untracked.splitlines()))
+                           % ", ".join(untracked.splitlines()))
 
         diffs = subprocess.check_output(["git", "diff-files", "--name-only"]).decode("utf8")
 
         if diffs:
             logger.warning("There are uncommited changes in following files: '%s'. "
-                        "Please review your commit."
-                        % ", ".join(diffs.splitlines()))
+                           "Please review your commit."
+                           % ", ".join(diffs.splitlines()))
 
         if not self.noninteractive:
             subprocess.call(["git", "status"])


Test patch:
diff --git a/tests/test_builder.py b/tests/test_builder.py
index 210e5eb..f6be324 100644
--- a/tests/test_builder.py
+++ b/tests/test_builder.py
@@ -1,9 +1,17 @@
 import glob
 import os
+import pytest
 import subprocess
+import time
 import yaml
 
+try:
+    from unittest.mock import call
+except ImportError:
+    from mock import call
+
 from cekit.builder import Builder
+from cekit.errors import CekitError
 
 
 def test_osbs_builder_defaults(mocker):
@@ -12,7 +20,9 @@ def test_osbs_builder_defaults(mocker):
     builder = Builder('osbs', 'tmp', {})
 
     assert builder._release is False
-    assert builder._rhpkg == 'fedpkg'
+    assert builder._fedpkg == 'fedpkg'
+    assert builder._koji == 'koji'
+    assert builder._koji_url == 'https://koji.fedoraproject.org/koji'
     assert builder._nowait is False
 
 
@@ -21,7 +31,9 @@ def test_osbs_builder_redhat(mocker):
 
     builder = Builder('osbs', 'tmp', {'redhat': True})
 
-    assert builder._rhpkg == 'rhpkg'
+    assert builder._fedpkg == 'rhpkg'
+    assert builder._koji == 'brew'
+    assert builder._koji_url == 'https://brewweb.engineering.redhat.com/brew'
 
 
 def test_osbs_builder_use_rhpkg_staget(mocker):
@@ -31,7 +43,9 @@ def test_osbs_builder_use_rhpkg_staget(mocker):
               'redhat': True}
     builder = Builder('osbs', 'tmp', params)
 
-    assert builder._rhpkg == 'rhpkg-stage'
+    assert builder._fedpkg == 'rhpkg-stage'
+    assert builder._koji == 'brew-stage'
+    assert builder._koji_url == 'https://brewweb.stage.engineering.redhat.com/brew'
 
 
 def test_osbs_builder_custom_commit_msg(mocker):
@@ -116,8 +130,7 @@ class DistGitMock(object):
         pass
 
 
-def create_osbs_build_object(mocker, builder_type, params):
-    mocker.patch.object(subprocess, 'check_output')
+def create_osbs_build_object(mocker, builder_type, params={}):
     mocker.patch('cekit.tools.decision')
 
     builder = Builder(builder_type, 'tmp', params)
@@ -127,61 +140,192 @@ def create_osbs_build_object(mocker, builder_type, params):
     return builder
 
 
-def test_osbs_builder_run_rhpkg_stage(mocker):
-    mocker.patch.object(subprocess, 'check_output')
-
+def test_osbs_builder_run_brew_stage(mocker):
     params = {'stage': True,
               'redhat': True}
 
-    check_call = mocker.patch.object(subprocess, 'check_call')
+    check_output = mocker.patch.object(subprocess, 'check_output', autospec=True, side_effect=[
+                                       b'ssh://user:password@something.redhat.com/containers/openjdk', b'c5a0731b558c8a247dd7f85b5f54462cd5b68b23', b'12345'])
     builder = create_osbs_build_object(mocker, 'osbs', params)
+    mocker.patch.object(builder, '_wait_for_osbs_task')
+    builder.dist_git.branch = "some-branch"
     builder.build()
 
-    check_call.assert_called_once_with(['rhpkg-stage', 'container-build', '--scratch'])
+    check_output.assert_has_calls([
+        call(['git', 'remote', 'get-url', 'origin']),
+        call(['git', 'rev-parse', 'HEAD']),
+        call(['brew-stage', 'call', '--python', 'buildContainer', '--kwargs',
+              "{'src': 'git://something.redhat.com/containers/openjdk#c5a0731b558c8a247dd7f85b5f54462cd5b68b23', 'target': 'some-branch-containers-candidate', 'opts': {'scratch': True, 'git_branch': 'some-branch', 'yum_repourls': []}}"])
+    ])
 
+    builder._wait_for_osbs_task.assert_called_once_with('12345')
 
-def test_osbs_builder_run_rhpkg(mocker):
-    mocker.patch.object(subprocess, 'check_output')
 
-    check_call = mocker.patch.object(subprocess, 'check_call')
-    builder = create_osbs_build_object(mocker, 'osbs', {'redhat': True})
+def test_osbs_builder_run_brew(mocker):
+    params = {'redhat': True}
+
+    check_output = mocker.patch.object(subprocess, 'check_output', autospec=True, side_effect=[
+                                       b'ssh://user:password@something.redhat.com/containers/openjdk', b'c5a0731b558c8a247dd7f85b5f54462cd5b68b23', b'12345'])
+    builder = create_osbs_build_object(mocker, 'osbs', params)
+    mocker.patch.object(builder, '_wait_for_osbs_task')
+    builder.dist_git.branch = "some-branch"
     builder.build()
 
-    check_call.assert_called_once_with(['rhpkg', 'container-build', '--scratch'])
+    check_output.assert_has_calls([
+        call(['git', 'remote', 'get-url', 'origin']),
+        call(['git', 'rev-parse', 'HEAD']),
+        call(['brew', 'call', '--python', 'buildContainer', '--kwargs',
+              "{'src': 'git://something.redhat.com/containers/openjdk#c5a0731b558c8a247dd7f85b5f54462cd5b68b23', 'target': 'some-branch-containers-candidate', 'opts': {'scratch': True, 'git_branch': 'some-branch', 'yum_repourls': []}}"])
+    ])
 
+    builder._wait_for_osbs_task.assert_called_once_with('12345')
 
-def test_osbs_builder_run_rhpkg_nowait(mocker):
-    mocker.patch.object(subprocess, 'check_output')
+
+def test_osbs_builder_run_koji(mocker):
+    params = {'redhat': False}
+
+    check_output = mocker.patch.object(subprocess, 'check_output', autospec=True, side_effect=[
+                                       b'ssh://user:password@something.redhat.com/containers/openjdk', b'c5a0731b558c8a247dd7f85b5f54462cd5b68b23', b'12345'])
+    builder = create_osbs_build_object(mocker, 'osbs', params)
+    mocker.patch.object(builder, '_wait_for_osbs_task')
+    builder.dist_git.branch = "some-branch"
+    builder.build()
+
+    check_output.assert_has_calls([
+        call(['git', 'remote', 'get-url', 'origin']),
+        call(['git', 'rev-parse', 'HEAD']),
+        call(['koji', 'call', '--python', 'buildContainer', '--kwargs',
+              "{'src': 'git://something.redhat.com/containers/openjdk#c5a0731b558c8a247dd7f85b5f54462cd5b68b23', 'target': 'some-branch-containers-candidate', 'opts': {'scratch': True, 'git_branch': 'some-branch', 'yum_repourls': []}}"])
+    ])
+
+    builder._wait_for_osbs_task.assert_called_once_with('12345')
+
+
+def test_osbs_builder_run_brew_nowait(mocker):
     params = {'nowait': True,
               'redhat': True}
 
-    check_call = mocker.patch.object(subprocess, 'check_call')
+    check_output = mocker.patch.object(subprocess, 'check_output', autospec=True, side_effect=[
+                                       b'ssh://user:password@something.redhat.com/containers/openjdk', b'c5a0731b558c8a247dd7f85b5f54462cd5b68b23', b'12345'])
     builder = create_osbs_build_object(mocker, 'osbs', params)
+    mocker.patch.object(builder, '_wait_for_osbs_task')
+    builder.dist_git.branch = "some-branch"
     builder.build()
 
-    check_call.assert_called_once_with(['rhpkg', 'container-build', '--nowait', '--scratch'])
+    builder._wait_for_osbs_task.assert_not_called()
 
 
-def test_osbs_builder_run_rhpkg_user(mocker):
+def test_osbs_builder_run_brew_user(mocker):
     params = {'user': 'Foo',
               'redhat': True}
 
-    check_call = mocker.patch.object(subprocess, 'check_call')
+    check_output = mocker.patch.object(subprocess, 'check_output', autospec=True, side_effect=[
+                                       b'ssh://user:password@something.redhat.com/containers/openjdk', b'c5a0731b558c8a247dd7f85b5f54462cd5b68b23', b'12345'])
     builder = create_osbs_build_object(mocker, 'osbs', params)
+    mocker.patch.object(builder, '_wait_for_osbs_task')
+    builder.dist_git.branch = "some-branch"
     builder.build()
 
-    check_call.assert_called_once_with(['rhpkg', '--user', 'Foo', 'container-build', '--scratch'])
+    check_output.assert_called_with(['brew', '--user', 'Foo', 'call', '--python', 'buildContainer', '--kwargs',
+                                     "{'src': 'git://something.redhat.com/containers/openjdk#c5a0731b558c8a247dd7f85b5f54462cd5b68b23', 'target': 'some-branch-containers-candidate', 'opts': {'scratch': True, 'git_branch': 'some-branch', 'yum_repourls': []}}"])
 
 
-def test_osbs_builder_run_rhpkg_target(mocker):
+def test_osbs_builder_run_brew_target(mocker):
     params = {'target': 'Foo',
               'redhat': True}
 
-    check_call = mocker.patch.object(subprocess, 'check_call')
+    check_output = mocker.patch.object(subprocess, 'check_output', autospec=True, side_effect=[
+                                       b'ssh://user:password@something.redhat.com/containers/openjdk', b'c5a0731b558c8a247dd7f85b5f54462cd5b68b23', b'12345'])
     builder = create_osbs_build_object(mocker, 'osbs', params)
+    mocker.patch.object(builder, '_wait_for_osbs_task')
+    builder.dist_git.branch = "some-branch"
     builder.build()
 
-    check_call.assert_called_once_with(['rhpkg', 'container-build', '--target', 'Foo', '--scratch'])
+    check_output.assert_called_with(['brew', 'call', '--python', 'buildContainer', '--kwargs',
+                                     "{'src': 'git://something.redhat.com/containers/openjdk#c5a0731b558c8a247dd7f85b5f54462cd5b68b23', 'target': 'Foo', 'opts': {'scratch': True, 'git_branch': 'some-branch', 'yum_repourls': []}}"])
+
+
+def test_osbs_wait_for_osbs_task_finished_successfully(mocker):
+    builder = create_osbs_build_object(mocker, 'osbs')
+
+    sleep = mocker.patch.object(time, 'sleep')
+    check_output = mocker.patch.object(subprocess, 'check_output', side_effect=[
+        b'''{
+            "state": 2,
+            "create_time": "2019-02-15 13:14:58.278557",
+            "create_ts": 1550236498.27856,
+            "owner": 2485,
+            "host_id": 283,
+            "method": "buildContainer",
+            "completion_ts": 1550237431.0166,
+            "arch": "noarch",
+            "id": 20222655
+        }'''])
+
+    assert builder._wait_for_osbs_task('12345') == True
+
+    check_output.assert_called_with(['brew', 'call', '--json-output', 'getTaskInfo', '12345'])
+    sleep.assert_not_called()
+
+
+def test_osbs_wait_for_osbs_task_in_progress(mocker):
+    builder = create_osbs_build_object(mocker, 'osbs')
+
+    sleep = mocker.patch.object(time, 'sleep')
+    check_output = mocker.patch.object(subprocess, 'check_output', side_effect=[
+        b'''{
+            "state": 1,
+            "create_time": "2019-02-15 13:14:58.278557",
+            "create_ts": 1550236498.27856,
+            "owner": 2485,
+            "host_id": 283,
+            "method": "buildContainer",
+            "completion_ts": 1550237431.0166,
+            "arch": "noarch",
+            "id": 20222655
+        }''', b'''{
+            "state": 2,
+            "create_time": "2019-02-15 13:14:58.278557",
+            "create_ts": 1550236498.27856,
+            "owner": 2485,
+            "host_id": 283,
+            "method": "buildContainer",
+            "completion_ts": 1550237431.0166,
+            "arch": "noarch",
+            "id": 20222655
+        }'''])
+
+    assert builder._wait_for_osbs_task('12345') == True
+
+    check_output.assert_has_calls([
+        call(['brew', 'call', '--json-output', 'getTaskInfo', '12345']),
+        call(['brew', 'call', '--json-output', 'getTaskInfo', '12345'])
+    ])
+    sleep.assert_called_once_with(20)
+
+
+def test_osbs_wait_for_osbs_task_failed(mocker):
+    builder = create_osbs_build_object(mocker, 'osbs')
+
+    sleep = mocker.patch.object(time, 'sleep')
+    check_output = mocker.patch.object(subprocess, 'check_output', side_effect=[
+        b'''{
+            "state": 5,
+            "create_time": "2019-02-15 13:14:58.278557",
+            "create_ts": 1550236498.27856,
+            "owner": 2485,
+            "host_id": 283,
+            "method": "buildContainer",
+            "completion_ts": 1550237431.0166,
+            "arch": "noarch",
+            "id": 20222655
+        }'''])
+
+    with pytest.raises(CekitError, match="Task 12345 did not finish successfully, please check the task logs!"):
+        builder._wait_for_osbs_task('12345')
+
+    check_output.assert_called_with(['brew', 'call', '--json-output', 'getTaskInfo', '12345'])
+    sleep.assert_not_called()
 
 
 def test_docker_builder_defaults():