From 09182866a5194181c127fd359ff8ba4301907f02 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Oct 30 2017 09:00:11 +0000
Subject: [PATCH 1/7] consolidate safe_rmtree, rmtree and shutil.rmtree


shutil.rmtree should be avoided in almost all cases
safe_rmtree has its usage in tasks module, but innards are replaced with
koji.util.rmtree, so we don't have two implementations of same task

Related: https://pagure.io/koji/issue/648

---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index 69927c7..478c0e7 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -4970,7 +4970,7 @@ def recycle_build(old, data):
     update.execute()
     builddir = koji.pathinfo.build(data)
     if os.path.exists(builddir):
-        shutil.rmtree(builddir)
+        koji.util.rmtree(builddir)
     koji.plugin.run_callbacks('postBuildStateChange', attribute='state',
                 old=old['state'], new=data['state'], info=data)
 
diff --git a/koji/tasks.py b/koji/tasks.py
index 917ff87..88604c7 100644
--- a/koji/tasks.py
+++ b/koji/tasks.py
@@ -84,26 +84,16 @@ def safe_rmtree(path, unmount=False, strict=True):
     if not os.path.exists(path):
         logger.debug("No such path: %s" % path)
         return
-    #first rm -f non-directories
+
     logger.debug('Scrubbing files in %s' % path)
-    rv = os.system("find '%s' -xdev \\! -type d -print0 |xargs -0 rm -f" % path)
-    msg = 'file removal failed (code %r) for %s' % (rv, path)
-    if rv != 0:
-        logger.warn(msg)
-        if strict:
-            raise koji.GenericError(msg)
-        else:
-            return rv
-    #them rmdir directories
-    #with -depth, we start at the bottom and work up
-    logger.debug('Scrubbing directories in %s' % path)
-    rv = os.system("find '%s' -xdev -depth -type d -print0 |xargs -0 rmdir" % path)
-    msg = 'dir removal failed (code %r) for %s' % (rv, path)
-    if rv != 0:
-        logger.warn(msg)
+    try:
+        koji.util.rmtree(path)
+    except:
+        logger.warn('file removal failed for %s' % path)
         if strict:
-            raise koji.GenericError(msg)
-    return rv
+            raise
+        return 1
+
 
 class ServerExit(Exception):
     """Raised to shutdown the server"""
diff --git a/plugins/hub/rpm2maven.py b/plugins/hub/rpm2maven.py
index b1310ea..c216fdc 100644
--- a/plugins/hub/rpm2maven.py
+++ b/plugins/hub/rpm2maven.py
@@ -8,6 +8,7 @@
 import koji
 from koji.context import context
 from koji.plugin import callback
+from koji.util import rmtree
 import ConfigParser
 import fnmatch
 import os
@@ -42,13 +43,13 @@ def maven_import(cbtype, *args, **kws):
     tmpdir = os.path.join(koji.pathinfo.work(), 'rpm2maven', koji.buildLabel(buildinfo))
     try:
         if os.path.exists(tmpdir):
-            shutil.rmtree(tmpdir)
+            rmtree(tmpdir)
         koji.ensuredir(tmpdir)
         expand_rpm(filepath, tmpdir)
         scan_and_import(buildinfo, rpminfo, tmpdir)
     finally:
         if os.path.exists(tmpdir):
-            shutil.rmtree(tmpdir)
+            rmtree(tmpdir)
 
 def expand_rpm(filepath, tmpdir):
     devnull = file('/dev/null', 'r+')
diff --git a/tests/test_lib/test_utils.py b/tests/test_lib/test_utils.py
index 1ff221c..f18269f 100644
--- a/tests/test_lib/test_utils.py
+++ b/tests/test_lib/test_utils.py
@@ -1,7 +1,7 @@
 from __future__ import absolute_import
 import mock
 import unittest
-from mock import call
+from mock import call, patch
 
 import os
 import optparse
@@ -488,6 +488,195 @@ class MavenUtilTestCase(unittest.TestCase):
             config.readfp(conf_file)
         return config
 
