cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

I feel like we could do a bit better validation on the user-provided config keys with a simple validate_config function that'd raise understandable errors on misconfig for certain keys:

Here are some examples of easy and hard tracebacks to understand, as the person can't just run cloud-init status --long to see the error message, they have to walk through /var/log/cloud-init.log and read through a traceback stack to get at what may be the problem.

What do you think about a bit more validation on required config values?

https://pastebin.ubuntu.com/p/4X9TJg3NSz/

Diff comments:

> diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
> index cbd0237..7a7fd7e 100644
> --- a/cloudinit/config/cc_ntp.py
> +++ b/cloudinit/config/cc_ntp.py
> @@ -185,34 +369,60 @@ def generate_server_names(distro):
>      return names
>  
>  
> -def write_ntp_config_template(cfg, cloud, path, template=None):
> -    servers = cfg.get('servers', [])
> -    pools = cfg.get('pools', [])
> +def write_ntp_config_template(distro_name, servers=None, pools=None,
> +                              path=None, template_fn=None, template=None):
> +    """Render a ntp client configuration for the specified client.
> +
> +    @param distro_name: string.  The distro class name.
> +    @param servers: A list of strings specifying ntp servers. Defaults to empty
> +    list.
> +    @param pools: A list of strings specifying ntp pools. Defaults to empty
> +    list.
> +    @param path: A string to specify where to write the rendered template.
> +    @param template_fn: A string to specify the template source file.
> +    @param template: A string specifying the contents of the template. This
> +    content will be written to a temporary file before being used to render
> +    the configuration file.
> +
> +    @raises: ValueError when path is None.
> +    @raises: ValueError when template_fn is None and template is None.
> +    """
> +    if not servers:
> +        servers = []
> +    if not pools:
> +        pools = []
>  
>      if len(servers) == 0 and len(pools) == 0:
> -        pools = generate_server_names(cloud.distro.name)
> +        pools = generate_server_names(distro_name)
>          LOG.debug(
>              'Adding distro default ntp pool servers: %s', ','.join(pools))
>  
> -    params = {
> -        'servers': servers,
> -        'pools': pools,
> -    }
> +    if not path:
> +        raise ValueError('Invalid value for path parameter')

This should probably be "Missing value for path parameter"

>  
> -    if template is None:
> -        template = 'ntp.conf.%s' % cloud.distro.name
> +    if not template_fn and not template:
> +        raise ValueError('Not template_fn or template provided')
>  
> -    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))
> +    params = {'servers': servers, 'pools': pools}
> +    if template:
> +        tfile = temp_utils.mkstemp(prefix='template_name-', suffix=".tmpl")
> +        template_fn = tfile[1]  # filepath is second item in tuple
> +        util.write_file(template_fn, content=template)
>  
>      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):
> +    """Restart or reload an ntp system service.
> +
> +    @param service: A string specifying the name of the service to be affected.
> +    @param systemd: A boolean indicating if the distro uses systemd, defaults
> +    to False.
> +    @returns: A tuple of stdout, stderr results from executing the action.
> +    """
>      if systemd:
>          cmd = ['systemctl', 'reload-or-restart', service]
>      else:


-- 
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.