Repository: cekit/cekit

Issue:
content_sets (or content_sets_file) cannot be overridden
cekit 3.5.0

With [this image.yaml](https://github.com/jmtd/openjdk/blob/97a8819e36d861c8421acfd4000d763942003c16/image.yaml#L37), using `content_sets_file`, and [this override](https://github.com/jmtd/openjdk/blob/97a8819e36d861c8421acfd4000d763942003c16/rhel8-jdk8-overrides.yaml#L34), using embedded `content_sets`, cekit crashes as follows

```
…
2019-10-24 13:41:03,773 cekit        DEBUG    Reading descriptor from 'image.yaml' file...
Traceback (most recent call last):
  File "/home/ce/cekit/3.5.0/bin/cekit", line 11, in <module>
    load_entry_point('cekit==3.5.0', 'console_scripts', 'cekit')()
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/cli.py", line 175, in build_osbs
    run_build(ctx, 'osbs')
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/cli.py", line 278, in run_build
    run_command(ctx, builder_impl)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/cli.py", line 247, in run_command
    Cekit(params).run(clazz)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/cli.py", line 337, in run
    command.execute()
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/builder.py", line 53, in execute
    self.before_generate()
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/builder.py", line 97, in before_generate
    self.generator.init()
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/generator/osbs.py", line 21, in init
    super(OSBSGenerator, self).init()
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/generator/base.py", line 112, in init
    self.image.apply_image_overrides(self._overrides)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/descriptor/image.py", line 248, in apply_image_overrides
    override.packages.content_sets, self.packages.content_sets)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/descriptor/base.py", line 183, in _merge_descriptors
    if k2 in desc1.skip_merging:
AttributeError: 'dict' object has no attribute 'skip_merging'

```

Discussion:

Patch:
diff --git a/cekit/descriptor/image.py b/cekit/descriptor/image.py
index 2409120..e782f47 100644
--- a/cekit/descriptor/image.py
+++ b/cekit/descriptor/image.py
@@ -228,40 +228,17 @@ class Image(Descriptor):
                     module_repositories[name] = repository
             self.modules._descriptor['repositories'] = list(module_repositories.values())
 
-            package_repositories = Image._to_dict(self.packages.repositories)
-            for repository in override.packages.repositories:
-                name = repository.name
-                if name in package_repositories:
-                    package_repositories[name] = repository.merge(package_repositories[name])
-                else:
-                    package_repositories[name] = repository
-            self.packages._descriptor['repositories'] = list(package_repositories.values())
-
-            if override.packages.manager:
-                self.packages._descriptor['manager'] = override.packages.manager
-
-            # If the key is set (value could be None!)
-            if 'content_sets' in override.packages:
-                # If value is different than None, merge it
-                if override.packages.content_sets:
-                    self.packages.content_sets = _merge_descriptors(
-                        override.packages.content_sets, self.packages.content_sets)
-                else:
-                    # If key exists and value is None - remove it from the main packages section
-                    self.packages._descriptor.pop('content_sets', None)
+            self.packages._descriptor = override.packages.merge(self.packages)
 
-            # If the key is set (value could be None!)
-            if 'content_sets_file' in override.packages:
-                # If value is different than None, set it
-                if override.packages.content_sets_file:
-                    self.packages.content_sets_file = override.packages.content_sets_file
-                else:
-                    # If key exists and value is None - remove it from the main packages section
-                    # Remove 'content_sets' too, because this is where it should already be translated into
-                    self.packages._descriptor.pop('content_sets_file', None)
+            # In case content sets are provided as null values
+            # Remove the key entirely.
+            # TODO: This should be handled probably at general level, for every key
+            for flag in ['content_sets', 'content_sets_file']:
+                if flag in override.packages and override.packages[flag] is None:
                     self.packages._descriptor.pop('content_sets', None)
+                    self.packages._descriptor.pop('content_sets_file', None)
 
-            if override.osbs != None:
+            if override.osbs is not None:
                 self.osbs = override.osbs.merge(self.osbs)
 
             for package in override.packages.install:
diff --git a/cekit/descriptor/osbs.py b/cekit/descriptor/osbs.py
index dcd23b5..dfee1da 100644
--- a/cekit/descriptor/osbs.py
+++ b/cekit/descriptor/osbs.py
@@ -98,7 +98,6 @@ class Configuration(Descriptor):
         self.schema = configuration_schema
         self.descriptor_path = descriptor_path
         super(Configuration, self).__init__(descriptor)
-        self.skip_merging = ['container', 'container_file']
 
         if 'container' in self and 'container_file' in self:
             raise CekitError('You cannot specify container and container_file together!')
diff --git a/cekit/descriptor/resource.py b/cekit/descriptor/resource.py
index 755cd73..a694c93 100644
--- a/cekit/descriptor/resource.py
+++ b/cekit/descriptor/resource.py
@@ -18,7 +18,7 @@ from cekit.config import Config
 from cekit.crypto import SUPPORTED_HASH_ALGORITHMS, check_sum
 from cekit.descriptor import Descriptor
 from cekit.errors import CekitError
-from cekit.tools import get_brew_url, Map
+from cekit.tools import get_brew_url, Map, Chdir
 
 logger = logging.getLogger('cekit')
 config = Config()
@@ -437,10 +437,15 @@ class _GitResource(Resource):
         return os.path.basename(descriptor.get('git', {}).get('url')).split(".", 1)[0]
 
     def _copy_impl(self, target):
-        cmd = ['git', 'clone', '--depth', '1', self.git.url, target, '-b',
-               self.git.ref]
-        logger.debug("Running '{}'".format(' '.join(cmd)))
+        cmd = ['git', 'clone', self.git.url, target]
+        logger.debug("Cloning Git repository: '{}'".format(' '.join(cmd)))
         subprocess.check_output(cmd, stderr=subprocess.STDOUT)
+
+        with Chdir(target):
+            cmd = ['git', 'checkout', self.git.ref]
+            logger.debug("Checking out '{}' ref: '{}'".format(self.git.ref, ' '.join(cmd)))
+            subprocess.check_output(cmd, stderr=subprocess.STDOUT)
+
         return target
 
 
diff --git a/cekit/generator/base.py b/cekit/generator/base.py
index 85ed250..d79739b 100644
--- a/cekit/generator/base.py
+++ b/cekit/generator/base.py
@@ -526,7 +526,7 @@ class ModuleRegistry(object):
 
             default_module = self.get_module(name, default_version)
 
-            if not suppress_warnings:
+            if not suppress_warnings and len(modules) > 1:
                 LOGGER.warning("Module version not specified for '{}' module, using '{}' default version".format(
                     name, default_version))
 


Test patch:
diff --git a/tests/test_integ_builder_osbs.py b/tests/test_integ_builder_osbs.py
index 09a181f..a717f3d 100644
--- a/tests/test_integ_builder_osbs.py
+++ b/tests/test_integ_builder_osbs.py
@@ -59,10 +59,13 @@ def run_cekit(cwd,
         return result
 
 
-def run_osbs(descriptor, image_dir, mocker, return_code=0, build_command=None):
+def run_osbs(descriptor, image_dir, mocker, return_code=0, build_command=None, general_command=None):
     if build_command is None:
         build_command = ['build', 'osbs']
 
+    if general_command is None:
+        general_command = ['--redhat']
+
     # 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')
@@ -82,14 +85,10 @@ def run_osbs(descriptor, image_dir, mocker, return_code=0, build_command=None):
         b"UUU"
     ])
 
