bind-charmers team mailing list archive

[Facundo Batista <facundo@xxxxxxxxxxxxx>]

Review: Needs Fixing

Still a couple of details to take care of. 

PLEASE respond my comments with what you think of them (even a "ok" is enough, if you're ok with the suggestion), thanks.

Diff comments:

> diff --git a/src/charm.py b/src/charm.py
> index 640fea5..a13da4c 100755
> --- a/src/charm.py
> +++ b/src/charm.py
> @@ -1,38 +1,140 @@
>  #!/usr/bin/env python3
> -# Copyright 2020 Tom Haddon
> -# See LICENSE file for licensing details.
>  
> -import logging
> +# Copyright 2020 Canonical Ltd.
> +# Licensed under the GPLv3, see LICENCE file for details.
>  
> +import io
> +import logging
>  from ops.charm import CharmBase
>  from ops.main import main
> -from ops.framework import StoredState
> +from ops.model import ActiveStatus, MaintenanceStatus
> +from pprint import pformat
> +from yaml import safe_load
>  
> -logger = logging.getLogger(__name__)
> +logger = logging.getLogger()
>  
> +REQUIRED_SETTINGS = ['bind_image_path']
>  
> -class CharmcraftReviewCharm(CharmBase):
> -    _stored = StoredState()
>  
> +class BindK8sCharm(CharmBase):
>      def __init__(self, *args):
> +        """Initialise our class, we only care about 'start' and 'config-changed' hooks."""
>          super().__init__(*args)
> -        self.framework.observe(self.on.config_changed, self._on_config_changed)
> -        self.framework.observe(self.on.fortune_action, self._on_fortune_action)
> -        self._stored.set_default(things=[])
> -
> -    def _on_config_changed(self, _):
> -        current = self.model.config["thing"]
> -        if current not in self._stored.things:
> -            logger.debug("found a new thing: %r", current)
> -            self._stored.things.append(current)
> -
> -    def _on_fortune_action(self, event):
> -        fail = event.params["fail"]
> -        if fail:
> -            event.fail(fail)
> +        self.framework.observe(self.on.start, self.on_config_changed)
> +        self.framework.observe(self.on.config_changed, self.on_config_changed)
> +
> +    def _check_for_config_problems(self):
> +        """Return a string describing any configuration problems (or an empty string if none)."""
> +        problems = ''
> +
> +        missing = self._missing_charm_settings()
> +        if missing:
> +            problems += 'required setting(s) empty: {}'.format(', '.join(sorted(missing))))

What's the point on adding to an empty list? just do `problems = ...`

> +
> +        return problems
> +
> +    def _missing_charm_settings(self):
> +        """Return a list of missing configuration settings (or an empty list if none)."""
> +        config = self.model.config
> +
> +        missing = [setting for setting in REQUIRED_SETTINGS if not config[setting]]
> +
> +        if config['bind_image_username'] and not config['bind_image_password']:
> +            missing.append('bind_image_password')
> +
> +        return sorted(set(missing))

Two things here:
- Don't convert missing to a set, just use a set from the start! (do a set comprehension above in line 362, for example)
- You're sorting here, and then sorting again when using this returned value

