nagios-charmers team mailing list archive

[Ryan Farrell <ryan.farrell@xxxxxxxxxxxxx>]

Review: Approve

+1 - just a few cases where multiline strings changed into single lines but have extra {"}s in them. Im pretty sure it doesn't break anything though.

Diff comments:

> diff --git a/bin/charm_helpers_sync.py b/bin/charm_helpers_sync.py
> index 7c0c194..a689b84 100644
> --- a/bin/charm_helpers_sync.py
> +++ b/bin/charm_helpers_sync.py
> @@ -141,47 +138,44 @@ def sync_directory(src, dest, opts=None):
>  def sync(src, dest, module, opts=None):
>  
>      # Sync charmhelpers/__init__.py for bootstrap code.
> -    sync_pyfile(_src_path(src, '__init__'), dest)
> +    sync_pyfile(_src_path(src, "__init__"), dest)
>  
>      # Sync other __init__.py files in the path leading to module.
>      m = []
> -    steps = module.split('.')[:-1]
> +    steps = module.split(".")[:-1]
>      while steps:
>          m.append(steps.pop(0))
> -        init = '.'.join(m + ['__init__'])
> -        sync_pyfile(_src_path(src, init),
> -                    os.path.dirname(_dest_path(dest, init)))
> +        init = ".".join(m + ["__init__"])
> +        sync_pyfile(_src_path(src, init), os.path.dirname(_dest_path(dest, init)))
>  
>      # Sync the module, or maybe a .py file.
>      if os.path.isdir(_src_path(src, module)):
>          sync_directory(_src_path(src, module), _dest_path(dest, module), opts)
>      elif _is_pyfile(_src_path(src, module)):
> -        sync_pyfile(_src_path(src, module),
> -                    os.path.dirname(_dest_path(dest, module)))
> +        sync_pyfile(_src_path(src, module), os.path.dirname(_dest_path(dest, module)))
>      else:
> -        logging.warn('Could not sync: %s. Neither a pyfile or directory, '
> -                     'does it even exist?' % module)
> +        logging.warn("Could not sync: %s. Neither a pyfile or directory, " "does it even exist?" % module)

Something funky here - this string seems broken up with stray {"}s

>  
>  
>  def parse_sync_options(options):
>      if not options:
>          return []
> -    return options.split(',')
> +    return options.split(",")
>  
>  
>  def extract_options(inc, global_options=None):
>      global_options = global_options or []
>      if global_options and isinstance(global_options, six.string_types):
>          global_options = [global_options]
> -    if '|' not in inc:
> +    if "|" not in inc:
>          return (inc, global_options)
> -    inc, opts = inc.split('|')
> +    inc, opts = inc.split("|")
>      return (inc, parse_sync_options(opts) + global_options)
>  
>  
>  def sync_helpers(include, src, dest, options=None):
>      if os.path.exists(dest):
> -        logging.debug('Removing existing directory: %s' % dest)
> +        logging.debug("Removing existing directory: %s" % dest)
>          shutil.rmtree(dest)
>      if not os.path.isdir(dest):
>          os.makedirs(dest)
> diff --git a/hooks/monitors_relation_changed.py b/hooks/monitors_relation_changed.py
> index 5e4f664..15ed680 100755
> --- a/hooks/monitors_relation_changed.py
> +++ b/hooks/monitors_relation_changed.py
> @@ -52,20 +56,17 @@ def _prepare_relation_data(unit, rid):
>      relation_data = relation_get(unit=unit, rid=rid)
>  
>      if not relation_data:
> -        msg = (
> -            'no relation data found for unit {} in relation {} - '
> -            'skipping'.format(unit, rid)
> -        )
> +        msg = "no relation data found for unit {} in relation {} - " "skipping".format(unit, rid)

another broken up string.

>          log(msg, level=DEBUG)
>          return {}
>  
> -    if rid.split(':')[0] == 'nagios':
> +    if rid.split(":")[0] == "nagios":
>          # Fake it for the more generic 'nagios' relation
> -        relation_data['target-id'] = unit.replace('/', '-')
> -        relation_data['monitors'] = {'monitors': {'remote': {}}}
> +        relation_data["target-id"] = unit.replace("/", "-")
> +        relation_data["monitors"] = {"monitors": {"remote": {}}}
>  
> -    if not relation_data.get('target-address'):
> -        relation_data['target-address'] = ingress_address(unit=unit, rid=rid)
> +    if not relation_data.get("target-address"):
> +        relation_data["target-address"] = ingress_address(unit=unit, rid=rid)
>  
>      for key in REQUIRED_REL_DATA_KEYS:
>          if not relation_data.get(key):
> @@ -73,10 +74,7 @@ def _prepare_relation_data(unit, rid):
>              # the relation at first (e.g. gnocchi). After a few hook runs,
>              # though, they add the key. For this reason I think using a logging
>              # level higher than DEBUG could be misleading
> -            msg = (
> -                '{} not found for unit {} in relation {} - '
> -                'skipping'.format(key, unit, rid)
> -            )
> +            msg = "{} not found for unit {} in relation {} - " "skipping".format(key, unit, rid)

broken up string

>              log(msg, level=DEBUG)
>              return {}
>  
> diff --git a/hooks/upgrade_charm.py b/hooks/upgrade_charm.py
> index ea7532c..705dc4c 100755
> --- a/hooks/upgrade_charm.py
> +++ b/hooks/upgrade_charm.py
> @@ -46,33 +50,41 @@ HTTP_ENABLED = ssl_config not in ["only"]
>  
>  def warn_legacy_relations():
>      """
> -    Checks the charm relations for legacy relations
> +    Check the charm relations for legacy relations.
> +
>      Inserts warnings into the log about legacy relations, as they will be removed
>      in the future
>      """
>      if legacy_relations is not None:
> -        hookenv.log("Relations have been radically changed."
> -                    " The monitoring interface is not supported anymore.",
> -                    "WARNING")
> -    hookenv.log("Please use the generic juju-info or the monitors interface",
> -                "WARNING")
> +        hookenv.log(
> +            "Relations have been radically changed." " The monitoring interface is not supported anymore.", "WARNING"

broken up string

> +        )
> +    hookenv.log("Please use the generic juju-info or the monitors interface", "WARNING")
>  
>  
> -# If the charm has extra configuration provided, write that to the
> -# proper nagios3 configuration file, otherwise remove the config
>  def write_extra_config():
> +    """
> +    Write Extra Config.
> +
> +    If the charm has extra configuration provided, write that to the proper
> +    nagios3 configuration file, otherwise remove the config.
> +    """
>      # Be predjudice about this - remove the file always.
> -    if host.file_hash('/etc/nagios3/conf.d/extra.cfg') is not None:
> -        os.remove('/etc/nagios3/conf.d/extra.cfg')
> +    if host.file_hash("/etc/nagios3/conf.d/extra.cfg") is not None:
> +        os.remove("/etc/nagios3/conf.d/extra.cfg")
>      # If we have a config, then write it. the hook reconfiguration will
>      # handle the details
>      if extra_config is not None:
> -        host.write_file('/etc/nagios3/conf.d/extra.cfg', extra_config)
> +        host.write_file("/etc/nagios3/conf.d/extra.cfg", extra_config)
>  
>  
> -# Equivalent of mkdir -p, since we can't rely on
> -# python 3.2 os.makedirs exist_ok argument
>  def mkdir_p(path):
> +    """
> +    Create directory recursively.
> +
> +    Equivalent of mkdir -p, since we can't rely on py32 os.makedirs
> +    `exist_ok` argument
> +    """
>      try:
>          os.makedirs(path)
>      except OSError as exc:  # Python >2.5


-- 
https://code.launchpad.net/~addyess/charm-nagios/+git/charm-nagios/+merge/387617
Your team Nagios Charm developers is subscribed to branch charm-nagios:master.