Juju Charms Collection

Bug #1430796
Comment #4

Comment 4 for bug 1430796

Revision history for this message

Matt Bruzek (mbruzek) wrote on 2015-03-31:

Hello Philip,

Thanks for working on this charm. The previous two reviews have brought up some valid points that I feel should be addressed.

We tend to avoid immutable configuration as this breaks the user experience. The Juju user has no feedback after a configuration option is set to know if the value change was successful. Also the user has no idea which configuration options are immutable or “regular” configuration. We do however, allow immutable configuration in rare cases. I would ask for a few more things: Please confirm/research if there is any way to change the “SST password” after a deployment has started. Notice this config-changed event runs all all units of the cluster. If there is not a way to change the value after deployment. Does this even need to be an option? Do users expect to change this, or could it be hard coded?

Secondly if there is no way to make this configuration option change once deployed, then please call that out clearly in the README.md. We usually like to see a section describing all the configuration options in more detail than config.yaml. If this configuration option is not able to change after deployment, it is different than all other configuration options and should be noted in the README.

There were a few additional things with the review that I would like to point out.

# charm proof
All the messages from charm proof were “I” or informational. While technically “I” messages are OK, we would like to resolve those messages if possible.

$ charm proof
I: No icon.svg file.
I: config.yaml: option root-password has no default value
I: config.yaml: option source has no default value
I: config.yaml: option sst-password has no default value
I: config.yaml: option access-network has no default value
I: config.yaml: option vip has no default value

# icon.svg
It looks like Galara Cluster has an icon that would make this charm stand out and easily identify this charm. When I go to http://galeracluster.com I see a “G” with a circle around it as their icon. Could you try to create an icon for this charm? https://jujucharms.com/docs/1.20/authors-charm-icon

# README.md
There were a few misspelled words in the readme, repositories, minimum, Limitations and channeled. Also please consider adding a section on configuration.

# tests
Trusty charms require tests. As was pointed out in the earlier reviews, we would like to see a `tests` directory that contains a deployment test of this charm. Tests in this format will be run in our automated testing infrastructure and help ensure the charm deploys and is of high quality.

# Conclusion

I am going to move this bug to “In Progress” while you address the comments here. When you are ready for another review please put the bug back in “New” or “Fix Committed” and someone will take a look at it shortly.

Thanks again!

Hello Philip,

Thanks for working on this charm.  The previous two reviews have brought up some valid points that I feel should be addressed.

We tend to avoid immutable configuration as this breaks the user experience. The Juju user has no feedback after a configuration option is set to know if the value change was successful.  Also the user has no idea which configuration options are immutable or “regular” configuration.   We do however, allow immutable configuration in rare cases.  I would ask for a few more things:  Please confirm/research if there is any way to change the “SST password” after a deployment has started.  Notice this config-changed event runs all all units of the cluster.  If there is not a way to change the value after deployment.  Does this even need to be an option?  Do users expect to change this, or could it be hard coded?

Secondly if there is no way to make this configuration option change once deployed, then please call that out clearly in the README.md.  We usually like to see a section describing all the configuration options in more detail than config.yaml.  If this configuration option is not able to change after deployment, it is different than all other configuration options and should be noted in the README.

There were a few additional things with the review that I would like to point out.

# charm proof
All the messages from charm proof were “I” or informational.  While technically “I” messages are OK, we would like to resolve those messages if possible.

# icon.svg
It looks like Galara Cluster has an icon that would make this charm stand out and easily identify this charm.  When I go to http://galeracluster.com I see a “G” with a circle around it as their icon.  Could you try to create an icon for this charm?  https://jujucharms.com/docs/1.20/authors-charm-icon

# README.md
There were a few misspelled words in the readme, repositories, minimum, Limitations and channeled.  Also please consider adding a section on configuration.

# tests
Trusty charms require tests.  As was pointed out in the earlier reviews, we would like to see a `tests` directory that contains a deployment test of this charm.  Tests in this format will be run in our automated testing infrastructure and help ensure the charm deploys and is of high quality.

# Conclusion

I am going to move this bug to “In Progress” while you address the comments here.  When you are ready for another review please put the bug back in “New” or “Fix Committed” and someone will take a look at it shortly.

Thanks again!