+class TestRmtree(unittest.TestCase):
+    @patch('koji.util._rmtree')
+    @patch('os.rmdir')
+    @patch('os.chdir')
+    @patch('os.getcwd')
+    @patch('stat.S_ISDIR')
+    @patch('os.lstat')
+    def test_rmtree_file(self, lstat, isdir, getcwd, chdir, rmdir, _rmtree):
+        """ Tests that the koji.util.rmtree function raises error when the
+        path parameter is not a directory.
+        """
+        stat = mock.MagicMock()
+        stat.st_dev = 'dev'
+        lstat.return_value = stat
+        isdir.return_value = False
+        getcwd.return_value = 'cwd'
+
+        with self.assertRaises(koji.GenericError):
+            koji.util.rmtree('/mnt/folder/some_file')
+        _rmtree.assert_not_called()
+        rmdir.assert_not_called()
+
+    @patch('koji.util._rmtree')
+    @patch('os.rmdir')
+    @patch('os.chdir')
+    @patch('os.getcwd')
+    @patch('stat.S_ISDIR')
+    @patch('os.lstat')
+    def test_rmtree_directory(self, lstat, isdir, getcwd, chdir, rmdir, _rmtree):
+        """ Tests that the koji.util.rmtree function returns nothing when the path is a directory.
+        """
+        stat = mock.MagicMock()
+        stat.st_dev = 'dev'
+        lstat.return_value = stat
+        isdir.return_value = True
+        getcwd.return_value = 'cwd'
+        path = '/mnt/folder'
+
+        self.assertEquals(koji.util.rmtree(path), None)
+        chdir.assert_called_with('cwd')
+        _rmtree.assert_called_once_with('dev')
+        rmdir.assert_called_once_with(path)
+
+    @patch('koji.util._rmtree')
+    @patch('os.rmdir')
+    @patch('os.chdir')
+    @patch('os.getcwd')
+    @patch('stat.S_ISDIR')
+    @patch('os.lstat')
+    def test_rmtree_directory_scrub_failure(self, lstat, isdir, getcwd, chdir, rmdir, _rmtree):
+        """ Tests that the koji.util.rmtree function returns a GeneralException
+        when the scrub of the files in the directory fails.
+        """
+        stat = mock.MagicMock()
+        stat.st_dev = 'dev'
+        lstat.return_value = stat
+        isdir.return_value = True
+        getcwd.return_value = 'cwd'
+        path = '/mnt/folder'
+        _rmtree.side_effect = OSError('xyz')
+
+        with self.assertRaises(OSError):
+            koji.util.rmtree(path)
+
+    @patch('os.chdir')
+    @patch('os.rmdir')
+    @patch('koji.util._stripcwd')
+    def test_rmtree_internal_empty(self, stripcwd, rmdir, chdir):
+        dev = 'dev'
+        stripcwd.return_value = []
+
+        koji.util._rmtree(dev)
+
+        stripcwd.assert_called_once_with(dev)
+        rmdir.assert_not_called()
+        chdir.assert_not_called()
+
+    @patch('os.chdir')
+    @patch('os.rmdir')
+    @patch('koji.util._stripcwd')
+    def test_rmtree_internal_dirs(self, stripcwd, rmdir, chdir):
+        dev = 'dev'
+        stripcwd.side_effect = (['a', 'b'], [], [])
+
+        koji.util._rmtree(dev)
+
+        stripcwd.assert_has_calls([call(dev), call(dev), call(dev)])
+        rmdir.assert_has_calls([call('b'), call('a')])
+        chdir.assert_has_calls([call('b'), call('..'), call('a'), call('..')])
+
+    @patch('os.chdir')
+    @patch('os.rmdir')
+    @patch('koji.util._stripcwd')
+    def test_rmtree_internal_fail(self, stripcwd, rmdir, chdir):
+        dev = 'dev'
+        stripcwd.side_effect = (['a', 'b'], [], [])
+        rmdir.side_effect = OSError()
+
+        # don't fail on anything
+        koji.util._rmtree(dev)
+
+        stripcwd.assert_has_calls([call(dev), call(dev), call(dev)])
+        rmdir.assert_has_calls([call('b'), call('a')])
+        chdir.assert_has_calls([call('b'), call('..'), call('a'), call('..')])
+
+
+    @patch('os.listdir')
+    @patch('os.lstat')
+    @patch('stat.S_ISDIR')
+    @patch('os.unlink')
+    def test_stripcwd_empty(dev, unlink, isdir, lstat, listdir):
+        # simple empty directory
+        dev = 'dev'
+        listdir.return_value = []
+
+        koji.util._stripcwd(dev)
+
+        listdir.assert_called_once_with('.')
+        unlink.assert_not_called()
+        isdir.assert_not_called()
+        lstat.assert_not_called()
+
+    @patch('os.listdir')
+    @patch('os.lstat')
+    @patch('stat.S_ISDIR')
+    @patch('os.unlink')
+    def test_stripcwd_all(dev, unlink, isdir, lstat, listdir):
+        # test valid file + dir
+        dev = 'dev'
+        listdir.return_value = ['a', 'b']
+        st = mock.MagicMock()
+        st.st_dev = dev
+        st.st_mode = 'mode'
+        lstat.return_value = st
+        isdir.side_effect = [True, False]
+
+        koji.util._stripcwd(dev)
+
+        listdir.assert_called_once_with('.')
+        unlink.assert_called_once_with('b')
+        isdir.assert_has_calls([call('mode'), call('mode')])
+        lstat.assert_has_calls([call('a'), call('b')])
+
+    @patch('os.listdir')
+    @patch('os.lstat')
+    @patch('stat.S_ISDIR')
+    @patch('os.unlink')
+    def test_stripcwd_diffdev(dev, unlink, isdir, lstat, listdir):
+        # ignore files on different devices
+        dev = 'dev'
+        listdir.return_value = ['a', 'b']
+        st1 = mock.MagicMock()
+        st1.st_dev = dev
+        st1.st_mode = 'mode'
+        st2 = mock.MagicMock()
+        st2.st_dev = 'other_dev'
+        st2.st_mode = 'mode'
+        lstat.side_effect = [st1, st2]
+        isdir.side_effect = [True, False]
+
+        koji.util._stripcwd(dev)
+
+        listdir.assert_called_once_with('.')
+        unlink.assert_not_called()
+        isdir.assert_called_once_with('mode')
+        lstat.assert_has_calls([call('a'), call('b')])
+
+    @patch('os.listdir')
+    @patch('os.lstat')
+    @patch('stat.S_ISDIR')
+    @patch('os.unlink')
+    def test_stripcwd_fails(dev, unlink, isdir, lstat, listdir):
+        # ignore all unlink errors
+        dev = 'dev'
+        listdir.return_value = ['a', 'b']
+        st = mock.MagicMock()
+        st.st_dev = dev
+        st.st_mode = 'mode'
+        lstat.return_value = st
+        isdir.side_effect = [True, False]
+        unlink.side_effect = OSError()
+
+        koji.util._stripcwd(dev)
+
+        listdir.assert_called_once_with('.')
+        unlink.assert_called_once_with('b')
+        isdir.assert_has_calls([call('mode'), call('mode')])
+        lstat.assert_has_calls([call('a'), call('b')])
+
 
 if __name__ == '__main__':
     unittest.main()
diff --git a/tests/test_lib_py2only/test_tasks.py b/tests/test_lib_py2only/test_tasks.py
index 6cbc298..3eb1334 100644
--- a/tests/test_lib_py2only/test_tasks.py
+++ b/tests/test_lib_py2only/test_tasks.py
@@ -128,66 +128,6 @@ class TasksTestCase(TestCase):
         except koji.GenericError as e:
             self.assertEquals(e.args[0], 'Unmounting incomplete: [\'/dev/shm\', \'/dev/mqueue\']')
 
-    @patch('os.path.isfile', return_value=True)
-    @patch('os.remove')
-    def test_safe_rmtree_file(self, mock_remove, mock_isfile):
-        """ Tests that the safe_rmtree function returns nothing when the path parameter is a file.
-        """
-        self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), None)
-
-    @patch('os.path.isfile', return_value=False)
-    @patch('os.path.islink', return_value=True)
-    @patch('os.remove')
-    def test_safe_rmtree_link(self, mock_remove, mock_islink, mock_isfile):
-        """ Tests that the safe_rmtree function returns nothing when the path parameter is a link.
-        """
-        self.assertEquals(safe_rmtree('/mnt/folder/some_link', False, True), None)
-
-    @patch('os.path.isfile', return_value=False)
-    @patch('os.path.islink', return_value=False)
-    @patch('os.path.exists', return_value=False)
-    def test_safe_rmtree_does_not_exist(self, mock_exists, mock_islink, mock_isfile):
-        """ Tests that the safe_rmtree function returns nothing if the path does not exist.
-        """
-        self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), None)
-
-    @patch('os.path.isfile', return_value=False)
-    @patch('os.path.islink', return_value=False)
-    @patch('os.path.exists', return_value=True)
-    @patch('os.system', side_effect=[0, 0])
-    def test_safe_rmtree_directory(self, mock_os_system, mock_exists, mock_islink, mock_isfile):
-        """ Tests that the safe_rmtree function returns nothing when the path is a directory.
-        """
-        self.assertEquals(safe_rmtree('/mnt/folder', False, True), 0)
-
-    @patch('os.path.isfile', return_value=False)
-    @patch('os.path.islink', return_value=False)
-    @patch('os.path.exists', return_value=True)
-    @patch('os.system', side_effect=[1, 0])
-    def test_safe_rmtree_directory_scrub_file_failure(self, mock_os_system, mock_exists, mock_islink, mock_isfile):
-        """ Tests that the safe_rmtree function returns a GeneralException when the path parameter is a directory
-        and the scrub of the files in the directory fails.
-        """
-        try:
-            safe_rmtree('/mnt/folder', False, True)
-            raise Exception('A GenericError was not raised during the test')
-        except koji.GenericError as e:
-            self.assertEquals(e.args[0], 'file removal failed (code 1) for /mnt/folder')
-
-    @patch('os.path.isfile', return_value=False)
-    @patch('os.path.islink', return_value=False)
-    @patch('os.path.exists', return_value=True)
-    @patch('os.system', side_effect=[0, 1])
-    def test_safe_rmtree_directory_scrub_directory_failure(self, mock_os_system, mock_exists, mock_islink, mock_isfile):
-        """ Tests that the safe_rmtree function returns a GeneralException when the path parameter is a directory
-        and the scrub of the directories in the directory fails.
-        """
-        try:
-            safe_rmtree('/mnt/folder', False, True)
-            raise Exception('A GenericError was not raised during the test')
-        except koji.GenericError as e:
-            self.assertEquals(e.args[0], 'dir removal failed (code 1) for /mnt/folder')
-
     def test_BaseTaskHandler_handler_not_set(self):
         """ Tests that an exception is thrown when the handler function is not overwritten by the child class.
         """
