Comment 11 for bug 1272083

Revision history for this message
Charles Butler (lazypower) wrote :

Greetings Xiaoming, I've had a deeper look at the charm hooks and the progress you've made over the last review cycle. Things have progressed really nicely. The fact your charm is Centos aware has been an interesting feature set to review.

I have some additional notes while reviewing the charm, and they are as follows:

# Charm Proof
No output, fully ready to progress to the next phase of reviewing! Excellent work.

# Hook Review
The start hook halts in failure if the upstart job is already running. This is a situation where unexpected behavior from the hook was not handled gracefully. Exit 1 status calls should be reserved for halting charm execution if something has gone seriously wrong. One option would be to short circuit your start hook to restart if starting the service fails:
`service hpcc-init start || service hpcc-init restart`

the conditional around the port checking path in the installation hook violates the charm store policy. The official bullet point is as follows:

 - Must call Juju API tools (relation-*, unit-*, config-*, etc) without a hard coded path.

Downloads from upstream are not SHA1 verified, and is a requirement for inclusion to the charm store.

## Pushing SSH Keys across Units
> If it is OK we only implement customized keys through config.yaml. If user doesn't set it we will use default keys shipped with hpcc charm

I'm still against shipping any SSH keys with the charm. The preferred method here, would be to have it as a required configuration option for the charm. It's perfectly OK to have a charm halt what its doing if required configuration options are not present. As you progress through exploring dynamically generated keys, this functionality can be extended and enhanced in future releases - and is not a requirement at this phase of development.

# Summary
Prior to your next audit, take a quick run through the charm store policy bullet points to ensure you're adhering to the regulations surrounding having your charm ready for inclusion to the store.

https://juju.ubuntu.com/docs/authors-charm-policy.html

Over all the charm is progressing nicely, and you are really close to being ready for a promulgation review. the new additions have helped to shape it into a great new addition and with a little more work it will be ready for addition to the charm store.

Thank you for the continued submissions, we appreciate the hard work that has gone into this charm.