Hi @0x12b ! Apologies for the delay here - caught up with some other things this week, but Iâll make sure this is assigned by Monday at the latest and we get the review completed next week.
(More differentiating text in this paragraph to be ignored).
The charm does what it is meant to do, per the demo or tutorial. *Note: I skipped doing this specifically for the review, but this charm is part of the Ops K8s Tutorial, so I and many others in Charm Tech have validated it regularly).
Integration tests exist, are run on every change to the default branch, and are passing. At minimum, the tests verify that the charm can be deployed and ends up in a success state, and that the charm can be integrated with at least one example for each âprovidesâ and ârequiresâ specified (including optional, excluding tracing) ending up in a success state.
If the charm provides an interface library, the libraryâs module docstring must contain the following information:
the interface(s) this library is for, and if it takes care of one or both of the providing/requiring sides
guidance on how to start when using the library to implement their end of the interface
If the charm provides a general library, the libraryâs module docstring must contain the following information:
the purpose of the library
the intended audience for the library: is this library intended for use only by the charm or the charming team, or is it a public library intended for anyone to use in their charm?
guidance on how to start using the library
Best practices
The following best practices are recommended for all charms, and are also
required for listing.
Use absolute paths in subprocesses to prevent security issues.
Execute processes directly rather than via the shell.
Name the repository using the pattern <charm name>-operator for a single charm, or <base charm name>-operators when the repository will hold multiple related charms. For the charm name, see {external+charmcraft:ref}Charmcraft | Specify a name <specify-a-name>.
Set the requires-python version in your pyproject.toml so that tooling will detect any use of Python features not available in the versions you support.
When using the charm plugin with charmcraft, ensure that you set strict dependencies to true. For example:
Ensure that tooling is configured to automatically detect new versions, particularly security releases, for all your dependencies.
Ensure that the pyproject.tomland the lock file are committed to version control, so that exact versions of charms can be reproduced.
Capture output to stdout and stderr in your charm and use the logging and warning functionality to send messages to the charm user, rather than rely on Juju capturing output. In particular, you should avoid print() calls, and ensure that any subprocess calls capture output.
Do not build log strings yourself: allow the logger to do this for you as required. That is:
Ensure that log messages are clear, meaningful, and provide enough information for the user to troubleshoot any issues. Avoid spurious logging. For instance, try not to log when event handlers are called, as the Juju controller does this automatically.
Never log credentials or other sensitive information.
All charms should provide the commands configured by the Charmcraft profile, to allow easy testing across the charm ecosystem. Itâs fine to tweak the configuration of individual tools, or to add additional commands, but keep the command names and meanings that the profile provides.
Write your charm to be stateless, where possible.
Libraries should never mutate the status of a unit or application. Instead, use return values, or raise exceptions and let them bubble back up to the charm for the charm author to handle as they see fit.
If youâre setting up a git repository: name it using the pattern <charm name>-operator. For the charm name, see :ref:specify-a-name.
For resources that are binary files, provide binaries for all the CPU architectures you intend to support.
Prefer lowercase alphanumeric names, and use hyphens (-) to separate words. For charms that have already standardised on underscores, it is not necessary to change them, and it is better to be consistent within a charm then to have some action names be dashed and some be underscored.
Just like Juju, a charm is an opinionated tool. Configure the application with the best defaults (ideally the application is deployable without providing any configuration at deploy time), and only expose application configuration options when necessary.
Prefer lowercase alphanumeric names, separated with dashes if required. For charms that have already standardised on underscores, it is not necessary to change them, and it is better to be consistent within a charm then to have some config names be dashed and some be underscored.
Documentation links should apply to the charm, and not to the application that is being charmed. Assume that the user already has basic competency in the use of the application.
The name should be slug-oriented (ASCII lowercase letters, numbers, and hyphens) and follow the pattern <workload name in full>[<function>][-k8s]. For example, argo-server-k8s.
Always include the optional key, rather than relying on the default value to indicate that the relation is required. Although this field is not enforced by Juju, including it makes it clear to users (and other tools) whether the relation is required.
Non-blocking notes:
update-ca-certificates is run without an absolute path - weâve made exceptions for this already with other charms.
Is something ensuring that youâre alerted when a dependency has a security update? I couldnât see an explicit Dependabot config, or something like Renovate.
Weâre trying to standardise the tox command names, and part of that is âformatâ rather than âfmtâ. This is the one least likely to be used in bulk, so not a big deal, but it would be nice to change it.
We prefer dashed config names rather than underscored ones, but I assume the charm has been around too long to change that now (particularly since thereâs already a deprecated config option).
I have listed the charm, but charms will not show up unless they are defaulted to a channel that has a stable release.
I did some investigation and your two options would be to change the default track to 1 (which I can do). Option two would be to release a revision to latest/stable.
