From 2e3e66723a93409968f5aa76802e11231f0c5c7f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mihai=20Dinc=C4=83?= <dincamihai@users.noreply.github.com>
Date: Mon, 23 May 2016 23:15:29 +0200
Subject: [PATCH 53/53] * Fix pkgrepo.managed gpgkey argument (bsc#979448)

* Call zypper refresh after adding/modifying a repository

* Calling `zypper --gpg-auto-import-keys refresh` is required after
adding/modifying a repository because `--gpg-auto-import-keys` doesn't
do anything when called with `zypper ar` or `zypper mr`.
Without calling `zypper --gpg-auto-import-keys refresh` here, calling
`zypper ref` after adding/removing would still ask for
accepting/rejecting the gpg key.

* Update test method names to pass pylint

* Reduce dicts and lists to one line where possible

* Reverse if conditions and rename variable

* Assert only gpgautoimport: True works

* Improve zypper_patcher_config looks

* DRY test
---
 salt/modules/zypper.py            |  27 +++--
 tests/unit/modules/zypper_test.py | 213 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 231 insertions(+), 9 deletions(-)

diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py
index 55ca7bf..371d758 100644
--- a/salt/modules/zypper.py
+++ b/salt/modules/zypper.py
@@ -754,6 +754,8 @@ def mod_repo(repo, **kwargs):
 
     # Modify added or existing repo according to the options
     cmd_opt = []
+    global_cmd_opt = []
+    call_refresh = False
 
     if 'enabled' in kwargs:
         cmd_opt.append(kwargs['enabled'] and '--enable' or '--disable')
@@ -767,23 +769,30 @@ def mod_repo(repo, **kwargs):
     if 'gpgcheck' in kwargs:
         cmd_opt.append(kwargs['gpgcheck'] and '--gpgcheck' or '--no-gpgcheck')
 
-    if kwargs.get('gpgautoimport') is True:
-        cmd_opt.append('--gpg-auto-import-keys')
-
     if 'priority' in kwargs:
         cmd_opt.append("--priority={0}".format(kwargs.get('priority', DEFAULT_PRIORITY)))
 
     if 'humanname' in kwargs:
         cmd_opt.append("--name='{0}'".format(kwargs.get('humanname')))
 
-    if cmd_opt:
-        cmd_opt.append(repo)
-        __zypper__.refreshable.xml.call('mr', *cmd_opt)
+    if kwargs.get('gpgautoimport') is True:
+        global_cmd_opt.append('--gpg-auto-import-keys')
+        call_refresh = True
 
-    # If repo nor added neither modified, error should be thrown
-    if not added and not cmd_opt:
+    if cmd_opt:
+        cmd_opt = global_cmd_opt + ['mr'] + cmd_opt + [repo]
+        __zypper__.refreshable.xml.call(*cmd_opt)
+
+    if call_refresh:
+        # when used with "zypper ar --refresh" or "zypper mr --refresh"
+        # --gpg-auto-import-keys is not doing anything
+        # so we need to specifically refresh here with --gpg-auto-import-keys
+        refresh_opts = global_cmd_opt + ['refresh'] + [repo]
+        __zypper__.xml.call(*refresh_opts)
+    elif not added and not cmd_opt:
         raise CommandExecutionError(
-            'Modification of the repository \'{0}\' was not specified.'.format(repo))
+            'Specified arguments did not result in modification of repo'
+        )
 
     return get_repo(repo)
 
diff --git a/tests/unit/modules/zypper_test.py b/tests/unit/modules/zypper_test.py
index 9ec2b83..c4f7597 100644
--- a/tests/unit/modules/zypper_test.py
+++ b/tests/unit/modules/zypper_test.py
@@ -9,7 +9,9 @@ from __future__ import absolute_import
 # Import Salt Testing Libs
 from salttesting import TestCase, skipIf
 from salttesting.mock import (
+    Mock,
     MagicMock,
+    call,
     patch,
     NO_MOCK,
     NO_MOCK_REASON
@@ -54,10 +56,26 @@ zypper.rpm = None
 
 @skipIf(NO_MOCK, NO_MOCK_REASON)
 class ZypperTestCase(TestCase):
+
     '''
     Test cases for salt.modules.zypper
     '''
 
+    def setUp(self):
+        self.new_repo_config = dict(
+            name='mock-repo-name',
+            url='http://repo.url/some/path'
+        )
+        side_effect = [
+            Mock(**{'sections.return_value': []}),
+            Mock(**{'sections.return_value': [self.new_repo_config['name']]})
+        ]
+        self.zypper_patcher_config = {
+            '_get_configured_repos': Mock(side_effect=side_effect),
+            '__zypper__': Mock(),
+            'get_repo': Mock()
+        }
+
     def test_list_upgrades(self):
         '''
         List package upgrades
@@ -438,6 +456,201 @@ class ZypperTestCase(TestCase):
             self.assertEqual(r_info['enabled'], alias == 'SLE12-SP1-x86_64-Update')
             self.assertEqual(r_info['autorefresh'], alias == 'SLE12-SP1-x86_64-Update')
 
+    def test_repo_add_nomod_noref(self):
+        '''
+        Test mod_repo adds the new repo and nothing else
+
+        :return:
+        '''
+        zypper_patcher = patch.multiple(
+            'salt.modules.zypper', **self.zypper_patcher_config)
+
+        url = self.new_repo_config['url']
+        name = self.new_repo_config['name']
+        with zypper_patcher:
+            zypper.mod_repo(name, **{'url': url})
+            self.assertEqual(
+                zypper.__zypper__.xml.call.call_args_list,
+                [call('ar', url, name)]
+            )
+            zypper.__zypper__.refreshable.xml.call.assert_not_called()
+
+    def test_repo_noadd_nomod_noref(self):
+        '''
+        Test mod_repo detects the repo already exists,
+        no modification was requested and no refresh requested either
+
+        :return:
+        '''
+        url = self.new_repo_config['url']
+        name = self.new_repo_config['name']
+        self.zypper_patcher_config['_get_configured_repos'] = Mock(
+            **{'return_value.sections.return_value': [name]}
+        )
+        zypper_patcher = patch.multiple(
+            'salt.modules.zypper', **self.zypper_patcher_config)
+
+        with zypper_patcher:
+            with self.assertRaisesRegexp(
+                Exception,
+                'Specified arguments did not result in modification of repo'
+            ):
+                zypper.mod_repo(name, **{'url': url})
+            with self.assertRaisesRegexp(
+                Exception,
+                'Specified arguments did not result in modification of repo'
+            ):
+                zypper.mod_repo(name, **{'url': url, 'gpgautoimport': 'a'})
+
+            zypper.__zypper__.xml.call.assert_not_called()
+            zypper.__zypper__.refreshable.xml.call.assert_not_called()
+
+    def test_repo_add_mod_noref(self):
+        '''
+        Test mod_repo adds the new repo and call modify to update autorefresh
+
+        :return:
+        '''
+        zypper_patcher = patch.multiple(
+            'salt.modules.zypper', **self.zypper_patcher_config)
+
+        url = self.new_repo_config['url']
+        name = self.new_repo_config['name']
+        with zypper_patcher:
+            zypper.mod_repo(name, **{'url': url, 'refresh': True})
+            self.assertEqual(
+                zypper.__zypper__.xml.call.call_args_list,
+                [call('ar', url, name)]
+            )
+            zypper.__zypper__.refreshable.xml.call.assert_called_once_with(
+                'mr', '--refresh', name
+            )
+
+    def test_repo_noadd_mod_noref(self):
+        '''
+        Test mod_repo detects the repository exists,
+        calls modify to update 'autorefresh' but does not call refresh
+
+        :return:
+        '''
+        url = self.new_repo_config['url']
+        name = self.new_repo_config['name']
+        self.zypper_patcher_config['_get_configured_repos'] = Mock(
+            **{'return_value.sections.return_value': [name]})
+        zypper_patcher = patch.multiple(
+            'salt.modules.zypper', **self.zypper_patcher_config)
+        with zypper_patcher:
+            zypper.mod_repo(name, **{'url': url, 'refresh': True})
+            zypper.__zypper__.xml.call.assert_not_called()
+            zypper.__zypper__.refreshable.xml.call.assert_called_once_with(
+                'mr', '--refresh', name
+            )
+
+    def test_repo_add_nomod_ref(self):
+        '''
+        Test mod_repo adds the new repo and refreshes the repo with
+            `zypper --gpg-auto-import-keys refresh <repo-name>`
+
+        :return:
+        '''
+        zypper_patcher = patch.multiple(
+            'salt.modules.zypper', **self.zypper_patcher_config)
+
+        url = self.new_repo_config['url']
+        name = self.new_repo_config['name']
+        with zypper_patcher:
+            zypper.mod_repo(name, **{'url': url, 'gpgautoimport': True})
+            self.assertEqual(
+                zypper.__zypper__.xml.call.call_args_list,
+                [
+                    call('ar', url, name),
+                    call('--gpg-auto-import-keys', 'refresh', name)
+                ]
+            )
+            zypper.__zypper__.refreshable.xml.call.assert_not_called()
+
+    def test_repo_noadd_nomod_ref(self):
+        '''
+        Test mod_repo detects the repo already exists,
+        has nothing to modify and refreshes the repo with
+            `zypper --gpg-auto-import-keys refresh <repo-name>`
+
+        :return:
+        '''
+        url = self.new_repo_config['url']
+        name = self.new_repo_config['name']
+        self.zypper_patcher_config['_get_configured_repos'] = Mock(
+            **{'return_value.sections.return_value': [name]}
+        )
+        zypper_patcher = patch.multiple(
+            'salt.modules.zypper', **self.zypper_patcher_config)
+
+        with zypper_patcher:
+            zypper.mod_repo(name, **{'url': url, 'gpgautoimport': True})
+            self.assertEqual(
+                zypper.__zypper__.xml.call.call_args_list,
+                [call('--gpg-auto-import-keys', 'refresh', name)]
+            )
+            zypper.__zypper__.refreshable.xml.call.assert_not_called()
+
+    def test_repo_add_mod_ref(self):
+        '''
+        Test mod_repo adds the new repo,
+        calls modify to update 'autorefresh' and refreshes the repo with
+            `zypper --gpg-auto-import-keys refresh <repo-name>`
+
+        :return:
+        '''
+        zypper_patcher = patch.multiple(
+            'salt.modules.zypper', **self.zypper_patcher_config)
+
+        url = self.new_repo_config['url']
+        name = self.new_repo_config['name']
+        with zypper_patcher:
+            zypper.mod_repo(
+                name,
+                **{'url': url, 'refresh': True, 'gpgautoimport': True}
+            )
+            self.assertEqual(
+                zypper.__zypper__.xml.call.call_args_list,
+                [
+                    call('ar', url, name),
+                    call('--gpg-auto-import-keys', 'refresh', name)
+                ]
+            )
+            zypper.__zypper__.refreshable.xml.call.assert_called_once_with(
+                '--gpg-auto-import-keys', 'mr', '--refresh', name
+            )
+
+    def test_repo_noadd_mod_ref(self):
+        '''
+        Test mod_repo detects the repo already exists,
+        calls modify to update 'autorefresh' and refreshes the repo with
+            `zypper --gpg-auto-import-keys refresh <repo-name>`
+
+        :return:
+        '''
+        url = self.new_repo_config['url']
+        name = self.new_repo_config['name']
+        self.zypper_patcher_config['_get_configured_repos'] = Mock(
+            **{'return_value.sections.return_value': [name]}
+        )
+        zypper_patcher = patch.multiple(
+            'salt.modules.zypper', **self.zypper_patcher_config)
+
+        with zypper_patcher:
+            zypper.mod_repo(
+                name,
+                **{'url': url, 'refresh': True, 'gpgautoimport': True}
+            )
+            self.assertEqual(
+                zypper.__zypper__.xml.call.call_args_list,
+                [call('--gpg-auto-import-keys', 'refresh', name)]
+            )
+            zypper.__zypper__.refreshable.xml.call.assert_called_once_with(
+                '--gpg-auto-import-keys', 'mr', '--refresh', name
+            )
+
 if __name__ == '__main__':
     from integration import run_tests
     run_tests(ZypperTestCase, needs_daemon=False)
-- 
2.8.3