@@ -759,3 +699,57 @@ class TasksTestCase(TestCase):
         # getTaskResult should be called in 2nd round only for task 3, as 4
         # will be skipped as 'canfail'
         obj.session.getTaskResult.assert_has_calls([call(3)])
+
+    @patch('koji.util.rmtree')
+    def test_safe_rmtree_file(self, mock_rmtree):
+        """ Tests that the koji.util.rmtree function returns nothing when the path parameter is a file.
+        """
+        self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), None)
+
+    @patch('koji.util.rmtree')
+    def test_rmtree_link(self, rmtree):
+        """ Tests that the koji.util.rmtree function returns nothing when the path parameter is a link.
+        """
+        self.assertEquals(safe_rmtree('/mnt/folder/some_link', False, True), None)
+
+    @patch('koji.util.rmtree')
+    def test_rmtree_does_not_exist(self, rmtree):
+        """ Tests that the koji.util.rmtree function returns nothing if the path does not exist.
+        """
+        self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), None)
+
+    @patch('koji.util.rmtree')
+    def test_rmtree_directory(self, rmtree):
+        """ Tests that the koji.util.rmtree function returns nothing when the path is a directory.
+        """
+        self.assertEquals(safe_rmtree('/mnt/folder', False, True), None)
+
+    @patch('koji.util.rmtree')
+    @patch('os.path.isfile', return_value=False)
+    @patch('os.path.islink', return_value=False)
+    @patch('os.path.exists', return_value=True)
+    def test_rmtree_directory_scrub_file_failure(self, exists, islink, isfile, rmtree):
+        """ Tests that the koji.util.rmtree function returns a GeneralException when the path parameter is a directory
+        and the scrub of the files in the directory fails.
+        """
+        rmtree.side_effect = koji.GenericError('xyz')
+        try:
+            safe_rmtree('/mnt/folder', False, True)
+            raise Exception('A GenericError was not raised during the test')
+        except koji.GenericError as e:
+            self.assertEquals(e.args[0], 'xyz')
+
+    @patch('koji.util.rmtree')
+    @patch('os.path.isfile', return_value=False)
+    @patch('os.path.islink', return_value=False)
+    @patch('os.path.exists', return_value=True)
+    def test_safe_rmtree_directory_scrub_directory_failure(self, exists, islink, isfile, rmtree):
+        """ Tests that the koji.util.rmtree function returns a GeneralException when the path parameter is a directory
+        and the scrub of the directories in the directory fails.
+        """
+        rmtree.side_effect = OSError('xyz')
+        try:
+            safe_rmtree('/mnt/folder', False, True)
+            raise Exception('An OSError was not raised during the test')
+        except OSError as e:
+            self.assertEquals(e.args[0], 'xyz')

