Repository: cekit/cekit

Issue:
Expired krb sessions don't fail the build
```
2019-05-01 01:27:34,395 cekit        DEBUG    Executing '/usr/bin/brew call --json-output listArchives checksum=cd68ad886a759d21f8dd0cb7646601ee type=maven'.
2019-05-01 01:27:35,812 [ERROR] koji: AuthError: unable to obtain a session
2019-05-01 01:27:35,858 cekit        ERROR    Can't fetch artifacts details from brew: 'b'''.
```

Resolved by running kinit again. I think it should fail hard here.

Discussion:

Patch:
diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md
index 00a9d47..ece94c5 100644
--- a/.github/ISSUE_TEMPLATE/bug_report.md
+++ b/.github/ISSUE_TEMPLATE/bug_report.md
@@ -2,7 +2,7 @@
 name: Bug report
 about: Found issue? Let us know!
 title: ''
-labels: type/bug
+labels: type/bug, status/review
 assignees: ''
 
 ---
diff --git a/.github/ISSUE_TEMPLATE/enhancement.md b/.github/ISSUE_TEMPLATE/enhancement.md
index 90e6779..1d1f8a8 100644
--- a/.github/ISSUE_TEMPLATE/enhancement.md
+++ b/.github/ISSUE_TEMPLATE/enhancement.md
@@ -2,7 +2,7 @@
 name: Enhancement
 about: Want to see something new? Let us know!
 title: ''
-labels: type/enhancement
+labels: status/review, type/enhancement
 assignees: ''
 
 ---
diff --git a/cekit/builders/docker_builder.py b/cekit/builders/docker_builder.py
index 8addd4e..74347bf 100644
--- a/cekit/builders/docker_builder.py
+++ b/cekit/builders/docker_builder.py
@@ -162,7 +162,7 @@ class DockerBuilder(Builder):
     def _tag(self, docker_client, image_id, tags):
         for tag in tags:
             if ':' in tag:
-                img_repo, img_tag = tag.split(":")
+                img_repo, img_tag = tag.rsplit(":", 1)
                 docker_client.tag(image_id, img_repo, tag=img_tag)
             else:
                 docker_client.tag(image_id, tag)
diff --git a/cekit/builders/osbs.py b/cekit/builders/osbs.py
index f735056..808c404 100644
--- a/cekit/builders/osbs.py
+++ b/cekit/builders/osbs.py
@@ -140,12 +140,11 @@ class OSBSBuilder(Builder):
             if os.path.exists("container.yaml"):
                 self._merge_container_yaml("container.yaml",
                                            os.path.join(self.dist_git_dir, "container.yaml"))
-            if os.path.exists("content_sets.yml"):
-                shutil.copy("content_sets.yml",
-                            os.path.join(self.dist_git_dir, "content_sets.yml"))
-            if os.path.exists("fetch-artifacts-url.yaml"):
-                shutil.copy("fetch-artifacts-url.yaml",
-                            os.path.join(self.dist_git_dir, "fetch-artifacts-url.yaml"))
+
+            for special_file in ["content_sets.yml", "fetch-artifacts-url.yaml", "help.md"]:
+                if os.path.exists(special_file):
+                    shutil.copy(special_file,
+                                os.path.join(self.dist_git_dir, special_file))
 
         # Copy also every artifact
         for artifact in self.artifacts:
@@ -232,7 +231,7 @@ class OSBSBuilder(Builder):
             try:
                 subprocess.check_output(cmd, stderr=subprocess.STDOUT)
             except subprocess.CalledProcessError as ex:
-                LOGGER.error("Cannot run '%s', ouput: '%s'" % (cmd, ex.output))
+                LOGGER.error("Cannot run '%s', output: '%s'" % (cmd, ex.output))
                 raise CekitError("Cannot update sources.")
 
         LOGGER.info("Update finished.")
@@ -345,10 +344,10 @@ class DistGit(object):
         if os.path.exists(self.output):
             with Chdir(self.output):
                 LOGGER.info("Pulling latest changes in repo %s..." % self.repo)
