From 788224cfe074a55dfe4ffcf5f77f05bd5c32a8b5 Mon Sep 17 00:00:00 2001
From: Clark Boylan <clark.boylan@gmail.com>
Date: Thu, 20 Aug 2020 08:37:47 -0700
Subject: [PATCH] Don't remove packages that are requested to be installed

Recently the source-repositories element was updated [0] to set git as a
build-only dep. This is fine if you don't request to install git
elsewhere as a regular package install [1]. If you mix the two then git
gets uninstalled when you expect it to be installed.

Address this by checking if a package is already requested to be
installed when we find a removal or build-only request. Similarly remove
a package from the uninstall list if we ask for it to be installed
normally. This means that explicit installs override any cleanup
actions.

[0] https://review.opendev.org/#/c/745678/1/diskimage_builder/elements/source-repositories/package-installs.yaml
[1] https://opendev.org/openstack/project-config/src/branch/master/nodepool/elements/infra-package-needs/package-installs.yaml#L22

Change-Id: Idc1aa86f10cddcd4549066d8ea1d6df6fd906bac
---
 .../bin/package-installs-squash               | 27 ++++++--
 .../tests/test_package_squash.py              | 67 +++++++++++++++++++
 2 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/diskimage_builder/elements/package-installs/bin/package-installs-squash b/diskimage_builder/elements/package-installs/bin/package-installs-squash
index f5074366..ee39e406 100755
--- a/diskimage_builder/elements/package-installs/bin/package-installs-squash
+++ b/diskimage_builder/elements/package-installs/bin/package-installs-squash
@@ -128,10 +128,29 @@ def collect_data(data, objs, element_name):
                          (element_name, pkg_name, param))
             phase = param.get('phase', 'install.d')
             installs = ["install"]
-            if 'uninstall' in param:
-                installs = ["uninstall"]
-            if 'build-only' in param:
-                installs = ["install", "uninstall"]
+            if 'uninstall' in param or 'build-only' in param:
+                # We don't add the package to the uninstall list if
+                # something else has requested we install it without
+                # removing it.
+                in_install = any(
+                    map(lambda x: x[0] == pkg_name, data[phase]["install"]))
+                not_in_uninstall = all(
+                    map(lambda x: x[0] != pkg_name, data[phase]["uninstall"]))
+                if in_install and not_in_uninstall:
+                    if 'build-only' not in param:
+                        # Just skip further processing as we have no uninstall
+                        # work to do
+                        continue
+                else:
+                    if 'uninstall' in param:
+                        installs = ["uninstall"]
+                    if 'build-only' in param:
+                        installs = ["install", "uninstall"]
+            else:
+                # Remove any uninstallations if we are trying to install
+                # the package without uninstallation elsewhere.
+                data[phase]["uninstall"] = [
+                    x for x in data[phase]["uninstall"] if x[0] != pkg_name]
 
             # Filter out incorrect installtypes
             installtype = param.get('installtype', None)
diff --git a/diskimage_builder/elements/package-installs/tests/test_package_squash.py b/diskimage_builder/elements/package-installs/tests/test_package_squash.py
index df39ea24..631c987f 100644
--- a/diskimage_builder/elements/package-installs/tests/test_package_squash.py
+++ b/diskimage_builder/elements/package-installs/tests/test_package_squash.py
@@ -221,3 +221,70 @@ class TestPackageInstall(base.BaseTestCase):
         }
 
         self.assertThat(result, IsMatchingInstallList(expected))
+
+    def test_install_overrides_uninstall_install_first(self):
+        '''Test an install overrides uninstall'''
+        objs = {
+            'test_package': ''
+        }
+
+        result = installs_squash.collect_data(
+            self.final_dict, objs, 'test_element1')
+
+        expected = {
+            'install.d': {
+                'install': [('test_package', 'test_element1')]
+            }
+        }
+
+        self.assertThat(result, IsMatchingInstallList(expected))
+
+        objs = {
+            'test_package': {'build-only': 'true'}
+        }
+
+        result = installs_squash.collect_data(
+            self.final_dict, objs, 'test_element2')
+
+        expected = {
+            'install.d': {
+                'install': [('test_package', 'test_element1'),
+                            ('test_package', 'test_element2')]
+            }
+        }
+
+        self.assertThat(result, IsMatchingInstallList(expected))
+
+    def test_install_overrides_uninstall_uninstall_first(self):
+        '''Test an install overrides uninstall'''
+        objs = {
+            'test_package': {'build-only': 'true'}
+        }
+
+        result = installs_squash.collect_data(
+            self.final_dict, objs, 'test_element1')
+
+        expected = {
+            'install.d': {
+                'install': [('test_package', 'test_element1')],
+                'uninstall': [('test_package', 'test_element1')]
+            }
+        }
+
+        self.assertThat(result, IsMatchingInstallList(expected))
+
+        objs = {
+            'test_package': ''
+        }
+
+        result = installs_squash.collect_data(
+            self.final_dict, objs, 'test_element2')
+
+        expected = {
+            'install.d': {
+                'install': [('test_package', 'test_element1'),
+                            ('test_package', 'test_element2')]
+            }
+        }
+
+        self.assertThat(result, IsMatchingInstallList(expected))