From c39a0723ba49dcece55c7f3fbfdd160fdd8fda58 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Oct 30 2017 09:00:11 +0000
Subject: [PATCH 2/7] update rmtree patch in unit test for recycle_build


---

diff --git a/tests/test_hub/test_recycle_build.py b/tests/test_hub/test_recycle_build.py
index 2179b04..119814f 100644
--- a/tests/test_hub/test_recycle_build.py
+++ b/tests/test_hub/test_recycle_build.py
@@ -16,7 +16,7 @@ class TestRecycleBuild():
                                 side_effect=self.getUpdate).start()
         self._dml = mock.patch('kojihub._dml').start()
         self.run_callbacks = mock.patch('koji.plugin.run_callbacks').start()
-        self.rmtree = mock.patch('shutil.rmtree').start()
+        self.rmtree = mock.patch('koji.util.rmtree').start()
         self.exists = mock.patch('os.path.exists').start()
         self.updates = []
 

From a4461da798ee2a5ce17e80ac17b62777823f4520 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Oct 30 2017 09:00:11 +0000
Subject: [PATCH 3/7] drop stale comment and avoid bare except in safe_rmtree


---

diff --git a/koji/tasks.py b/koji/tasks.py
index 88604c7..e5824b1 100644
--- a/koji/tasks.py
+++ b/koji/tasks.py
@@ -67,8 +67,6 @@ def umount_all(topdir):
 
 def safe_rmtree(path, unmount=False, strict=True):
     logger = logging.getLogger("koji.build")
-    #safe remove: with -xdev the find cmd will not cross filesystems
-    #             (though it will cross bind mounts from the same filesystem)
     if unmount:
         umount_all(path)
     if os.path.isfile(path) or os.path.islink(path):
@@ -88,7 +86,7 @@ def safe_rmtree(path, unmount=False, strict=True):
     logger.debug('Scrubbing files in %s' % path)
     try:
         koji.util.rmtree(path)
-    except:
+    except Exception:
         logger.warn('file removal failed for %s' % path)
         if strict:
             raise

From d3bb59cfcbb0afe4b0cc67ad6606bdc829ff1e2a Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Oct 30 2017 09:00:11 +0000
Subject: [PATCH 4/7] safe_rmtree: make return value consistent with past behavior


---

diff --git a/koji/tasks.py b/koji/tasks.py
index e5824b1..b8ad060 100644
--- a/koji/tasks.py
+++ b/koji/tasks.py
@@ -78,10 +78,11 @@ def safe_rmtree(path, unmount=False, strict=True):
                 raise
             else:
                 logger.warn("Error removing: %s", exc_info=True)
-        return
+                return 1
+        return 0
     if not os.path.exists(path):
         logger.debug("No such path: %s" % path)
-        return
+        return 0
 
     logger.debug('Scrubbing files in %s' % path)
     try:
@@ -91,6 +92,7 @@ def safe_rmtree(path, unmount=False, strict=True):
         if strict:
             raise
         return 1
+    return 0
 
 
 class ServerExit(Exception):

From bfe5a00cad030535a4d85569bb2b95a5606822a1 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Oct 30 2017 09:00:11 +0000
Subject: [PATCH 5/7] safe_rmtree: fix tests for backward-compatibility


