Comment 1 for bug 1430796

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

Greetings Philip

Thank you for the contribution to the Galera-Cluster charm! This looks like you're off to a great start. I've taken a moment to look over this submission and I've come up with the following notes:

When I ran through the documented process to deploy the galera cluster - I was left with the galera-cluster charm in an error state, with nothing present in the logs to indicate as to why it had failed. Subsequent `juju resolved -r` retry attempts did not complete successfully.

I see that there are hook unit-tests in the charm, and that's a brilliant finding! Ideally we would also like to see a deployment test written in either amulet or mojo to exercise the configuration options so we can introduce the charm into our CI infrastructure to ensure that subsequent merges do not break the charms functionality, and have a good basis of comparison when the next stable series lands so we can promote the charms automatically that have passing tests.

I like how well documented the charm is in the README. When running 'charm proof' - I received only I: messages - which are prefectly acceptable outcomes, and are to be treated as Informational, nice-to-have messages and areas of improvement. The missing icon.svg file is a good one to address before final acceptance into the charm store.

The charm does appear to have immutable configuration - which would need to be addressed before it passes the final review as well. During the code review I saw a #TODO note left at the top of the file that the 'sstuser and root' passwords need to support changes, so I feel that you have a good grasp on this being a requirement.

All in all there's only a few minor things to correct and implement before this charm is ready to hit the lime light. You're nearly there in terms of having a solid store-ready submission barring a few modifications and will have this ready in no time.

Thanks again, and all the best

Charles