Repository: cekit/cekit

Issue:
Review caching
- [x] `cekit-cache` command without parameters prints nothing
- [x] Caching local file should not use involve usage of cacher, even if defined
- [x] Error handling - in case an error occurs while caching artifact, leftovers should be cleaned up.
- [x] Artifact added to cache should have computed checksum for every supported algorithm
  * This also helps with the artifact duplication if the `add` command was run on with the same artifact, but with different algorithms.

Discussion:
goldmann: @rnc This is now ready for review. There are a few commits. You can review it all in once, but for better understating on the change I suggest to look at commit separately.

I suggest also to look at tasks listed in #361.

Patch:
diff --git a/.pylintrc b/.pylintrc
index a2c5df1..ad93845 100644
--- a/.pylintrc
+++ b/.pylintrc
@@ -1,5 +1,5 @@
 [MESSAGES CONTROL]
-disable=logging-format-interpolation,logging-not-lazy,useless-object-inheritance
+disable=logging-format-interpolation,logging-not-lazy,useless-object-inheritance,invalid-name,missing-docstring,no-self-use
 
 [FORMAT]
 ignore-long-lines=(^\s*(# )?<?https?://\S+>?$)|(^@click\.)
diff --git a/cekit/cache/artifact.py b/cekit/cache/artifact.py
index 614efc3..ffa2091 100644
--- a/cekit/cache/artifact.py
+++ b/cekit/cache/artifact.py
@@ -5,7 +5,7 @@ import uuid
 import yaml
 
 from cekit.config import Config
-from cekit.crypto import SUPPORTED_HASH_ALGORITHMS
+from cekit.crypto import SUPPORTED_HASH_ALGORITHMS, get_sum
 from cekit.errors import CekitError
 
 CONFIG = Config()
@@ -19,21 +19,21 @@ class ArtifactCache():
     """
 
     def __init__(self):
-        self._cache_dir = os.path.expanduser(
+        self.cache_dir = os.path.expanduser(
             os.path.join(CONFIG.get('common', 'work_dir'), 'cache'))
-        if not os.path.exists(self._cache_dir):
-            os.makedirs(self._cache_dir)
+        if not os.path.exists(self.cache_dir):
+            os.makedirs(self.cache_dir)
 
     def _get_cache(self):
         cache = {}
-        for index_file in glob.glob(os.path.join(self._cache_dir, '*.yaml')):
+        for index_file in glob.glob(os.path.join(self.cache_dir, '*.yaml')):
             with open(index_file, 'r') as file_:
                 cache[os.path.basename(index_file)] = yaml.safe_load(file_)
 
         return cache
 
     def _update_cache(self, cache_entry, artifact_id):
-        index_file = os.path.join(self._cache_dir, artifact_id + '.yaml')
+        index_file = os.path.join(self.cache_dir, artifact_id + '.yaml')
         tmp_cache_file = index_file + str(os.getpid())
         with open(tmp_cache_file, 'w') as file_:
             yaml.safe_dump(cache_entry, file_)
@@ -51,16 +51,16 @@ class ArtifactCache():
 
         artifact_id = str(uuid.uuid4())
 
-        artifact_file = os.path.expanduser(os.path.join(self._cache_dir, artifact_id))
+        artifact_file = os.path.expanduser(os.path.join(self.cache_dir, artifact_id))
         if not os.path.exists(artifact_file):
             artifact.guarded_copy(artifact_file)
 
         cache_entry = {'names': [artifact['name']],
                        'cached_path': artifact_file}
 
+        # We should populate the cache entry with checksums for all supported algorithms
         for alg in SUPPORTED_HASH_ALGORITHMS:
-            if alg in artifact:
-                cache_entry.update({alg: artifact[alg]})
+            cache_entry.update({alg: get_sum(artifact_file, alg)})
 
         self._update_cache(cache_entry, artifact_id)
         return artifact_id
@@ -68,8 +68,8 @@ class ArtifactCache():
     def delete(self, artifact_uuid):
         # check if artifact exists
         self._get_cache()
-        os.remove(os.path.join(self._cache_dir, artifact_uuid))
-        os.remove(os.path.join(self._cache_dir, artifact_uuid + '.yaml'))
+        os.remove(os.path.join(self.cache_dir, artifact_uuid))
+        os.remove(os.path.join(self.cache_dir, artifact_uuid + '.yaml'))
 
     def get(self, artifact):
         for alg in SUPPORTED_HASH_ALGORITHMS:
diff --git a/cekit/cache/cli.py b/cekit/cache/cli.py
index e202e9c..462e30c 100644
--- a/cekit/cache/cli.py
+++ b/cekit/cache/cli.py
@@ -1,4 +1,5 @@
 import logging
+import shutil
 import sys
 
 import click
@@ -12,8 +13,8 @@ from cekit.tools import Map
 from cekit.version import version
 
 setup_logging()
-logger = logging.getLogger('cekit')
-config = Config()
+LOGGER = logging.getLogger('cekit')
+CONFIG = Config()
 
 
 @click.group(context_settings=dict(max_content_width=100))
@@ -21,7 +22,7 @@ config = Config()
 @click.option('--config', metavar="PATH", help="Path to configuration file.", default="~/.cekit/config", show_default=True)
 @click.option('--work-dir', metavar="PATH", help="Location of the working directory.", default="~/.cekit", show_default=True)
 @click.version_option(message="%(version)s", version=version)
-def cli(config, verbose, work_dir):
+def cli(config, verbose, work_dir):  # pylint: disable=unused-argument
     pass
 
 
@@ -35,7 +36,7 @@ def ls():
 @click.option('--md5', metavar="CHECKSUM", help="The md5 checksum of the artifact.")
 @click.option('--sha1', metavar="CHECKSUM", help="The sha1 checksum of the artifact.")
 @click.option('--sha256', metavar="CHECKSUM", help="The sha256 checksum of the artifact.")
-def add(location, md5, sha1, sha256):
+def add(location, md5, sha1, sha256):  # pylint: disable=unused-argument
     if not (md5 or sha1 or sha256):
         raise click.UsageError("At least one checksum must be provided")
 
@@ -48,6 +49,11 @@ def rm(uuid):
     CacheCli.prepare().rm(uuid)
 
 
+@cli.command(name="clear", short_help="Remove all artifacts from the cache")
+def clear():
+    CacheCli.prepare().clear()
+
+
 class CacheCli():
     @staticmethod
     def prepare():
@@ -58,11 +64,11 @@ class CacheCli():
 
         # TODO: logging is used only when adding the artifact, we need to find out if it would be possible to do it better
         if args.verbose:
-            logger.setLevel(logging.DEBUG)
+            LOGGER.setLevel(logging.DEBUG)
         else:
-            logger.setLevel(logging.INFO)
+            LOGGER.setLevel(logging.INFO)
 
-        config.configure(args.config, {'work_dir': args.work_dir})
+        CONFIG.configure(args.config, {'work_dir': args.work_dir})
 
     def add(self, location, md5, sha1, sha256):
         artifact_cache = ArtifactCache()
@@ -90,7 +96,7 @@ class CacheCli():
         try:
             artifact_id = artifact_cache.add(artifact)
             click.echo("Artifact {} cached with UUID '{}'".format(location, artifact_id))
-        except Exception as ex:
+        except Exception as ex:  # pylint: disable=broad-except
             click.secho("Cannot cache artifact {}: {}".format(location, str(ex)), fg='red')
             sys.exit(1)
 
@@ -118,10 +124,29 @@ class CacheCli():
         try:
             artifact_cache.delete(uuid)
             click.echo("Artifact with UUID '{}' removed".format(uuid))
-        except Exception:
+        except Exception:  # pylint: disable=broad-except
             click.secho("Artifact with UUID '{}' doesn't exists in the cache".format(uuid), fg='yellow')
             sys.exit(1)
 
+    def clear(self):
+        """
+        Removes the artifact cache directory with all artifacts.
+
+        Use with caution!
+        """
+        artifact_cache = ArtifactCache()
+
+        if not click.confirm("Are you sure to remove all artifacts from cache?", show_default=True):
+            return
+
+        try:
+            shutil.rmtree(artifact_cache.cache_dir)
+            click.echo("Artifact cache cleared!")
+        except Exception:  # pylint: disable=broad-except
+            click.secho("An error occurred while removing the artifact cache directory '{}'".format(
+                artifact_cache.cache_dir), fg='red')
+            sys.exit(1)
+
 
 if __name__ == "__main__":
     cli()  # pylint: disable=no-value-for-parameter
diff --git a/cekit/crypto.py b/cekit/crypto.py
index 74c2969..ac30590 100644
--- a/cekit/crypto.py
+++ b/cekit/crypto.py
@@ -9,11 +9,13 @@ SUPPORTED_HASH_ALGORITHMS = ['sha256', 'sha1', 'md5']
 def get_sum(target, algorithm):
     hash_function = getattr(hashlib, algorithm)()
 
+    logger.debug("Computing {} checksum for '{}' file".format(algorithm, target))
+
     with open(target, "rb") as f:
         for chunk in iter(lambda: f.read(65536), b""):
             hash_function.update(chunk)
     return hash_function.hexdigest()
-    
+
 
 def check_sum(target, algorithm, expected, name=None):
     """ Check that file chksum is correct
diff --git a/cekit/descriptor/resource.py b/cekit/descriptor/resource.py
index ed76acd..b506057 100644
--- a/cekit/descriptor/resource.py
+++ b/cekit/descriptor/resource.py
@@ -121,7 +121,7 @@ class Resource(Descriptor):
             self._copy_impl(target)
         except Exception as ex:
             logger.warning("Cekit is not able to fetch resource '%s' automatically. "
-                        "Please use cekit-cache command to add this artifact manually." % self.name)
+                           "Please use cekit-cache command to add this artifact manually." % self.name)
 
             if self.description:
                 logger.info(self.description)
@@ -200,12 +200,22 @@ class Resource(Descriptor):
 
             if res.getcode() != 200:
                 raise CekitError("Could not download file from %s" % url)
-            with open(destination, 'wb') as f:
-                while True:
-                    chunk = res.read(1024)
-                    if not chunk:
-                        break
-                    f.write(chunk)
+
+            try:
+                with open(destination, 'wb') as f:
+                    while True:
+                        chunk = res.read(1024)
+                        if not chunk:
+                            break
+                        f.write(chunk)
+            except Exception:
+                try:
+                    logger.debug("Removing incompletely downloaded '{}' file".format(destination))
+                    os.remove(destination)
+                except OSError:
+                    logger.warning("An error occurred while removing file '{}'".format(destination))
+
+                raise
         else:
             raise CekitError("Unsupported URL scheme: %s" % (url))
 


Test patch:
diff --git a/tests/test_unit_resource.py b/tests/test_unit_resource.py
index 7d301c6..b140573 100644
--- a/tests/test_unit_resource.py
+++ b/tests/test_unit_resource.py
@@ -1,3 +1,4 @@
+import logging
 import pytest
 import os
 import yaml
@@ -8,6 +9,7 @@ from cekit.errors import CekitError
 
 config = Config()
 
+
 def setup_function(function):
     config.cfg['common'] = {'work_dir': '/tmp'}
 
@@ -192,6 +194,65 @@ def test_path_resource_relative():
     assert res.path == '/foo/bar'
 
 
+def test_path_local_existing_resource_no_cacher_use(mocker):
+    config.cfg['common']['cache_url'] = '#filename#,#algorithm#,#hash#'
+    mocker.patch('os.path.exists', return_value=True)
+    shutil_mock = mocker.patch('shutil.copy')
+
+    res = Resource({'name': 'foo',
+                    'path': 'bar'}, directory='/foo')
+
+    mocker.spy(res, '_download_file')
+
+    res.guarded_copy('target')
+
+    shutil_mock.assert_called_with('/foo/bar', 'target')
+    assert res._download_file.call_count == 0
+
+
+def test_path_local_non_existing_resource_with_cacher_use(mocker):
+    config.cfg['common']['cache_url'] = '#filename#,#algorithm#,#hash#'
+    mocker.patch('os.path.exists', return_value=False)
+    mocker.patch('os.makedirs')
+
+    res = Resource({'name': 'foo',
+                    'path': 'bar'}, directory='/foo')
+
+    mocker.spy(res, '_download_file')
+    download_file_mock = mocker.patch.object(res, '_download_file')
+
+    res.guarded_copy('target')
+
+    download_file_mock.assert_called_with('/foo/bar', 'target')
+
+
+def test_url_resource_download_cleanup_after_failure(mocker, tmpdir, caplog):
+    caplog.set_level(logging.DEBUG, logger="cekit")
+
+    mocker.patch('os.path.exists', return_value=False)
+    mocker.patch('os.makedirs')
+    os_remove_mock = mocker.patch('os.remove')
+
+    urlopen_class_mock = mocker.patch('cekit.descriptor.resource.urlopen')
+    urlopen_mock = urlopen_class_mock.return_value
+    urlopen_mock.getcode.return_value = 200
+    urlopen_mock.read.side_effect = Exception
+
+    res = Resource({'url': 'http://server.org/dummy',
+                    'sha256': 'justamocksum'})
+
+    targetfile = os.path.join(str(tmpdir), 'targetfile')
+
+    with pytest.raises(CekitError) as excinfo:
+        res.guarded_copy(targetfile)
+
+    assert "Error copying resource: 'dummy'. See logs for more info" in str(excinfo.value)
+    assert "Removing incompletely downloaded '{}' file".format(targetfile) in caplog.text
+
+    urlopen_class_mock.assert_called_with('http://server.org/dummy', context=mocker.ANY)
+    os_remove_mock.assert_called_with(targetfile)
+
+
 def test_overide_resource_remove_chksum():
     image = Image(yaml.safe_load("""
     from: foo
diff --git a/tests/test_unit_tools.py b/tests/test_unit_tools.py
index faef21b..b146a0e 100644
--- a/tests/test_unit_tools.py
+++ b/tests/test_unit_tools.py
@@ -10,14 +10,14 @@ from cekit.errors import CekitError
 from cekit import tools
 
 
-class TestDescriptor(Descriptor):
+class MockedDescriptor(Descriptor):
     def __init__(self, descriptor):
         self.schemas = [yaml.safe_load("""type: any""")]
-        super(TestDescriptor, self).__init__(descriptor)
+        super(MockedDescriptor, self).__init__(descriptor)
 
         for key, val in descriptor.items():
             if isinstance(val, dict):
-                self._descriptor[key] = TestDescriptor(val)
+                self._descriptor[key] = MockedDescriptor(val)
 
 
 def test_merging_description_image():
@@ -51,15 +51,15 @@ def test_merging_description_override():
 
 
 def test_merging_plain_descriptors():
-    desc1 = TestDescriptor({'name': 'foo',
+    desc1 = MockedDescriptor({'name': 'foo',
                             'a': 1,
                             'b': 2})
 
-    desc2 = TestDescriptor({'name': 'foo',
+    desc2 = MockedDescriptor({'name': 'foo',
                             'b': 5,
                             'c': 3})
 
-    expected = TestDescriptor({'name': 'foo',
+    expected = MockedDescriptor({'name': 'foo',
                                'a': 1,
                                'b': 2,
                                'c': 3})
@@ -68,17 +68,17 @@ def test_merging_plain_descriptors():
 
 
 def test_merging_emdedded_descriptors():
-    desc1 = TestDescriptor({'name': 'a',
+    desc1 = MockedDescriptor({'name': 'a',
                             'a': 1,
                             'b': {'name': 'b',
                                   'b1': 10,
                                   'b2': 20}})
-    desc2 = TestDescriptor({'b': {'name': 'b',
+    desc2 = MockedDescriptor({'b': {'name': 'b',
                                   'b2': 50,
                                   'b3': 30},
                             'c': {'name': 'c'}})
 
-    expected = TestDescriptor({'name': 'a',
+    expected = MockedDescriptor({'name': 'a',
                                'a': 1,
                                'b': {'name': 'b',
                                      'b1': 10,
@@ -104,19 +104,19 @@ def test_merging_plain_list_of_list():
 
 
 def test_merging_list_of_descriptors():
-    desc1 = [TestDescriptor({'name': 1,
+    desc1 = [MockedDescriptor({'name': 1,
                              'a': 1,
                              'b': 2})]
 
-    desc2 = [TestDescriptor({'name': 2,
+    desc2 = [MockedDescriptor({'name': 2,
                              'a': 123}),
-             TestDescriptor({'name': 1,
+             MockedDescriptor({'name': 1,
                              'b': 3,
                              'c': 3})]
 
-    expected = [TestDescriptor({'name': 2,
+    expected = [MockedDescriptor({'name': 2,
                                 'a': 123}),
-                TestDescriptor({'name': 1,
+                MockedDescriptor({'name': 1,
                                 'a': 1,
                                 'b': 2,
                                 'c': 3})]
diff --git a/tests/test_validate_cache.py b/tests/test_validate_cache.py
index 9b5e323..b7be767 100644
--- a/tests/test_validate_cache.py
+++ b/tests/test_validate_cache.py
@@ -1,15 +1,14 @@
 import os
 import re
 import pytest
-import sys
+
+from click.testing import CliRunner
 
 from cekit.cache.cli import cli
 from cekit.crypto import SUPPORTED_HASH_ALGORITHMS
 
-from click.testing import CliRunner
-
 
-def test_cekit_cache_ls(mocker, tmpdir):
+def test_cekit_cache_ls(tmpdir):
     work_dir = str(tmpdir.mkdir('work_dir'))
 
     result = run_cekit_cache(['-v',
@@ -20,7 +19,7 @@ def test_cekit_cache_ls(mocker, tmpdir):
     assert 'No artifacts cached!' in result.output
 
 
-def test_cekit_cache_rm_non_existing(mocker, tmpdir):
+def test_cekit_cache_rm_non_existing(tmpdir):
     work_dir = str(tmpdir.mkdir('work_dir'))
     run_cekit_cache(['-v',
                      '--work-dir',
@@ -29,7 +28,7 @@ def test_cekit_cache_rm_non_existing(mocker, tmpdir):
                      '12345'], 1)
 
 
-def test_cekit_cache_add_artifact(mocker, tmpdir):
+def test_cekit_cache_add_artifact(tmpdir):
     work_dir = str(tmpdir.mkdir('work_dir'))
 
     artifact = os.path.join(work_dir, 'artifact')
@@ -45,8 +44,21 @@ def test_cekit_cache_add_artifact(mocker, tmpdir):
 
     assert "cached with UUID" in result.output
 
+    # Artifact added to cache should have all supported checksums computed
+    result = run_cekit_cache(['-v',
+                              '--work-dir',
+                              work_dir,
+                              'ls'])
+
+    for alg in SUPPORTED_HASH_ALGORITHMS:
+        assert alg in result.output
+
+    assert "sha256: e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" in result.output
+    assert "sha1: da39a3ee5e6b4b0d3255bfef95601890afd80709" in result.output
+    assert "md5: d41d8cd98f00b204e9800998ecf8427e" in result.output
 
-def test_cekit_cache_add_artifact_existing(mocker, tmpdir):
+
+def test_cekit_cache_add_artifact_existing(tmpdir):
     work_dir = str(tmpdir.mkdir('work_dir'))
 
     artifact = os.path.join(work_dir, 'artifact')
@@ -66,7 +78,7 @@ def test_cekit_cache_add_artifact_existing(mocker, tmpdir):
     assert 'is already cached!' in result.output
 
 
-def test_cekit_cache_delete_artifact(mocker, tmpdir):
+def test_cekit_cache_delete_artifact(tmpdir):
     work_dir = str(tmpdir.mkdir('work_dir'))
     artifact = os.path.join(work_dir, 'artifact')
     open(artifact, 'a').close()
@@ -88,7 +100,7 @@ def test_cekit_cache_delete_artifact(mocker, tmpdir):
                      artifact_uuid])
 
 
-def test_cekit_cache_delete_not_existing_artifact(mocker, tmpdir):
+def test_cekit_cache_delete_not_existing_artifact(tmpdir):
     work_dir = str(tmpdir.mkdir('work_dir'))
 
     result = run_cekit_cache(['-v',
@@ -99,7 +111,7 @@ def test_cekit_cache_delete_not_existing_artifact(mocker, tmpdir):
     assert "Artifact with UUID 'foo' doesn't exists in the cache" in result.output
 
 
-def test_cekit_cannot_add_artifact_without_checksum(mocker, tmpdir):
+def test_cekit_cannot_add_artifact_without_checksum(tmpdir):
     work_dir = str(tmpdir.mkdir('work_dir'))
     artifact = os.path.join(work_dir, 'artifact')
     open(artifact, 'a').close()
@@ -113,7 +125,7 @@ def test_cekit_cannot_add_artifact_without_checksum(mocker, tmpdir):
 
 
 @pytest.mark.parametrize('algorithm', SUPPORTED_HASH_ALGORITHMS)
-def test_cekit_supported_algorithms(mocker, tmpdir, algorithm):
+def test_cekit_supported_algorithms(tmpdir, algorithm):
     """
     This is a bit counter intuitive, but we try to cache an artifact
     with all known supported hash algorithms. In case when the algorithm
@@ -125,20 +137,93 @@ def test_cekit_supported_algorithms(mocker, tmpdir, algorithm):
     artifact = os.path.join(work_dir, 'artifact')
     open(artifact, 'a').close()
 
-    result = run_cekit_cache(['-v',
-                              '--work-dir',
-                              work_dir,
-                              'add',
-                              artifact,
-                              "--{}".format(algorithm),
-                              'WRONG!'], 1)
+    result = run_cekit_cache(
+        [
+            '-v',
+            '--work-dir',
+            work_dir,
+            'add',
+            artifact,
+            "--{}".format(algorithm),
+            'WRONG!'
+        ],
+        1)
 
     assert "Cannot cache artifact {}: Artifact checksum verification failed!".format(
         artifact) in result.output
 
 
-def run_cekit_cache(args, rc=0):
-    result = CliRunner().invoke(cli, args, catch_exceptions=False)
-    assert result.exit_code == rc
+def test_cekit_cache_clear(mocker, tmpdir):
+    work_dir = str(tmpdir.mkdir('work_dir'))
+    mock_rmtree = mocker.patch('shutil.rmtree')
+
+    result = run_cekit_cache(
+        [
+            '-v',
+            '--work-dir',
+            work_dir,
+            'clear'],
+        0,
+        'y\n')
+
+    mock_rmtree.assert_called_once_with(os.path.join(work_dir, 'cache'))
+    assert "Artifact cache cleared!" in result.output
+
+
+def test_cekit_cache_clear_no_confirm(mocker, tmpdir):
+    work_dir = str(tmpdir.mkdir('work_dir'))
+    mock_rmtree = mocker.patch('shutil.rmtree')
+
+    result = run_cekit_cache(
+        [
+            '-v',
+            '--work-dir',
+            work_dir,
+            'clear'],
+        0,
+        'n\n')
+
+    mock_rmtree.assert_not_called()
+    assert "Artifact cache cleared!" not in result.output
+
+
+def test_cekit_cache_clear_no_confirm_by_default(mocker, tmpdir):
+    work_dir = str(tmpdir.mkdir('work_dir'))
+    mock_rmtree = mocker.patch('shutil.rmtree')
+
+    result = run_cekit_cache(
+        [
+            '-v',
+            '--work-dir',
+            work_dir,
+            'clear'],
+        0,
+        '\n')
+
+    mock_rmtree.assert_not_called()
+    assert "Artifact cache cleared!" not in result.output
+
+
+def test_cekit_cache_clear_with_error(mocker, tmpdir):
+    work_dir = str(tmpdir.mkdir('work_dir'))
+
+    mocker.patch('shutil.rmtree', side_effect=Exception)
+
+    result = run_cekit_cache(
+        [
+            '-v',
+            '--work-dir',
+            work_dir,
+            'clear'],
+        1,
+        'y\n')
+
+    assert "An error occurred while removing the artifact cache directory '{}'".format(
+        os.path.join(work_dir, 'cache')) in result.output
+
+
+def run_cekit_cache(args, return_code=0, i=None):
+    result = CliRunner().invoke(cli, args, input=i, catch_exceptions=False)
+    assert result.exit_code == return_code
 
     return result