Repository: cekit/cekit

Issue:
Modules should use name instead of git repository name + ref
We need the directory containing the features (copied to target/test) to be deterministic and not bound to the kind of module repo acces (GIT vs local file system).

The name of the module should be used when creating the directory.

An example usage in the feature file :
https://github.com/wildfly/temp-eap-modules/blob/EAP7-1216/tests/features/7/CD/security_domains.feature#L121
The scripts we are copying are there:
https://github.com/wildfly/temp-eap-modules/tree/EAP7-1216/tests/features/7/scripts
They are located inside the features dir of the module, so copied to the
target/tests dir.
It works when we use a local file system path for the jboss-eap-modules
but fails when GIT is in use:
https://github.com/wildfly/temp-openshift-image/blob/EAP7-1216/image.yaml#L38


Discussion:

Patch:
diff --git a/cekit/builder.py b/cekit/builder.py
index 39f0c5f..2b6bac3 100644
--- a/cekit/builder.py
+++ b/cekit/builder.py
@@ -80,11 +80,6 @@ class Builder(Command):
                                         self.params.target,
                                         self.params.overrides)
 
-        if self.params.get('koji_target', False):
-            LOGGER.warning("The '--koji-target' switch is deprecated, please use overrides " +
-                           "(http://docs.cekit.io/en/latest/handbook/overrides.html) to adjust the koji " +
-                           "target, '--koji-target' will be removed in version 3.6")
-
         if CONFIG.get('common', 'redhat'):
             # Add the redhat specific stuff after everything else
             self.generator.add_redhat_overrides()
diff --git a/cekit/builders/osbs.py b/cekit/builders/osbs.py
index f1513f7..defa77c 100644
--- a/cekit/builders/osbs.py
+++ b/cekit/builders/osbs.py
@@ -258,11 +258,6 @@ class OSBSBuilder(Builder):
                 # Default to computed target based on branch
                 target = "{}-containers-candidate".format(self.dist_git.branch)
 
-            # Check if it was specified on command line
-            # TODO: Remove in 3.6
-            if self.params.koji_target:
-                target = self.params.koji_target
-
             scratch = True
 
             if self.params.release:
diff --git a/cekit/cli.py b/cekit/cli.py
index ec62129..8532996 100644
--- a/cekit/cli.py
+++ b/cekit/cli.py
@@ -144,13 +144,11 @@ def build_podman(ctx, pull, no_squash, tags):  # pylint: disable=unused-argument
 @click.option('--user', metavar="USER", help="User used to kick the build as.")
 @click.option('--nowait', help="Do not wait for the task to finish.", is_flag=True)
 @click.option('--stage', help="Use stage environmen.", is_flag=True)
-# TODO: Remove in 3.6
-@click.option('--koji-target', metavar="TARGET", help="Override the default Koji target.")
 @click.option('--sync-only', help="Generate files and sync with dist-git, but do not execute build.", is_flag=True)
 @click.option('--commit-message', metavar="MESSAGE", help="Custom dist-git commit message.")
 @click.option('--assume-yes', '-y', help="Execute build in non-interactive mode.", is_flag=True)
 @click.pass_context
