Gate-unbreaking combo review

Fix argument precedence hack
  Working around issues in os-client-config <= 1.18.0

  This is ugly because the issues in o-c-c 1.19.1 run even deeper
  than in 1.18.0, so we're going to use 1.19.0 get_one_cloud() that
  is known to work for OSC and fix o-c-c with an axe.

Remove return values for set commands
  'identity provider set' and 'service provider set' were still
  returning their show-like data, this is a fail for set commands
  now, don't know how this ever passed before...

Constraints are ready to be used for tox.ini
  Per email[1] from Andreas, we don't need to hack at install_command
  any longer.

  [1] http://openstack.markmail.org/thread/a4l7tokbotwqvuoh

Co-authorioed-by: Steve Martinelli <s.martinelli@gmail.com>
Depends-On: I49313dc7d4f44ec897de7a375f25b7ed864226f1
Change-Id: I426548376fc7d3cdb36501310dafd8c44d22ae30

6 files changed, 232 insertions, 86 deletions

diff --git a/openstackclient/common/client_config.py b/openstackclient/common/client_config.py
new file mode 100644
index 00000000..cc6cad58
--- /dev/null
+++ b/openstackclient/common/client_config.py
@@ -0,0 +1,186 @@
+#   Licensed under the Apache License, Version 2.0 (the "License"); you may
+#   not use this file except in compliance with the License. You may obtain
+#   a copy of the License at
+#
+#        http://www.apache.org/licenses/LICENSE-2.0
+#
+#   Unless required by applicable law or agreed to in writing, software
+#   distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#   WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#   License for the specific language governing permissions and limitations
+#   under the License.
+#
+
+"""OpenStackConfig subclass for argument compatibility"""
+
+import logging
+
+from os_client_config.config import OpenStackConfig
+
+
+LOG = logging.getLogger(__name__)
+
+
+# Sublcass OpenStackConfig in order to munge config values
+# before auth plugins are loaded
+class OSC_Config(OpenStackConfig):
+
+    def _auth_select_default_plugin(self, config):
+        """Select a default plugin based on supplied arguments
+
+        Migrated from auth.select_auth_plugin()
+        """
+
+        identity_version = config.get('identity_api_version', '')
+
+        if config.get('username', None) and not config.get('auth_type', None):
+            if identity_version == '3':
+                config['auth_type'] = 'v3password'
+            elif identity_version.startswith('2'):
+                config['auth_type'] = 'v2password'
+            else:
+                # let keystoneauth figure it out itself
+                config['auth_type'] = 'password'
+        elif config.get('token', None) and not config.get('auth_type', None):
+            if identity_version == '3':
+                config['auth_type'] = 'v3token'
+            elif identity_version.startswith('2'):
+                config['auth_type'] = 'v2token'
+            else:
+                # let keystoneauth figure it out itself
+                config['auth_type'] = 'token'
+        else:
+            # The ultimate default is similar to the original behaviour,
+            # but this time with version discovery
+            if not config.get('auth_type', None):
+                config['auth_type'] = 'password'
+
+        LOG.debug("Auth plugin %s selected" % config['auth_type'])
+        return config
+
+    def _auth_v2_arguments(self, config):
+        """Set up v2-required arguments from v3 info
+
+        Migrated from auth.build_auth_params()
+        """
+
+        if ('auth_type' in config and config['auth_type'].startswith("v2")):
+            if 'project_id' in config['auth']:
+                config['auth']['tenant_id'] = config['auth']['project_id']
+            if 'project_name' in config['auth']:
+                config['auth']['tenant_name'] = config['auth']['project_name']
+        return config
+
+    def _auth_v2_ignore_v3(self, config):
+        """Remove v3 arguemnts if present for v2 plugin
+
+        Migrated from clientmanager.setup_auth()
+        """
+
+        # NOTE(hieulq): If USER_DOMAIN_NAME, USER_DOMAIN_ID, PROJECT_DOMAIN_ID
+        # or PROJECT_DOMAIN_NAME is present and API_VERSION is 2.0, then
+        # ignore all domain related configs.
+        if (config.get('identity_api_version', '').startswith('2') and
+                config.get('auth_type', None).endswith('password')):
+            domain_props = [
+                'project_domain_id',
+                'project_domain_name',
+                'user_domain_id',
+                'user_domain_name',
+            ]
+            for prop in domain_props:
+                if config['auth'].pop(prop, None) is not None:
+                    LOG.warning("Ignoring domain related config " +
+                                prop + " because identity API version is 2.0")
+        return config
+
+    def _auth_default_domain(self, config):
+        """Set a default domain from available arguments
+
+        Migrated from clientmanager.setup_auth()
+        """
+
+        identity_version = config.get('identity_api_version', '')
+        auth_type = config.get('auth_type', None)
+
+        # TODO(mordred): This is a usability improvement that's broadly useful
+        # We should port it back up into os-client-config.
+        default_domain = config.get('default_domain', None)
+        if (identity_version == '3' and
+                not auth_type.startswith('v2') and
+                default_domain):
+
+            # NOTE(stevemar): If PROJECT_DOMAIN_ID or PROJECT_DOMAIN_NAME is
+            # present, then do not change the behaviour. Otherwise, set the
+            # PROJECT_DOMAIN_ID to 'OS_DEFAULT_DOMAIN' for better usability.
+            if (
+                    not config['auth'].get('project_domain_id') and
+                    not config['auth'].get('project_domain_name')
+            ):
+                config['auth']['project_domain_id'] = default_domain
+
+            # NOTE(stevemar): If USER_DOMAIN_ID or USER_DOMAIN_NAME is present,
+            # then do not change the behaviour. Otherwise, set the
+            # USER_DOMAIN_ID to 'OS_DEFAULT_DOMAIN' for better usability.
+            # NOTE(aloga): this should only be set if there is a username.
+            # TODO(dtroyer): Move this to os-client-config after the plugin has
+            # been loaded so we can check directly if the options are accepted.
+            if (
+                    auth_type in ("password", "v3password", "v3totp") and
+                    not config['auth'].get('user_domain_id') and
+                    not config['auth'].get('user_domain_name')
+            ):
+                config['auth']['user_domain_id'] = default_domain
+        return config
+
+    def auth_config_hook(self, config):
+        """Allow examination of config values before loading auth plugin
+
+        OpenStackClient will override this to perform additional chacks
+        on auth_type.
+        """
+
+        config = self._auth_select_default_plugin(config)
+        config = self._auth_v2_arguments(config)
+        config = self._auth_v2_ignore_v3(config)
+        config = self._auth_default_domain(config)
+
+        LOG.debug("auth_config_hook(): %s" % config)
+        return config
+
+    def _validate_auth_ksc(self, config, cloud):
+        """Old compatibility hack for OSC, no longer needed/wanted"""
+        return config
+
+    def _validate_auth(self, config, loader):
+        """Validate auth plugin arguments"""
+        # May throw a keystoneauth1.exceptions.NoMatchingPlugin
+
+        plugin_options = loader.get_options()
+
+        for p_opt in plugin_options:
+            # if it's in config, win, move it and kill it from config dict
+            # if it's in config.auth but not in config we're good
+            # deprecated loses to current
+            # provided beats default, deprecated or not
+            winning_value = self._find_winning_auth_value(p_opt, config)
+            if not winning_value:
+                winning_value = self._find_winning_auth_value(
+                    p_opt, config['auth'])
+
+            # Clean up after ourselves
+            for opt in [p_opt.name] + [o.name for o in p_opt.deprecated]:
+                opt = opt.replace('-', '_')
+                config.pop(opt, None)
+                config['auth'].pop(opt, None)
+
+            if winning_value:
+                # Prefer the plugin configuration dest value if the value's key
+                # is marked as depreciated.
+                if p_opt.dest is None:
+                    config['auth'][p_opt.name.replace('-', '_')] = (
+                        winning_value)
+                else:
+                    config['auth'][p_opt.dest] = winning_value
+
+        return config
diff --git a/openstackclient/identity/v3/identity_provider.py b/openstackclient/identity/v3/identity_provider.py
index 0453e888..b6b03188 100644
--- a/openstackclient/identity/v3/identity_provider.py
+++ b/openstackclient/identity/v3/identity_provider.py
@@ -204,11 +204,10 @@ class SetIdentityProvider(command.Command):
         if parsed_args.remote_id_file or parsed_args.remote_id:
             kwargs['remote_ids'] = remote_ids
 
-        identity_provider = federation_client.identity_providers.update(
-            parsed_args.identity_provider, **kwargs)
-
-        identity_provider._info.pop('links', None)
-        return zip(*sorted(six.iteritems(identity_provider._info)))
+        federation_client.identity_providers.update(
+            parsed_args.identity_provider,
+            **kwargs
+        )
 
 
 class ShowIdentityProvider(command.ShowOne):
diff --git a/openstackclient/identity/v3/service_provider.py b/openstackclient/identity/v3/service_provider.py
index 440eba40..8548ae1f 100644
--- a/openstackclient/identity/v3/service_provider.py
+++ b/openstackclient/identity/v3/service_provider.py
@@ -182,12 +182,13 @@ class SetServiceProvider(command.Command):
         elif parsed_args.disable is True:
             enabled = False
 
-        service_provider = federation_client.service_providers.update(
-            parsed_args.service_provider, enabled=enabled,
+        federation_client.service_providers.update(
+            parsed_args.service_provider,
+            enabled=enabled,
             description=parsed_args.description,
             auth_url=parsed_args.auth_url,
-            sp_url=parsed_args.service_provider_url)
-        return zip(*sorted(six.iteritems(service_provider._info)))
+            sp_url=parsed_args.service_provider_url,
+        )
 
 
 class ShowServiceProvider(command.ShowOne):
