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. _______________________________________________ Mailing list: https://launchpad.net/~cloud-init-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~cloud-init-dev More help : https://help.launchpad.net/ListHelp
