-
Notifications
You must be signed in to change notification settings - Fork 991
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 #36688 - Provide option to use wget for the new Register Host feature #9808
Conversation
Can one of the admins verify this patch? |
2 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Issues: #36688 |
I have opened a PR to document this in |
app/controllers/concerns/foreman/controller/registration_commands.rb
Outdated
Show resolved
Hide resolved
app/views/unattended/provisioning_templates/registration/global_registration.erb
Outdated
Show resolved
Hide resolved
Thanks @ekohl. I've incorporated your feedback and rebased my branch. |
ok to test |
Hey @ofedoren, thanks again for your review. |
command << utility[:output_pipe] | ||
end | ||
headers&.each { |header| command << header } | ||
utility[:format_params].call(params).each { |param| command << param } if params |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to fix this because it generated (in case of curl):
`--data "packages=Paket1\ Paket2" which would mean it tries to install a package with the name "Paket1 Paket2" (which does not work because 2 packages are meant")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Bernhard, I removed the quotes around the post parameters. This made it necessary to escape the ampersands in the wget
post parameter.
In my tests installing multiple packages during host registration now worked as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ofedoren Any thoughts about the solution with removing the quotes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should provide some additional information 😅
Post parameters have to be provided differently for curl
(one --data
argument for each post parameter) and wget
(one --post-data
argument containing all post parameters). Initially I removed the quotes of the post parameters in the Global Registration template to unify them and when formatting the post parameters for the curl
or wget
command respectively I added quotes around each post parameter in the case of curl
and around the whole --post-data
argument in the case of wget
. But by this I added quotes also to those post parameters that where not quoted before (like the packages
post parameter) which is causing the described problem.
While investigating the problem with installing multiple packages, I was thinking about whether it even makes sense to quote any of the post parameters as the parameters that could cause problems are already escaped. So my new approach is to not quote the parameters and escape them correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--data "packages=Paket1\ Paket2" <- this need to be fixed. see comment above
I talked with @sbernhard and we were wondering why there are no snapshot tests for the Global Registration template. I am currently working on this and added an initial approach. |
Rebased |
Ok, I tried to add sensible and reasonable snapshot tests for the Global Registration template with various parameters set. But this consumes more time than expected. How do we want to proceed with this PR? I hope it can be merged soon. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@ekohl / @stejskalleos you requested changes. Can you please check again. Thanks. |
@stejskalleos Friendly reminder to please re-review this PR. |
No response since month.
The Register hosts page uses
curl
for registering hosts. This seems to be a good approach for EL based systems as most of these come withcurl
preinstalled. On Debian based systems this is not always the case (at least for recent versions of Debian and Ubuntu).This feature request aims to provide an option to use
wget
for registering hosts aswget
is as widely used ascurl
. By this system administrators that only havewget
installed are not forced to also install curl anymore.