@dylanstathis 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 in Charm-Tech for any questions.
@jproque15130 I will be starting the review tomorrow. One of the steps is a video call where you can explain the charm to me (does not need to be tomorrow). Please send me a message with your availability.
I am unclear on the release process. There does not seem to be any automation in place to release to unstable channels. According to the approval criteria, the charm needs to be released to an unstable channel before stable.
The charm does not clean up after itself. If the charm is removed, nothing happens. You need to implement the remove hook to uninstall the charm.
Test coverage is not good
Integration tests are not run as part of CI/CD
Once you address these issues, let me know and I can take another pass at reviewing the charm.
This point in unclear to me too, can you elaborate and provide us some guidance on what exactly do I need to modify? (that’s our firs charm and we’re struggling a bit)
@tony-meyer didn’t mention that point as well but clean up can take care of the removal of the connector file and the uninstall of the SDC debian package.
I assumed the contact point for the charm was indeed the OpenStack Charmers mailing list. If that isn’t the right one for the other charm, please do update it in that repo also.
I guess I’m more used to the K8s case, or where machines aren’t reused. I don’t recall any specific guidance on uninstalling things in the remove hook, but I guess it is cleanest for the case where a machine will be re-used. I’ll look into this more.
I’ve asked around, and this is something that’s going vary across charmers.
If you need to unregister or otherwise cleanup in remove (like dropping out of an HA system) then clearly the charm needs to do that.
A lot of (machine) charms don’t uninstall (e.g. snaps, debs) in remove.
It is tidy, and particularly if you have some reason to expect machine re-use then it’s probably worth doing, and also particularly if it’s fairly straightforward (like an uninstall command, without having to look for a bunch of other files).
It’s fine to bring it up in a review as a suggestion, but we don’t feel like it ought to block listing. If we have “tiers” of charms in the future, it might be a blocker for getting to higher tiers, but not for just being listed.
We definitely don’t want to encourage people spending large amounts of time figuring out how to perfectly clean up on remove, when, in the majority of the time, the machine will be removed anyway.
So, after some discussion, the charm does still need to clean itself up after removal as it is a subordinate charm and it is not expected that the machine goes away on removal.
To clean up, simply observe the stop hook. In that hook, uninstall any packages you have installed and delete any files you have created.
The charm is tested in an OpenStack environment with the Dell Powerflex system available outside the GitHub CI before the charm is pushed to a stable channel
The charm is tested in an OpenStack environment with the Dell Powerflex system available outside the GitHub CI before the charm is pushed to a stable channel
Okay can you link to a list of the tests that are run? All I currently have to go off of is the one integration test which just deploys and does nothing else.
Sorry to do this, but I actually have another technical issue with the charm.
This charm relates to nova-compute charm over juju-info. The problem here is that juju-info is meant to be used to relate to any charm. But if you relate to any charm that is not nova-compute, this will not behave correctly. Rather than using juju-info, you should create a new interface that is supported by both powerflex-nova and nova-compute so that they can only relate to each other.
