From 381a05cddbed6081941083d2e9f7504379b80f25 Mon Sep 17 00:00:00 2001 From: Kelvin Liu Date: Fri, 13 Sep 2019 13:41:51 +1000 Subject: [PATCH 1/4] Add 'deployment' field support in metadata.yaml --- charmtools/charms.py | 43 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/charmtools/charms.py b/charmtools/charms.py index 565bcaf4..04d919e4 100644 --- a/charmtools/charms.py +++ b/charmtools/charms.py @@ -17,7 +17,7 @@ from charmtools.linter import Linter from charmtools.utils import validate_display_name -KNOWN_METADATA_KEYS = [ +KNOWN_METADATA_KEYS = ( 'name', 'display-name', 'summary', @@ -39,16 +39,17 @@ 'terms', 'resources', 'devices', -] + 'deployment', +) -REQUIRED_METADATA_KEYS = [ +REQUIRED_METADATA_KEYS = ( 'name', 'summary', -] +) -KNOWN_RELATION_KEYS = ['interface', 'scope', 'limit', 'optional'] +KNOWN_RELATION_KEYS = ('interface', 'scope', 'limit', 'optional') -KNOWN_SCOPES = ['global', 'container'] +KNOWN_SCOPES = ('global', 'container') TEMPLATE_PATH = os.path.abspath(os.path.dirname(__file__)) @@ -331,6 +332,7 @@ def proof(self): validate_payloads(charm, lint, proof_extensions.get('payloads')) validate_terms(charm, lint) validate_resources(charm, lint, proof_extensions.get('resources')) + validate_deployment(charm, lint) if not os.path.exists(os.path.join(charm_path, 'icon.svg')): lint.info("No icon.svg file.") @@ -701,6 +703,35 @@ def validate_resources(charm, linter, proof_extensions=None): linter.err('resources.{}: {}'.format(k, v)) +def validate_deployment(charm, linter): + """Validate deployment in charm metadata. + + :param charm: dict of charm metadata parsed from metadata.yaml + :param linter: :class:`CharmLinter` object to which info/warning/error + messages will be written + + """ + if 'deployment' not in charm: + return + + deployment = charm['deployment'] + if not isinstance(deployment, dict): + linter.err('deployment: must be a dict of config') + + known_field_types = { + 'type': str, + 'service': str, + 'daemonset': bool, + 'min-version': str, + } + for k, v in deployment.items(): + field_type = known_field_types.get(k, None) + if field_type is None: + linter.err('deployment.{} is not supported'.format(k)) + elif not isinstance(v, field_type): + linter.err('deployment.{} must be {} but got {}'.format(k, field_type, type(v))) + + def validate_extra_bindings(charm, linter): """Validate extra-bindings in charm metadata. From ac52e8d2d46e247b1e7c470326cf7b5f23c8b892 Mon Sep 17 00:00:00 2001 From: Kelvin Liu Date: Fri, 13 Sep 2019 14:16:19 +1000 Subject: [PATCH 2/4] Add tests; --- charmtools/charms.py | 3 +- tests/test_charm_proof.py | 116 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 1 deletion(-) diff --git a/charmtools/charms.py b/charmtools/charms.py index 04d919e4..3a4f3414 100644 --- a/charmtools/charms.py +++ b/charmtools/charms.py @@ -717,6 +717,7 @@ def validate_deployment(charm, linter): deployment = charm['deployment'] if not isinstance(deployment, dict): linter.err('deployment: must be a dict of config') + return known_field_types = { 'type': str, @@ -729,7 +730,7 @@ def validate_deployment(charm, linter): if field_type is None: linter.err('deployment.{} is not supported'.format(k)) elif not isinstance(v, field_type): - linter.err('deployment.{} must be {} but got {}'.format(k, field_type, type(v))) + linter.err('deployment.{} must be {} but got {}'.format(k, field_type.__name__, type(v).__name__)) def validate_extra_bindings(charm, linter): diff --git a/tests/test_charm_proof.py b/tests/test_charm_proof.py index ec91be47..2040b99f 100644 --- a/tests/test_charm_proof.py +++ b/tests/test_charm_proof.py @@ -43,6 +43,7 @@ from charmtools.charms import validate_actions # noqa from charmtools.charms import validate_terms # noqa from charmtools.charms import validate_resources # noqa +from charmtools.charms import validate_deployment # noqa class TestCharmProof(TestCase): @@ -814,6 +815,121 @@ def test_storage_proof_extensions(self): self.assertEqual(linter.err.call_args_list, []) + +class DeploymentValidationTest(TestCase): + def test_deployment(self): + """Charm has valid deployment.""" + linter = Mock() + charm = { + 'deployment': { + 'type': 'stateful', + 'service': 'omit', + 'daemonset': True, + 'min-version': "1.15", + } + } + validate_deployment(charm, linter) + self.assertFalse(linter.err.called) + + def test_invalid_deployment(self): + """Charm has invalid deployment.""" + linter = Mock() + charm = { + 'deployment': [], + } + validate_deployment(charm, linter) + self.assertEqual(linter.err.call_count, 1) + linter.err.assert_has_calls([ + call('deployment: must be a dict of config'), + ], any_order=True) + + def test_deployment_unsupported_field(self): + """Charm has the invalid deployment field.""" + linter = Mock() + charm = { + 'deployment': { + 'type': 'stateful', + 'service': 'omit', + 'daemonset': True, + 'min-version': "1.15", + 'unknow-field': 'xxx', + } + } + validate_deployment(charm, linter) + self.assertEqual(linter.err.call_count, 1) + linter.err.assert_has_calls([ + call('deployment.unknow-field is not supported'), + ], any_order=True) + + def test_deployment_invalid_type(self): + """Charm has the invalid deployment type.""" + linter = Mock() + charm = { + 'deployment': { + 'type': True, + 'service': 'omit', + 'daemonset': True, + 'min-version': "1.15", + } + } + validate_deployment(charm, linter) + self.assertEqual(linter.err.call_count, 1) + linter.err.assert_has_calls([ + call("deployment.type must be str but got bool"), + ], any_order=True) + + def test_deployment_invalid_service(self): + """Charm has the invalid deployment service.""" + linter = Mock() + charm = { + 'deployment': { + 'type': 'stateful', + 'service': 1, + 'daemonset': True, + 'min-version': "1.15", + } + } + validate_deployment(charm, linter) + self.assertEqual(linter.err.call_count, 1) + linter.err.assert_has_calls([ + call("deployment.service must be str but got int"), + ], any_order=True) + + def test_deployment_invalid_daemonset(self): + """Charm has the invalid deployment daemonset.""" + linter = Mock() + charm = { + 'deployment': { + 'type': 'stateful', + 'service': 'omit', + 'daemonset': 'True', + 'min-version': "1.15", + } + } + validate_deployment(charm, linter) + self.assertEqual(linter.err.call_count, 1) + linter.err.assert_has_calls([ + call("deployment.daemonset must be bool but got str"), + ], any_order=True) + + def test_deployment_invalid_min_version(self): + """Charm has the invalid deployment min-version.""" + linter = Mock() + charm = { + 'deployment': { + 'type': 'stateful', + 'service': 'omit', + 'daemonset': True, + 'min-version': 1.15, + } + } + validate_deployment(charm, linter) + self.assertEqual(linter.err.call_count, 1) + linter.err.assert_has_calls([ + call("deployment.min-version must be str but got float"), + ], any_order=True) + + class ResourcesValidationTest(TestCase): def test_minimal_resources_config(self): """Charm has the minimum allowed resources configuration.""" From 3fad4175429dabfd96a19a2428c64c6485bf085f Mon Sep 17 00:00:00 2001 From: Kelvin Liu Date: Mon, 16 Sep 2019 21:39:09 +1000 Subject: [PATCH 3/4] Change to use colander.MappingSchema to do validation on deployment; --- charmtools/charms.py | 66 +++++++++++++++++++++++++++++---------- tests/test_charm_proof.py | 24 +++++++------- 2 files changed, 61 insertions(+), 29 deletions(-) diff --git a/charmtools/charms.py b/charmtools/charms.py index 3a4f3414..a9f62d1e 100644 --- a/charmtools/charms.py +++ b/charmtools/charms.py @@ -332,7 +332,7 @@ def proof(self): validate_payloads(charm, lint, proof_extensions.get('payloads')) validate_terms(charm, lint) validate_resources(charm, lint, proof_extensions.get('resources')) - validate_deployment(charm, lint) + validate_deployment(charm, lint, proof_extensions.get('deployment')) if not os.path.exists(os.path.join(charm_path, 'icon.svg')): lint.info("No icon.svg file.") @@ -564,12 +564,42 @@ def schema_type(self, **kw): colander.String(), name='type', ) + count = colander.SchemaNode( colander.Integer(), missing=1, ) +class DeploymentItem(colander.MappingSchema): + def schema_type(self, **kw): + return StrictMapping() + + type_ = colander.SchemaNode( + colander.String(), + validator=colander.OneOf(['stateless', 'stateful']), + name='type', + ) + + service = colander.SchemaNode( + colander.String(), + validator=colander.OneOf(['loadbalancer', 'cluster', 'omit']), + name='service', + ) + + daemonset = colander.SchemaNode( + Boolean(), + name='daemonset', + missing=False, + ) + + min_version = colander.SchemaNode( + colander.String(), + name='min-version', + missing='', + ) + + class StorageItem(colander.MappingSchema): def schema_type(self, **kw): return StrictMapping() @@ -703,7 +733,7 @@ def validate_resources(charm, linter, proof_extensions=None): linter.err('resources.{}: {}'.format(k, v)) -def validate_deployment(charm, linter): +def validate_deployment(charm, linter, proof_extensions=None): """Validate deployment in charm metadata. :param charm: dict of charm metadata parsed from metadata.yaml @@ -711,26 +741,28 @@ def validate_deployment(charm, linter): messages will be written """ - if 'deployment' not in charm: + + deployment = charm.get('deployment', {}) + if deployment == {}: return - deployment = charm['deployment'] if not isinstance(deployment, dict): linter.err('deployment: must be a dict of config') return - - known_field_types = { - 'type': str, - 'service': str, - 'daemonset': bool, - 'min-version': str, - } - for k, v in deployment.items(): - field_type = known_field_types.get(k, None) - if field_type is None: - linter.err('deployment.{} is not supported'.format(k)) - elif not isinstance(v, field_type): - linter.err('deployment.{} must be {} but got {}'.format(k, field_type.__name__, type(v).__name__)) + + deployment = dict(deployment=deployment) + schema = colander.SchemaNode(colander.Mapping()) + for item in deployment: + schema.add(DeploymentItem(name=item)) + + try: + try: + schema.deserialize(deployment) + except colander.Invalid as e: + _try_proof_extensions(e, proof_extensions) + except colander.Invalid as e: + for k, v in e.asdict().items(): + linter.err('deployment.{}: {}'.format(k, v)) def validate_extra_bindings(charm, linter): diff --git a/tests/test_charm_proof.py b/tests/test_charm_proof.py index 2040b99f..27f629eb 100644 --- a/tests/test_charm_proof.py +++ b/tests/test_charm_proof.py @@ -815,7 +815,6 @@ def test_storage_proof_extensions(self): self.assertEqual(linter.err.call_args_list, []) - class DeploymentValidationTest(TestCase): def test_deployment(self): """Charm has valid deployment.""" @@ -825,7 +824,7 @@ def test_deployment(self): 'type': 'stateful', 'service': 'omit', 'daemonset': True, - 'min-version': "1.15", + 'min-version': "1.15.0", } } validate_deployment(charm, linter) @@ -851,15 +850,16 @@ def test_deployment_unsupported_field(self): 'type': 'stateful', 'service': 'omit', 'daemonset': True, - 'min-version': "1.15", + 'min-version': "1.15.0", 'unknow-field': 'xxx', } } validate_deployment(charm, linter) self.assertEqual(linter.err.call_count, 1) linter.err.assert_has_calls([ - call('deployment.unknow-field is not supported'), + call('deployment.deployment: Unrecognized keys in mapping: "{\'unknow-field\': \'xxx\'}"'), ], any_order=True) + def test_deployment_invalid_type(self): """Charm has the invalid deployment type.""" @@ -869,13 +869,13 @@ def test_deployment_invalid_type(self): 'type': True, 'service': 'omit', 'daemonset': True, - 'min-version': "1.15", + 'min-version': "1.15.0", } } validate_deployment(charm, linter) self.assertEqual(linter.err.call_count, 1) linter.err.assert_has_calls([ - call("deployment.type must be str but got bool"), + call("deployment.deployment.type: True is not a string: {'type': ''}"), ], any_order=True) def test_deployment_invalid_service(self): @@ -886,13 +886,13 @@ def test_deployment_invalid_service(self): 'type': 'stateful', 'service': 1, 'daemonset': True, - 'min-version': "1.15", + 'min-version': "1.15.0", } } validate_deployment(charm, linter) self.assertEqual(linter.err.call_count, 1) linter.err.assert_has_calls([ - call("deployment.service must be str but got int"), + call("deployment.deployment.service: 1 is not a string: {'service': ''}"), ], any_order=True) def test_deployment_invalid_daemonset(self): @@ -902,14 +902,14 @@ def test_deployment_invalid_daemonset(self): 'deployment': { 'type': 'stateful', 'service': 'omit', - 'daemonset': 'True', - 'min-version': "1.15", + 'daemonset': 'xx', + 'min-version': "1.15.0", } } validate_deployment(charm, linter) self.assertEqual(linter.err.call_count, 1) linter.err.assert_has_calls([ - call("deployment.daemonset must be bool but got str"), + call('deployment.deployment.daemonset: "xx" is not one of true, false'), ], any_order=True) def test_deployment_invalid_min_version(self): @@ -926,7 +926,7 @@ def test_deployment_invalid_min_version(self): validate_deployment(charm, linter) self.assertEqual(linter.err.call_count, 1) linter.err.assert_has_calls([ - call("deployment.min-version must be str but got float"), + call("deployment.deployment.min-version: 1.15 is not a string: {'min-version': ''}"), ], any_order=True) From 52f58695f8d7128d33df871a468614cbb61a0eff Mon Sep 17 00:00:00 2001 From: Kelvin Liu Date: Tue, 17 Sep 2019 00:59:16 +1000 Subject: [PATCH 4/4] Add tests for unsupported deployment.service, deployment.type;; --- tests/test_charm_proof.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/test_charm_proof.py b/tests/test_charm_proof.py index 27f629eb..64a8caf1 100644 --- a/tests/test_charm_proof.py +++ b/tests/test_charm_proof.py @@ -878,6 +878,23 @@ def test_deployment_invalid_type(self): call("deployment.deployment.type: True is not a string: {'type': ''}"), ], any_order=True) + def test_deployment_unsupported_type(self): + """Charm has the unsupported deployment type.""" + linter = Mock() + charm = { + 'deployment': { + 'type': 'foo', + 'service': 'omit', + 'daemonset': True, + 'min-version': "1.15.0", + } + } + validate_deployment(charm, linter) + self.assertEqual(linter.err.call_count, 1) + linter.err.assert_has_calls([ + call('deployment.deployment.type: "foo" is not one of stateless, stateful'), + ], any_order=True) + def test_deployment_invalid_service(self): """Charm has the invalid deployment service.""" linter = Mock() @@ -895,6 +912,23 @@ def test_deployment_invalid_service(self): call("deployment.deployment.service: 1 is not a string: {'service': ''}"), ], any_order=True) + def test_deployment_unsupported_service(self): + """Charm has the unsupported deployment service.""" + linter = Mock() + charm = { + 'deployment': { + 'type': 'stateful', + 'service': 'foo', + 'daemonset': True, + 'min-version': "1.15.0", + } + } + validate_deployment(charm, linter) + self.assertEqual(linter.err.call_count, 1) + linter.err.assert_has_calls([ + call('deployment.deployment.service: "foo" is not one of loadbalancer, cluster, omit'), + ], any_order=True) + def test_deployment_invalid_daemonset(self): """Charm has the invalid deployment daemonset.""" linter = Mock()