The Catalogue K8s charm deploys a simple UI built with Vanilla Framework, allowing solution authors to include a “starting page” that links to all of the UIs available in the solution. The actual workload is something I put together myself to be included in COS, but the charm lends itself to inclusion in any solution.
Hi @0x12b ! I’d like to use this request (also others coming in) to try out some of the improvements Charm-Tech wants to make in 25.10 to address the issues people have raised with the review process. So the review will be a little different than the usual one – apologies for any inconvenience and thanks in advance for your presumed willingness to go with this . If it’s a problem let me know and we can switch back to the old system.
For 25.10 we’re going to use a smaller number of reviewers while we get a handle on the adjustments, so for this one I’ve asked @james-garner.
The checklist is pretty similar, but some duplicate/outdated bits have been removed and we’re trying to make it clearer what each item is referring to (fewer terms like “reasonable test suite” for example) and the balance between code review and higher level review. There’s not much that’s new (one example is a security doc, which I think we’re somewhat expecting these days anyway). All up for discussion, either specifically for this charm or in general, of course.
Also, the “charm maturity” list has been distributed across the charming documentation – we’re making a tool that collates it back into a single list, so @james-garner will check that too, and it’ll be available in a single place again hopefully soon (in the meantime, it’s all the “Best practice” boxes).
Hi @0x12b , I’m trying out our new review format on your charm – apologies if any of the expectations don’t align with what you’re used to from past reviews. Overall things look good, but some minimal steps to deploy and verify the charm is working would be great, and it looks like the charmhub.io page and docs could use a little bit of love. There also seems to be a divergence between main and the review PR, which lead to some confusion about the metadata and deps – see the full review below.
Checked
Review item
Objective
Review criteria
Intended functionality
The charm must work!
Charm does what it is meant to do. For charms that are easily used without specialised infrastructure, successfully completing the tutorial is sufficient. For more complex charms, the submitter should provide a (possibly prerecorded) demo.
A complete and consistent appearance of the charm is required for a quality impression of the charm collection.
The overall appearance looks good and the documentation looks reasonable.
✓
Release automation implementation
An implementation for automated releasing to charmhub.io improves the ability to provide updates covering vulnerabilities quickly.
Release automation for unstable channels to enable testing when new versions of the charm code or the workload become available.
Integration tests implemented
In particular for the review of charms, assuring a reasonable test suite is important to allow for automated releases in future. With this test, for every build, it is ensured that the integration with other charms is successful.
Integration 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) ending up in a success state. The integration tests are currently passing.
✓
Coding conventions
The source code of the charm is accessible in the sense of approachability. Consistent source code style and formatting are also considered a sign of being committed to quality.
The implemented checks for coding conventions are reasonable and implemented in the regular CI/CD implementation.
✓
Documentation for contributing
The documentation for contributing to the charm should be separate from the documentation for developing or using the charm.
URL to this documentation is present and resolves with 2xx.
✓
Licensing statement
For the charm shared, OSS or not, the licensing terms of the charm are clarified (which also implies an identified authorship of the charm).
URL to the ruling licensing statement is present and resolves with 2xx.
Metadata Links
A complete and consistent appearance of the charm is required.
charmcraft.yaml includes name, title, summary, and description fields that are not the default profile values, and a links field that contains documentation, issues, source, and contact fields, which resolve with 2xx.
Intended functionality – please provide some minimal steps to verify this. These could be worth adding to the charmhub page in the form of a quick start tutorial.
Charmhub.io charm detail page – very minimal. There’s some good information in charm/README.md that could be surfaced here. Usage documentation in particular would be nice.
Integration tests – “the charm can be integrated with at least one example for each ‘provides’ and ‘requires’ specified”. Missing tests for ingress and tracing integrations.
Metadata Links – missing ‘a links field that contains documentation, issues, source, and contact fields’ in the review PR. The metadata files in the review source branch are split, while in main there’s a single charmcraft.yaml which does have the links field. What’s going on there?
Best practices
Use absolute paths in subprocesses to prevent security issues.
Should update-ca-certificates use an absolute path then?
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.
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.
Charm specifies dependencies in requirements.txt
No lock file is committed (requirements.txt uses a mixture of exact versions, version ranges, and unpinned dependencies)
main correctly uses a pyproject.toml and uv.lock file
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.
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.
The quality assurance pipeline of a charm should be automated using a continuous integration (CI) system.
Write your charm to be stateless, where possible.
Charm doesn’t persist data in StoredState or
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.
For resources that are binary files, provide binaries for all the CPU architectures you intend to support.
Resource is a rock for amd64. charmcraft.yaml in PR doesn’t specify architecture (charmcraft.yaml in main does though)
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.
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.
Documentation links aren’t present currently. However, I don’t think this criteria applies here, as the application is bespoke to the charm, so document away.
charm name: 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. Include the -k8s suffix on all charms that run on a Kubernetes cloud, unless the charm has no workload or you know that there will never be a machine version of the charm. Don’t include an organisation or publisher in the name. Don’t add an operator or charm prefix or suffix.
What to do with testing tracing came up in a Charm-Tech discussion earlier this week (after James had done his review). It looks like our recommendation is going to be that charms don’t add tests that verify that the tracing ops[tracing] provides works (because that’s something for ops to verify), and only verify any additional spans, events, etc that the charm adds. The intention is to add support in Scenario^w ops[testing] to make that easier, and maybe also provide a basic tracing app that can be used in integration tests.
I’ll update the draft updated listing doc to clarify this.
Of course, this charm is currently using the charm_tracing lib and not ops[tracing]. However, I’d say that there’s no point adding integration tests for tracing at this point, and if it moves to ops[tracing] in the future then only partial as noted above. So I think we ignore this point for the listing request.
It does seem like it would be good to have a test that integrating with an ingress works, though, unless there’s some reason not to do that.
Some minimal steps (or a demo) to verify the “intended functionality”. @james-garner’s suggestion is that a small quick start tutorial would be a nice way of doing this, but the requirement is just that the reviewer can see that the charm does what it says it does. If there isn’t some sort of tutorial, let’s at least have the usage information from the README on the Charmhub landing page.
An integration test that verifies that the charm works when a relation providing the ingress interface is provided.
Charming best practice is to use a full path when running subprocesses. Can we do that for update-ca-certificates? If not, can you explain why not?
I think the other issues @james-garner found are already fixed in main.
I tried changing update-ca-certificates to /usr/sbin/update-ca-certificates, but for some reason tests are failing; since this would probably require some investigation (which I doubt would be prioritized anytime soon), could we have a temporary exception for this charm? There are a few quirks here:
this charm is one of the very few that doesn’t use a rock yet
usually we run update-ca-certificates in the workload container, but here it seems to be running in the charm-container?
. If it’s a problem let me know and we can switch back to the old system.