Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert some templates to EPP #1568

Merged
merged 2 commits into from
Aug 31, 2023
Merged

Convert some templates to EPP #1568

merged 2 commits into from
Aug 31, 2023

Conversation

jay7x
Copy link
Member

@jay7x jay7x commented Jul 7, 2023

No description provided.

Those contexts were checking for different data types while ago (integer,
string)
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played around with this a bit and I wrote https://gist.github.com/ekohl/28443e6ba178c931bb69285e976568eb. It's not perfect, but it gets you a long way. The biggest thing to be aware of is that you need to change if x into if x {, but it converts end into } already as well as @x into $x. I'd suggest to give it a try and see.

@jay7x
Copy link
Member Author

jay7x commented Jul 9, 2023

Conversion is mostly completed. Need to recheck whitespace differences though. Will work on this later.

@jay7x jay7x marked this pull request as ready for review July 14, 2023 17:27
@jay7x jay7x marked this pull request as draft July 14, 2023 17:31
@jay7x jay7x force-pushed the erb2epp branch 6 times, most recently from b5e7e66 to 1e5fa36 Compare July 14, 2023 22:14
@jay7x jay7x marked this pull request as ready for review July 14, 2023 22:16
Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you forgot to add templates/prepend_append.epp 😉

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive! I just spot a tiny worrying bit.

spec/default_module_facts.yml Outdated Show resolved Hide resolved
spec/default_module_facts.yml Outdated Show resolved Hide resolved
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to ignore my suggestions. They're aimed at making the template even easier.

I notice 1.15.0 is used quite often in these templates which made me check, but sadly Debian 10 still uses nginx 1.14.2.

manifests/resource/mailhost.pp Outdated Show resolved Hide resolved
manifests/resource/mailhost.pp Outdated Show resolved Hide resolved
templates/mailhost/mailhost.epp Outdated Show resolved Hide resolved
templates/mailhost/mailhost_common.epp Outdated Show resolved Hide resolved
templates/mailhost/mailhost_ssl.epp Outdated Show resolved Hide resolved
@jay7x
Copy link
Member Author

jay7x commented Jul 18, 2023

All suggestions looks reasonable.. I guess I'll implement it this Friday

spec/default_module_facts.yml Outdated Show resolved Hide resolved
@jay7x jay7x force-pushed the erb2epp branch 2 times, most recently from bad5572 to bc8e31b Compare July 23, 2023 17:18
Comment on lines -173 to -176
Optional[Variant[Array, String]] $raw_prepend = undef,
Optional[Variant[Array, String]] $raw_append = undef,
Optional[Hash] $mailhost_cfg_prepend = undef,
Optional[Hash] $mailhost_cfg_append = undef,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with merging this, but should we mark it as backwards-incompatible because of the type changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, as far as I am concerned, it looks like the previous data types where too relaxed and allowed to provide configuration that would cause nginx to fail to start. Existing valid config should still be valid so I would not mark it as backwards incompatible, and if we find some corner cases that used to be valid and are now broken, we can quickly issue a new patch release to fix these.

@bastelfreak bastelfreak added the enhancement New feature or request label Aug 31, 2023
@bastelfreak bastelfreak merged commit da6b41c into voxpupuli:master Aug 31, 2023
28 of 29 checks passed
@jay7x jay7x deleted the erb2epp branch August 31, 2023 10:14
@TheMeier TheMeier added this to the 6.0.0 milestone Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants