Merge ~chad.smith/cloud-init:ntp-schema into cloud-init:master

Proposed by Chad Smith on 2017-10-20
Status: Merged
Approved by: Scott Moser on 2017-10-20
Approved revision: 545b0dd39f6e3741d6e9b698bdfd36d73e82abf7
Merged at revision: 6bc504e41666329631cdfd5b947ed5b0e2529a76
Proposed branch: ~chad.smith/cloud-init:ntp-schema
Merge into: cloud-init:master
Diff against target: 127 lines (+53/-15)
4 files modified
cloudinit/config/cc_ntp.py (+3/-1)
tests/cloud_tests/testcases/modules/ntp.yaml (+2/-2)
tests/unittests/test_handler/test_handler_ntp.py (+12/-11)
tests/unittests/test_handler/test_schema.py (+36/-1)
Reviewer Review Type Date Requested Status
Scott Moser 2017-10-20 Approve on 2017-10-20
Joshua Powers Approve on 2017-10-20
Server Team CI bot continuous-integration Approve on 2017-10-20
Review via email: mp+332540@code.launchpad.net

Commit message

ntp: fix config module schema to allow empty ntp config

Fix three things related to the ntp module:
  1. Fix invalid cloud-config schema in the integration test which
     provided empty dicts instead of emptylists for pools and servers
  2. Correct logic in the ntp module to allow support for the minimal
     cloud-config 'ntp:' without raising a RuntimeError. Docs and schema
     definitions already describe that cloud-config's ntp can be empty.
     An ntp configuration with neither pools nor servers will be
     configured with a default set of ntp pools. As such, the ntp module
     now officially allows the following ntp cloud-configs:
     - ntp:
     - ntp: {}
     - ntp:
         servers: []
         pools: []
  3. Add a simple unit test which validates all cloud-config provided to
      our integration tests to ensure it adheres to any defined module
      schema so as more jsonschema definitions are added, we validate our
      integration test configs.

LP: #1724951

Description of the change

ntp: fix config module schema to allow empty ntp config

Fix three things related to the ntp module:
  1. Fix invalid cloud-config schema in the integration test which
     provided empty dicts instead of emptylists for pools and servers
  2. Correct logic in the ntp module to allow support for the minimal
     cloud-config 'ntp:' without raising a RuntimeError. Docs and schema
     definitions already describe that cloud-config's ntp can be empty.
     An ntp configuration with neither pools nor servers will be
     configured with a default set of ntp pools. As such, the ntp module
     now officially allows the following ntp cloud-configs:
     - ntp:
     - ntp: {}
     - ntp:
         servers: []
         pools: []
  3. Add a simple unit test which validates all cloud-config provided to
      our integration tests to ensure it adheres to any defined module
      schema so as more jsonschema definitions are added, we validate our
      integration test configs.

LP: #1724951

To post a comment you must log in.

FAILED: Continuous integration, rev:545b0dd39f6e3741d6e9b698bdfd36d73e82abf7
https://jenkins.ubuntu.com/server/job/cloud-init-ci/415/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    FAILED: MAAS Compatability Testing

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/415/rebuild

review: Needs Fixing (continuous-integration)

PASSED: Continuous integration, rev:545b0dd39f6e3741d6e9b698bdfd36d73e82abf7
https://jenkins.ubuntu.com/server/job/cloud-init-ci/416/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/416/rebuild

review: Approve (continuous-integration)
Chad Smith (chad.smith) :
Joshua Powers (powersj) wrote :

LGTM - Thanks for fixing the integration test!

review: Approve
Scott Moser (smoser) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
2index 15ae1ec..d43d060 100644
3--- a/cloudinit/config/cc_ntp.py
4+++ b/cloudinit/config/cc_ntp.py
5@@ -100,7 +100,9 @@ def handle(name, cfg, cloud, log, _args):
6 LOG.debug(
7 "Skipping module named %s, not present or disabled by cfg", name)
8 return
9- ntp_cfg = cfg.get('ntp', {})
10+ ntp_cfg = cfg['ntp']
11+ if ntp_cfg is None:
12+ ntp_cfg = {} # Allow empty config which will install the package
13
14 # TODO drop this when validate_cloudconfig_schema is strict=True
15 if not isinstance(ntp_cfg, (dict)):
16diff --git a/tests/cloud_tests/testcases/modules/ntp.yaml b/tests/cloud_tests/testcases/modules/ntp.yaml
17index fbef431..2530d72 100644
18--- a/tests/cloud_tests/testcases/modules/ntp.yaml
19+++ b/tests/cloud_tests/testcases/modules/ntp.yaml
20@@ -4,8 +4,8 @@
21 cloud_config: |
22 #cloud-config
23 ntp:
24- pools: {}
25- servers: {}
26+ pools: []
27+ servers: []
28 collect_scripts:
29 ntp_installed: |
30 #!/bin/bash
31diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py
32index 4f29124..3abe578 100644
33--- a/tests/unittests/test_handler/test_handler_ntp.py
34+++ b/tests/unittests/test_handler/test_handler_ntp.py
35@@ -293,23 +293,24 @@ class TestNtp(FilesystemMockingTestCase):
36
37 def test_ntp_handler_schema_validation_allows_empty_ntp_config(self):
38 """Ntp schema validation allows for an empty ntp: configuration."""
39- invalid_config = {'ntp': {}}
40+ valid_empty_configs = [{'ntp': {}}, {'ntp': None}]
41 distro = 'ubuntu'
42 cc = self._get_cloud(distro)
43 ntp_conf = os.path.join(self.new_root, 'ntp.conf')
44 with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
45 stream.write(NTP_TEMPLATE)
46- with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
47- cc_ntp.handle('cc_ntp', invalid_config, cc, None, [])
48+ for valid_empty_config in valid_empty_configs:
49+ with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
50+ cc_ntp.handle('cc_ntp', valid_empty_config, cc, None, [])
51+ with open(ntp_conf) as stream:
52+ content = stream.read()
53+ default_pools = [
54+ "{0}.{1}.pool.ntp.org".format(x, distro)
55+ for x in range(0, cc_ntp.NR_POOL_SERVERS)]
56+ self.assertEqual(
57+ "servers []\npools {0}\n".format(default_pools),
58+ content)
59 self.assertNotIn('Invalid config:', self.logs.getvalue())
60- with open(ntp_conf) as stream:
61- content = stream.read()
62- default_pools = [
63- "{0}.{1}.pool.ntp.org".format(x, distro)
64- for x in range(0, cc_ntp.NR_POOL_SERVERS)]
65- self.assertEqual(
66- "servers []\npools {0}\n".format(default_pools),
67- content)
68
69 @skipIf(_missing_jsonschema_dep, "No python-jsonschema dependency")
70 def test_ntp_handler_schema_validation_warns_non_string_item_type(self):
71diff --git a/tests/unittests/test_handler/test_schema.py b/tests/unittests/test_handler/test_schema.py
72index b8fc893..648573f 100644
73--- a/tests/unittests/test_handler/test_schema.py
74+++ b/tests/unittests/test_handler/test_schema.py
75@@ -4,11 +4,12 @@ from cloudinit.config.schema import (
76 CLOUD_CONFIG_HEADER, SchemaValidationError, annotated_cloudconfig_file,
77 get_schema_doc, get_schema, validate_cloudconfig_file,
78 validate_cloudconfig_schema, main)
79-from cloudinit.util import write_file
80+from cloudinit.util import subp, write_file
81
82 from cloudinit.tests.helpers import CiTestCase, mock, skipIf
83
84 from copy import copy
85+import os
86 from six import StringIO
87 from textwrap import dedent
88 from yaml import safe_load
89@@ -364,4 +365,38 @@ class MainTest(CiTestCase):
90 self.assertIn(
91 'Valid cloud-config file {0}'.format(myyaml), m_stdout.getvalue())
92
93+
94+class CloudTestsIntegrationTest(CiTestCase):
95+ """Validate all cloud-config yaml schema provided in integration tests.
96+
97+ It is less expensive to have unittests validate schema of all cloud-config
98+ yaml provided to integration tests, than to run an integration test which
99+ raises Warnings or errors on invalid cloud-config schema.
100+ """
101+
102+ @skipIf(_missing_jsonschema_dep, "No python-jsonschema dependency")
103+ def test_all_integration_test_cloud_config_schema(self):
104+ """Validate schema of cloud_tests yaml files looking for warnings."""
105+ schema = get_schema()
106+ testsdir = os.path.dirname(os.path.dirname(os.path.dirname(__file__)))
107+ integration_testdir = os.path.sep.join(
108+ [testsdir, 'cloud_tests', 'testcases'])
109+ errors = []
110+ out, _ = subp(['find', integration_testdir, '-name', '*yaml'])
111+ for filename in out.splitlines():
112+ test_cfg = safe_load(open(filename))
113+ cloud_config = test_cfg.get('cloud_config')
114+ if cloud_config:
115+ cloud_config = safe_load(
116+ cloud_config.replace("#cloud-config\n", ""))
117+ try:
118+ validate_cloudconfig_schema(
119+ cloud_config, schema, strict=True)
120+ except SchemaValidationError as e:
121+ errors.append(
122+ '{0}: {1}'.format(
123+ filename, e))
124+ if errors:
125+ raise AssertionError(', '.join(errors))
126+
127 # vi: ts=4 expandtab syntax=python

Subscribers

People subscribed via source and target branches