wordpress-charmers team mailing list archive

[Tom Haddon <tom.haddon@xxxxxxxxxxxxx>]

Some inline comments, and you have a test failure in test_is_config_valid.

Haven't tested yet, but will do so shortly.

Diff comments:

> diff --git a/src/charm.py b/src/charm.py
> index edade47..efe9026 100755
> --- a/src/charm.py
> +++ b/src/charm.py
> @@ -115,20 +117,20 @@ class WordpressCharm(CharmBase):
>          self.framework.observe(self.on.update_status, self.on_config_changed)
>          self.framework.observe(self.on.wordpress_initialise, self.on_wordpress_initialise)
>  
> +        self.db = MySQLClient(self, 'db')
> +        self.framework.observe(self.on.db_relation_created, self.on_db_relation_created)
> +        self.framework.observe(self.on.db_relation_broken, self.on_db_relation_broken)
> +        self.framework.observe(self.db.on.database_changed, self.on_database_changed)
> +
> +        c = self.model.config

Might be worth using "config" rather that "c" here as you do later in the same file.

>          self.state.set_default(
> -            initialised=False, valid=False,
> +            initialised=False, valid=False, has_db_relation=False,
> +            db_host=c["db_host"], db_name=c["db_name"], db_user=c["db_user"], db_password=c["db_password"]
>          )
> -
> -        self.wordpress = Wordpress(self.model.config)
> +        self.wordpress = Wordpress(c)
>  
>      def on_config_changed(self, event):
> -        is_valid = self.is_valid_config()
> -        if not is_valid:
> -            return
> -
> -        self.configure_pod()
> -        if not self.state.initialised:
> -            self.on.wordpress_initialise.emit()
> +        self.config_changed()
>  
>      def on_wordpress_initialise(self, event):
>          wordpress_needs_configuring = False
> @@ -161,6 +163,40 @@ class WordpressCharm(CharmBase):
>              logger.info("Wordpress workload pod is ready and configured")
>              self.model.unit.status = ActiveStatus()
>  
> +    def on_db_relation_created(self, event):

Can you add docstrings for this and other functions you're adding? I realise not all of them in this charm have them, but it'd be good to make sure the ones we're adding do have them.

