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

Fixes #37891 - Add enable-puppet8 parameter to provisioning templates #10346

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

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Oct 11, 2024

Previously the enable-official-puppet8-repo parameter was added, but for users who supply their own packages (like Katello users) and still want to use AIO packaging need a parameter. Technically they could use enable-puppet5, enable-puppet6 or enable-puppet7 but this is inconsistent. It would be better to add a new version independent parameter, but this first aims for consistency.

Fixes: c35092b ("fixes #36939 - Allow for deployment of puppet 8")

Previously the enable-official-puppet8-repo parameter was added, but for
users who supply their own packages (like Katello users) and still want
to use AIO packaging need a parameter. Technically they could use
enable-puppet5, enable-puppet6 or enable-puppet7 but this is
inconsistent. It would be better to add a new version independent
parameter, but this first aims for consistency.

Fixes: c35092b ("fixes #36939 - Allow for deployment of puppet 8")
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

In general +1, one comment about readability

@@ -12,7 +12,7 @@ description: |
os_family = @host.operatingsystem.family
os_name = @host.operatingsystem.name

aio_enabled = host_param_true?('enable-puppetlabs-repo') || host_param_true?('enable-official-puppet8-repo') || host_param_true?('enable-official-puppet7-repo') || host_param_true?('enable-puppet7') || host_param_true?('enable-puppetlabs-puppet6-repo') || host_param_true?('enable-puppet6') || host_param_true?('enable-puppetlabs-puppet5-repo') || host_param_true?('enable-puppet5')
aio_enabled = host_param_true?('enable-puppetlabs-repo') || host_param_true?('enable-official-puppet8-repo') || host_param_true?('enable-puppet8') || host_param_true?('enable-official-puppet7-repo') || host_param_true?('enable-puppet7') || host_param_true?('enable-puppetlabs-puppet6-repo') || host_param_true?('enable-puppet6') || host_param_true?('enable-puppetlabs-puppet5-repo') || host_param_true?('enable-puppet5')
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: this is slowly getting hard to read

Suggested change
aio_enabled = host_param_true?('enable-puppetlabs-repo') || host_param_true?('enable-official-puppet8-repo') || host_param_true?('enable-puppet8') || host_param_true?('enable-official-puppet7-repo') || host_param_true?('enable-puppet7') || host_param_true?('enable-puppetlabs-puppet6-repo') || host_param_true?('enable-puppet6') || host_param_true?('enable-puppetlabs-puppet5-repo') || host_param_true?('enable-puppet5')
puppet_params = %w(
puppetlabs-repo
puppet8 official-puppet8-repo
puppet7 official-puppet7-repo
puppet6 puppetlabs-puppet6-repo
puppet5 puppetlabs-puppet5-repo
)
aio_enabled = puppet_params.any? { |param| host_param_true?("enabled-#{param}") }

Copy link
Member Author

Choose a reason for hiding this comment

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

I avoided that because I want to cherry pick this to 3.12.

What I'd propose for the future is that we update to some generic parameter like puppet-aio instead of enable-puppetX where X is 5, 6, 7 or 8 (or 9 in the future). We'd still have various repo parameters. Version 5 has been removed and 6 is EOL so we can probably drop those.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a plan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants