Skip to content

Conversation

maximiliankolb
Copy link
Contributor

What changes are you introducing?

Replace step that uses Web UI with Hammer CLI command.

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

All steps but one relies on using CLI, so we should not force the user to complete this single step via WebUI.

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

Tested on Foreman 3.14/Katello 4.16.

Contributor checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.16/Katello 4.18 (Satellite 6.18)
  • Foreman 3.15/Katello 4.17
  • Foreman 3.14/Katello 4.16 (Satellite 6.17; orcharhino 7.4)
  • Foreman 3.13/Katello 4.15 (EL9 only)
  • Foreman 3.12/Katello 4.14 (Satellite 6.16; orcharhino 7.2 on EL9 only; orcharhino 7.3)
  • Foreman 3.11/Katello 4.13 (orcharhino 6.11 on EL8 only; orcharhino 7.0 on EL8+EL9; orcharhino 7.1 with Leapp)
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • We do not accept PRs for Foreman older than 3.9.

Review checklists

Tech review (performed by an Engineer who did not author the PR; can be skipped if tech review is unnecessary):

  • The PR documents a recommended, user-friendly path.
  • The PR removes steps that have been made unnecessary or obsolete.
  • Any steps introduced or updated in the PR have been tested to confirm that they lead to the documented end result.

Style review (by a Technical Writer who did not author the PR):

  • The PR conforms with the team's style guidelines.
  • The PR introduces documentation that describes a user story rather than a product feature.

All steps but one relies on using CLI, so we should not force the user
to complete this single step via WebUI.

Tested on Foreman 3.14/Katello 4.16.
@github-actions github-actions bot added Needs tech review Requires a review from the technical perspective Needs style review Requires a review from docs style/grammar perspective Needs testing Requires functional testing labels Sep 12, 2025
@aneta-petrova aneta-petrova added style review done No issues from docs style/grammar perspective Not yet reviewed and removed Needs style review Requires a review from docs style/grammar perspective labels Sep 12, 2025
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.

I was surprised by this setting even existing and especially where it's defined. It's in Katello:

https://github.com/Katello/katello/blob/a053c92d0fa4413400d6032492fc6b90224d31f6/lib/katello/plugin.rb#L678-L682

If we tell users to set this to true, is there a good reason why it's default to false?

@adamruzicka you added the setting in Katello/katello@840cbe6. Can you shine a light on that?

@adamruzicka
Copy link
Contributor

I was surprised by this setting even existing and especially where it's defined. It's in Katello

Remote execution has a mechanism for picking a proxy to be used. The mechanism takes many things into consideration and is generally not compatible with katello's "host is registered to a proxy so everything has to be done through that proxy, regardless on subnets and things like that" world view. Hence katello comes with a setting to bend the behaviour of Rex to match katello's customs.

I don't like it either, but probably for a different reason.

If we tell users to set this to true, is there a good reason why it's default to false?

Mostly to have vanilla foreman and foreman with katello behave the same way with an option to selectively enable different behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs tech review Requires a review from the technical perspective Needs testing Requires functional testing style review done No issues from docs style/grammar perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants