Hi Benedikt,
Thanks for bringing these items. I generally agree with the points you listed and will leave a few more comments inline below.
Our puppet modules were developed initially some time ago(even long before I joined the team) and we have many legacy implementations. It'd be nice if we can adapt to modern design patterns.
However at the same time we have limited resources in the project now so addressing all these topics in a short term might be difficult. As we maintain number of modules in this project, I'd like to make sure we have the consistent design pattern across all modules. So we can probably determine the priorities of these works and also milestones(especially in which cycle we implement each change).
Thank you, Takashi
On Tue, May 30, 2023 at 9:37 PM Benedikt Trefzer < benedikt.trefzer@cirrax.com> wrote:
Hi all
I use the openstack puppet modules to deploy openstack. I'd like to suggest some improvements to the modules and like to know what the community thinks about:
1.) use proper types for parameters Parameter validation is done with 'validate_legacy...' instead of defining the class/resource parameter with a proper type all over the code. I cannot imaging of any advantage not using proper type definitions. Instead using typed parameters would be more efficient an code readability would increase.
I think we have to prioritize this since puppetlabs-stdlib 9.0.0 deprecated the validate_legacy function and emits large warnings.
The reason we(at least, I) hesitated implementing the strict type validation is that this is likely to break existing manifests(in fact, even our modules or manifests have experienced breakage caused by new validations added in dependent modules) and also can heavily affect TripleO (though the project has been deprecated now).
We can start with just replacing all validate_legacy functions by typed parameters first and target this work during this cycle then discuss how we improve coverage of type validations further.
2.) params.pp This is the legacy way to define parameter defaults for different OS's. In the modern puppet world a module specific hiera structure is used, which eliminates the need of params.pp class (with inheritance and include). The usage of hiera improves readability and flexibility (every parameter can be overwritten on request, eg. change of packages names etc.) This also eliminate the restriction that the modules can only be used by certain OS'es (osfamily 'RedHat' or 'Debian').
+1. Though I'd like to understand which OS users would like to use with our modules so that we can ideally implement CI coverage.
3.) Eliminate "if OS=='bla' {" statements in code These statements make the code very inflexible. It cannot be overruled if necessary (eg. if I use custom packages to install and do not need the code provided in the if statement). Instead a parameter should be used with a default provided in hiera.
+1 . We can partially cover this as part of 2 but might leave this for the last work among 3, I guess.
Since there is lot of code to change I do not expect this to be done in a single commit (per module) but in steps probably in more than one release cycle. But defining this as best practice for openstack puppet modules and start using above in new commits would bring the code forward.
I tend to disagree that we start these new patterns in new commits. Having partial migration causes difficulty in maintenance. I really want to see these are implemented consistently in a single moules as well as among all modules, so that we are not confused when we are forced to implement global changes in all of our modules.
We can probably start with a few "independent" modules such as puppet-vswitch(or p-o-i) and once we agree with the pattern then we can schedule when we start implementing these changes in all modules in a single release cycle.
Finally: These are suggestions open for discussion. In no way I like to critic the current state of the puppet modules (which is quite good, but a bit legacy) or the people working on the modules. This is just a feedback/suggestion from an operator using the modules on a daily basis.
Regards
Benedikt Trefzer