Hello, Thanks for bringing this up! I agree with all your points. It would be great if you wanted to help get these goals completed and we could work together on all that. As Takashi said we’re having very few contributors these days and we appreciate any help we can get, he has been doing an amazing job on cleaning up and doing maintenance. I agree with Takashi’s feedback below, starting with removing validate_legacy to get us started seems like a good option. We will be on the OpenInfra Summit next week and are going to meet up, if you are there we would love to have you join us in a discussion/planning on moving forward. Best regards Tobias On 5 Jun 2023, at 03:33, Takashi Kajinami <tkajinam@redhat.com> wrote: 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<mailto: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

Hi

As Takashi said we’re having very few contributors these days and we appreciate any help we can get, he has been doing an amazing job on cleaning up and doing maintenance.

Ack and fully agree.

We will be on the OpenInfra Summit next week and are going to meet up, if you are there we would love to have you join us in a discussion/planning on moving forward.

I will not be there, but willing to contribute. It would be nice if you could define a hiera.yaml file to use for all youre modules at the summit. Think this is a work that can better be done in a meetup than on reviews.

...

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.

Agree, an like to add: "And ensure for all modules that new parameters introduced are properly typed."

...

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.

Ack. We use Debian. But with self brewed openstack packages. Which means, we maintain a patchset for eliminating if "OS=='Debian' " statements (in code and in params.pp) for many of the puppet modules.

...

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.

Although this is the most hurting part for us, I agree that for proper solutions point 2 is needed.

...

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.

Probably although a good thing to discuss at Vancouver. Would be nice if somebody could communicate after Vancouver what the outcome was for the further development. Thanks Benedikt