Hello,
the powerflex-cinder charm allows cinder to manage volumes with a Dell PowerFlex storage system as the backend. From configuration to operations, it enables the end user to automate the deployment and the usage of Cinder with Dell PowerFlex.
We would like to have this charm visible and searchable from Charmhub.
@tigarmo can you please help with this review? You can go through and
tick off the items on the checklist
and post the result in this thread. This prior listing request could serve as an example of how to go about it.
Please ping me or anyone else from Charm-Tech for any questions.
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.
x
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.
x
Installation test results
Availability of test results is mandatory for a working collaborative project.
URL to test results from CI/CD automation.
x
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.
x
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.
I assume I can’t just deploy this on my local Juju and have it work. Is there a demo video or something similar available so that I can tick off the first item?
Issues:
In the resource description, could “Debian” please be capitalised?
On the Charmhub page, there are no documentation links. Could we add some?
On the Charmhub page, the publisher name is “dell openstack” - I assume there should be some capitalisation here.
Could we add some contact details (this could just be an issue tracker) to the Charmhub page?
The repo README has a broken link (“Juju Documentation” in the Configuration section).
There’s an “installation test” (the one integration test) but it’s not run in CI. Could that be added? (The test itself also looks like it’s just the profile one, so needs to be updated with the proper resource information).
There are no other integration tests. Can a couple be added? Ones that cover the Juju integrations are ideal.
In terms of the “coding conventions” item, there’s linting and I don’t think anything is a blocker. But here are some suggestions:
Use the unified charmcraft.yaml rather than separate metadata.yaml, config.yaml, and actions.yaml files.
Pin your requirements. Ideally, this would be by having them listed in pyproject.toml (either not pinned or pinned to major versions) and then having the requirements.txt file be a lock file (or have a uv.lock if preferred)
Ideally, the unit test would use the new framework rather than Harness. Please reach out to Charm-Tech (e.g. in Charm Development in Matrix) if you’re ready to do this and need any help!
Rather than “from ops import model”, we suggest using “import ops” and “ops.model” - but actually here you should be doing “import ops” and “ops.BlockedStatus”, not getting it from model.
Don’t import from ops.main. Ideally, you just do “import ops” and “ops.main(…)” but if you really want to do “from” imports, you can do “from ops import main” and “main(…)”.
The __init__ method doesn’t do anything so can be removed.
There’s a password in the config. Could that be moved to a Juju user secret?
Logging shouldn’t use .format(). Use %s and pass the arguments.
(1) In the resource description, could “Debian” please be capitalised?
(2) On the Charmhub page, there are no documentation links. Could we add some?
(3) On the Charmhub page, the publisher name is “dell openstack” - I assume there should be some capitalisation here.
(4) Could we add some contact details (this could just be an issue tracker) to the Charmhub page?
(5) The repo README has a broken link (“Juju Documentation” in the Configuration section).
(6) There’s an “installation test” (the one integration test) but it’s not run in CI. Could that be added? (The test itself also looks like it’s just the profile one, so needs to be updated with the proper resource information).
(7) There are no other integration tests. Can a couple be added? Ones that cover the Juju integrations are ideal.
Sure: generally you just need to add to your charmcraft.yaml or metadata.yaml file - particularly the links section and then Charmhub will take care of displaying everything for you.
For 3, I’m not actually sure - it’s not in the metadata, so I assume it must be in the Charmhub profile? If you log in to Charmhub and look at your list of published charms, can you change it there? If not, we should reach out to the Charmhub/Store team and they’ll be able to explain how to do it.
For 2 and 4, here’s the postgresql-k8s file as an example. You don’t have to have as many as they do or exactly the same set of links, but something along those lines would be great.
I’ve opened a pull request with a fix. If you want the link to go to a different destination feel free to just reject the PR and fix it however you’d like (but at least it should be clear now where the issue is).
