A suggested way to organise the code inside the `charm.py` file.

charm
,
1

A few days ago, our dear @dnplas asked us in our Observability Practice channel in Matrix:

Hi folks! Happy Tuesday! Is there any recommendation for placing the MetricsEdnpointProvider constructor in the init method? Say before or after the self.framework.observe(...) calls?

After that there was an interesting exchange about how to organise the code within charm.py, but more specifically within the charm class itself.

About how to organise the code, there are as many opinions as developers. This is just the way I like to organise the code inside the charm file because I believe it is clean, and helps your future you (or another team member) to understand the code.

Table of Contents

Before the __init__ method

As a regular (executable) python file, charm.py will have at the top:

  • The shebang
  • A Copyright notice
  • A small docstring
  • All the imports
  • Constants definition
  • logger instantiation
  • Utility functions that are not part of the charm class.

For instance:

#!/usr/bin/env python3

# Copyright 2020 Canonical Ltd.
# See LICENSE file for licensing details.

"""A Juju charm for Prometheus on Kubernetes."""
import hashlib
import logging
import re
import socket
import subprocess
from pathlib import Path
from typing import Dict, Optional, cast
from urllib.parse import urlparse

import yaml
from charms.alertmanager_k8s.v1.alertmanager_dispatch import AlertmanagerConsumer

...

KEY_PATH = f"{PROMETHEUS_DIR}/server.key"
CERT_PATH = f"{PROMETHEUS_DIR}/server.cert"
CA_CERT_PATH = f"{PROMETHEUS_DIR}/ca.cert"
WEB_CONFIG_PATH = f"{PROMETHEUS_DIR}/prometheus-web-config.yml"

logger = logging.getLogger(__name__)


def sha256(hashable) -> str:
    """Use instead of the builtin hash() for repeatable values."""
    if isinstance(hashable, str):
        hashable = hashable.encode("utf-8")
    return hashlib.sha256(hashable).hexdigest()

The __init__ method

When writing a Charmed Operator, the __init__ method is really important since you define there the libraries the charm uses, and how the charm will behave when receives an event.

Despite of the fact you can place the __init__ method wherever you want inside the class, it is a good practice to place its definition at the top of it so it is the first thing you see at glance.

Inside the __init__ method

Inside the __init__ method a good way to have everything tided up is to instantiate the libraries first, and then define the events handler registration:

class MimirCoordinatorK8SOperatorCharm(ops.CharmBase):
    """The Mimir Coordinator charm class."""

    def __init__(self, *args: Any):
        super().__init__(*args)

        self._nginx_container = self.unit.get_container("nginx")
        self.server_cert = CertHandler(
            charm=self,
            key="mimir-server-cert",
            sans=[self.hostname],
        )
        self.s3_requirer = S3Requirer(self, S3_RELATION_NAME, BUCKET_NAME)
        ...

        ######################################
        # === EVENT HANDLER REGISTRATION === #
        ######################################
        self.framework.observe(self.on.config_changed, self._on_config_changed)
        self.framework.observe(self.on.collect_unit_status, self._on_collect_status)
        self.framework.observe(self.on.nginx_pebble_ready, self._on_nginx_pebble_ready)
        self.framework.observe(
            self.on.mimir_cluster_relation_changed, self._on_mimir_cluster_changed
        )

After the __init__ method

Now it is time to write the methods and properties of the class. For that I believe the following order will provide a clean organisation of the charm class:

  1. Event handler methods
  2. Properties
  3. Utility methods

Event handlers methods

class MimirCoordinatorK8SOperatorCharm(ops.CharmBase):
    ....

    ##########################
    # === EVENT HANDLERS === #
    ##########################

    def _on_config_changed(self, _: ops.ConfigChangedEvent):
        """Handle changed configuration."""
        s3_config_data = self._get_s3_storage_config()
        self.publish_config(s3_config_data)

    def _on_server_cert_changed(self, _):
        self._update_cert()
        self._ensure_pebble_layer()
        self.publish_config(tls=self._is_cert_available)

    def _on_mimir_cluster_changed(self, _):
        self._process_cluster_and_s3_credentials_changes()
        self.publish_config(tls=self._is_cert_available)

Properties

