Refs #37440 - Readd host registration repo parameters with deprecatio… #10187

nadjaheitmann · 2024-05-28T10:24:24Z

…n warning

nadjaheitmann · 2024-05-28T10:25:23Z

@stejskalleos Fix for re-adding the host registration parameters.

Theoretically, you can add both parameters now, repo and repo_data. repo_data would overwrite repo parameters if available.

stejskalleos · 2024-05-30T13:19:39Z

app/controllers/api/v2/registration_commands_controller.rb

@@ -18,6 +18,9 @@ class RegistrationCommandsController < V2::BaseController
        param :insecure, :bool, desc: N_("Enable insecure argument for the initial curl")
        param :packages, String, desc: N_("Packages to install on the host when registered. Can be set by `host_packages` parameter, example: `pkg1 pkg2`")
        param :update_packages, :bool, desc: N_("Update all packages on the host")
+        param :repo, String, desc: N_("Repository URL / details, for example for Debian OS family: 'deb http://deb.example.com/ buster 1.0', for Red Hat and SUSE OS family: 'http://yum.theforeman.org/client/latest/el8/x86_64/'"), deprecated: true


When I run bin/hammer host-registration generate-command --help, I don't see any information about the deprecation.
@ofedoren What does the deprecated do? I thought it would mark the fields in hammer --help output as deprecated.

Unfortunately, hammer doesn't do that. Mostly we need to do that manually, e.g. in the command definition itself we need to add build_options without: repo, :repo_gpg_key_url. And then add them as options manually with the deprecation warning like in https://github.com/theforeman/hammer-cli-foreman/blob/master/lib/hammer_cli_foreman/command_extensions/compute_resource_subcommand.rb#L5

So how is the workflow? Shall I open a hammer PR in parallel or do we get this one in, first?

So how do we proceed here? I think it should go in asap because #10162 has a breaking change in the API (that is reverted in this PR) and is part of 3.11. So the fix should also go into 3.11

stejskalleos · 2024-05-30T13:24:36Z

app/controllers/api/v2/registration_controller.rb

@@ -23,13 +23,20 @@ class RegistrationController < V2::BaseController
      param :setup_remote_execution, :bool, desc: N_("Set 'host_registration_remote_execution' parameter for the host. If it is set to true, SSH keys will be installed on the host")
      param :packages, String, desc: N_("Packages to install on the host when registered. Can be set by `host_packages` parameter, example: `pkg1 pkg2`")
      param :update_packages, :bool, desc: N_("Update all packages on the host")
+      param :repo, String, desc: N_("Repository URL / details, for example for Debian OS family: 'deb http://deb.example.com/ buster 1.0', for Red Hat and SUSE OS family: 'http://yum.theforeman.org/client/latest/el8/x86_64/'"), deprecated: true


When I run the hammer command

bundle exec bin/hammer host-registration generate-command --repo "https://repo.com"

I don't see any deprecation warning in Foreman / Hammer log.

@nadjaheitmann does it work for you?

I can see it in the console output of my Forklift so I assume it would appear in production log. Not sure how helpful that is, though.

Well, I can see it if I run the command, not for the help output.

If you look at the apidoc, the parameters appear as deprecated, though.

ofedoren · 2024-05-31T13:21:58Z

app/controllers/concerns/foreman/controller/registration.rb

+    if params["repo"].present?
+      repo_data = {}
+      repo_data[params['repo']] = params['repo_gpg_key_url'] || ''
+    end


It doesn't seem to be the same, is this intentional? https://github.com/theforeman/foreman/pull/10162/files#diff-05645e27e9ae6b798390ab2c548df6516a13db34d245e54eee0690c965bffbf4R21-R24

It is supposed to create a hash where the key is the repository URL and the value is the corresponding GPG key. In this case, if you have the old parameters given, it just creates a hash with a single key value pair that is evaluated at a later point in time.

I think I know what you mean. I messed up the params names but should be sorted out now.

ofedoren · 2024-05-31T13:33:47Z

app/controllers/api/v2/registration_commands_controller.rb

@@ -18,6 +18,9 @@ class RegistrationCommandsController < V2::BaseController
        param :insecure, :bool, desc: N_("Enable insecure argument for the initial curl")
        param :packages, String, desc: N_("Packages to install on the host when registered. Can be set by `host_packages` parameter, example: `pkg1 pkg2`")
        param :update_packages, :bool, desc: N_("Update all packages on the host")
+        param :repo, String, desc: N_("Repository URL / details, for example for Debian OS family: 'deb http://deb.example.com/ buster 1.0', for Red Hat and SUSE OS family: 'http://yum.theforeman.org/client/latest/el8/x86_64/'"), deprecated: true


Unfortunately, hammer doesn't do that. Mostly we need to do that manually, e.g. in the command definition itself we need to add build_options without: repo, :repo_gpg_key_url. And then add them as options manually with the deprecation warning like in https://github.com/theforeman/hammer-cli-foreman/blob/master/lib/hammer_cli_foreman/command_extensions/compute_resource_subcommand.rb#L5

stejskalleos

Let's update the description for parameters as well, the hammer part can be done later on and we don't have to block it on it.

app/controllers/api/v2/registration_commands_controller.rb

app/controllers/api/v2/registration_controller.rb

stejskalleos

I did quick test and it doesn't work for me as expected:
I added two repositories:

