Hockeypuck K8s operator is a Juju12-factorcharm deploying and managing Hockeypuck on Kubernetes. Hockeypuck is an OpenPGP public keyserver tool used to manage public key infrastructure for PGP (Pretty Good Privacy). PGP is a system for securing communication through encryption and digital signatures.
A complete and consistent appearance of the charm is required for a quality impression of the charm collection.
The overall appearance looks good, which means: * The name complies with the naming guidelines. * The publisher is identified. * The links are provided. * The documentation looks reasonable.
âś“
Source repository
Generally, the source code for charms must be accessible by the community for transparency and collaboration.
It is not entirely mandatory to have the charm published as OSS for review, but the repository must be accessible from the persons working on the review request.
âś“
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.
âś“
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.
âś“
Unit tests implementation
In particular, for the charms review, assuring a reasonable test suite is essential to allow for automated releases in future.
The unit tests show relevant coverage. It is a case-dependent review.At the time of review, the test runs successfully.
âś“
Unit tests results
Availability of test results is mandatory for a working collaborative project.
URL to test results from CI/CD automation.
âś“
Installation test implemented (could be part of the integration test)
In particular, for the charm review, assuring a reasonable test suite is essential to allow for automated releases in future. With this test, for every build, it is ensured that the installation is successful.
An implementation for checking the installation is present. The implementation should also check for successful installation as part of the automation, and the workload behaves as expected. At the time of review, the test runs successfully.
âś“
Installation test results
Availability of test results is mandatory for a working collaborative project.
URL to test results from CI/CD automation.
âś“
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.
An implementation for testing the required integrations (if applicable) is present. The implementation should also check for successful integration as part of the automation and the workload behaves as expected. At the time of review, the test runs successfully.
âś“
Integration test results
Availability of test results is mandatory for a working collaborative project.
URL to test results from CI/CD automation.
âś“
Documentation for usage
The documentation for using the charm should be separate from the documentation for developing or contributing to the charm.
URL to this documentation is present.
âś“
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.
âś“
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.
A couple of issues, neither of which blocks listing:
There’s a link in the landing CharmHub page to https://juju.is/docs/olm/charmed-operators - this is a 404 after last cycle’s doc restructuring, so needs to be updated.
The “Metrics” heading in the “Reference” section in the sidebar seems to have two # by mistake (double markdown headings or something?)
Some recommendations for improvements, which again aren’t blockers for listing:
The app secret key in the config would be better as a Juju secret, unless there’s some reason that can’t be done that I’m missing. I presume this really needs to be done in the paas-charm base, though.
Ideally, new charms use the newer ops[testing] framework (“Scenario”) for unit tests rather than Harness. It’d be great to switch over at some point, and also if new tests start out that way. If there’s some sort of blocker, then please ask about that in Charm Development in Matrix.
The timing was obviously wrong, since 1.0 only came out a few weeks ago, but ideally the integration tests would move from pytest-operator/python-libjuju to Jubilant at some point.
This is probably also a template thing, but we’re trying to standardise the dev commands for formatting, linting, and so on. The spec is not entirely approved yet, but is mostly there. The main changes would be “tox -e format” rather than “tox -e fmt” and what “lint” and “static” run (there’s still a bit of discussion about this happening, so this is mostly just something to be aware of for later charms or for tweaks here later).
The unit test coverage is low (except for the traefik route observer), although the 12-factor functionality is tested there. It feels like there could easily be unit tests for the actions, which would increase the coverage, but the integration tests are checking things, and I understand that many charmers prefer to rely on integration tests for the most part.
I just need to run through the tutorial as part of the “intended functionality” part, and the best practices list, but looking good. I should get that done in the next 24 hours.
In the tutorial, there’s a command juju deploy hockeypuck-k8s --channel=2.2/edge --config metrics-port=9626 app-port=11371 - that should be juju deploy hockeypuck-k8s --channel=2.2/edge --config metrics-port=9626 --config app-port=11371 so that the second config option is recognised as config, otherwise Juju thinks it should be the app name.
Other notes, all non-blocking:
It would be good to move from the charm plug-in to uv or at least python. Or at minimum have strict dependencies on. But seems like this comes from paas-charm, so probably needs to be done upstream.
def install_gnupg(self, _: ops.ActionEvent) - this shouldn’t be ActionEvent, it should be ops.EventBase or ops.InstallEvent | ops.UpgradeEvent
In charmcraft.yaml, please add optional: true to the traefik-route relation - this is a newer request from SolQA, because they want to be explicit that you have considered the optionality and it is optional, rather than just haven’t thought about it and forgot to put in optional: false.
@swetha1654 thanks for the submission and building the charm! Please do review the suggestions above, particularly the broken links and broken tutorial command.