-    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',
+    return run_cekit(image_dir, general_command + ['-v',
                                  '--work-dir', image_dir,
                                  '--config', 'config'] + build_command,
                      return_code=return_code)
@@ -521,3 +520,37 @@ def test_osbs_builder_with_fetch_artifacts_file_removal(tmpdir, mocker, caplog):
 
     assert not os.path.exists(os.path.join(str(tmpdir), 'osbs', 'repo', 'fetch-artifacts-url.yaml'))
     assert "Removing old 'fetch-artifacts-url.yaml' file" in caplog.text
+
+
+@pytest.mark.parametrize('flag', [[], ['--redhat']])
+def test_osbs_builder_container_yaml_existence(tmpdir, mocker, caplog, flag):
+    """
+    Make sure that the osbs section is properly merged.
+    The evidence is that the container.yaml file is generated.
+
+    https://github.com/cekit/cekit/issues/631
+    """
+
+    caplog.set_level(logging.DEBUG, logger="cekit")
+
+    mocker.patch('cekit.tools.decision', return_value=True)
+    mocker.patch('cekit.descriptor.resource.urlopen')
+    mocker.patch('cekit.generator.osbs.get_brew_url', return_value='http://random.url/path')
+    mocker.patch.object(subprocess, 'check_output')
+    mocker.patch('cekit.builders.osbs.DistGit.push')
+
+    tmpdir.mkdir('osbs').mkdir('repo')
+
+    with Chdir(os.path.join(str(tmpdir), 'osbs', 'repo')):
+        subprocess.call(["git", "init"])
+        subprocess.call(["touch", "file"])
+        subprocess.call(["git", "add", "file"])
+        subprocess.call(["git", "commit", "-m", "Dummy"])
+
+    descriptor = image_descriptor.copy()
+
+    descriptor["osbs"]["configuration"] = {'container': {'compose': {'pulp_repos': True}}}
+
+    run_osbs(descriptor, str(tmpdir), mocker, general_command=flag)
+
+    assert os.path.exists(os.path.join(str(tmpdir), 'osbs', 'repo', 'container.yaml'))
diff --git a/tests/test_unit_image.py b/tests/test_unit_image.py
index 5e78f66..2a1fac5 100644
--- a/tests/test_unit_image.py
+++ b/tests/test_unit_image.py
@@ -58,6 +58,108 @@ def test_image_overrides_with_content_sets_file_none(mocker):
     assert 'content_sets_file' not in image.packages
 
 
+def test_image_overrides_with_content_sets():
+    image = Image(yaml.safe_load("""
+    from: foo
+    name: test/foo
+    version: 1.9
+    packages:
+      install:
+        - abc
+        - def
+      content_sets:
+        arch:
+          - namea
+          - nameb
+    """), 'foo')
+
+    assert image.packages.content_sets == {'arch': ['namea', 'nameb']}
+    assert 'content_sets_file' not in image.packages
+
+    image.apply_image_overrides(
+        [Overrides({'packages': {'content_sets': {'arch': ['new-arch']}}}, "a/path")])
+
+    assert image.packages.content_sets == {'arch': ['new-arch']}
+    assert 'content_sets_file' not in image.packages
+
+
+def test_image_overrides_with_content_sets_file(mocker):
+    image = Image(yaml.safe_load("""
+    from: foo
+    name: test/foo
+    version: 1.9
+    packages:
+      install:
+        - abc
+        - def
+      content_sets:
+        arch:
+          - namea
+          - nameb
+    """), 'foo')
+
+    assert image.packages.content_sets == {'arch': ['namea', 'nameb']}
+    assert 'content_sets_file' not in image.packages
+
+    with mocker.mock_module.patch('cekit.descriptor.packages.os.path.exists') as exists_mock:
+        exists_mock.return_value = True
+        with mocker.mock_module.patch('cekit.descriptor.packages.open', mocker.mock_open(read_data='{"arch": ["a", "b"]}')):
+            image.apply_image_overrides(
+                [Overrides({'packages': {'content_sets_file': 'some-path'}}, "a/path")])
+
+    assert image.packages.content_sets == {'arch': ['a', 'b']}
+    assert 'content_sets_file' not in image.packages
+
+
+def test_image_overrides_packages_repositories_add():
+    image = Image(yaml.safe_load("""
+        from: foo
+        name: test/foo
+        version: 1.9
+        packages:
+            repositories:
+                - name: scl
+                  rpm: centos-release-scl
+        """), 'foo')
+
+    assert len(image.packages.repositories) == 1
+    assert image.packages.repositories[0].rpm == 'centos-release-scl'
+    assert image.packages.repositories[0].name == 'scl'
+
+    image.apply_image_overrides(
+        [Overrides({'packages': {'repositories': [{"id": "rhel7-extras-rpm", "name": "extras"}]}}, "a/path")])
+
+    assert len(image.packages.repositories) == 2
+    assert image.packages.repositories[0].rpm == 'centos-release-scl'
+    assert image.packages.repositories[0].name == 'scl'
+    assert image.packages.repositories[1].id == 'rhel7-extras-rpm'
+    assert image.packages.repositories[1].name == 'extras'
+
+
+def test_image_overrides_packages_repositories_replace():
+    image = Image(yaml.safe_load("""
+        from: foo
+        name: test/foo
+        version: 1.9
+        packages:
+            repositories:
+                - name: scl
+                  rpm: centos-release-scl
+        """), 'foo')
+
+    assert len(image.packages.repositories) == 1
+    assert image.packages.repositories[0].rpm == 'centos-release-scl'
+    assert image.packages.repositories[0].name == 'scl'
+
+    image.apply_image_overrides(
+        [Overrides({'packages': {'repositories': [{"id": "rhel7-extras-rpm", "name": "scl"}]}}, "a/path")])
+
+    assert len(image.packages.repositories) == 1
+    assert image.packages.repositories[0].name == 'scl'
+    assert image.packages.repositories[0].id == 'rhel7-extras-rpm'
+    assert 'rpm' not in image.packages.repositories[0]
+
+
 def test_module_processing_simple_modules_order_to_install():
     image = Image(yaml.safe_load("""
         from: foo
@@ -192,6 +294,45 @@ def test_module_processing_modules_with_multiple_versions(caplog):
     assert "Module version not specified for 'org.test.module.b' module, using '1.1' default version" in caplog.text
 
 
+# https://github.com/cekit/cekit/issues/617
+def test_module_processing_modules_with_single_versions(caplog):
+    caplog.set_level(logging.DEBUG, logger="cekit")
+
+    image = Image(yaml.safe_load("""
+        from: foo
+        name: test/foo
+        version: 1.9
+        """), 'foo')
+
+    module_a = Module(yaml.safe_load("""
+        name: org.test.module.a
+        version: 1.0
+        """), 'path', 'artifact_path')
+
+    module_b = Module(yaml.safe_load("""
+        name: org.test.module.b
+        version: 1.0
+        """), 'path', 'artifact_path')
+
+    module_registry = ModuleRegistry()
+    module_registry.add_module(module_a)
+    module_registry.add_module(module_b)
+
+    resulting_install_list = OrderedDict()
+
+    to_install_list = [
+        Map({'name': 'org.test.module.a', 'version': '1.0'}),
+        Map({'name': 'org.test.module.b'})
+    ]
+
+    image.process_install_list(image, to_install_list, resulting_install_list, module_registry)
+
+    assert resulting_install_list == OrderedDict(
+        [('org.test.module.a', {'name': 'org.test.module.a', 'version': '1.0'}), ('org.test.module.b', {'name': 'org.test.module.b'})])
+
+    assert "Module version not specified for" not in caplog.text
+
+
 def test_module_processing_fail_when_a_module_aready_exists_in_registry():
     module_a = Module(yaml.safe_load("""
         name: org.test.module.a
diff --git a/tests/test_unit_resource.py b/tests/test_unit_resource.py
index 3092044..2a9bd12 100644
--- a/tests/test_unit_resource.py
+++ b/tests/test_unit_resource.py
@@ -7,6 +7,12 @@ from cekit.descriptor import Image, Overrides
 from cekit.descriptor.resource import create_resource
 from cekit.config import Config
 from cekit.errors import CekitError
+from cekit.tools import Chdir
+
+try:
+    from unittest.mock import call
+except ImportError:
+    from mock import call
 
 config = Config()
 
@@ -21,13 +27,18 @@ def setup_function(function):
 def test_repository_dir_is_constructed_properly(mocker):
     mocker.patch('subprocess.check_output')
     mocker.patch('os.path.isdir', ret='True')
+    mocker.patch('cekit.descriptor.resource.Chdir', autospec=True)
+
     res = create_resource({'git': {'url': 'http://host.com/url/repo.git', 'ref': 'ref'}})
+
     assert res.copy('dir') == 'dir/repo'
 
 
 def test_repository_dir_uses_name_if_defined(mocker):
     mocker.patch('subprocess.check_output')
     mocker.patch('os.path.isdir', ret='True')
+    mocker.patch('cekit.descriptor.resource.Chdir', autospec=True)
+
     res = create_resource(
         {'name': 'some-id', 'git': {'url': 'http://host.com/url/repo.git', 'ref': 'ref'}})
     assert res.copy('dir') == 'dir/some-id'
@@ -36,6 +47,8 @@ def test_repository_dir_uses_name_if_defined(mocker):
 def test_repository_dir_uses_target_if_defined(mocker):
     mocker.patch('subprocess.check_output')
     mocker.patch('os.path.isdir', ret='True')
+    mocker.patch('cekit.descriptor.resource.Chdir', autospec=True)
+
     res = create_resource(
         {'target': 'some-name', 'git': {'url': 'http://host.com/url/repo.git', 'ref': 'ref'}})
     assert res.copy('dir') == 'dir/some-name'
@@ -44,17 +57,14 @@ def test_repository_dir_uses_target_if_defined(mocker):
 def test_git_clone(mocker):
     mock = mocker.patch('subprocess.check_output')
     mocker.patch('os.path.isdir', ret='True')
+    mocker.patch('cekit.descriptor.resource.Chdir', autospec=True)
+
     res = create_resource({'git': {'url': 'http://host.com/url/path.git', 'ref': 'ref'}})
     res.copy('dir')
-    mock.assert_called_with(['git',
-                             'clone',
-                             '--depth',
-                             '1',
-                             'http://host.com/url/path.git',
-                             'dir/path',
-                             '-b',
-                             'ref'],
-                            stderr=-2)
+    mock.assert_has_calls([
+        call(['git', 'clone', 'http://host.com/url/path.git', 'dir/path'], stderr=-2),
+        call(['git', 'checkout', 'ref'], stderr=-2)
+    ])
 
 
 def get_res(mocker):