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

Drop mentions of --foreman-proxy-register-in-foreman #3080

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Jun 19, 2024

By removing mentions of this, it becomes an unsupported feature. This is good because various parts of the documentation assume it's turned on. Users can easily get into unexpected errors way further down the line if they disable this feature.

Where there were explicit instructions to disable it, the text is changed into troubleshooting.

I'd appreciate some feedback on the phrasing of the changed lines.

Please cherry-pick my commits into:

  • Foreman 3.11/Katello 4.13
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13; orcharhino 6.6/6.7)
  • We do not accept PRs for Foreman older than 3.5.

By removing mentions of this, it becomes an unsupported feature. This is
good because various parts of the documentation assume it's turned on.
Users can easily get into unexpected errors way further down the line if
they disable this feature.

Where there were explicit instructions to disable it, the text is
changed into troubleshooting.
@ekohl
Copy link
Member Author

ekohl commented Jun 19, 2024

@ehelms @evgeni should we introduce a migration in the installer that resets the answer (so it's turned on) or will that cause more problems than it solves?

@evgeni
Copy link
Member

evgeni commented Jun 19, 2024

I am cool w/o a migration.

@ekohl ekohl changed the title Drop mentions ofr --foreman-proxy-register-in-foreman Drop mentions of --foreman-proxy-register-in-foreman Jun 19, 2024
Comment on lines +58 to +59
When network connections or ports to {Project} are not yet open, registration of the {SmartProxy} will fail.
Correctly configure the network and firewalls before rerunning the installer.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be covered in Prerequisites. It may need a different wording.

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 was thinking about the same thing.

Comment on lines +57 to +58
When network connections or ports to {Project} are not yet open, registration of the {SmartProxy} will fail.
Correctly configure the network and firewalls before rerunning the installer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Not yet reviewed labels Jun 19, 2024
@@ -38,7 +38,6 @@ Retain a copy of the `{foreman-installer}` command that the `{certs-generate}` c
_output omitted_
{installer-scenario-smartproxy} \
--certs-tar-file "/root/{smart-proxy-context}_cert/_{smartproxy-example-com}_-certs.tar" \
--foreman-proxy-register-in-foreman "true" \
Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually command output and comes from https://github.com/theforeman/foreman-installer/blob/2ec4fbdac78e208dbe83dc9414f73ab0dd60ee69/katello_certs/hooks/boot/02-message-helpers.rb#L60 so I think this particular line needs to be kept in until we drop it from the installer.

@ehelms
Copy link
Member

ehelms commented Jun 24, 2024

@ehelms @evgeni should we introduce a migration in the installer that resets the answer (so it's turned on) or will that cause more problems than it solves?

Reset sounds good, and cleaning up the output message that includes it.

@ekohl ekohl marked this pull request as draft August 8, 2024 11:10
@ekohl
Copy link
Member Author

ekohl commented Aug 8, 2024

Moving back to draft because it needs some installer work as well before we can proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting on contributor Requires an action from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants