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

Move templates #132

Closed
alexjfisher opened this issue Apr 4, 2020 · 8 comments
Closed

Move templates #132

alexjfisher opened this issue Apr 4, 2020 · 8 comments
Milestone

Comments

@alexjfisher
Copy link
Member

The templates are currently in directories based on the OS codename. The fact for codename isn't always available. We should move the templates to directories based on the standard OS facts.
This is a breaking change, but the next released version is scheduled to be a major anyway.

See #122 (comment)

@alexjfisher alexjfisher added this to the 3.0.0 milestone Apr 4, 2020
@neomilium
Copy link
Contributor

If I understand correctly the discussion at #122, we should remove any facts['os']['family'] occurrence, right?

@gcoxmoz
Copy link

gcoxmoz commented Apr 4, 2020

I believe the opposite is true. I believe that PR #122 should be rejected in favor of this issue.

  • Use already-existing facts-from-facter to determine the template(s) to use, and
  • remove lsb-based references to codenames, which require an extra package (duplicating facter's functionality) while being inadequate (because the lsb names are not specific enough across major versions, in cases like CentOS).

@neomilium
Copy link
Contributor

neomilium commented Apr 5, 2020

I believe that PR #122 should be rejected in favor of this issue.

So am I.

* Use already-existing facts-from-facter to determine the template(s) to use, and

Currently there is no code that use lsb facts except specs.

Is the wanted feature to determine automatically what template to use without providing any parameter to puppet class?

* remove lsb-based references to codenames, which require an extra package (duplicating facter's functionality) while being inadequate (because the lsb names are not specific enough across major versions, in cases like CentOS).

As there is no code that uses lsb facts except specs, its mainly a README.md rewrite.

Sorry for asking the obvious but as its a breaking change, IMHO, we need to have a really clear goal.

@gcoxmoz
Copy link

gcoxmoz commented Apr 5, 2020

You're right, I needed to go study my profile notes+code more. Let's try this again.

Most of the README.md examples call for passing the ${::lsbdistcodename} fact into config_file_template to use the templates that the module provides as being there "with the recommended parameters". For puppet agents that don't have lsb installed, this means you need to install lsb (or fake hard-coding it), or refer to some outside-the-module template that you have to build yourself, in order to make fail2ban.conf appear.

So yes you're right, the module's code doesn't use lsb, but for any amount of reasonable configuration change the module doesn't help you out by providing templates that resemble the current world. Even if you have lsb installed (again, package sprawl on the agents), in some cases the module's templates don't match your OS's offerings, because CentOS[6-8] look different under the same lsb name.

Is the wanted feature to determine automatically what template to use without providing any parameter to puppet class?

IMO, yes, that would be on track for the right thing to do. Something like:

  • a Boolean like manage_conf on whether or not to manage the config. When it's true, then...
  • a code path through params.conf that uses facts to find your default templates (if the module supports you in metadata.json, there's a template set that is structured like the base OS's).
  • README updates to reflect that you don't have to pass in the template usage unless you're doing something weird that goes outside what the module manages.

@neomilium
Copy link
Contributor

IMO, yes, that would be on track for the right thing to do. Something like:

* a Boolean like `manage_conf` on whether or not to manage the config.  When it's true, then...

Is there is a case were user will use this module without managing the configuration?

* a code path through `params.conf` that uses facts to find your default templates (if the module supports you in `metadata.json`, there's a template set that is structured like the base OS's).

I moved all parameters from params.pp to hiera based ones (see #134). So, I suggest to automatically pick the template using the same pattern as hiera (ie. the same hierarchy) if user do not provide its own template.

* README updates to reflect that you don't have to pass in the template usage unless you're doing something weird that goes outside what the module manages.

And README.md rework seems to be the hardest part to me ;-)

@gcoxmoz
Copy link

gcoxmoz commented Apr 6, 2020

Is there is a case were user will use this module without managing the configuration?

My experience with modules is that invariably they are less mature than the service you're trying to manage. I don't say that to disparage work on modules, but I take it as a general statement of what I've gone through trying to modernize our puppet at $WORK.

The areas I'm able to modernize the fastest are almost always ones that have good abilities to STOP them from poorly managing our servers, because they've decided to manage something that doesn't work for us (most famously here, any module that wants to manage a yumrepo, we have to disable: because no module owner ever thinks about needing to go through a proxy).

So, "do I have a use case?" In this situation, not offhand. But I very much appreciate when a module lets me choose whether or not to manage a resource, because there's a world of variation in systems administration, and just because I can't think of a reason doesn't mean someone else can't.

@neomilium
Copy link
Contributor

PR started at #135

@dhoppe
Copy link
Member

dhoppe commented Apr 21, 2020

This issue has been solved by #135.

@dhoppe dhoppe closed this as completed Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants