Skip to content

Enable iop class #1229

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 2 commits into
base: master
Choose a base branch
from
Open

Enable iop class #1229

wants to merge 2 commits into from

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Jul 15, 2025

This switches from iop_advisor_engine implementation to the full puppet-iop implementation. For now the application flag remains the same, but the foreman_rh_cloud code will be refactored to update to a new flag or ideally it will use the smart-proxy features.

Signed-off-by: Eric D. Helms <[email protected]>
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.

Are you planning to expose the various parameters like $enable_advisor and $enable_vulnerability too?

Design wise I'm starting to wonder if we should expose iop as a top level class and make that include foreman::plugin::rh_cloud instead. The user experience would then be --enable-iop and specifics like --iop-enable-advisor true.

Especially once we get rid of the config file (so resolve https://github.com/theforeman/puppet-foreman/pull/1229/files#r2237458876) that will be a nicer user experience IMHO. Downstream we always enable the plugin anyway so we don't need to expose it in the installer (so no --enable-foreman-plugin-rh-cloud instruction anywhere, especially not in the help).


if $enable_iop {
class { 'iop':
register_as_smartproxy => true,
Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at https://github.com/theforeman/puppet-iop/blob/fe93bde89aef825f6ab710cd99b49460ae666fd5/manifests/init.pp#L38-L54 and I see it doesn't support setting the base URL. I'd like it to use $foreman::foreman_url. Not a blocker for this PR, but something that is the correct thing to do.

@ehelms
Copy link
Member Author

ehelms commented Jul 30, 2025

Are you planning to expose the various parameters like $enable_advisor and $enable_vulnerability too?

No. Those were potential helpers, but in reality it's all or nothing.

Design wise I'm starting to wonder if we should expose iop as a top level class and make that include foreman::plugin::rh_cloud instead.

I do not think this is possible until the code itself relies only on the existence of the smart-proxy? Otherwise, how would the code properly indicate to the plugin that iop is enabled?

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
@ekohl
Copy link
Member

ekohl commented Jul 31, 2025

I do not think this is possible until the code itself relies only on the existence of the smart-proxy? Otherwise, how would the code properly indicate to the plugin that iop is enabled?

My impression is that IoP itself is a Smart Proxy and there is explicit registration code. Not sure what you exactly mean.

@ehelms
Copy link
Member Author

ehelms commented Jul 31, 2025

My impression is that IoP itself is a Smart Proxy and there is explicit registration code. Not sure what you exactly mean.

We will need theforeman/foreman_rh_cloud#1049 so that we do not need to configure a setting (https://github.com/theforeman/puppet-foreman/pull/1229/files#diff-25e640d8df59d7c62bc0ad9e10e140f94b21efc915bd10072717dca8e2a6ba48R10). That will mean we can then just include foreman::plugin::rh_cloud rather than needing to define it using class { declaration.

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