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!
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."