---

diff --git a/tests/test_lib_py2only/test_tasks.py b/tests/test_lib_py2only/test_tasks.py
index 3eb1334..b8ea382 100644
--- a/tests/test_lib_py2only/test_tasks.py
+++ b/tests/test_lib_py2only/test_tasks.py
@@ -1,7 +1,7 @@
 from __future__ import absolute_import
 import random
+import shutil
 from os import path, makedirs
-from shutil import rmtree
 from tempfile import gettempdir
 from unittest import TestCase
 from mock import patch, MagicMock, Mock, call
@@ -74,7 +74,7 @@ class TasksTestCase(TestCase):
         temp_dir_root = get_temp_dir_root()
 
         if path.isdir(temp_dir_root):
-            rmtree(get_temp_dir_root())
+            shutil.rmtree(get_temp_dir_root())
 
     def test_scan_mounts_results(self):
         """ Tests the scan_mounts function with a mocked /proc/mounts file. A list containing mount points
@@ -170,7 +170,7 @@ class TasksTestCase(TestCase):
         obj = TestTask(123, 'some_method', ['random_arg'], None, None, temp_path)
         obj.createWorkdir()
         self.assertEquals(path.isdir(temp_path), True)
-        rmtree(get_temp_dir_root())
+        shutil.rmtree(get_temp_dir_root())
 
     def test_BaseTaskHandler_removeWorkdir(self):
         """ Tests that the removeWOrkdir function deletes a folder based on the path given to the
@@ -704,25 +704,25 @@ class TasksTestCase(TestCase):
     def test_safe_rmtree_file(self, mock_rmtree):
         """ Tests that the koji.util.rmtree function returns nothing when the path parameter is a file.
         """
-        self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), None)
+        self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), 0)
 
     @patch('koji.util.rmtree')
     def test_rmtree_link(self, rmtree):
         """ Tests that the koji.util.rmtree function returns nothing when the path parameter is a link.
         """
-        self.assertEquals(safe_rmtree('/mnt/folder/some_link', False, True), None)
+        self.assertEquals(safe_rmtree('/mnt/folder/some_link', False, True), 0)
 
     @patch('koji.util.rmtree')
     def test_rmtree_does_not_exist(self, rmtree):
         """ Tests that the koji.util.rmtree function returns nothing if the path does not exist.
         """
-        self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), None)
+        self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), 0)
 
     @patch('koji.util.rmtree')
     def test_rmtree_directory(self, rmtree):
         """ Tests that the koji.util.rmtree function returns nothing when the path is a directory.
         """
-        self.assertEquals(safe_rmtree('/mnt/folder', False, True), None)
+        self.assertEquals(safe_rmtree('/mnt/folder', False, True), 0)
 
     @patch('koji.util.rmtree')
     @patch('os.path.isfile', return_value=False)
