From 2aacefd9cd41bc4b198232149cd55c376bf9d258 Mon Sep 17 00:00:00 2001 From: Qixiang Wan Date: Thu, 23 Mar 2017 09:18:15 -0600 Subject: [PATCH] checks.py: show warning message for alias option Show warning message for any alias option find in config instance. Example warning message: WARNING: Config option 'product_name' is deprecated and now an alias to 'release_name', please use 'release_name' instead. In: {'release_name': 'dummy product', 'product_name': 'dummy product'} Signed-off-by: Qixiang Wan --- pungi/checks.py | 86 +++++++++++++++++++++++++------------------- tests/test_checks.py | 15 +++++--- tests/test_config.py | 6 ++-- 3 files changed, 62 insertions(+), 45 deletions(-) diff --git a/pungi/checks.py b/pungi/checks.py index 8ff6b2d8..a9acfb39 100644 --- a/pungi/checks.py +++ b/pungi/checks.py @@ -35,6 +35,7 @@ When a new config option is added, the schema must be updated (see the ``CONFIG_DEPS`` mapping. """ +import contextlib import os.path import platform import jsonschema @@ -196,6 +197,10 @@ def validate(config): for error in validator.iter_errors(config): if isinstance(error, ConfigDeprecation): warnings.append(REMOVED.format('.'.join(error.path), error.message)) + elif isinstance(error, ConfigOptionWarning): + warnings.append(error.message) + elif isinstance(error, ConfigOptionError): + errors.append(error.message) elif not error.path and error.validator == 'additionalProperties': allowed_keys = set(error.schema['properties'].keys()) used_keys = set(error.instance.keys()) @@ -241,62 +246,61 @@ def _extend_with_default_and_alias(validator_class): validate_required = validator_class.VALIDATORS['required'] validate_additional_properties = validator_class.VALIDATORS['additionalProperties'] - def _replace_alias(properties, instance, schema): + @contextlib.contextmanager + def _hook_errors(properties, instance, schema): """ - If 'alias' is defined for a property, and the property is not present - in instance, add the property with value from the alias property to - instance before remove the alias property. If both the propery and its - alias are present, it will yield an error. + Hook the instance and yield errors and warnings. """ + errors = [] for property, subschema in properties.iteritems(): + # update instance for alias option + # If alias option for the property is present and property is not specified, + # update the property in instance with value from alias option. if "alias" in subschema: - if property in instance and subschema['alias'] in instance: - # the order of validators is in random, so we remove the alias - # property at the first time when it's found, then validators - # won't raise the same error later. - instance.pop(subschema['alias']) - yield jsonschema.ValidationError( - "%s is an alias of %s, only one can be used." % ( - subschema['alias'], property) - ) + if subschema['alias'] in instance: + msg = "WARNING: Config option '%s' is deprecated and now an alias to '%s', " \ + "please use '%s' instead. " \ + "In:\n%s" % (subschema['alias'], property, property, instance) + errors.append(ConfigOptionWarning(msg)) + if property in instance: + msg = "ERROR: Config option '%s' is an alias of '%s', only one can be used. In:\n%s" \ + % (subschema['alias'], property, instance) + errors.append(ConfigOptionError(msg)) + instance.pop(subschema['alias']) + else: + instance.setdefault(property, instance.pop(subschema['alias'])) + yield errors - if property not in instance and subschema['alias'] in instance: - instance.setdefault(property, instance.pop(subschema['alias'])) - - def set_defaults_and_aliases(validator, properties, instance, schema): + def _set_defaults(validator, properties, instance, schema): """ Assign default values to options that have them defined and are not - specified. And if a property has 'alias' defined and the property is - not specified, look for the alias property and copy alias property's - value to that property before remove the alias property. + specified. """ for property, subschema in properties.iteritems(): if "default" in subschema and property not in instance: instance.setdefault(property, subschema["default"]) - for error in _replace_alias(properties, instance, schema): - yield error + with _hook_errors(properties, instance, schema) as errors: + for error in errors: + yield error for error in validate_properties(validator, properties, instance, schema): yield error - def ignore_alias_properties(validator, aP, instance, schema): - """ - If there is a property has alias defined in schema, and the property is not - present in instance, set the property with the value of alias property, - remove alias property from instance. - """ + def _validate_additional_properties(validator, aP, instance, schema): properties = schema.get("properties", {}) - for error in _replace_alias(properties, instance, schema): - yield error + with _hook_errors(properties, instance, schema) as errors: + for error in errors: + yield error for error in validate_additional_properties(validator, aP, instance, schema): yield error - def validate_required_with_alias(validator, required, instance, schema): + def _validate_required(validator, required, instance, schema): properties = schema.get("properties", {}) - for error in _replace_alias(properties, instance, schema): - yield error + with _hook_errors(properties, instance, schema) as errors: + for error in errors: + yield error for error in validate_required(validator, required, instance, schema): yield error @@ -323,11 +327,11 @@ def _extend_with_default_and_alias(validator_class): yield error return jsonschema.validators.extend( - validator_class, {"properties": set_defaults_and_aliases, + validator_class, {"properties": _set_defaults, "deprecated": error_on_deprecated, "type": validate_regex_type, - "required": validate_required_with_alias, - "additionalProperties": ignore_alias_properties}, + "required": _validate_required, + "additionalProperties": _validate_additional_properties}, ) @@ -335,6 +339,14 @@ class ConfigDeprecation(jsonschema.exceptions.ValidationError): pass +class ConfigOptionWarning(jsonschema.exceptions.ValidationError): + pass + + +class ConfigOptionError(jsonschema.exceptions.ValidationError): + pass + + def _make_schema(): return { "$schema": "http://json-schema.org/draft-04/schema#", diff --git a/tests/test_checks.py b/tests/test_checks.py index a8a55ae7..dd844a50 100644 --- a/tests/test_checks.py +++ b/tests/test_checks.py @@ -238,7 +238,8 @@ class TestSchemaValidator(unittest.TestCase): config = self._load_conf_from_string(string) errors, warnings = checks.validate(config) self.assertEqual(len(errors), 0) - self.assertEqual(len(warnings), 0) + self.assertEqual(len(warnings), 1) + self.assertRegexpMatches(warnings[0], r"^WARNING: Config option 'product_name' is deprecated and now an alias to 'release_name'.*") self.assertEqual(config.get("release_name", None), "dummy product") @mock.patch('pungi.checks._make_schema') @@ -285,7 +286,8 @@ class TestSchemaValidator(unittest.TestCase): config = self._load_conf_from_string(string) errors, warnings = checks.validate(config) self.assertEqual(len(errors), 0) - self.assertEqual(len(warnings), 0) + self.assertEqual(len(warnings), 1) + self.assertRegexpMatches(warnings[0], r"^WARNING: Config option 'product_name' is deprecated and now an alias to 'release_name'.*") self.assertEqual(config.get("release_name", None), "dummy product") @mock.patch('pungi.checks._make_schema') @@ -309,8 +311,9 @@ class TestSchemaValidator(unittest.TestCase): config = self._load_conf_from_string(string) errors, warnings = checks.validate(config) self.assertEqual(len(errors), 1) - self.assertIn('Failed validation in : product_name is an alias of release_name, only one can be used.', errors) - self.assertEqual(len(warnings), 0) + self.assertRegexpMatches(errors[0], r"^ERROR: Config option 'product_name' is an alias of 'release_name', only one can be used.*") + self.assertEqual(len(warnings), 1) + self.assertRegexpMatches(warnings[0], r"^WARNING: Config option 'product_name' is deprecated and now an alias to 'release_name'.*") self.assertEqual(config.get("release_name", None), "dummy product") @mock.patch('pungi.checks._make_schema') @@ -348,7 +351,9 @@ class TestSchemaValidator(unittest.TestCase): config = self._load_conf_from_string(string) errors, warnings = checks.validate(config) self.assertEqual(len(errors), 0) - self.assertEqual(len(warnings), 0) + self.assertEqual(len(warnings), 2) + self.assertRegexpMatches(warnings[0], r"^WARNING: Config option '.+' is deprecated and now an alias to '.+'.*") + self.assertRegexpMatches(warnings[1], r"^WARNING: Config option '.+' is deprecated and now an alias to '.+'.*") self.assertEqual(config.get("release_name", None), "dummy product") self.assertEqual(config.get("foophase", {}).get("repo", None), "http://www.exampe.com/os") diff --git a/tests/test_config.py b/tests/test_config.py index 53dfafbe..ce6708a7 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -273,7 +273,7 @@ class OstreeConfigTestCase(ConfigTestCase): "x86_64": { "treefile": "fedora-atomic-docker-host.json", "config_url": "https://git.fedorahosted.org/git/fedora-atomic.git", - "source_repo_from": "Everything", + "repo_from": "Everything", "ostree_repo": "/mnt/koji/compose/atomic/Rawhide/" } }) @@ -298,7 +298,7 @@ class OstreeInstallerConfigTestCase(ConfigTestCase): ostree_installer=[ ("^Atomic$", { "x86_64": { - "source_repo_from": "Everything", + "repo_from": "Everything", "release": None, "installpkgs": ["fedora-productimg-atomic"], "add_template": ["/spin-kickstarts/atomic-installer/lorax-configure-repo.tmpl"], @@ -326,7 +326,7 @@ class OstreeInstallerConfigTestCase(ConfigTestCase): ostree_installer=[ ("^Atomic$", { "x86_64": { - "source_repo_from": "Everything", + "repo_from": "Everything", "release": None, "installpkgs": ["fedora-productimg-atomic"], "add_template": ["/spin-kickstarts/atomic-installer/lorax-configure-repo.tmpl"],