Comment 10 for bug 1441622

Hi Johnny,

I had the chance to review the past comments made on this charm and perform my own initial review, and I'd like to share that feedback with you.

One thing to note is that not all reviewers are ~charmers; some may also be members of the Juju community. While Antonio performed the initial review, that doesn't make his the authoritative review. If there are questions about the feedback you've received, though, we are always happy to clarify.

Per the Charm Store Policy (https://jujucharms.com/docs/stable/authors-charm-policy), charms:

- Must include tests for trusty series and any series afterwards. Testing is defined as unit tests, functional tests, or integration tests.

- Must have hooks that are idempotent.

Idempotent in this context means that any hook should be able to be executed multiple times and there should be no observable difference. For example, if install (or config-changed, as discussed below) were called more than once, it should not create duplicate entries in /etc/hosts. More on that here: https://jujucharms.com/docs/1.24/authors-charm-hooks

Stopping charm installation when the domain value is missing is certainly valid, but setting the value after the fact should allow installation to continue. As Cory noted above, moving that functionality to the config-changed hook will resolve that issue.

The install hook would be a good place to use status-set (https://jujucharms.com/docs/1.24/reference-status). You can use this to signal to Juju that the charm is in a blocked state. For example, the install hook can simply be a check if XCAT_DOMAIN is set. If not, call `status-set blocked "Please set the XCAT_DOMAIN."`. If it is set, call `status-set maintenance "Installing software"`. The rest of the code would be moved to config-changed. Once XCAT_DOMAIN is set, the config-changed hook will fire and continue with the installation of the xcat software.

You can also remove the following hooks:
- relation-name-relation-broken
- relation-name-relation-changed
- relation-name-relation-departed
- relation-name-relation-joined

These are part of the default charm template, and not implemented in xCAT.

In metadata.yaml, there is a small typo. The description reads "pacakges" instead of "packages". Also, as noted by Amir and Cory, a http hook is defined in metadata.yaml but is not defined. If xCAT does not use the http relation (there is no hook for one), it can be removed from the metadata.

The upgrade-charm hook does not currently work. The recommended best practice would be for it to call the install hook, followed by the config-changed hook.

Thanks for your work iterating on the feedback you've received. Once you're ready for another review, please set the status back to Fix Committed and we will take another look. Also, feel free to reach out on #juju on irc.freenode.net or the juju mailing list if you have any other questions or concerns.