@@ -734,7 +734,7 @@ class TasksTestCase(TestCase):
         """
         rmtree.side_effect = koji.GenericError('xyz')
         try:
-            safe_rmtree('/mnt/folder', False, True)
+            safe_rmtree('/mnt/folder', False, 1)
             raise Exception('A GenericError was not raised during the test')
         except koji.GenericError as e:
             self.assertEquals(e.args[0], 'xyz')

From 3974b6380534214704bed7b328264683b78ca5a9 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Oct 30 2017 09:00:11 +0000
Subject: [PATCH 6/7] safe_rmtree: more constraints on tests


---

diff --git a/tests/test_lib_py2only/test_tasks.py b/tests/test_lib_py2only/test_tasks.py
index b8ea382..7b2d0f2 100644
--- a/tests/test_lib_py2only/test_tasks.py
+++ b/tests/test_lib_py2only/test_tasks.py
@@ -700,56 +700,113 @@ class TasksTestCase(TestCase):
         # will be skipped as 'canfail'
         obj.session.getTaskResult.assert_has_calls([call(3)])
 
+class TestSafeRmtree(TestCase):
+    @patch('os.path.exists', return_value=True)
+    @patch('os.path.isfile', return_value=True)
+    @patch('os.path.islink', return_value=False)
+    @patch('os.remove')
     @patch('koji.util.rmtree')
-    def test_safe_rmtree_file(self, mock_rmtree):
+    def test_safe_rmtree_file(self, rmtree, remove, islink, isfile, exists):
         """ Tests that the koji.util.rmtree function returns nothing when the path parameter is a file.
         """
-        self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), 0)
+        path = '/mnt/folder/some_file'
+        self.assertEquals(safe_rmtree(path, False, True), 0)
+        isfile.assert_called_once_with(path)
+        islink.assert_not_called()
+        exists.assert_not_called()
+        remove.assert_called_once_with(path)
+        rmtree.assert_not_called()
 
+    @patch('os.path.exists', return_value=True)
+    @patch('os.path.isfile', return_value=False)
+    @patch('os.path.islink', return_value=True)
+    @patch('os.remove')
     @patch('koji.util.rmtree')
-    def test_rmtree_link(self, rmtree):
+    def test_rmtree_link(self, rmtree, remove, islink, isfile, exists):
         """ Tests that the koji.util.rmtree function returns nothing when the path parameter is a link.
         """
-        self.assertEquals(safe_rmtree('/mnt/folder/some_link', False, True), 0)
+        path = '/mnt/folder/some_link'
+        self.assertEquals(safe_rmtree(path, False, True), 0)
+        isfile.assert_called_once_with(path)
+        islink.assert_called_once_with(path)
+        exists.assert_not_called()
+        remove.assert_called_once_with(path)
+        rmtree.assert_not_called()
+
 
+    @patch('os.path.exists', return_value=False)
+    @patch('os.path.isfile', return_value=False)
+    @patch('os.path.islink', return_value=False)
+    @patch('os.remove')
     @patch('koji.util.rmtree')
-    def test_rmtree_does_not_exist(self, rmtree):
+    def test_rmtree_does_not_exist(self, rmtree, remove, islink, isfile, exists):
         """ Tests that the koji.util.rmtree function returns nothing if the path does not exist.
         """
-        self.assertEquals(safe_rmtree('/mnt/folder/some_file', False, True), 0)
+        path = '/mnt/folder/some_file'
+        self.assertEquals(safe_rmtree(path, False, True), 0)
+        isfile.assert_called_once_with(path)
+        islink.assert_called_once_with(path)
+        exists.assert_called_once_with(path)
+        remove.assert_not_called()
+        rmtree.assert_not_called()
 
+    @patch('os.path.exists', return_value=True)
+    @patch('os.path.isfile', return_value=False)
+    @patch('os.path.islink', return_value=False)
+    @patch('os.remove')
     @patch('koji.util.rmtree')
-    def test_rmtree_directory(self, rmtree):
+    def test_rmtree_directory(self, rmtree, remove, islink, isfile, exists):
         """ Tests that the koji.util.rmtree function returns nothing when the path is a directory.
         """
-        self.assertEquals(safe_rmtree('/mnt/folder', False, True), 0)
+        path = '/mnt/folder'
+        self.assertEquals(safe_rmtree(path, False, True), 0)
+        isfile.assert_called_once_with(path)
+        islink.assert_called_once_with(path)
+        exists.assert_called_once_with(path)
+        remove.assert_not_called()
+        rmtree.assert_called_once_with(path)
 
-    @patch('koji.util.rmtree')
+    @patch('os.path.exists', return_value=True)
     @patch('os.path.isfile', return_value=False)
     @patch('os.path.islink', return_value=False)
-    @patch('os.path.exists', return_value=True)
-    def test_rmtree_directory_scrub_file_failure(self, exists, islink, isfile, rmtree):
+    @patch('os.remove')
+    @patch('koji.util.rmtree')
+    def test_rmtree_directory_scrub_file_failure(self, rmtree, remove, islink, isfile, exists):
         """ Tests that the koji.util.rmtree function returns a GeneralException when the path parameter is a directory
         and the scrub of the files in the directory fails.
         """
         rmtree.side_effect = koji.GenericError('xyz')
+        path = '/mnt/folder'
         try:
-            safe_rmtree('/mnt/folder', False, 1)
+            safe_rmtree(path, False, 1)
             raise Exception('A GenericError was not raised during the test')
         except koji.GenericError as e:
             self.assertEquals(e.args[0], 'xyz')
+        isfile.assert_called_once_with(path)
+        islink.assert_called_once_with(path)
+        exists.assert_called_once_with(path)
+        remove.assert_not_called()
+        rmtree.assert_called_once_with(path)
 
-    @patch('koji.util.rmtree')
+    @patch('os.path.exists', return_value=True)
     @patch('os.path.isfile', return_value=False)
     @patch('os.path.islink', return_value=False)
-    @patch('os.path.exists', return_value=True)
-    def test_safe_rmtree_directory_scrub_directory_failure(self, exists, islink, isfile, rmtree):
+    @patch('os.remove')
+    @patch('koji.util.rmtree')
+    def test_safe_rmtree_directory_scrub_directory_failure(self, rmtree, remove, islink, isfile, exists):
         """ Tests that the koji.util.rmtree function returns a GeneralException when the path parameter is a directory
         and the scrub of the directories in the directory fails.
         """
         rmtree.side_effect = OSError('xyz')
+        path = '/mnt/folder'
         try:
-            safe_rmtree('/mnt/folder', False, True)
+            safe_rmtree(path, False, True)
             raise Exception('An OSError was not raised during the test')
         except OSError as e:
             self.assertEquals(e.args[0], 'xyz')
+
+        isfile.assert_called_once_with(path)
+        islink.assert_called_once_with(path)
+        exists.assert_called_once_with(path)
+        remove.assert_not_called()
+        rmtree.assert_called_once_with(path)

From 04d8ad56387ceb33dee30269045f1712b95af1c5 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Oct 30 2017 09:00:11 +0000
Subject: [PATCH 7/7] also use rmtree in delete_build/reset_build


---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index 478c0e7..cbad388 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -7119,19 +7119,10 @@ def _delete_build(binfo):
     update.execute()
     update = """UPDATE build SET state=%(st_deleted)i WHERE id=%(build_id)i"""
     _dml(update, locals())
-    #now clear the build dirs
-    dirs_to_clear = []
+    # now clear the build dir
     builddir = koji.pathinfo.build(binfo)
     if os.path.exists(builddir):
-        dirs_to_clear.append(builddir)
-    for filedir in dirs_to_clear:
-        rv = os.system(r"find '%s' -xdev \! -type d -print0 |xargs -0 rm -f" % filedir)
-        if rv != 0:
-            raise koji.GenericError('file removal failed (code %r) for %s' % (rv, filedir))
-        #and clear out the emptied dirs
-        rv = os.system(r"find '%s' -xdev -depth -type d -print0 |xargs -0 rmdir" % filedir)
-        if rv != 0:
-            raise koji.GenericError('directory removal failed (code %r) for %s' % (rv, filedir))
+        koji.util.rmtree(builddir)
     koji.plugin.run_callbacks('postBuildStateChange', attribute='state', old=binfo['state'], new=st_deleted, info=binfo)
 
 def reset_build(build):
@@ -7195,19 +7186,10 @@ def reset_build(build):
     binfo['state'] = koji.BUILD_STATES['CANCELED']
     update = """UPDATE build SET state=%(state)s, task_id=NULL, volume_id=0 WHERE id=%(id)s"""
     _dml(update, binfo)
-    #now clear the build dirs
-    dirs_to_clear = []
+    # now clear the build dir
     builddir = koji.pathinfo.build(binfo)
     if os.path.exists(builddir):
-        dirs_to_clear.append(builddir)
-    for filedir in dirs_to_clear:
-        rv = os.system(r"find '%s' -xdev \! -type d -print0 |xargs -0 rm -f" % filedir)
-        if rv != 0:
-            raise koji.GenericError('file removal failed (code %r) for %s' % (rv, filedir))
-        #and clear out the emptied dirs
-        rv = os.system(r"find '%s' -xdev -depth -type d -print0 |xargs -0 rmdir" % filedir)
-        if rv != 0:
-            raise koji.GenericError('directory removal failed (code %r) for %s' % (rv, filedir))
+        koji.util.rmtree(builddir)
     koji.plugin.run_callbacks('postBuildStateChange', attribute='state', old=binfo['state'], new=koji.BUILD_STATES['CANCELED'], info=binfo)
 
 def cancel_build(build_id, cancel_task=True):

