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