diff --git a/openstackclient/shell.py b/openstackclient/shell.py
index 7a042e1e..67c51998 100644
--- a/openstackclient/shell.py
+++ b/openstackclient/shell.py
@@ -25,6 +25,7 @@ from oslo_utils import importutils
 import six
 
 import openstackclient
+from openstackclient.common import client_config as cloud_config
 from openstackclient.common import clientmanager
 from openstackclient.common import commandmanager
 
@@ -127,9 +128,33 @@ class OpenStackShell(shell.OpenStackShell):
     def initialize_app(self, argv):
         super(OpenStackShell, self).initialize_app(argv)
 
-        # For now we need to build our own ClientManager so re-do what
-        # has already been done :(
-        # TODO(dtroyer): remove when osc-lib is fixed
+        # Argument precedence is really broken in multiple places
+        # so we're just going to fix it here until o-c-c and osc-lib
+        # get sorted out.
+        # TODO(dtroyer): remove when os-client-config and osc-lib are fixed
+
+        # First, throw away what has already been done with o-c-c and
+        # use our own.
+        try:
+            cc = cloud_config.OSC_Config(
+                override_defaults={
+                    'interface': None,
+                    'auth_type': self._auth_type,
+                },
+            )
+        except (IOError, OSError) as e:
+            self.log.critical("Could not read clouds.yaml configuration file")
+            self.print_help_if_requested()
+            raise e
+
+        if not self.options.debug:
+            self.options.debug = None
+        self.cloud = cc.get_one_cloud(
+            cloud=self.options.cloud,
+            argparse=self.options,
+        )
+
+        # Then, re-create the client_manager with the correct arguments
         self.client_manager = clientmanager.ClientManager(
             cli_options=self.cloud,
             api_version=self.api_version,
diff --git a/openstackclient/tests/identity/v3/test_identity_provider.py b/openstackclient/tests/identity/v3/test_identity_provider.py
index b5d784ef..d86ac11e 100644
--- a/openstackclient/tests/identity/v3/test_identity_provider.py
+++ b/openstackclient/tests/identity/v3/test_identity_provider.py
@@ -356,19 +356,11 @@ class TestIdentityProviderSet(TestIdentityProvider):
             ('remote_id', None)
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        columns, data = self.cmd.take_action(parsed_args)
+        self.cmd.take_action(parsed_args)
         self.identity_providers_mock.update.assert_called_with(
             identity_fakes.idp_id,
             description=new_description,
         )
-        self.assertEqual(self.columns, columns)
-        datalist = (
-            identity_fakes.idp_description,
-            False,
-            identity_fakes.idp_id,
-            identity_fakes.idp_remote_ids
-        )
-        self.assertEqual(datalist, data)
 
     def test_identity_provider_disable(self):
         """Disable Identity Provider
@@ -402,22 +394,13 @@ class TestIdentityProviderSet(TestIdentityProvider):
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
-        columns, data = self.cmd.take_action(parsed_args)
+        self.cmd.take_action(parsed_args)
         self.identity_providers_mock.update.assert_called_with(
             identity_fakes.idp_id,
             enabled=False,
             remote_ids=identity_fakes.idp_remote_ids
         )
 
-        self.assertEqual(self.columns, columns)
-        datalist = (
-            identity_fakes.idp_description,
-            False,
-            identity_fakes.idp_id,
-            identity_fakes.idp_remote_ids
-        )
-        self.assertEqual(datalist, data)
-
     def test_identity_provider_enable(self):
         """Enable Identity Provider.
 
@@ -448,12 +431,10 @@ class TestIdentityProviderSet(TestIdentityProvider):
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
-        columns, data = self.cmd.take_action(parsed_args)
+        self.cmd.take_action(parsed_args)
         self.identity_providers_mock.update.assert_called_with(
             identity_fakes.idp_id, enabled=True,
             remote_ids=identity_fakes.idp_remote_ids)
-        self.assertEqual(self.columns, columns)
-        self.assertEqual(self.datalist, data)
 
     def test_identity_provider_replace_remote_ids(self):
         """Enable Identity Provider.
@@ -488,18 +469,10 @@ class TestIdentityProviderSet(TestIdentityProvider):
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
-        columns, data = self.cmd.take_action(parsed_args)
+        self.cmd.take_action(parsed_args)
         self.identity_providers_mock.update.assert_called_with(
             identity_fakes.idp_id, enabled=True,
             remote_ids=[self.new_remote_id])
-        self.assertEqual(self.columns, columns)
-        datalist = (
-            identity_fakes.idp_description,
-            True,
-            identity_fakes.idp_id,
-            [self.new_remote_id]
-        )
-        self.assertEqual(datalist, data)
 
     def test_identity_provider_replace_remote_ids_file(self):
         """Enable Identity Provider.
@@ -538,18 +511,10 @@ class TestIdentityProviderSet(TestIdentityProvider):
         mocker.return_value = self.new_remote_id
         with mock.patch("openstackclient.identity.v3.identity_provider."
                         "utils.read_blob_file_contents", mocker):
-            columns, data = self.cmd.take_action(parsed_args)
+            self.cmd.take_action(parsed_args)
         self.identity_providers_mock.update.assert_called_with(
             identity_fakes.idp_id, enabled=True,
             remote_ids=[self.new_remote_id])
-        self.assertEqual(self.columns, columns)
-        datalist = (
-            identity_fakes.idp_description,
-            True,
-            identity_fakes.idp_id,
-            [self.new_remote_id]
-        )
-        self.assertEqual(datalist, data)
 
     def test_identity_provider_no_options(self):
         def prepare(self):
@@ -580,12 +545,7 @@ class TestIdentityProviderSet(TestIdentityProvider):
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
-        columns, data = self.cmd.take_action(parsed_args)
-
-        # expect take_action() to return (None, None) as
-        # neither --enable nor --disable was specified
-        self.assertEqual(self.columns, columns)
-        self.assertEqual(self.datalist, data)
+        self.cmd.take_action(parsed_args)
 
 
 class TestIdentityProviderShow(TestIdentityProvider):
diff --git a/openstackclient/tests/identity/v3/test_service_provider.py b/openstackclient/tests/identity/v3/test_service_provider.py
index f5270d83..873ab1e7 100644
--- a/openstackclient/tests/identity/v3/test_service_provider.py
+++ b/openstackclient/tests/identity/v3/test_service_provider.py
@@ -289,7 +289,7 @@ class TestServiceProviderSet(TestServiceProvider):
             ('disable', True),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
-        columns, data = self.cmd.take_action(parsed_args)
+        self.cmd.take_action(parsed_args)
         self.service_providers_mock.update.assert_called_with(
             service_fakes.sp_id,
             enabled=False,
@@ -298,9 +298,6 @@ class TestServiceProviderSet(TestServiceProvider):
             sp_url=None
         )
 
-        self.assertEqual(self.columns, columns)
-        self.assertEqual(self.datalist, data)
-
     def test_service_provider_enable(self):
         """Enable Service Provider.
 
@@ -327,19 +324,10 @@ class TestServiceProviderSet(TestServiceProvider):
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
-        columns, data = self.cmd.take_action(parsed_args)
+        self.cmd.take_action(parsed_args)
         self.service_providers_mock.update.assert_called_with(
             service_fakes.sp_id, enabled=True, description=None,
             auth_url=None, sp_url=None)
-        self.assertEqual(self.columns, columns)
-        datalist = (
-            service_fakes.sp_auth_url,
-            service_fakes.sp_description,
-            True,
-            service_fakes.sp_id,
-            service_fakes.service_provider_url
-        )
-        self.assertEqual(datalist, data)
 
     def test_service_provider_no_options(self):
         def prepare(self):
@@ -372,20 +360,7 @@ class TestServiceProviderSet(TestServiceProvider):
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
-        columns, data = self.cmd.take_action(parsed_args)
-
-        # expect take_action() to return (None, None) as none of --disabled,
-        # --enabled, --description, --service-provider-url, --auth_url option
-        # was set.
-        self.assertEqual(self.columns, columns)
-        datalist = (
-            service_fakes.sp_auth_url,
-            service_fakes.sp_description,
-            True,
-            service_fakes.sp_id,
-            service_fakes.service_provider_url
-        )
-        self.assertEqual(datalist, data)
+        self.cmd.take_action(parsed_args)
 
 
 class TestServiceProviderShow(TestServiceProvider):