> +        self.state.has_db_relation = True
> +        self.state.db_host = None
> +        self.state.db_name = None
> +        self.state.db_user = None
> +        self.state.db_password = None
> +        self.config_changed()
> +
> +    def on_db_relation_broken(self, event):
> +        self.state.has_db_relation = False
> +        self.config_changed()
> +
> +    def on_database_changed(self, event):
> +        self.state.db_host = event.host
> +        self.state.db_name = event.database
> +        self.state.db_user = event.user
> +        self.state.db_password = event.password
> +        self.config_changed()
> +
> +    def config_changed(self):
> +        if not self.state.has_db_relation:
> +            self.state.db_host = self.model.config["db_host"] or None
> +            self.state.db_name = self.model.config["db_name"] or None
> +            self.state.db_user = self.model.config["db_user"] or None
> +            self.state.db_password = self.model.config["db_password"] or None
> +
> +        is_valid = self.is_valid_config()
> +        if not is_valid:
> +            return
> +
> +        self.configure_pod()
> +        if not self.state.initialised:
> +            self.on.wordpress_initialise.emit()
> +
>      def configure_pod(self):
>          # Only the leader can set_spec().
>          if self.model.unit.is_leader():
> diff --git a/src/mysql.py b/src/mysql.py
> new file mode 100644
> index 0000000..2cbab87
> --- /dev/null
> +++ b/src/mysql.py
> @@ -0,0 +1,166 @@
> +"""
> +MySQL endpoing implementation for the Operator Framework.

endpoint

> +
> +Ported to the Operator Framework from the canonical-osm Reactive
> +charms at https://git.launchpad.net/canonical-osm
> +"""
> +
> +import logging
> +
> +import ops.charm
> +import ops.framework
> +import ops.model
> +
> +
> +__all__ = ["MySQLClient", "MySQLClientEvents", "MySQLRelationEvent", "MySQLDatabaseChangedEvent"]
> +
> +
> +class _MySQLConnectionDetails(object):
> +    database: str = None
> +    host: str = None
> +    port: int = 3306
> +    user: str = None
> +    password: str = None
> +    root_password: str = None
> +    connection_string: str = None
> +    sanitized_connection_string: str = None  # With no secrets, for logging.
> +    is_available: bool = False
> +
> +    def __init__(self, relation: ops.model.Relation, unit: ops.model.Unit):
> +        reldata = relation.data.get(unit, {})
> +        self.database = reldata.get("database", None)
> +        self.host = reldata.get("host", None)
> +        self.port = int(reldata.get("port", 3306))
> +        self.user = reldata.get("user", None)
> +        self.password = reldata.get("password", None)
> +        self.root_password = reldata.get("root_password", None)
> +
> +        if all([self.database, self.host, self.port, self.user, self.password, self.root_password]):
> +            self.sanitized_connection_string = (
> +                f"host={self.host} port={self.port} dbname={self.database} user={self.user}"
> +            )
> +            self.connection_string = (
> +                self.sanitized_connection_string + f" password={self.password} root_password={self.root_password}"
> +            )
> +        else:
> +            self.sanitized_connection_string = None
> +            self.connection_string = None
> +        self.is_available = self.connection_string is not None
> +
> +
> +class MySQLRelationEvent(ops.charm.RelationEvent):
> +    def __init__(self, *args, **kw):
> +        super().__init__(*args, **kw)
> +        self._conn = _MySQLConnectionDetails(self.relation, self.unit)
> +
> +    @property
> +    def is_available(self) -> bool:
> +        """True if the database is available for use."""
> +        return self._conn.is_available
> +
> +    @property
> +    def connection_string(self) -> str:
> +        """The connection string, if available, or None.
> +
> +        The connection string will be in the format:
> +
> +            'host={host} port={port} dbname={database} user={user} password={password} root_password={root_password}'
> +        """
> +        return self._conn.connection_string
> +
> +    @property
> +    def database(self) -> str:
> +        """The name of the provided database, or None."""
> +        return self._conn.database
> +
> +    @property
> +    def host(self) -> str:
> +        """The host for the provided database, or None."""
> +        return self._conn.host
> +
> +    @property
> +    def port(self) -> int:
> +        """The port to the provided database."""
> +        # If not available, returns the default port of 3306.
> +        return self._conn.port
> +
> +    @property
> +    def user(self) -> str:
> +        """The username for the provided database, or None."""
> +        return self._conn.user
> +
> +    @property
> +    def password(self) -> str:
> +        """The password for the provided database, or None."""
> +        return self._conn.password
> +
> +    @property
> +    def root_password(self) -> str:
> +        """The password for the root user, or None."""
> +        return self._conn.root_password
> +
> +    def restore(self, snapshot) -> None:
> +        super().restore(snapshot)
> +        self._conn = _MySQLConnectionDetails(self.relation, self.unit)
> +
> +
> +class MySQLDatabaseChangedEvent(MySQLRelationEvent):
> +    """The database connection details on the relation have changed.
> +
> +    This event is emitted when the database first becomes available
> +    for use, when the connection details have changed, and when it
> +    becomes unavailable.
> +    """
> +
> +    pass
> +
> +
> +class MySQLClientEvents(ops.framework.ObjectEvents):
> +    database_changed = ops.framework.EventSource(MySQLDatabaseChangedEvent)
> +
> +
> +class MySQLClient(ops.framework.Object):
> +    """Requires side of a MySQL Endpoint"""
> +
> +    on = MySQLClientEvents()
> +    _state = ops.framework.StoredState()
> +
> +    relation_name: str = None
> +    log: logging.Logger = None
> +
> +    def __init__(self, charm: ops.charm.CharmBase, relation_name: str):
> +        super().__init__(charm, relation_name)
> +
> +        self.relation_name = relation_name
> +        self.log = logging.getLogger("mysql.client.{}".format(relation_name))
> +        self._state.set_default(rels={})
> +
> +        self.framework.observe(charm.on[relation_name].relation_changed, self._on_changed)
> +        self.framework.observe(charm.on[relation_name].relation_broken, self._on_broken)
> +
> +    def _on_changed(self, event: ops.charm.RelationEvent) -> None:
> +        if event.unit is None:
> +            return  # Ignore application relation data events.
> +
> +        prev_conn_str = self._state.rels.get(event.relation.id, None)
> +        new_cd = _MySQLConnectionDetails(event.relation, event.unit)
> +        new_conn_str = new_cd.connection_string
> +
> +        if prev_conn_str != new_conn_str:
> +            self._state.rels[event.relation.id] = new_conn_str
> +            if new_conn_str is None:
> +                self.log.info(f"Database on relation {event.relation.id} is no longer available.")
> +            else:
> +                self.log.info(
> +                    f"Database on relation {event.relation.id} available at {new_cd.sanitized_connection_string}."
> +                )
> +            self.on.database_changed.emit(relation=event.relation, app=event.app, unit=event.unit)
> +
> +    def _on_broken(self, event: ops.charm.RelationEvent) -> None:
> +        self.log.info(f"Database relation {event.relation.id} is gone.")
> +        prev_conn_str = self._state.rels.get(event.relation.id, None)
> +        if event.relation.id in self._state.rels:
> +            del self._state.rels[event.relation.id]
> +        if prev_conn_str is None:
> +            return
> +        self.on.database_changed.emit(relation=event.relation, app=event.app, unit=None)


-- 
https://code.launchpad.net/~stub/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/395826
Your team Wordpress Charmers is requested to review the proposed merge of ~stub/charm-k8s-wordpress:mysql-relation into charm-k8s-wordpress:master.