cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

Romanos,

Over all it looks really good.

In order to take this, you'll need to sign the contributors agreement (https://www.ubuntu.com/legal/contributors). See the hacking section on readthedocs for more info
http://cloudinit.readthedocs.io/en/latest/topics/hacking.html .

Also, please feel free to ping me in Freenode #cloud-init or /query me, or via email.

I've left a few minor questions inline also, but the 'needs fixing' is really only wrt the contributors agreement.


Diff comments:

> diff --git a/cloudinit/config/cc_puppet.py b/cloudinit/config/cc_puppet.py
> index bfd630d..d5351f6 100644
> --- a/cloudinit/config/cc_puppet.py
> +++ b/cloudinit/config/cc_puppet.py
> @@ -71,10 +81,23 @@ import socket
>  from cloudinit import helpers
>  from cloudinit import util
>  
> -PUPPET_CONF_PATH = '/etc/puppet/puppet.conf'
> -PUPPET_SSL_CERT_DIR = '/var/lib/puppet/ssl/certs/'
> -PUPPET_SSL_DIR = '/var/lib/puppet/ssl'
> -PUPPET_SSL_CERT_PATH = '/var/lib/puppet/ssl/certs/ca.pem'
> +DEFAULT_PACKAGE_NAME = 'puppet'
> +DEFAULT_SSL_DIR = '/var/lib/puppet/ssl'
> +DEFAULT_CONF_DIR = '/etc/puppet'

any reason why you've chosen to allow the directory, and assume/hard code the conf to be in that directory at 'puppet.conf' rather than allowing the configuration of the puppet config file path directly ?

> +
> +
> +class PuppetConstants(object):
> +
> +    def __init__(self,
> +                 puppet_conf_dir,
> +                 puppet_ssl_dir,
> +                 log):
> +        self.conf_dir = puppet_conf_dir
> +        self.conf_path = os.path.join(puppet_conf_dir, "puppet.conf")
> +        self.ssl_dir = puppet_ssl_dir
> +        self.ssl_cert_dir = os.path.join(puppet_ssl_dir, "certs")
> +        self.ssl_cert_path = os.path.join(self.ssl_cert_dir,
> +                                          "ca.pem")
>  
>  
>  def _autostart_puppet(log):
> @@ -101,22 +124,35 @@ def handle(name, cfg, cloud, log, _args):
>          return
>  
>      puppet_cfg = cfg['puppet']
> -
>      # Start by installing the puppet package if necessary...
>      install = util.get_cfg_option_bool(puppet_cfg, 'install', True)
>      version = util.get_cfg_option_str(puppet_cfg, 'version', None)
> +    package_name = util.get_cfg_option_str(puppet_cfg,
> +                                           'package_name',
> +                                           DEFAULT_PACKAGE_NAME)
> +    conf_dir = util.get_cfg_option_str(puppet_cfg,
> +                                       'conf_dir',
> +                                       DEFAULT_CONF_DIR)
> +    ssl_dir = util.get_cfg_option_str(puppet_cfg,
> +                                      'ssl_dir',
> +                                      DEFAULT_SSL_DIR)
> +
> +    p_constants = PuppetConstants(conf_dir,

i'd just join these three to save vertical space:
  p_constants = PuppetConstants(conf_dir, ssl_dir, log)

> +                                  ssl_dir,
> +                                  log)
>      if not install and version:
>          log.warn(("Puppet install set false but version supplied,"
>                    " doing nothing."))
>      elif install:
>          log.debug(("Attempting to install puppet %s,"),
>                    version if version else 'latest')
> -        cloud.distro.install_packages(('puppet', version))
> +
> +        cloud.distro.install_packages((package_name, version))
>  
>      # ... and then update the puppet configuration
>      if 'conf' in puppet_cfg:
>          # Add all sections from the conf object to puppet.conf
> -        contents = util.load_file(PUPPET_CONF_PATH)
> +        contents = util.load_file(p_constants.conf_path)
>          # Create object for reading puppet.conf values
>          puppet_config = helpers.DefaultingConfigParser()
>          # Read puppet.conf values from original file in order to be able to


-- 
https://code.launchpad.net/~rski/cloud-init/+git/cloud-init/+merge/312284
Your team cloud init development team is requested to review the proposed merge of ~rski/cloud-init:puppet_4 into cloud-init:master.