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

Do not enable Puppet if there is no Pulp 2 #365

Merged
merged 1 commit into from
Oct 23, 2020

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Oct 22, 2020

No description provided.

manifests/params.pp Outdated Show resolved Hide resolved
manifests/application.pp Outdated Show resolved Hide resolved
@@ -39,4 +39,10 @@
String[1] $postgresql_evr_package = $katello::globals::postgresql_evr_package,
Boolean $pulp2_support = $katello::globals::pulp2_support,
) inherits katello::globals {

Copy link
Member

Choose a reason for hiding this comment

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

This empty line breaks CI once we update to the latest lint module. Please drop it.

Copy link
Member Author

Choose a reason for hiding this comment

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

That still makes me sad :/ The empty line provides a nice break when reading the code.

manifests/params.pp 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.

Apologies for not spotting this in the earlier review.

@@ -146,7 +146,7 @@
enable_rpm => $katello::params::enable_yum,
enable_iso => $katello::params::enable_file,
enable_deb => $katello::params::enable_deb,
enable_puppet => $katello::params::enable_puppet,
enable_puppet => $katello::params::_enable_puppet,
Copy link
Member

Choose a reason for hiding this comment

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

This part doesn't make a lot of sense. By definition we have Pulp 2 support at this point.

Suggested change
enable_puppet => $katello::params::_enable_puppet,
enable_puppet => $katello::params::enable_puppet,

@@ -50,7 +50,7 @@
$enable_ostree = $katello::params::enable_ostree
$enable_yum = $katello::params::enable_yum
$enable_file = $katello::params::enable_file
$enable_puppet = $katello::params::enable_puppet
$enable_puppet = $katello::params::_enable_puppet
Copy link
Member

Choose a reason for hiding this comment

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

Thinking more about it, should we fix it here:

Suggested change
$enable_puppet = $katello::params::_enable_puppet
$enable_puppet = $katello::params::pulp2_support and $katello::params::enable_puppet

That avoids the additional variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I really had no idea, this module becoming really hard to follow after the re-factor to params/globals/application. Hopefully we can just drop all of that organization looking ahead.

Copy link
Member

Choose a reason for hiding this comment

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

The long term goal I had was to stop using init.pp altogether. Then globals would be exposed in kafo as well as application and candlepin. params wouldn't be. theforeman/foreman-installer#421 would do that. I rebased it, but it needs migrations. However, what that allows us to do is to remove the pulp module from the answers file on EL8, which means you only have --pulp-* options on EL7. Due to different priorities I was never able to finish this work.

@@ -39,4 +39,5 @@
String[1] $postgresql_evr_package = $katello::globals::postgresql_evr_package,
Boolean $pulp2_support = $katello::globals::pulp2_support,
) inherits katello::globals {
$_enable_puppet = $pulp2_support and $enable_puppet
Copy link
Member

Choose a reason for hiding this comment

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

If both other suggestions are accepted, this can be dropped

Suggested change
$_enable_puppet = $pulp2_support and $enable_puppet

@ekohl ekohl merged commit 40891d8 into theforeman:master Oct 23, 2020
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.

4 participants