Repository: cekit/cekit

Issue:
Plain artifacts and cache_url does not work well together
```
2019-02-20 19:56:43,514 cekit        INFO     Preparing resource 'hawkular-javaagent'
2019-02-20 19:56:43,540 cekit        DEBUG    Trying to download artifact hawkular-javaagent from remote cache
Traceback (most recent call last):
 File "/usr/lib/python3.6/site-packages/cekit/descriptor/resource.py", line 309, in _copy_impl
   self._download_file(self.url, target)
AttributeError: '_PlainResource' object has no attribute 'url'
```

Discussion:
goldmann: @rnc Rebased to fix conflict after merging other PR.

Patch:
diff --git a/cekit/descriptor/resource.py b/cekit/descriptor/resource.py
index b506057..2917776 100644
--- a/cekit/descriptor/resource.py
+++ b/cekit/descriptor/resource.py
@@ -156,16 +156,19 @@ class Resource(Descriptor):
 
     def __substitute_cache_url(self, url):
         cache = config.get('common', 'cache_url')
+
         if not cache:
             return url
 
         for algorithm in SUPPORTED_HASH_ALGORITHMS:
             if algorithm in self:
-                logger.debug("Using %s to fetch artifacts from cacher."
-                             % algorithm)
-                return (cache.replace('#filename#', self.name)
-                        .replace('#algorithm#', algorithm)
-                        .replace('#hash#', self[algorithm]))
+                logger.debug("Using {} checksum to fetch artifacts from cacher".format(algorithm))
+
+                url = cache.replace('#filename#', self.name).replace(
+                    '#algorithm#', algorithm).replace('#hash#', self[algorithm])
+
+                logger.debug("Using cache url '{}'".format(url))
+
         return url
 
     def _download_file(self, url, destination, use_cache=True):
@@ -312,26 +315,30 @@ class _PlainResource(Resource):
         self.target = self.target_file_name()
 
     def _copy_impl(self, target):
-
+        # First of all try to download the file using cacher if specified
         if config.get('common', 'cache_url'):
-            logger.debug("Trying to download artifact %s from remote cache" % self.name)
-            # If cacher URL is set, use it
             try:
-                self._download_file(self.url, target)
+                self._download_file(None, target)
                 return target
-            except:
-                logger.warning("Could not download artifact %s from the remote cache" % self.name)
+            except Exception as e:
+                logger.debug(str(e))
+                logger.warning("Could not download '{}' artifact using cacher".format(self.name))
 
         md5 = self.get('md5')
 
+        # Next option is to download it from Brew directly but only if the md5 checkum
+        # is provided and we are running with the --redhat switch
         if md5 and config.get('common', 'redhat'):
-            logger.debug("Trying to download artifact %s from Brew directly" % self.name)
+            logger.debug("Trying to download artifact '{}' from Brew directly".format(self.name))
 
             try:
-                self.url = get_brew_url(md5)
-                self._download_file(self.url, target, use_cache=False)
+                # Generate the URL
+                url = get_brew_url(md5)
+                # Use the URL to download the file
+                self._download_file(url, target, use_cache=False)
                 return target
-            except:
-                logger.warning("Could not download artifact %s from Brew" % self.name)
+            except Exception as e:
+                logger.debug(str(e))
+                logger.warning("Could not download artifact '{}' from Brew".format(self.name))
 
-        raise CekitError("Artifact %s could not be found" % self.name)
+        raise CekitError("Artifact {} could not be found".format(self.name))


Test patch:
diff --git a/tests/test_unit_resource.py b/tests/test_unit_resource.py
index b140573..feba92b 100644
--- a/tests/test_unit_resource.py
+++ b/tests/test_unit_resource.py
@@ -70,6 +70,7 @@ def test_fetching_with_ssl_verify(mocker):
     mock_urlopen = get_mock_urlopen(mocker)
 
     res = Resource({'name': 'file', 'url': 'https:///dummy'})
+
     try:
         res.copy()
     except:
@@ -88,6 +89,7 @@ def test_fetching_disable_ssl_verify(mocker):
     get_mock_ssl(mocker, ctx)
 
     res = Resource({'name': 'file', 'url': 'https:///dummy'})
+
     try:
         res.copy()
     except:
@@ -153,7 +155,9 @@ def test_fetching_file_exists_no_hash_fetched_again(mocker):
 
     with open('file', 'w') as f:  # noqa: F841
         pass
+
     res = Resource({'name': 'file', 'url': 'http:///dummy'})
+
     with pytest.raises(CekitError):
         # url is not valid so we get error, but we are not interested
         # in it. We just need to check that we attempted to downlad.
@@ -253,6 +257,63 @@ def test_url_resource_download_cleanup_after_failure(mocker, tmpdir, caplog):
     os_remove_mock.assert_called_with(targetfile)
 
 
+def test_copy_plain_resource_with_cacher(mocker, tmpdir):
+    config.cfg['common']['cache_url'] = '#filename#,#algorithm#,#hash#'
+    config.cfg['common']['work_dir'] = str(tmpdir)
+
+    urlopen_class_mock = mocker.patch('cekit.descriptor.resource.urlopen')
+    mock_urlopen = urlopen_class_mock.return_value
+    mock_urlopen.getcode.return_value = 200
+    mock_urlopen.read.side_effect = [b"one", b"two", None]
+
+    ctx = get_ctx(mocker)
+    get_mock_ssl(mocker, ctx)
+
+    with open('file', 'w') as f:  # noqa: F841
+        pass
+
+    res = Resource({'name': 'foo',
+                    'md5': '5b9164ad6f496d9dee12ec7634ce253f'})
+
+    substitute_cache_url_mock = mocker.patch.object(
+        res, '_Resource__substitute_cache_url', return_value='http://cache/abc')
+
+    res.copy(str(tmpdir))
+
+    substitute_cache_url_mock.assert_called_once_with(None)
+    urlopen_class_mock.assert_called_with('http://cache/abc', context=ctx)
+
+
+def test_copy_plain_resource_from_brew(mocker, tmpdir):
+    config.cfg['common']['work_dir'] = str(tmpdir)
+    config.cfg['common']['redhat'] = True
+
+    urlopen_class_mock = mocker.patch('cekit.descriptor.resource.urlopen')
+    mock_urlopen = urlopen_class_mock.return_value
+    mock_urlopen.getcode.return_value = 200
+    mock_urlopen.read.side_effect = [b"one", b"two", None]
+
+    ctx = get_ctx(mocker)
+    get_mock_ssl(mocker, ctx)
+
+    with open('file', 'w') as f:  # noqa: F841
+        pass
+
+    res = Resource({'name': 'foo',
+                    'md5': '5b9164ad6f496d9dee12ec7634ce253f'})
+
+    mocker.spy(res, '_Resource__substitute_cache_url')
+
+    mock_get_brew_url = mocker.patch(
+        'cekit.descriptor.resource.get_brew_url', return_value='http://cache/abc')
+
+    res.copy(str(tmpdir))
+
+    mock_get_brew_url.assert_called_once_with('5b9164ad6f496d9dee12ec7634ce253f')
+    res._Resource__substitute_cache_url.call_count == 0
+    urlopen_class_mock.assert_called_with('http://cache/abc', context=ctx)
+
+
 def test_overide_resource_remove_chksum():
     image = Image(yaml.safe_load("""
     from: foo