> +
> +    def on_config_changed(self, event):
> +        """Check that we're leader, and if so, set up the pod."""
> +        if self.model.unit.is_leader():
> +            # Only the leader can set_spec().
> +            resources = self.make_pod_resources()
> +            spec = self.make_pod_spec()
> +            spec.update(resources)
> +
> +            msg = "Configuring pod"
> +            logger.info(msg)
> +            self.model.unit.status = MaintenanceStatus(msg)
> +            self.model.pod.set_spec(spec)
> +
> +            msg = "Pod configured"
> +            logger.info(msg)
> +            self.model.unit.status = ActiveStatus(msg)
> +        else:
> +            logger.info("Spec changes ignored by non-leader")
> +            self.model.unit.status = ActiveStatus()
> +
> +    def make_pod_resources(self):
> +        """Compile and return our pod resources (e.g. ingresses)."""
> +        # LP#1889746: We need to define a manual ingress here to work around LP#1889703.
> +        resources = {}  # TODO
> +        logger.info("This is the Kubernetes Pod resources <<EOM\n{}\nEOM".format(pformat(resources)))
> +        return resources
> +
> +    def generate_pod_config(self, secured=True):
> +        """Kubernetes pod config generator.
> +
> +        generate_pod_config generates Kubernetes deployment config.
> +        If the secured keyword is set then it will return a sanitised copy
> +        without exposing secrets.
> +        """
> +        config = self.model.config
> +        pod_config = {}
> +        if config["container_config"].strip():
> +            pod_config = safe_load(config["container_config"])
> +
> +        if config["custom_config_repo"].strip():
> +            pod_config["CUSTOM_CONFIG_REPO"] = config["custom_config_repo"]
> +
> +        if config["https_proxy"].strip():
> +            pod_config["http_proxy"] = config["https_proxy"]
> +            pod_config["https_proxy"] = config["https_proxy"]
> +
> +        if secured:
> +            return pod_config
> +
> +        if config["container_secrets"].strip():
> +            container_secrets = safe_load(config["container_secrets"])
>          else:
> -            event.set_results({"fortune": "A bug in the code is worth two in the documentation."})
> +            container_secrets = {}
> +
> +        pod_config.update(container_secrets)
> +        return pod_config
> +
> +    def make_pod_spec(self):
> +        """Set up and return our full pod spec here."""
> +        config = self.model.config
> +        full_pod_config = self.generate_pod_config(secured=False)
> +        secure_pod_config = self.generate_pod_config(secured=True)
> +
> +        ports = [
> +            {"name": "domain-tcp", "containerPort": 53, "protocol": "TCP"},
> +            {"name": "domain-udp", "containerPort": 53, "protocol": "UDP"},
> +        ]
> +
> +        spec = {
> +            "version": 2,
> +            "containers": [
> +                {
> +                    "name": self.app.name,
> +                    "imageDetails": {"imagePath": config["bind_image_path"]},
> +                    "ports": ports,
> +                    "config": secure_pod_config,
> +                    "kubernetes": {"readinessProbe": {"exec": {"command": ["/usr/local/bin/dns-check.sh"]}}},
> +                }
> +            ],
> +        }
> +
> +        logger.info("This is the Kubernetes Pod spec config (sans secrets) <<EOM\n{}\nEOM".format(pformat(spec)))
> +
> +        if config.get("bind_image_username") and config.get("bind_image_password"):
> +            spec.get("containers")[0].get("imageDetails")["username"] = config["bind_image_username"]
> +            spec.get("containers")[0].get("imageDetails")["password"] = config["bind_image_password"]
> +
> +        secure_pod_config.update(full_pod_config)
> +
> +        return spec
>  
>  
>  if __name__ == "__main__":
> -    main(CharmcraftReviewCharm)
> +    main(BindK8sCharm)
> diff --git a/tox.ini b/tox.ini
> new file mode 100644
> index 0000000..7cf3297
> --- /dev/null
> +++ b/tox.ini
> @@ -0,0 +1,48 @@
> +[tox]
> +skipsdist=True
> +envlist = unit, functional
> +
> +[testenv]
> +basepython = python3
> +setenv =
> +  PYTHONPATH = {toxinidir}/build/lib:{toxinidir}/build/venv
> +
> +[testenv:unit]
> +commands =
> +    pytest --ignore mod --ignore {toxinidir}/tests/functional \
> +      {posargs:-v  --cov=src --cov-report=term-missing --cov-branch}
> +deps = -r{toxinidir}/tests/unit/requirements.txt
> +       -r{toxinidir}/requirements.txt
> +setenv =
> +  PYTHONPATH={toxinidir}/src:{toxinidir}/build/lib:{toxinidir}/build/venv
> +  TZ=UTC
> +
> +[testenv:functional]
> +passenv =
> +  HOME
> +  JUJU_REPOSITORY
> +  PATH
> +commands =
> +	pytest -v --ignore mod --ignore {toxinidir}/tests/unit {posargs}
> +deps = -r{toxinidir}/tests/functional/requirements.txt
> +       -r{toxinidir}/requirements.txt
> +
> +[testenv:black]
> +commands = black --skip-string-normalization --line-length=120 src/ tests/
> +deps = black
> +
> +[testenv:lint]
> +commands = flake8 src/ tests/
> +# Pin flake8 to 3.7.9 to match focal
> +deps =
> +    flake8==3.7.9
> +
> +[flake8]
> +exclude =
> +    .git,
> +    __pycache__,
> +    .tox,
> +# Ignore E231 because using black creates errors with this https://github.com/psf/black/issues/1289

But that issue is closed. Which `black` version are you using, from where?

> +ignore = E231
> +max-line-length = 120
> +max-complexity = 10


-- 
https://code.launchpad.net/~bind-charmers/charm-k8s-bind/+git/charmcraft-review/+merge/389995
Your team Bind Charmers is subscribed to branch ~bind-charmers/charm-k8s-bind/+git/charmcraft-review:master.