More minor inline comments and updated schema @
http://paste.ubuntu.com/p/2by6yyn8dT/
Diff comments:
> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index cbd0237..099b8d8 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -83,7 +102,56 @@ schema = {
> List of ntp servers. If both pools and servers are
> empty, 4 default pool servers will be provided with
> the format ``{0-3}.{distro}.pool.ntp.org``.""")
> - }
> + },
> + 'ntp_client': {
> + 'type': 'string',
> + 'description': dedent("""\
> + Name of an NTP client to use to configure system NTP
> + """),
> + },
> + 'enabled': {
> + 'type': 'boolean',
> + 'description': "",
> + },
> + 'config': {
> + 'type': ['object', 'null'],
> + 'properties': {
> + 'confpath': {
> + 'type': 'string',
> + 'description': "",
> + },
> + 'check_exe': {
> + 'type': 'string',
> + 'description': "",
> + },
> + 'name': {
> + 'type': 'string',
> + 'description': "",
> + },
> + 'packages': {
> + 'type': 'array',
> + 'items': {
> + 'type': 'string',
> + },
> + 'uniqueItems': True,
> + 'description': dedent("""\
> + List of packages needed to be installed for
> the
> + selected ``ntp_client``."""),
> + },
> + 'service_name': {
> + 'type': 'string',
> + 'description': "",
> + },
> + 'template_name': {
Two user-visible config keys that don't make sense and I think can be dropped.
"name" seems unused (and it is also a duplicate value of the top-level
ntp_client in all of our default cases).
"template_name" only makes sense if someone delivers a new template into the
same cloud-init templates directory and then references it. I don't want to
encourage people to write additional content into cloud-init's directories.
They should just use template instead and provide their entire content if they
really want to roll their own. I also don't really see a use case for someone
trying to override template_name for one distro with another distro's
cloud-init template file.
> + 'type': 'string',
> + 'description': "",
> + },
> + 'template': {
> + 'type': 'string',
> + 'description': "",
> + },
> + },
> + },
> },
> 'required': [],
> 'additionalProperties': False
> @@ -199,17 +253,27 @@ def write_ntp_config_template(cfg, cloud, path,
> template=None):
> 'pools': pools,
> }
>
> - if template is None:
> - template = 'ntp.conf.%s' % cloud.distro.name
> + path = clientcfg.get('confpath')
> + template_name = clientcfg.get('template_name').replace('{distro}',
> + cloud.distro.name)
> + template = clientcfg.get('template')
> + if template:
> + tfile = tempfile.NamedTemporaryFile(delete=False,
We should probably use temp_util.mkstmp here as we might hit race conditions
with temp files being blown away during the initial boot process.
> + prefix='template_name',
> + suffix=".tmpl")
> + template_fn = tfile.name
> + util.write_file(template_fn, content=template)
> + else:
> + template_fn = cloud.get_template_filename(template_name)
>
> - template_fn = cloud.get_template_filename(template)
> if not template_fn:
> - template_fn = cloud.get_template_filename('ntp.conf')
> - if not template_fn:
> - raise RuntimeError(
> - 'No template found, not rendering {path}'.format(path=path))
> + raise RuntimeError(
> + 'No template found, not rendering {}'.format(path))
>
> templater.render_to_file(template_fn, path, params)
> + # clean up temporary template
> + if template:
> + util.del_file(template_fn)
>
>
> def reload_ntp(service, systemd=False):
> diff --git a/cloudinit/distros/debian.py b/cloudinit/distros/debian.py
> index 33cc0bf..2d5fd74 100644
> --- a/cloudinit/distros/debian.py
> +++ b/cloudinit/distros/debian.py
> @@ -39,6 +39,18 @@ ENI_HEADER = """# This file is generated from information
> provided by
> NETWORK_CONF_FN = "/etc/network/interfaces.d/50-cloud-init.cfg"
> LOCALE_CONF_FN = "/etc/default/locale"
>
> +NTP_CLIENT_CONFIG = {
> + 'chrony': {
> + 'check_exe': 'chronyd',
> + 'confpath': '/etc/chrony/chrony.conf',
The only difference we are updating, from the base distro definition, is the
confpath. So, why don't we just declare NTP_CLIENT_CONF_PATH =
'/etc/chrony/chrony.conf' and self.ntp_client_config['confpath'] =
NTP_CLIENT_CONF_PATH instead. It'll make it clear what's really being updated.
> + 'name': 'chrony',
> + 'packages': ['chrony'],
> + 'service_name': 'chrony',
> + 'template_name': 'chrony.conf.{distro}',
> + 'template': None,
> + },
> +}
> +
>
> class Distro(distros.Distro):
> hostname_conf_fn = "/etc/hostname"
--
https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/339438
Your team cloud-init commiters is requested to review the proposed merge of
~raharper/cloud-init:feature/update-ntp-spec into cloud-init:master.
_______________________________________________
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