class MimirCoordinatorK8SOperatorCharm(ops.CharmBase):
    ....

    ##########################
    # === EVENT HANDLERS === #
    ##########################
    ....

    ######################
    # === PROPERTIES === #
    ######################

    @property
    def hostname(self) -> str:
        """Unit's hostname."""
        return socket.getfqdn()

    @property
    def _is_cert_available(self) -> bool:
        return (
            self.server_cert.enabled
            and (self.server_cert.server_cert is not None)
            and (self.server_cert.private_key is not None)
            and (self.server_cert.ca_cert is not None)
        )

    @property
    def mimir_worker_relations(self) -> List[ops.Relation]:
        """Returns the list of worker relations."""
        return self.model.relations.get("mimir_worker", [])

Utility methods

class MimirCoordinatorK8SOperatorCharm(ops.CharmBase):
    ....

    ##########################
    # === EVENT HANDLERS === #
    ##########################
    ....

    ######################
    # === PROPERTIES === #
    ######################

    ....

    ###########################
    # === UTILITY METHODS === #
    ###########################

    def has_multiple_workers(self) -> bool:
        """Return True if there are multiple workers forming the Mimir cluster."""
        mimir_cluster_relations = self.model.relations.get("mimir-cluster", [])
        remote_units_count = sum(
            len(relation.units)
            for relation in mimir_cluster_relations
            if relation.app != self.model.app
        )
        return remote_units_count > 1

    def publish_config(self, s3_config_data: Optional[_S3ConfigData] = None, tls: bool = False):
        """Generate config file and publish to all workers."""
        mimir_config = self.coordinator.build_config(
            s3_config_data=s3_config_data, tls_enabled=tls
        )
        self.cluster_provider.publish_configs(mimir_config)

        if tls:
            self.publish_grant_secrets()

Final notes

This kind of comment blocks in the code:

        ######################################
        # === EVENT HANDLER REGISTRATION === #
        ######################################

are not mandatory (well, I think almost everything is not mandatory), but helps you to understand at glance what those methods/properties does.

5 Likes
2

@tony-meyer @benhoyt perhaps something we can consider adding to the style guide?

3

I’m not sure. We want to encourage good practice, but we don’t want to tell you how to write your code in super detail.

In addition, some of this is not charm-specific, e.g.:

  • The shebang has to be first for it to work
  • Imports at the top is pep8
  • Location of the module docstring has to be where it is for it to be a docstring
  • pep8 doesn’t mandate it, but implies the positioning of constants
  • Putting __init__ at the start of a class is general Python practice (I don’t think I have ever seen someone put it anywhere else) even though I don’t think this is actually mandated anywhere
  • The top-level logger = logging.getLogger(__name__) I would also class as general Python style rather than charm style.

I would rather just refer to other documents (or tools) for general Python style.

Some of it I’m not overly convinced is best-practice:

  • I know it’s what happens with Canonical code, but I don’t think a copyright notice in every file is good practice, and wouldn’t want to encourage outside charmers to pick that up. If they already do this, then I would lump this in the “general Python organisation”, rather than being specific to charms.
  • I’d rather see the charm class before any utility functions, because when reading the code that’s probably what I’m most interested in. In general, I’d also expect that these are staticmethods if they are specific to the charm functionality or are in a separate helper module if they’re more generic.

Is there any particular benefit to this style? For example, as opposed to organising normal hook events first, then lifecycle ones, then lib ones, and setting up related attributes next to each? I think that if we are going to tell all charmers that they should follow an order here then we’d want to have good reason for it.

I find this interesting. Is this the common practice? (If no-one knows, I can take a look). I definitely agree that some organisation is good - I’m not entirely certain that it should be exactly this, but if that’s what people have settled on that would be strong evidence.

For what it’s worth, I find these very noisy. A plainer:

## Event handler registration

suffices, I feel (or a page break, if you’re Barry Warsaw :slight_smile:).

By the way, instead of:

class MyCharm(ops.CharmBase):
    def __init__(self, *args):
        super().__init__(*args)
        ...
        self.framework.observe(...)

@benhoyt and I lean towards:

class MyCharm(ops.CharmBase):
    def __init__(self, framework):
        super().__init__(framework)
        ...
        framework.observe(...)

When reading this, it’s obvious where framework comes from - you don’t need to know/intuit that a parent class is setting it as an attribute. Explicit is better than implicit :slight_smile:. Having *args provides some forward-compatibility, but realistically ops is not going to make changes that would force everyone to handle another argument (within 2.x), you probably want to adjust your code if there ever is a new one, and it’s not handling keyword arguments so is insufficient anyway.

2 Likes
4

This post, and suggestions are based in my personal preferences for writing and maintaining charms. For instance, about how we order the code inside __init__, from my point of view it is easier to read first what are the libs a charm need to instantiate, and then how the charm will behave to the events those libs emmit.

I do not think it is a common practice.

I like this!

2 Likes