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

Fixes #34710 - Use foreman request address #9171

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

sbernhard
Copy link
Contributor

  1. Next-server in windows iPXE default template does only work in dhcp environments but not with static IPs
  2. A method which only returns the request address (fqdn + port) of foreman or the related smart proxy is often useful

@theforeman-bot
Copy link
Member

Issues: #34710

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @sbernhard, seems OK to me, just few nitpicks. Also,

Next-server in windows iPXE default template does only work in dhcp environments but not with static IPs

Does this change effect working in DHCP environments?

app/services/foreman/foreman_url_renderer.rb Outdated Show resolved Hide resolved
app/services/foreman/foreman_url_renderer.rb Outdated Show resolved Hide resolved
app/services/foreman/foreman_url_renderer.rb Outdated Show resolved Hide resolved
@sbernhard
Copy link
Contributor Author

Thank you very much for your review and your comments. I added them.

Does this change effect working in DHCP environments?

It should but let me ask my colleagues again.

@SimonLorentz
Copy link

Does this change effect working in DHCP environments?

Hello, the change will only effect working in DHCP, if you want to use another server than foreman or proxy as next server and want strictly to set this via DHCP and not an extra variable.

So normally it has no negativ effect for DHCP scenarios

ofedoren
ofedoren previously approved these changes Apr 7, 2022
Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @sbernhard and @SimonLorentz. 🟢 from me, but I'd leave it a bit for another pair of eyes in case I missed something :/

ezr-ondrej
ezr-ondrej previously approved these changes Apr 13, 2022
Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Apart of confusing name, and possible imporovements on the docs, LGTM 👍
I'll leave up to @ofedoren to judge if those improvements are worth while :)

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @sbernhard, @ezr-ondrej, merging :)

@ofedoren ofedoren merged commit 2f2ec13 into theforeman:develop Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants