The Charmed Apache Spark History Server K8s Operator is part of the Charmed Apache Spark solution. Please refer to the solution docs for more information about how to deploy it and configure it (see here and here). More information about how the Spark History server fit in the Charmed Spark solution can be found in the explanation section.
FYI, I’m handling this review. I’m using the review (and other ones that are coming in early this cycle) to help adjust the review process to address issues that have been raised (particularly around consistency).
I’ll have the review posted tomorrow (Friday NZ time). Apologies for not being quicker with it - the guideline in the draft new process is 3 days and I’ve missed that (because I was trying to get some additional automation done).
Sorry, trying to do this once I’d finished the automation part was a bad decision, because delays on that are ok, but I shouldn’t have delayed this.
I need to finish working through the tutorial for the solution so I can tick of the last (first) box, but otherwise it looks good, with just a few suggestions, listed below. I’ll try to finish doing the tutorial this evening.
Really nice organisation/structure for the charm!
Great to see Jubilant tests! Please let us know if you had any problems there or have suggestions for improvements.
Suggestions (in order of importance):
I couldn’t see anything that’s tracking security issues in dependencies (Dependabot or Renovate or similar). That would be good to add - I assume your team must have some sort of typical process for this.
Ideally, there’d be a SECURITY.md file that covers which versions (tracks?) get security fixes, how to report a security issue, etc (basically as per the SSDLC specs).
It would be good if charmcraft.yaml had a links field that had at least issues, source, and contact fields.
There are a small number of logging calls that use f-strings. Ideally, you let the logger (formatter) handle this, using old-school percentage formatting. None of these cases seem problematic, so it’s probably fine to leave them as-is, but you might consider changing them. I wonder if perhaps t-strings are going to properly solve this in future Python…
There are a couple of logging calls that are just saying what event is being handled. Juju already logs that, so ideally that doesn’t happen, unless it’s because you’re often filtering the logs in a way that would exclude those.
There’s a subprocess call to chmod - we recommend using a full path (/usr/bin/chmod I assume) to avoid issues if someone malicious has somehow manipulated the PATH.
AbstractWorkload.exec takes a string command and splits it. It seems safe how it’s being used, but I don’t love that as a design pattern - it’s easy to accidentally misuse it and allow shell injection. I’d suggest that in some future version the caller has to pass in a list instead.
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 each ‘provides’ and ‘requires’ specified 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, which resolve with 2xx.
Best practices:
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.
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.
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.
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.
charm name: The name should be slug-oriented (ASCII lowercase letters, numbers, and hyphens) and follow the pattern <workload name in full>[<function>][-k8s].
Config: Prefer lowercase alphanumeric names, separated with dashes if required.
requires/provides in charmcraft.yaml: 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.
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.
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.
.