-                subprocess.check_output(["git", "fetch"])
-                subprocess.check_output(
+                subprocess.check_call(["git", "fetch"])
+                subprocess.check_call(
                     ["git", "checkout", "-f", self.branch], stderr=subprocess.STDOUT)
-                subprocess.check_output(
+                subprocess.check_call(
                     ["git", "reset", "--hard", "origin/%s" % self.branch])
             LOGGER.debug("Changes pulled")
         else:
@@ -364,7 +363,7 @@ class DistGit(object):
                 cmd += ['--user', user]
             cmd += ["-q", "clone", "-b", self.branch, self.repo, self.output]
             LOGGER.debug("Cloning: '%s'" % ' '.join(cmd))
-            subprocess.check_output(cmd)
+            subprocess.check_call(cmd)
             LOGGER.debug("Repository %s cloned" % self.repo)
 
     def clean(self):
@@ -382,12 +381,10 @@ class DistGit(object):
     def add(self):
         # Add new Dockerfile
         subprocess.check_call(["git", "add", "Dockerfile"])
-        if os.path.exists("container.yaml"):
-            subprocess.check_call(["git", "add", "container.yaml"])
-        if os.path.exists("content_sets.yml"):
-            subprocess.check_call(["git", "add", "content_sets.yml"])
-        if os.path.exists("fetch-artifacts-url.yaml"):
-            subprocess.check_call(["git", "add", "fetch-artifacts-url.yaml"])
+
+        for f in ["container.yaml", "content_sets.yml", "fetch-artifacts-url.yaml", "help.md"]:
+            if os.path.exists(f):
+                subprocess.check_call(["git", "add", f])
 
         for d in ["repos", "modules"]:
             # we probably do not care about non existing files and other errors here
@@ -405,7 +402,7 @@ class DistGit(object):
 
         # Commit the change
         LOGGER.info("Commiting with message: '%s'" % commit_msg)
-        subprocess.check_output(["git", "commit", "-q", "-m", commit_msg])
+        subprocess.check_call(["git", "commit", "-q", "-m", commit_msg])
 
         untracked = subprocess.check_output(
             ["git", "ls-files", "--others", "--exclude-standard"]).decode("utf8")
@@ -438,7 +435,7 @@ class DistGit(object):
             LOGGER.info("Pushing change to the upstream repository...")
             cmd = ["git", "push", "-q", "origin", self.branch]
             LOGGER.debug("Running command '%s'" % ' '.join(cmd))
-            subprocess.check_output(cmd)
+            subprocess.check_call(cmd)
             LOGGER.info("Change pushed.")
         else:
             LOGGER.info("Changes are not pushed, exiting")
diff --git a/cekit/tools.py b/cekit/tools.py
index 4c9bd05..30a4387 100644
--- a/cekit/tools.py
+++ b/cekit/tools.py
@@ -64,25 +64,42 @@ def decision(question):
 
 def get_brew_url(md5):
     try:
-        LOGGER.debug("Getting brew details for an artifact with '%s' md5 sum" % md5)
+        LOGGER.debug("Getting brew details for an artifact with '{}' md5 sum".format(md5))
         list_archives_cmd = ['/usr/bin/brew', 'call', '--json-output', 'listArchives',
-                             'checksum=%s' % md5, 'type=maven']
-        LOGGER.debug("Executing '%s'." % " ".join(list_archives_cmd))
-        archive_yaml = yaml.safe_load(subprocess.check_output(list_archives_cmd))
+                             "checksum={}".format(md5), 'type=maven']
+        LOGGER.debug("Executing '{}'.".format(" ".join(list_archives_cmd)))
 
-        if not archive_yaml:
-            raise CekitError("Artifact with md5 checksum %s could not be found in Brew" % md5)
+        try:
+            json_archives = subprocess.check_output(list_archives_cmd).strip().decode("utf8")
+        except subprocess.CalledProcessError as ex:
+            if ex.output is not None and 'AuthError' in ex.output:
+                LOGGER.warning(
+                    "Brew authentication failed, please make sure you have a valid Kerberos ticket")
+            raise CekitError("Could not fetch archives for checksum {}".format(md5), ex)
 
-        archive = archive_yaml[0]
+        archives = yaml.safe_load(json_archives)
+
+        if not archives:
+            raise CekitError("Artifact with md5 checksum {} could not be found in Brew".format(md5))
+
+        archive = archives[0]
         build_id = archive['build_id']
         filename = archive['filename']
         group_id = archive['group_id']
         artifact_id = archive['artifact_id']
         version = archive['version']
 
-        get_build_cmd = ['brew', 'call', '--json-output', 'getBuild', 'buildInfo=%s' % build_id]
-        LOGGER.debug("Executing '%s'" % " ".join(get_build_cmd))
-        build = yaml.safe_load(subprocess.check_output(get_build_cmd))
+        get_build_cmd = ['brew', 'call', '--json-output',
+                         'getBuild', "buildInfo={}".format(build_id)]
+
+        LOGGER.debug("Executing '{}'".format(" ".join(get_build_cmd)))
+
+        try:
+            json_build = subprocess.check_output(get_build_cmd).strip().decode("utf8")
+        except subprocess.CalledProcessError as ex:
+            raise CekitError("Could not fetch build {} from Brew".format(build_id), ex)
+
+        build = yaml.safe_load(json_build)
 
         build_states = ['BUILDING', 'COMPLETE', 'DELETED', 'FAILED', 'CANCELED']
 


Test patch:
diff --git a/tests/test_integ_builder_osbs.py b/tests/test_integ_builder_osbs.py
new file mode 100644
index 0000000..3ae9568
--- /dev/null
+++ b/tests/test_integ_builder_osbs.py
@@ -0,0 +1,195 @@
+# -*- encoding: utf-8 -*-
+
+# pylint: disable=protected-access
+
+import logging
+import os
+import subprocess
+import yaml
+
+import pytest
+
+from click.testing import CliRunner
+
+from cekit.cli import Cekit, Map, cli
+from cekit.tools import Chdir
+from cekit.config import Config
+from cekit.errors import CekitError
+from cekit.builders.osbs import OSBSBuilder
+from cekit.tools import Map
+
+config = Config()
+
+
+@pytest.fixture(autouse=True)
+def reset_config():
+    config.cfg['common'] = {}
+
+
+config = Config()
+config.cfg['common'] = {'redhat': True}
+
+image_descriptor = {
+    'schema_version': 1,
+    'from': 'centos:latest',
+    'name': 'test/image',
+    'version': '1.0',
+    'labels': [{'name': 'foo', 'value': 'bar'}, {'name': 'labela', 'value': 'a'}],
+    'osbs': {
+            'repository': {
+                'name': 'repo',
+                'branch': 'branch'
+            }
+    }
+}
+
+
+def run_cekit(cwd,
+              parameters=['build', '--dry-run', 'docker'],
+              message=None):
+    with Chdir(cwd):
+        result = CliRunner().invoke(cli, parameters, catch_exceptions=False)
+        if message:
+            assert message in result.output
+
+        return result
+
+
+def run_osbs(descriptor, image_dir, mocker):
+    # We are mocking it, so do not require it at test time
+    mocker.patch('cekit.builders.osbs.OSBSBuilder.dependencies', return_value={})
+    mocker.patch('cekit.builders.osbs.OSBSBuilder._wait_for_osbs_task')
+    mocker.patch('cekit.builders.osbs.DistGit.clean')
+    mocker.patch('cekit.builders.osbs.DistGit.prepare')
+    mocker.patch('cekit.tools.decision', return_value=True)
+
+    mocker_check_call = mocker.patch.object(subprocess, 'check_output', side_effect=[
+        b"true",  # git rev-parse --is-inside-work-tree
+        b"/home/repos/path",  # git rev-parse --show-toplevel
+        b"branch",  # git rev-parse --abbrev-ref HEAD
+        b"3b9283cb26b35511517ff5c0c3e11f490cba8feb",  # git rev-parse HEAD
+        b"",  # git ls-files --others --exclude-standard
+        b"",  # git diff-files --name-only
+        b"ssh://someuser@somehost.com/containers/somerepo",  # git config --get remote.origin.url
+        b"3b9283cb26b35511517ff5c0c3e11f490cba8feb",  # git rev-parse HEAD
+        b"1234",  # brew call --python...
+    ])
+
+    with open(os.path.join(image_dir, 'config'), 'w') as fd:
+        fd.write("[common]\n")
+        fd.write("redhat = True")
+
+    with open(os.path.join(image_dir, 'image.yaml'), 'w') as fd:
+        yaml.dump(descriptor, fd, default_flow_style=False)
+
+    return run_cekit(image_dir, ['-v',
+                                 '--work-dir', image_dir,
+                                 '--config',
+                                 'config',
+                                 'build',
+                                 'osbs'])
+
+
+def run_cekit(cwd,
+              parameters=['build', '--dry-run', 'docker'],
+              message=None):
+    with Chdir(cwd):
+        result = CliRunner().invoke(cli, parameters, catch_exceptions=False)
+        if message:
+            assert message in result.output
+
+        return result
+
+
+def test_osbs_builder_kick_build_without_push(tmpdir, mocker, caplog):
+    """
+    Does not push sources to dist-git. This is the case when the
+    generated files are the same as already existing in dist-git
+    """
+
+    caplog.set_level(logging.DEBUG, logger="cekit")
+
+    mocker.patch.object(subprocess, 'call', return_value=0)
+
+    source_dir = tmpdir.mkdir('source')
+    repo_dir = source_dir.mkdir('osbs').mkdir('repo')
+
+    mock_check_call = mocker.patch.object(subprocess, 'check_call')
+
+    descriptor = image_descriptor.copy()
+
+    run_osbs(descriptor, str(source_dir), mocker)
+
+    assert os.path.exists(str(repo_dir.join('Dockerfile'))) is True
+
+    mock_check_call.assert_has_calls(
+        [
+            mocker.call(['git', 'add', 'Dockerfile']),
+        ])
+
+    assert "No changes made to the code, committing skipped" in caplog.text
+    assert "Image was built successfully in OSBS!" in caplog.text
+
+
+def test_osbs_builder_kick_build_with_push(tmpdir, mocker, caplog):
+    """
+    Does not push sources to dist-git. This is the case when the
+    generated files are the same as already existing in dist-git
+    """
+
+    caplog.set_level(logging.DEBUG, logger="cekit")
+
+    source_dir = tmpdir.mkdir('source')
+    repo_dir = source_dir.mkdir('osbs').mkdir('repo')
+
+    mocker.patch.object(subprocess, 'call', return_value=1)
+
+    mock_check_call = mocker.patch.object(subprocess, 'check_call')
+
+    descriptor = image_descriptor.copy()
+
+    run_osbs(descriptor, str(source_dir), mocker)
+
+    assert os.path.exists(str(repo_dir.join('Dockerfile'))) is True
+
+    mock_check_call.assert_has_calls(
+        [
+            mocker.call(['git', 'add', 'Dockerfile']),
+            mocker.call(['git', 'commit', '-q', '-m',
+                         'Sync with path, commit 3b9283cb26b35511517ff5c0c3e11f490cba8feb']),
+            mocker.call(['git', 'push', '-q', 'origin', 'branch'])
+        ])
+
+    assert "Commiting with message: 'Sync with path, commit 3b9283cb26b35511517ff5c0c3e11f490cba8feb'" in caplog.text
+    assert "Image was built successfully in OSBS!" in caplog.text
+
+
+# https://github.com/cekit/cekit/issues/504
+def test_osbs_builder_add_help_file(tmpdir, mocker, caplog):
+    """
+    Checks if help.md file is generated and added to dist-git
+    """
+
+    caplog.set_level(logging.DEBUG, logger="cekit")
+
+    source_dir = tmpdir.mkdir('source')
+    repo_dir = source_dir.mkdir('osbs').mkdir('repo')
+
+    mocker.patch.object(subprocess, 'call', return_value=0)
+    mock_check_call = mocker.patch.object(subprocess, 'check_call')
+
+    descriptor = image_descriptor.copy()
+    descriptor['help'] = {'add': True}
+
+    run_osbs(descriptor, str(source_dir), mocker)
+
+    assert os.path.exists(str(repo_dir.join('Dockerfile'))) is True
+    assert os.path.exists(str(repo_dir.join('help.md'))) is True
+
+    mock_check_call.assert_has_calls(
+        [
+            mocker.call(['git', 'add', 'Dockerfile']),
+            mocker.call(['git', 'add', 'help.md']),
+        ])
+
+    assert "Image was built successfully in OSBS!" in caplog.text
diff --git a/tests/test_unit_builder_docker.py b/tests/test_unit_builder_docker.py
index 26d6f3b..bdd8378 100644
--- a/tests/test_unit_builder_docker.py
+++ b/tests/test_unit_builder_docker.py
@@ -134,3 +134,19 @@ def test_docker_client_build_with_failure(mocker, caplog):
     docker_client_build.assert_called_once_with(path='something/image', pull=None, rm=True)
     assert "Docker: Step 3/159 : COPY modules /tmp/scripts/" in caplog.text
     assert "You can look inside the failed image by running 'docker run --rm -ti 81a88b63f47f bash'" in caplog.text
+
+
+# https://github.com/cekit/cekit/issues/508
+def test_docker_tag(mocker):
+    builder = DockerBuilder(Map({'target': 'something'}), Map({'tags': ['foo', 'bar']}))
+
+    docker_client_mock = mocker.Mock()
+
+    builder._tag(docker_client_mock, "image_id", ["image:latest", "host:5000/repo/image:tag"])
+
+    assert len(docker_client_mock.tag.mock_calls) == 2
+
+    docker_client_mock.tag.assert_has_calls([
+        mocker.call("image_id", "image", tag="latest"),
+        mocker.call("image_id", "host:5000/repo/image", tag="tag")
+    ])
diff --git a/tests/test_unit_tools.py b/tests/test_unit_tools.py
index 859d5c5..24ffe42 100644
--- a/tests/test_unit_tools.py
+++ b/tests/test_unit_tools.py
@@ -1,5 +1,6 @@
 import logging
 import pytest
+import subprocess
 import yaml
 
 from contextlib import contextmanager
@@ -187,7 +188,7 @@ def brew_call_ok(*args, **kwargs):
             "artifact_id": "artifact_id",
             "version": "version",
           }
-        ]"""
+        ]""".encode("utf8")
     if 'getBuild' in args[0]:
         return """
         {
@@ -195,8 +196,8 @@ def brew_call_ok(*args, **kwargs):
           "release": "release",
           "state": 1
         }
-        """
-    return ""
+        """.encode("utf8")
+    return "".encode("utf8")
 
 
 def brew_call_removed(*args, **kwargs):
@@ -210,7 +211,7 @@ def brew_call_removed(*args, **kwargs):
             "artifact_id": "artifact_id",
             "version": "version",
           }
-        ]"""
+        ]""".encode("utf8")
     if 'getBuild' in args[0]:
         return """
         {
@@ -218,8 +219,8 @@ def brew_call_removed(*args, **kwargs):
           "release": "release",
           "state": 2
         }
-        """
-    return ""
+        """.encode("utf8")
+    return "".encode("utf8")
 
 
 def test_get_brew_url(mocker):
@@ -239,6 +240,22 @@ def test_get_brew_url_when_build_was_removed(mocker):
         excinfo.value)
 
 
+# https://github.com/cekit/cekit/issues/502
+def test_get_brew_url_no_kerberos(mocker, caplog):
+    caplog.set_level(logging.DEBUG, logger="cekit")
+
+    kerberos_error = subprocess.CalledProcessError(1, 'CMD')
+    kerberos_error.output = "2019-05-06 14:58:44,502 [ERROR] koji: AuthError: unable to obtain a session"
+
+    mocker.patch('subprocess.check_output', side_effect=kerberos_error)
+
+    with pytest.raises(CekitError) as excinfo:
+        tools.get_brew_url('aa')
+
+    assert 'Could not fetch archives for checksum aa' in str(excinfo.value)
+    assert "Brew authentication failed, please make sure you have a valid Kerberos ticket" in caplog.text
+
+
 @contextmanager
 def mocked_dependency_handler(mocker, data="ID=fedora\nNAME=somefedora\nVERSION=123"):
     dh = None