Repository: cekit/cekit

Issue:
docker build cannot handle more than one colon passed to the --tag option
**Describe the bug**
When passing more than one colon in the value for docker --tag, cekit will throw an error. This bug does not exist in 2.7, but does in 3.0.

**To reproduce**
This will fail:
`cekit build --overrides-file branch-overrides.yaml docker --tag 172.30.1.1:5000/dward/rhdm74-kieserver:7.4.0`

However, this will pass:
`cekit build --overrides-file branch-overrides.yaml docker --tag 172.30.1.1/dward/rhdm74-kieserver:7.4.0`
as will this:
`cekit build --overrides-file branch-overrides.yaml docker --tag dward/rhdm74-kieserver:7.4.0`

It's the colon for the port that is unexpected.

**Expected behavior**
No matter how many colons are in it, cekit should be able to tag the docker-built image with the tag provided.

**Logs**
Traceback (most recent call last):
```
  File "/home/dward/Packages/cekit/3.0/bin/cekit", line 10, in <module>
    sys.exit(cli())
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/cekit/cli.py", line 106, in build_docker
    run_build(ctx, 'docker')
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/cekit/cli.py", line 282, in run_build
    run_command(ctx, builder_impl)
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/cekit/cli.py", line 251, in run_command
    Cekit(common_params).run(clazz, params)
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/cekit/cli.py", line 341, in run
    command.execute()
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/cekit/builder.py", line 60, in execute
    self.run()
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/cekit/builders/docker_builder.py", line 189, in run
    self._tag(docker_client, image_id, tags)
  File "/home/dward/Packages/cekit/3.0/lib/python3.7/site-packages/cekit/builders/docker_builder.py", line 165, in _tag
    img_repo, img_tag = tag.split(":")
ValueError: too many values to unpack (expected 2)
```

This is the key part:
**img_repo, img_tag = tag.split(":") ValueError: too many values to unpack (expected 2)**

**Environment information:**
 - Operating system: Fedora 29 (Linux 5.0.10-200.fc29.x86_64)
 - CEKit version: Result of `cekit --version` 3.0.1 (pip3)


Discussion:

Patch:
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")


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")
+    ])