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

allow overriding the name of the metapackage #481

Merged
merged 1 commit into from
Oct 30, 2023
Merged

allow overriding the name of the metapackage #481

merged 1 commit into from
Oct 30, 2023

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Oct 24, 2023

this is useful for downstream branding

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.

Just some random thoughts. I'd like to make the meta package optional. We're not there yet, but perhaps something to consider while doing this. Should we at some point accept String[0] to disable this?

manifests/init.pp Outdated Show resolved Hide resolved
@@ -26,4 +26,5 @@
Stdlib::HTTPSUrl $candlepin_url = "https://${candlepin_host}:${candlepin_port}/candlepin",
String[1] $candlepin_client_keypair_group = 'foreman',
String[1] $postgresql_evr_package = $katello::globals::postgresql_evr_package,
String[1] $meta_package = $katello::globals::meta_package,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to expose this? If we limit it to globals then the installer can set this via Hiera parameters. I don't see a use case (at least in the short term) where the user would want to override this.

It may not be clear today, but it used to have a parameter (until 188a71f) that was controlled by the installer.

The long term idea was that you have:

  • globals.pp is set by the installer
  • params.pp gets exposed to the user via installer parameters
  • application.pp and candlepin.pp are exposed instead of init.pp to allow installation of only Candlepin or Foreman.

Then we would expose foreman_proxy_content as --content-* instead.

An intermediate step was #209 which introduced manage flags for each component.

Or we can take the easy solution here and inline it. We should do the same with postgresql_evr_package which used to have EL7/EL8 logic. When we dropped EL7 support, there's little benefit to keeping it in globals anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I don't particularly care where it's defined as long as it can be overridden via installer/hiera. I'll play around how to minify this.

Copy link
Member Author

Choose a reason for hiding this comment

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

do you have a preference? both ways "work", but I find inlining into params.pp nicer as in init.pp we never touch katello::globals directly, so package { $katello::globals::meta_package … } reads odd to me (but works as it's loaded by katello::params)

@evgeni
Copy link
Member Author

evgeni commented Oct 30, 2023

@ekohl the tests fail and I briefly recall this is because of the security fix in candlepin that makes something non idempotent?

@ekohl
Copy link
Member

ekohl commented Oct 30, 2023

Correct. 3a2be23 is the fix for that.

manifests/params.pp Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
this is useful for downstream branding
@ekohl ekohl merged commit 12fabfc into master Oct 30, 2023
4 of 5 checks passed
@ekohl ekohl deleted the metapackage branch October 30, 2023 11:59
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