From 8ddd9180967fae11803b59e92cab0e51faa99cf6 Mon Sep 17 00:00:00 2001
From: Ben Nemec <[email protected]>
Date: Tue, 31 Mar 2015 15:37:25 +0000
Subject: Fix logging of deprecated opts with dest override

When a deprecated opt is used and the new opt name has the dest
property overridden, we were logging the dest property instead of
the actual opt name in the deprecation warning.  This is confusing
because it leads to log messages such as:

Option "username" from group "nova" is deprecated. Use option
"username" from group "nova".

This is nonsense.  In this case the proper new name was "user-name"
but because it had a dest override of "username" that's what was
logged.

Because the deprecation check code does not have access to the
actual opt itself, we need to pass the undeprecated name down from
the opt into the namespace code that does the check.

I also tried simply adding a (group, name) tuple to the beginning
of the names list, but this subtly broke the CLI opt tests because
there are specific combinations of - and _ variants expected, and
this seems to violate those assumptions.  I did not pursue it any
further because I felt it was better to simply be explicit about
the current name and not mess around with adding non-existent opt
names to the lookup list solely for logging purposes.

Change-Id: Ib1ea8ae2d60a9c508935bad27d7ddc2cdb5ab505
Closes-Bug: #1438314
---
 oslo_config/cfg.py            | 14 +++++++++-----
 oslo_config/tests/test_cfg.py | 16 ++++++++++++++++
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/oslo_config/cfg.py b/oslo_config/cfg.py
index 77fd44d..f472139 100644
--- a/oslo_config/cfg.py
+++ b/oslo_config/cfg.py
@@ -717,6 +717,7 @@ class Opt(object):
         :param group_name: a group name
         """
         names = [(group_name, self.dest)]
+        current_name = (group_name, self.name)
 
         for opt in self.deprecated_opts:
             dname, dgroup = opt.name, opt.group
@@ -724,7 +725,8 @@ class Opt(object):
                 names.append((dgroup if dgroup else group_name,
                               dname if dname else self.dest))
 
-        value = namespace._get_value(names, self.multi, self.positional)
+        value = namespace._get_value(names, self.multi, self.positional,
+                                     current_name)
         # The previous line will raise a KeyError if no value is set in the
         # config file, so we'll only log deprecations for set options.
         if self.deprecated_for_removal and not self._logged_deprecation:
@@ -1469,7 +1471,7 @@ class MultiConfigParser(object):
     def get(self, names, multi=False):
         return self._get(names, multi=multi)
 
-    def _get(self, names, multi=False, normalized=False):
+    def _get(self, names, multi=False, normalized=False, current_name=None):
         """Fetch a config file value from the parsed files.
 
         :param names: a list of (section, name) tuples
@@ -1488,7 +1490,8 @@ class MultiConfigParser(object):
                 if section not in sections:
                     continue
                 if name in sections[section]:
-                    self._check_deprecated((section, name), names[0],
+                    current_name = current_name or names[0]
+                    self._check_deprecated((section, name), current_name,
                                            names[1:])
                     val = sections[section][name]
                     if multi:
@@ -1641,7 +1644,7 @@ class _Namespace(argparse.Namespace):
 
         raise KeyError
 
-    def _get_value(self, names, multi, positional):
+    def _get_value(self, names, multi, positional, current_name):
         """Fetch a value from config files.
 
         Multiple names for a given configuration option may be supplied so
@@ -1658,7 +1661,8 @@ class _Namespace(argparse.Namespace):
             pass
 
         names = [(g if g is not None else 'DEFAULT', n) for g, n in names]
-        values = self._parser._get(names, multi=multi, normalized=True)
+        values = self._parser._get(names, multi=multi, normalized=True,
+                                   current_name=current_name)
         return values if multi else values[-1]
 
 
diff --git a/oslo_config/tests/test_cfg.py b/oslo_config/tests/test_cfg.py
index 5518f53..e007494 100644
--- a/oslo_config/tests/test_cfg.py
+++ b/oslo_config/tests/test_cfg.py
@@ -3724,3 +3724,19 @@ class DeprecationWarningTests(DeprecationWarningTestBase):
                     'removal.  Its value may be silently ignored in the '
                     'future.\n')
         self.assertEqual(expected, self.log_fixture.output)
+
+    def test_deprecated_with_dest(self):
+        self.conf.register_group(cfg.OptGroup('other'))
+        self.conf.register_opt(cfg.StrOpt('foo-bar', deprecated_name='bar',
+                                          dest='foo'),
+                               group='other')
+        content = 'bar=baz'
+        paths = self.create_tempfiles([('test',
+                                        '[other]\n' +
+                                        content + '\n')])
+
+        self.conf(['--config-file', paths[0]])
+        self.assertEqual('baz', self.conf.other.foo)
+        expected = (self._parser_class._deprecated_opt_message %
+                    ('bar', 'other', 'foo-bar', 'other') + '\n')
+        self.assertEqual(expected, self.log_fixture.output)
-- 
cgit v0.11.2