From 0c6040ef6b85a77abd277a9e63be614877ca7c6a Mon Sep 17 00:00:00 2001 From: Ben Nemec Date: Tue, 28 Feb 2017 00:02:16 +0000 Subject: [PATCH] Handle both - and _ forms of deprecated opts While config file opts cannot have a - in their name, CLI opts can and the current deprecation code doesn't handle that case properly. Additionally, because oslo.config has some magic to translate one to the other it is possible we have existing users relying on the wrong behavior. For this reason, let's add both forms of the opt when adding deprecated opts. This will allow both to work as expected. The config generator is updated to ignore the - form of deprecated opts since they won't work in a config file anyway. I'm unsure how to filter the extra opts out of the help text, but since both forms will work in that case it's a minor cosmetic issue. Change-Id: I4d2584b33d97fe00a427b992682d9eac391725a2 Closes-Bug: 1279973 --- oslo_config/cfg.py | 11 +++++-- oslo_config/generator.py | 8 +++-- oslo_config/tests/test_cfg.py | 45 ++++++++++++++++------------- oslo_config/tests/test_generator.py | 17 +++++++++++ 4 files changed, 57 insertions(+), 24 deletions(-) diff --git a/oslo_config/cfg.py b/oslo_config/cfg.py index 1047bf62..f17a0d0f 100644 --- a/oslo_config/cfg.py +++ b/oslo_config/cfg.py @@ -888,13 +888,20 @@ class Opt(object): self.deprecated_reason = deprecated_reason self.deprecated_since = deprecated_since self._logged_deprecation = False - if deprecated_name is not None: - deprecated_name = deprecated_name.replace('-', '_') self.deprecated_opts = copy.deepcopy(deprecated_opts) or [] + for o in self.deprecated_opts: + if '-' in o.name: + self.deprecated_opts.append(DeprecatedOpt( + o.name.replace('-', '_'), + group=o.group)) if deprecated_name is not None or deprecated_group is not None: self.deprecated_opts.append(DeprecatedOpt(deprecated_name, group=deprecated_group)) + if deprecated_name and '-' in deprecated_name: + self.deprecated_opts.append(DeprecatedOpt( + deprecated_name.replace('-', '_'), + group=deprecated_group)) self._check_default() self.mutable = mutable diff --git a/oslo_config/generator.py b/oslo_config/generator.py index 746faf62..3bfa7f80 100644 --- a/oslo_config/generator.py +++ b/oslo_config/generator.py @@ -270,8 +270,12 @@ class _OptFormatter(object): (opt.dest, err)) for d in opt.deprecated_opts: - lines.append('# Deprecated group/name - [%s]/%s\n' % - (d.group or group_name, d.name or opt.dest)) + # NOTE(bnemec): opt names with a - are not valid in a config file, + # but it is possible to add a DeprecatedOpt with a - name. We + # want to ignore those as they won't work anyway. + if d.name and '-' not in d.name: + lines.append('# Deprecated group/name - [%s]/%s\n' % + (d.group or group_name, d.name or opt.dest)) if opt.deprecated_for_removal: if opt.deprecated_since: diff --git a/oslo_config/tests/test_cfg.py b/oslo_config/tests/test_cfg.py index 6a46884a..fbc16a40 100644 --- a/oslo_config/tests/test_cfg.py +++ b/oslo_config/tests/test_cfg.py @@ -195,6 +195,20 @@ class HelpTestCase(BaseTestCase): list = [abc, ghi, zba] self.assertEqual(sorted(list), list) + def test_print_help_with_deprecated(self): + f = moves.StringIO() + abc = cfg.StrOpt('a-bc', + deprecated_opts=[cfg.DeprecatedOpt('d-ef')]) + uvw = cfg.StrOpt('u-vw', + deprecated_name='x-yz') + + self.conf.register_cli_opt(abc) + self.conf.register_cli_opt(uvw) + self.conf([]) + self.conf.print_help(file=f) + self.assertIn('--a-bc A_BC, --d-ef A_BC, --d_ef A_BC', f.getvalue()) + self.assertIn('--u-vw U_VW, --x-yz U_VW, --x_yz U_VW', f.getvalue()) + class FindConfigFilesTestCase(BaseTestCase): @@ -2380,8 +2394,12 @@ class MappingInterfaceTestCase(BaseTestCase): self.assertEqual(self.conf['blaa'], self.conf.blaa) -class OptNameSeparatorTestCast(BaseTestCase): +class OptNameSeparatorTestCase(BaseTestCase): + # NOTE(bnemec): The broken* values in these scenarios are opt dests or + # config file names that are not actually valid, but can be added via a + # DeprecatedOpt. The tests only verify that those values do not end up + # in the final config object. scenarios = [ ('hyphen', dict(opt_name='foo-bar', @@ -2390,8 +2408,7 @@ class OptNameSeparatorTestCast(BaseTestCase): cf_name='foo_bar', broken_cf_name='foo-bar', cli_name='foo-bar', - broken_cli_name='foo_bar', - broken=True)), # FIXME(markmc): see #1279973 + hyphen=True)), ('underscore', dict(opt_name='foo_bar', opt_dest='foo_bar', @@ -2399,8 +2416,7 @@ class OptNameSeparatorTestCast(BaseTestCase): cf_name='foo_bar', broken_cf_name='foo-bar', cli_name='foo_bar', - broken_cli_name='foo_bar', - broken=False)), + hyphen=False)), ] def test_attribute_and_key_name(self): @@ -2445,11 +2461,7 @@ class OptNameSeparatorTestCast(BaseTestCase): self.conf.register_cli_opt(cfg.BoolOpt('foobar', deprecated_name=self.opt_name)) - # FIXME(markmc): this should be self.cli_name, see #1279973 - if self.broken: - self.conf(['--' + self.broken_cli_name]) - else: - self.conf(['--' + self.cli_name]) + self.conf(['--' + self.cli_name]) self.assertTrue(self.conf.foobar) @@ -2491,16 +2503,9 @@ class OptNameSeparatorTestCast(BaseTestCase): self.conf.register_opt(cfg.BoolOpt('foobar', deprecated_opts=oldopts)) - # FIXME(markmc): this should be self.cf_name, see #1279973 - if self.broken: - paths = self.create_tempfiles([('test', - '[DEFAULT]\n' + - self.broken_cf_name + - ' = True\n')]) - else: - paths = self.create_tempfiles([('test', - '[DEFAULT]\n' + - self.cf_name + ' = True\n')]) + paths = self.create_tempfiles([('test', + '[DEFAULT]\n' + + self.cf_name + ' = True\n')]) self.conf(['--config-file', paths[0]]) diff --git a/oslo_config/tests/test_generator.py b/oslo_config/tests/test_generator.py index c9af936b..8896b54b 100644 --- a/oslo_config/tests/test_generator.py +++ b/oslo_config/tests/test_generator.py @@ -103,6 +103,11 @@ class GeneratorTestCase(base.BaseTestCase): 'deprecated_opt_with_deprecated_group': cfg.StrOpt( 'bar', deprecated_name='foobar', deprecated_group='group1', help='deprecated'), + 'opt_with_DeprecatedOpt': cfg.BoolOpt( + 'foo-bar', + help='Opt with DeprecatedOpt', + deprecated_opts = [cfg.DeprecatedOpt('foo-bar', + group='deprecated')]), # Unknown Opt default must be a string 'unknown_type': cfg.Opt('unknown_opt', default='123', @@ -838,6 +843,18 @@ class GeneratorTestCase(base.BaseTestCase): # a string (string value) #str_opt = foo bar +''')), + ('opt_with_DeprecatedOpt', + dict(opts=[('test', [(None, [opts['opt_with_DeprecatedOpt']])])], + expected='''[DEFAULT] + +# +# From test +# + +# Opt with DeprecatedOpt (boolean value) +# Deprecated group/name - [deprecated]/foo_bar +#foo_bar = ''')), ]