Skip to content

Draft: Eliminate params.pp #426

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cocker-cc
Copy link
Contributor

@cocker-cc cocker-cc commented May 27, 2025

Pull Request (PR) description

params.pp and Inheritance in general is not best Practice anymore. Hiera provides better Definition of Parameters.

  • move Defaultvalues to Modulelevel-Hiera
  • eliminate params.pp
  • sort YAML-Files alphabetically

@cocker-cc cocker-cc force-pushed the Eliminate_paramspp branch from e6ce389 to 8c0cc7f Compare May 27, 2025 20:01
@cocker-cc
Copy link
Contributor Author

Rebase against master.

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.

Nice! I added in-line comments regarding init.pp / common.yaml related to documentation / behavior. Thank you!

Comment on lines +43 to +46
postfix::alias_maps: hash:/etc/aliases
postfix::amavis_procs: 2
postfix::confdir: "/etc/postfix"
postfix::conffiles: {}
postfix::configs: {}
postfix::hashes: {}
postfix::inet_interfaces: all
postfix::inet_protocols: all
postfix::ldap: false
postfix::ldap_packages: []
postfix::lookup_table_type: hash
postfix::mail_user: vmail
postfix::mailaliases: {}
postfix::mailman: false
postfix::mailx_ensure: present
postfix::mailx_package: mailx
postfix::maincf_source: puppet:///modules/postfix/main.cf
postfix::manage_aliases: true
postfix::manage_conffiles: true
postfix::manage_mailname: true
postfix::manage_mailx: true
postfix::manage_root_alias: true
postfix::maps: {}
postfix::master_bounce_command: bounce
postfix::master_defer_command: bounce
postfix::master_entries: []
postfix::master_os_template: postfix/master.cf.default.erb
postfix::mta: false
postfix::mydestination: "$myhostname, localhost.$mydomain, localhost"
postfix::mynetworks: 127.0.0.0/8 [::ffff:127.0.0.0]/104 [::1]/128
postfix::myorigin: "%{facts.networking.fqdn}"
postfix::postfix_ensure: present
postfix::restart_cmd: "/etc/init.d/postfix reload"
postfix::root_group: root
postfix::root_mail_recipient: nobody
postfix::satellite: false
postfix::service_enabled: true
postfix::service_ensure: running
postfix::smtp_listen: 127.0.0.1
postfix::transports: {}
postfix::use_amavisd: false
postfix::use_dovecot_lda: false
postfix::use_schleuder: false
postfix::use_sympa: false
postfix::virtuals: {}
Copy link
Member

Choose a reason for hiding this comment

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

Only values which are OS-dependent (i.e. present in multiple hiera .yaml files) should be in common.yaml. Values which are not OS-dependent must be kept in init.pp so that they are documented in REFERENCE.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Values which are not OS-dependent must be kept in init.pp

I'm afraid having to disagree here. Since the Appearance of Hiera, APL (automatic Parameterlookup) is definitely the prefered Method for Defaultvalues. OS-Dependency is not a Criterion.
IMHO

Copy link
Member

Choose a reason for hiding this comment

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

The Vox Pupuli convention is as @smortex explained: OS-independent defaults as class parameter defaults so that they are documented in REFERENCE.md. OS-dependent defaults go into hiera data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Vox Pupuli convention is as @smortex explained

Where to find this?

The Link to Puppet Labs style guide in your README.md is dead, as well as the Link on voxpupuli.org

This Puppet-7-Style-Guide reads:

Use Hiera data in your module to set parameter defaults.

This exactly is, what I stick to, because

Only values which are OS-dependent (i.e. present in multiple hiera .yaml files) should be in common.yaml.

is plain inconsistent IMHO. The whole Sense of Hiera is to decouple Data from Code, to make the Code OS-agnostic. This very importand Paradigm should not be sacrificed for REFERENCE.md. (why not let puppet strings generate read from Hiera? *wink*)

Recognizing, that this is the wrong Location to discuss this Subject, feel free to adopt my Changes under your own Author-Name.

Copy link
Member

Choose a reason for hiding this comment

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

The Vox Pupuli convention is documented here: https://voxpupuli.org/docs/reviewing_pr/

Quoting that page:

Static data that is equal across every supported operating system must stay in the init.pp, it shouldn’t be moved to a common.yaml due to puppet-strings issue #250.

Yes, Perforce broke all of the puppet docs links.

params.pp and Inheritance in general is not best Practice anymore.
Hiera provides better Definition of Parameters.

- move Defaultvalues to Modulelevel-Hiera
- eliminate params.pp
- sort YAML-Files alphabetically
@cocker-cc cocker-cc force-pushed the Eliminate_paramspp branch from 8c0cc7f to 8289129 Compare May 27, 2025 21:40
@cocker-cc cocker-cc changed the title Eliminate params.pp Draft: Eliminate params.pp May 27, 2025
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

Successfully merging this pull request may close these issues.

3 participants