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
diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
index 15ae1ec..d43d060 100644
--- a/cloudinit/config/cc_ntp.py
+++ b/cloudinit/config/cc_ntp.py
@@ -100,7 +100,9 @@ def handle(name, cfg, cloud, log, _args):
100 LOG.debug(100 LOG.debug(
101 "Skipping module named %s, not present or disabled by cfg", name)101 "Skipping module named %s, not present or disabled by cfg", name)
102 return102 return
103 ntp_cfg = cfg.get('ntp', {})103 ntp_cfg = cfg['ntp']
104 if ntp_cfg is None:
105 ntp_cfg = {} # Allow empty config which will install the package
104106
105 # TODO drop this when validate_cloudconfig_schema is strict=True107 # TODO drop this when validate_cloudconfig_schema is strict=True
106 if not isinstance(ntp_cfg, (dict)):108 if not isinstance(ntp_cfg, (dict)):
diff --git a/tests/cloud_tests/testcases/modules/ntp.yaml b/tests/cloud_tests/testcases/modules/ntp.yaml
index fbef431..2530d72 100644
--- a/tests/cloud_tests/testcases/modules/ntp.yaml
+++ b/tests/cloud_tests/testcases/modules/ntp.yaml
@@ -4,8 +4,8 @@
4cloud_config: |4cloud_config: |
5 #cloud-config5 #cloud-config
6 ntp:6 ntp:
7 pools: {}7 pools: []
8 servers: {}8 servers: []
9collect_scripts:9collect_scripts:
10 ntp_installed: |10 ntp_installed: |
11 #!/bin/bash11 #!/bin/bash
diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py
index 4f29124..3abe578 100644
--- a/tests/unittests/test_handler/test_handler_ntp.py
+++ b/tests/unittests/test_handler/test_handler_ntp.py
@@ -293,23 +293,24 @@ class TestNtp(FilesystemMockingTestCase):
293293
294 def test_ntp_handler_schema_validation_allows_empty_ntp_config(self):294 def test_ntp_handler_schema_validation_allows_empty_ntp_config(self):
295 """Ntp schema validation allows for an empty ntp: configuration."""295 """Ntp schema validation allows for an empty ntp: configuration."""
296 invalid_config = {'ntp': {}}296 valid_empty_configs = [{'ntp': {}}, {'ntp': None}]
297 distro = 'ubuntu'297 distro = 'ubuntu'
298 cc = self._get_cloud(distro)298 cc = self._get_cloud(distro)
299 ntp_conf = os.path.join(self.new_root, 'ntp.conf')299 ntp_conf = os.path.join(self.new_root, 'ntp.conf')
300 with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:300 with open('{0}.tmpl'.format(ntp_conf), 'wb') as stream:
301 stream.write(NTP_TEMPLATE)301 stream.write(NTP_TEMPLATE)
302 with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):302 for valid_empty_config in valid_empty_configs:
303 cc_ntp.handle('cc_ntp', invalid_config, cc, None, [])303 with mock.patch('cloudinit.config.cc_ntp.NTP_CONF', ntp_conf):
304 cc_ntp.handle('cc_ntp', valid_empty_config, cc, None, [])
305 with open(ntp_conf) as stream:
306 content = stream.read()
307 default_pools = [
308 "{0}.{1}.pool.ntp.org".format(x, distro)
309 for x in range(0, cc_ntp.NR_POOL_SERVERS)]
310 self.assertEqual(
311 "servers []\npools {0}\n".format(default_pools),
312 content)
304 self.assertNotIn('Invalid config:', self.logs.getvalue())313 self.assertNotIn('Invalid config:', self.logs.getvalue())
305 with open(ntp_conf) as stream:
306 content = stream.read()
307 default_pools = [
308 "{0}.{1}.pool.ntp.org".format(x, distro)
309 for x in range(0, cc_ntp.NR_POOL_SERVERS)]
310 self.assertEqual(
311 "servers []\npools {0}\n".format(default_pools),
312 content)
313314
314 @skipIf(_missing_jsonschema_dep, "No python-jsonschema dependency")315 @skipIf(_missing_jsonschema_dep, "No python-jsonschema dependency")
315 def test_ntp_handler_schema_validation_warns_non_string_item_type(self):316 def test_ntp_handler_schema_validation_warns_non_string_item_type(self):
diff --git a/tests/unittests/test_handler/test_schema.py b/tests/unittests/test_handler/test_schema.py
index b8fc893..648573f 100644
--- a/tests/unittests/test_handler/test_schema.py
+++ b/tests/unittests/test_handler/test_schema.py
@@ -4,11 +4,12 @@ from cloudinit.config.schema import (
4 CLOUD_CONFIG_HEADER, SchemaValidationError, annotated_cloudconfig_file,4 CLOUD_CONFIG_HEADER, SchemaValidationError, annotated_cloudconfig_file,
5 get_schema_doc, get_schema, validate_cloudconfig_file,5 get_schema_doc, get_schema, validate_cloudconfig_file,
6 validate_cloudconfig_schema, main)6 validate_cloudconfig_schema, main)
7from cloudinit.util import write_file7from cloudinit.util import subp, write_file
88
9from cloudinit.tests.helpers import CiTestCase, mock, skipIf9from cloudinit.tests.helpers import CiTestCase, mock, skipIf
1010
11from copy import copy11from copy import copy
12import os
12from six import StringIO13from six import StringIO
13from textwrap import dedent14from textwrap import dedent
14from yaml import safe_load15from yaml import safe_load
@@ -364,4 +365,38 @@ class MainTest(CiTestCase):
364 self.assertIn(365 self.assertIn(
365 'Valid cloud-config file {0}'.format(myyaml), m_stdout.getvalue())366 'Valid cloud-config file {0}'.format(myyaml), m_stdout.getvalue())
366367
368
369class CloudTestsIntegrationTest(CiTestCase):
370 """Validate all cloud-config yaml schema provided in integration tests.
371
372 It is less expensive to have unittests validate schema of all cloud-config
373 yaml provided to integration tests, than to run an integration test which
374 raises Warnings or errors on invalid cloud-config schema.
375 """
376
377 @skipIf(_missing_jsonschema_dep, "No python-jsonschema dependency")
378 def test_all_integration_test_cloud_config_schema(self):
379 """Validate schema of cloud_tests yaml files looking for warnings."""
380 schema = get_schema()
381 testsdir = os.path.dirname(os.path.dirname(os.path.dirname(__file__)))
382 integration_testdir = os.path.sep.join(
383 [testsdir, 'cloud_tests', 'testcases'])
384 errors = []
385 out, _ = subp(['find', integration_testdir, '-name', '*yaml'])
386 for filename in out.splitlines():
387 test_cfg = safe_load(open(filename))
388 cloud_config = test_cfg.get('cloud_config')
389 if cloud_config:
390 cloud_config = safe_load(
391 cloud_config.replace("#cloud-config\n", ""))
392 try:
393 validate_cloudconfig_schema(
394 cloud_config, schema, strict=True)
395 except SchemaValidationError as e:
396 errors.append(
397 '{0}: {1}'.format(
398 filename, e))
399 if errors:
400 raise AssertionError(', '.join(errors))
401
367# vi: ts=4 expandtab syntax=python402# vi: ts=4 expandtab syntax=python

Subscribers

People subscribed via source and target branches