http://rpm111.example.com, key: https://test11.com
http://rpm222.example.com, https://test22.com

The output of the template is:

[foreman_register1]
name=foreman_register1
baseurl=''
enabled=1
type=rpm-md
EOF
  echo gpgcheck=1 >> /tmp/foreman_registration.repo
  echo gpgkey=https://test22.com >> /tmp/foreman_registration.repo

I don't see the second repo set anywhere
There is no baseurl
The gpgkey for repo2 is used for repo1

…n warning

nadjaheitmann · 2024-06-06T07:32:52Z

I did quick test and it doesn't work for me as expected: I added two repositories:

http://rpm111.example.com, key: https://test11.com
http://rpm222.example.com, https://test22.com

The output of the template is:

[foreman_register1]
name=foreman_register1
baseurl=''
enabled=1
type=rpm-md
EOF
  echo gpgcheck=1 >> /tmp/foreman_registration.repo
  echo gpgkey=https://test22.com >> /tmp/foreman_registration.repo

1. I don't see the second repo set anywhere

2. There is no `baseurl`

3. The gpgkey for repo2 is used for repo1

There was a small bug which affected the UI. Can you retest, please?

stejskalleos

🍏 LGTM

UI Form is working
API accepts new parameters correctly
Old parameters work as well, but logs the deprecation message
Repos are set correctly, and the CentOS stream 9 machine has been registered successfully

stejskalleos · 2024-06-07T07:58:13Z

Thanks @nadjaheitmann

nadjaheitmann · 2024-06-07T08:26:13Z

Thanks for testing @stejskalleos and @Manisha15

pr-processor bot added the Not yet reviewed label May 28, 2024

nadjaheitmann force-pushed the 37440_readd_api_params branch from 16f57b8 to 87e5376 Compare May 28, 2024 10:29

stejskalleos self-requested a review May 28, 2024 12:13

stejskalleos self-assigned this May 28, 2024

nadjaheitmann referenced this pull request May 28, 2024

Fixes #37440 - Enable multiple repositories for host registration

f04d47c

stejskalleos reviewed May 30, 2024

View reviewed changes

ofedoren reviewed May 31, 2024

View reviewed changes

nadjaheitmann force-pushed the 37440_readd_api_params branch 4 times, most recently from a82fa16 to ae67cf5 Compare June 5, 2024 14:37

stejskalleos requested changes Jun 5, 2024

View reviewed changes

pr-processor bot added Waiting on contributor and removed Not yet reviewed labels Jun 5, 2024

stejskalleos requested changes Jun 5, 2024

View reviewed changes

Refs #37440 - Readd host registration repo parameters with deprecatio…

a68f861

…n warning

nadjaheitmann force-pushed the 37440_readd_api_params branch from ae67cf5 to a68f861 Compare June 6, 2024 07:28

pr-processor bot added Needs re-review and removed Waiting on contributor labels Jun 6, 2024

github-actions bot added the UI label Jun 6, 2024

nadjaheitmann requested a review from stejskalleos June 6, 2024 10:45

stejskalleos approved these changes Jun 7, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Jun 7, 2024

stejskalleos merged commit fc26ab9 into theforeman:develop Jun 7, 2024
63 of 67 checks passed

nadjaheitmann deleted the 37440_readd_api_params branch June 9, 2024 05:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refs #37440 - Readd host registration repo parameters with deprecatio… #10187

Refs #37440 - Readd host registration repo parameters with deprecatio… #10187

nadjaheitmann commented May 28, 2024

nadjaheitmann commented May 28, 2024 •

edited

Loading

stejskalleos May 30, 2024

ofedoren May 31, 2024

nadjaheitmann Jun 3, 2024 •

edited

Loading

nadjaheitmann Jun 5, 2024 •

edited

Loading

stejskalleos May 30, 2024

nadjaheitmann May 31, 2024

nadjaheitmann May 31, 2024 •

edited

Loading

ofedoren May 31, 2024

nadjaheitmann Jun 3, 2024

nadjaheitmann Jun 5, 2024

ofedoren May 31, 2024

stejskalleos left a comment

stejskalleos left a comment

nadjaheitmann commented Jun 6, 2024

stejskalleos left a comment

stejskalleos commented Jun 7, 2024

nadjaheitmann commented Jun 7, 2024 •

edited

Loading

Refs #37440 - Readd host registration repo parameters with deprecatio… #10187

Refs #37440 - Readd host registration repo parameters with deprecatio… #10187

Conversation

nadjaheitmann commented May 28, 2024

nadjaheitmann commented May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadjaheitmann Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

nadjaheitmann Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadjaheitmann May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stejskalleos left a comment

Choose a reason for hiding this comment

stejskalleos left a comment

Choose a reason for hiding this comment

nadjaheitmann commented Jun 6, 2024

stejskalleos left a comment

Choose a reason for hiding this comment

stejskalleos commented Jun 7, 2024

nadjaheitmann commented Jun 7, 2024 • edited Loading

nadjaheitmann commented May 28, 2024 •

edited

Loading

nadjaheitmann Jun 3, 2024 •

edited

Loading

nadjaheitmann Jun 5, 2024 •

edited

Loading

nadjaheitmann May 31, 2024 •

edited

Loading

nadjaheitmann commented Jun 7, 2024 •

edited

Loading