-def build_osbs(ctx, release, user, nowait, stage, koji_target, sync_only, commit_message, assume_yes):  # pylint: disable=unused-argument
+def build_osbs(ctx, release, user, nowait, stage, sync_only, commit_message, assume_yes):  # pylint: disable=unused-argument
     """
     DESCRIPTION
 
diff --git a/cekit/descriptor/resource.py b/cekit/descriptor/resource.py
index ab46646..755cd73 100644
--- a/cekit/descriptor/resource.py
+++ b/cekit/descriptor/resource.py
@@ -434,10 +434,7 @@ class _GitResource(Resource):
         super(_GitResource, self).__init__(descriptor)
 
     def _get_default_name_value(self, descriptor):
-        return os.path.basename(descriptor.get('git', {}).get('url'))
-
-    def _get_default_target_value(self, descriptor):
-        return "{}-{}".format(os.path.basename(descriptor.get('git').get('url')), descriptor.get('git').get('ref'))
+        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',
diff --git a/docs/handbook/building/builder-engines.rst b/docs/handbook/building/builder-engines.rst
index ad8c615..78b2a44 100644
--- a/docs/handbook/building/builder-engines.rst
+++ b/docs/handbook/building/builder-engines.rst
@@ -133,10 +133,6 @@ Parameters
         Do not wait for the task to finish
     ``--stage``
         Use stage environment
-    ``--koji-target``
-        .. deprecated:: 3.5
-
-        Overrides the default ``koji`` target
     ``--commit-message``
         Custom commit message for dist-git
     ``--sync-only``


Test patch:
diff --git a/tests/test_builder.py b/tests/test_builder.py
index 0fa49d0..39d91b5 100644
--- a/tests/test_builder.py
+++ b/tests/test_builder.py
@@ -264,22 +264,6 @@ def test_osbs_builder_run_brew_user(mocker):
                                      "{'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_brew_target_defined_on_cli(mocker):
-    config.cfg['common'] = {'redhat': True}
-    params = {'koji_target': 'Foo'}
-
-    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_builder_object(mocker, 'osbs', params)
-    builder.generator = Map({'image': Map({})})
-    mocker.patch.object(builder, '_wait_for_osbs_task')
-    builder.dist_git.branch = "some-branch"
-    builder.run()
-
-    check_output.assert_called_with(['/usr/bin/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_builder_run_brew_target_defined_in_descriptor(mocker):
     config.cfg['common'] = {'redhat': True}
 
diff --git a/tests/test_integ_builder_osbs.py b/tests/test_integ_builder_osbs.py
index 342e18e..60658bd 100644
--- a/tests/test_integ_builder_osbs.py
+++ b/tests/test_integ_builder_osbs.py
@@ -448,24 +448,3 @@ def test_osbs_builder_with_koji_target_in_descriptor(tmpdir, mocker, caplog):
     run_osbs(descriptor, str(tmpdir), mocker)
 
     assert "About to execute '/usr/bin/brew call --python buildContainer --kwargs {'src': 'git://somehost.com/containers/somerepo#3b9283cb26b35511517ff5c0c3e11f490cba8feb', 'target': 'some-target', 'opts': {'scratch': True, 'git_branch': 'branch', 'yum_repourls': []}}'." in caplog.text
-
-
-# TODO: Remove in 3.6
-def test_osbs_builder_with_koji_target_in_descriptor_and_overriden_in_cli(tmpdir, mocker, caplog):
-    mocker.patch('cekit.tools.decision', return_value=True)
-    mocker.patch('cekit.descriptor.resource.urlopen')
-    mocker.patch.object(subprocess, 'call')
-    mocker.patch.object(subprocess, 'check_call')
-
-    tmpdir.mkdir('osbs').mkdir('repo').mkdir(
-        '.git').join('other').write_text(u'Some content', 'utf8')
-
-    descriptor = image_descriptor.copy()
-
-    descriptor['osbs']['koji_target'] = 'some-target'
-
-    run_osbs(descriptor, str(tmpdir), mocker, build_command=[
-             'build', 'osbs', '--koji-target', 'custom-target'])
-
-    assert "The '--koji-target' switch is deprecated, please use overrides (http://docs.cekit.io/en/latest/handbook/overrides.html) to adjust the koji target, '--koji-target' will be removed in version 3.6" in caplog.text
-    assert "About to execute '/usr/bin/brew call --python buildContainer --kwargs {'src': 'git://somehost.com/containers/somerepo#3b9283cb26b35511517ff5c0c3e11f490cba8feb', 'target': 'custom-target', 'opts': {'scratch': True, 'git_branch': 'branch', 'yum_repourls': []}}'." in caplog.text
diff --git a/tests/test_unit_args.py b/tests/test_unit_args.py
index 0d9878b..7ac1f16 100644
--- a/tests/test_unit_args.py
+++ b/tests/test_unit_args.py
@@ -83,7 +83,7 @@ def _get_class_by_name(clazz):
         {
             'descriptor': 'image.yaml', 'verbose': False, 'work_dir': '~/.cekit', 'config': '~/.cekit/config',
             'redhat': False, 'target': 'target', 'validate': False, 'dry_run': False, 'overrides': (), 'nowait': False,
-            'release': False, 'user': None, 'stage': False, 'koji_target': None, 'sync_only': False, 'commit_message': None, 'assume_yes': False
+            'release': False, 'user': None, 'stage': False, 'sync_only': False, 'commit_message': None, 'assume_yes': False
         }
     ),
     # Test setting user for OSBS
@@ -93,7 +93,7 @@ def _get_class_by_name(clazz):
         {
             'descriptor': 'image.yaml', 'verbose': False, 'work_dir': '~/.cekit', 'config': '~/.cekit/config',
             'redhat': False, 'target': 'target', 'validate': False, 'dry_run': False, 'overrides': (), 'nowait': False,
-            'release': False, 'user': 'SOMEUSER', 'stage': False, 'koji_target': None, 'sync_only': False,
+            'release': False, 'user': 'SOMEUSER', 'stage': False, 'sync_only': False,
             'commit_message': None, 'assume_yes': False
         }
     ),
@@ -104,7 +104,7 @@ def _get_class_by_name(clazz):
         {
             'descriptor': 'image.yaml', 'verbose': False, 'work_dir': '~/.cekit', 'config': '~/.cekit/config',
             'redhat': False, 'target': 'target', 'validate': False, 'dry_run': False, 'overrides': (), 'nowait': False,
-            'release': False, 'user': None, 'stage': True, 'koji_target': None, 'sync_only': False, 'commit_message': None, 'assume_yes': False
+            'release': False, 'user': None, 'stage': True, 'sync_only': False, 'commit_message': None, 'assume_yes': False
         }
     ),
     # Test setting nowait for OSBS
@@ -114,7 +114,7 @@ def _get_class_by_name(clazz):
         {
             'descriptor': 'image.yaml', 'verbose': False, 'work_dir': '~/.cekit', 'config': '~/.cekit/config',
             'redhat': False, 'target': 'target', 'validate': False, 'dry_run': False, 'overrides': (), 'nowait': True,
-            'release': False, 'user': None, 'stage': False, 'koji_target': None, 'sync_only': False, 'commit_message': None, 'assume_yes': False
+            'release': False, 'user': None, 'stage': False, 'sync_only': False, 'commit_message': None, 'assume_yes': False
         }
     ),
     (
@@ -141,7 +141,7 @@ def _get_class_by_name(clazz):
         {
             'descriptor': 'image.yaml', 'verbose': False, 'work_dir': '~/.cekit', 'config': '~/.cekit/config',
             'redhat': False, 'target': 'target', 'validate': False, 'dry_run': False, 'overrides': (), 'release': False,
-            'user': None, 'nowait': False, 'stage': False, 'koji_target': None, 'sync_only': False, 'commit_message': None, 'assume_yes': False
+            'user': None, 'nowait': False, 'stage': False, 'sync_only': False, 'commit_message': None, 'assume_yes': False
         }),
     (
         ['build', 'docker'],
diff --git a/tests/test_unit_resource.py b/tests/test_unit_resource.py
index 277b4ce..3092044 100644
--- a/tests/test_unit_resource.py
+++ b/tests/test_unit_resource.py
@@ -21,21 +21,37 @@ def setup_function(function):
 def test_repository_dir_is_constructed_properly(mocker):
     mocker.patch('subprocess.check_output')
     mocker.patch('os.path.isdir', ret='True')
-    res = create_resource({'git': {'url': 'url/repo', 'ref': 'ref'}})
-    assert res.copy('dir') == 'dir/repo-ref'
+    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')
+    res = create_resource(
+        {'name': 'some-id', 'git': {'url': 'http://host.com/url/repo.git', 'ref': 'ref'}})
+    assert res.copy('dir') == 'dir/some-id'
+
+
+def test_repository_dir_uses_target_if_defined(mocker):
+    mocker.patch('subprocess.check_output')
+    mocker.patch('os.path.isdir', ret='True')
+    res = create_resource(
+        {'target': 'some-name', 'git': {'url': 'http://host.com/url/repo.git', 'ref': 'ref'}})
+    assert res.copy('dir') == 'dir/some-name'
 
 
 def test_git_clone(mocker):
     mock = mocker.patch('subprocess.check_output')
     mocker.patch('os.path.isdir', ret='True')
-    res = create_resource({'git': {'url': 'url', 'ref': 'ref'}})
+    res = create_resource({'git': {'url': 'http://host.com/url/path.git', 'ref': 'ref'}})
     res.copy('dir')
     mock.assert_called_with(['git',
                              'clone',
                              '--depth',
                              '1',
-                             'url',
-                             'dir/url-ref',
+                             'http://host.com/url/path.git',
+                             'dir/path',
                              '-b',
                              'ref'],
                             stderr=-2)
@@ -119,8 +135,8 @@ def test_fetching_file_exists_but_used_as_is(mocker):
         pass
     mock_urlopen = get_mock_urlopen(mocker)
     res = create_resource({'name': 'file',
-                    'url': 'http:///dummy',
-                    'md5': 'd41d8cd98f00b204e9800998ecf8427e'})
+                           'url': 'http:///dummy',
+                           'md5': 'd41d8cd98f00b204e9800998ecf8427e'})
     res.copy()
     mock_urlopen.assert_not_called()
 
@@ -174,7 +190,7 @@ def test_generated_url_without_cacher():
 def test_resource_verify(mocker):
     mock = mocker.patch('cekit.descriptor.resource.check_sum')
     res = create_resource({'url': 'dummy',
-                    'sha256': 'justamocksum'})
+                           'sha256': 'justamocksum'})
     res._Resource__verify('dummy')
     mock.assert_called_with('dummy', 'sha256', 'justamocksum', 'dummy')
 
@@ -182,20 +198,20 @@ def test_resource_verify(mocker):
 def test_generated_url_with_cacher():
     config.cfg['common']['cache_url'] = '#filename#,#algorithm#,#hash#'
     res = create_resource({'url': 'dummy',
-                    'sha256': 'justamocksum'})
+                           'sha256': 'justamocksum'})
     res.name = 'file'
     assert res._Resource__substitute_cache_url('file') == 'file,sha256,justamocksum'
 
 
 def test_path_resource_absolute():
     res = create_resource({'name': 'foo',
-                    'path': '/bar'}, directory='/foo')
+                           'path': '/bar'}, directory='/foo')
     assert res.path == '/bar'
 
 
 def test_path_resource_relative():
     res = create_resource({'name': 'foo',
-                    'path': 'bar'}, directory='/foo')
+                           'path': 'bar'}, directory='/foo')
     assert res.path == '/foo/bar'
 
 
@@ -205,7 +221,7 @@ def test_path_local_existing_resource_no_cacher_use(mocker):
     shutil_mock = mocker.patch('shutil.copy2')
 
     res = create_resource({'name': 'foo',
-                    'path': 'bar'}, directory='/foo')
+                           'path': 'bar'}, directory='/foo')
 
     mocker.spy(res, '_download_file')
 
@@ -221,7 +237,7 @@ def test_path_local_non_existing_resource_with_cacher_use(mocker):
     mocker.patch('os.makedirs')
 
     res = create_resource({'name': 'foo',
-                    'path': 'bar'}, directory='/foo')
+                           'path': 'bar'}, directory='/foo')
 
     mocker.spy(res, '_download_file')
     download_file_mock = mocker.patch.object(res, '_download_file')
@@ -244,7 +260,7 @@ def test_url_resource_download_cleanup_after_failure(mocker, tmpdir, caplog):
     urlopen_mock.read.side_effect = Exception
 
     res = create_resource({'url': 'http://server.org/dummy',
-                    'sha256': 'justamocksum'})
+                           'sha256': 'justamocksum'})
 
     targetfile = os.path.join(str(tmpdir), 'targetfile')
 
@@ -274,7 +290,7 @@ def test_copy_plain_resource_with_cacher(mocker, tmpdir):
         pass
 
     res = create_resource({'name': 'foo',
-                    'md5': '5b9164ad6f496d9dee12ec7634ce253f'})
+                           'md5': '5b9164ad6f496d9dee12ec7634ce253f'})
 
     substitute_cache_url_mock = mocker.patch.object(
         res, '_Resource__substitute_cache_url', return_value='http://cache/abc')
@@ -301,7 +317,7 @@ def test_copy_plain_resource_from_brew(mocker, tmpdir):
         pass
 
     res = create_resource({'name': 'foo',
-                    'md5': '5b9164ad6f496d9dee12ec7634ce253f'})
+                           'md5': '5b9164ad6f496d9dee12ec7634ce253f'})
 
     mocker.spy(res, '_Resource__substitute_cache_url')