Comment 8 for bug 1421230

Revision history for this message
Kevin W Monroe (kwmonroe) wrote :

Hey Cory - thanks for splitting the bird/acl-manager charms into separate proposals. Deploying/relating per the README worked, and unit tests look good now that you've added a testdep target. Excellent!

That said, we have a Charm Authors best practice [0] regarding immutable configuration. For neutron-calico, this comes into play with the 'calico-origin' config option. Your description for that option is clear ("this must be set before the 'install' hook runs"), but a user might not read this (gasp!) and assume that setting a new calico-origin ppa would re-install the deployed unit. This doesn't happen currently, which might lead to confusion and a poor user experience.

While having immutable config in the charm is not a strict deal-breaker for recommended status, I would like to ask your thoughts on removing calico-origin as a config option (with the default ppa hard coded and perhaps a blurb in the README telling users how to set their own by customizing a local copy of your charm). This would align your charm with our best practices, but may not be a good alternative if you foresee users depending on this option during initial deployment. Another alternative would be to clean up and re-run install during config-changed, but that feels like a bigger can of worms than removing the option.

Let me know what you think, and again, thanks for the work here!

[0] https://jujucharms.com/docs/1.20/authors-charm-best-practice, paragraph starting with "The configuration should not be immutable."