Add Puppet proxy (CA) bulk actions by nadjaheitmann · Pull Request #429 · theforeman/foreman_puppet

nadjaheitmann · 2025-09-24T10:52:47Z

@adamruzicka @jeremylenz I have created a draft for the Puppet Proxy update bulk action. It is not more than a draft and I know there is a bunch of things missing (e.g. clearing proxy, CA proxy, tests, ...). I basically took code from Foreman and Katello and mixed it into this. Do you mind checking whether the general approach is the direction we want to go. UI wise, I think this is straight forward, but I was not sure whether

extending the BulkHostManager
adding a new API endpoint to the HostsBulkActionsController
adding the API endpoint itself to the Foreman rather than the foreman_puppet namespace

is the way to go.

I appreciate your opinions.

adamruzicka

Left some notes inline, can't speak much to the frontend

adamruzicka · 2025-09-24T12:01:14Z

+
+        api :PUT, "/hosts/bulk/change_puppet_proxy", N_("Change Puppet Proxy")
+        param_group :bulk_host_ids
+        param :proxy_id, :number, :required => true, :desc => N_("ID of the Puppet proxy to reassign the hosts to")


This being required means it can be used to reassign the host to a different proxy, but this cannot be used to unassign it, correct?

Correct. I have not yet implemented the unassign method, so the implementation details about that one are still open.

adamruzicka · 2025-09-24T12:02:31Z

+              host.puppet_ca_proxy = proxy
+            else
+              host.puppet_proxy = proxy
+            end
+            host.save(:validate => false)


Do we need callbacks to be run or could we take a shortcut and assign the proxy with a single update query?

Not sure, if we need callbacks to be honest. The original action does run some validations about validating the proxy I think: https://github.com/theforeman/foreman_puppet/blob/master/app/controllers/concerns/foreman_puppet/extensions/hosts_controller_extensions.rb#L143

Can the update command also take the validate => false parameter?

nadjaheitmann

Thanks @adamruzicka !

nadjaheitmann · 2025-09-24T12:21:18Z

+
+        api :PUT, "/hosts/bulk/change_puppet_proxy", N_("Change Puppet Proxy")
+        param_group :bulk_host_ids
+        param :proxy_id, :number, :required => true, :desc => N_("ID of the Puppet proxy to reassign the hosts to")


Correct. I have not yet implemented the unassign method, so the implementation details about that one are still open.

nadjaheitmann · 2025-09-24T12:28:00Z

+              host.puppet_ca_proxy = proxy
+            else
+              host.puppet_proxy = proxy
+            end
+            host.save(:validate => false)


Not sure, if we need callbacks to be honest. The original action does run some validations about validating the proxy I think: https://github.com/theforeman/foreman_puppet/blob/master/app/controllers/concerns/foreman_puppet/extensions/hosts_controller_extensions.rb#L143

Can the update command also take the validate => false parameter?

jeremylenz

Did a quick read of the frontend code, a couple comments below

nadjaheitmann · 2026-03-09T16:13:53Z

This PR uses theforeman/foreman@40938e4 , so it will only work with Foreman 3.19+

nadjaheitmann · 2026-04-24T12:45:21Z

@jeremylenz @adamruzicka Can you please re-review the PR? It is not completely in line with what the Bulk actions in Foreman core do/return but it could be implemented in a follow-up. (it does not return a nice link to the failed hosts)

jeremylenz

JS changes LGTM, did not test. 👍

Copilot

Pull request overview

Adds draft bulk actions for Puppet Proxy and Puppet CA Proxy on the Hosts index, including new React modals and a new backend bulk API/controller to apply the changes to selected hosts.

Changes:

Adds Hosts index “Puppet” bulk-action menu items plus four bulk-action modals (change/remove proxy + change/remove CA proxy).
Introduces shared React “common” components and Redux API actions for change/remove operations.
Adds a new ForemanPuppet API controller/routes plus a BulkHostsManager extension and accompanying Ruby tests.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
webpack/src/Extends/Hosts/BulkActions/BulkRemovePuppetProxy/index.js	Scene wiring for “Remove Puppet proxy” modal.
webpack/src/Extends/Hosts/BulkActions/BulkRemovePuppetProxy/tests/index.test.js	Unit test ensuring correct props passed to common remove modal.
webpack/src/Extends/Hosts/BulkActions/BulkRemovePuppetCAProxy/index.js	Scene wiring for “Remove Puppet CA proxy” modal.
webpack/src/Extends/Hosts/BulkActions/BulkRemovePuppetCAProxy/tests/index.test.js	Unit test ensuring correct CA props/messages passed.
webpack/src/Extends/Hosts/BulkActions/BulkRemoveProxyCommon/index.js	Shared “remove proxy” confirmation modal + API dispatch.
webpack/src/Extends/Hosts/BulkActions/BulkRemoveProxyCommon/actions.js	Redux API action for bulk remove endpoint.
webpack/src/Extends/Hosts/BulkActions/BulkRemoveProxyCommon/tests/actions.test.js	Unit tests validating APIActions.put usage.
webpack/src/Extends/Hosts/BulkActions/BulkChangePuppetProxy/index.js	Scene wiring for “Change Puppet proxy” modal.
webpack/src/Extends/Hosts/BulkActions/BulkChangePuppetProxy/tests/index.test.js	Unit test ensuring correct props passed to common change modal.
webpack/src/Extends/Hosts/BulkActions/BulkChangePuppetCAProxy/index.js	Scene wiring for “Change Puppet CA proxy” modal.
webpack/src/Extends/Hosts/BulkActions/BulkChangePuppetCAProxy/tests/index.test.js	Unit test ensuring correct CA props/messages passed.
webpack/src/Extends/Hosts/BulkActions/BulkChangeProxyCommon/index.js	Shared “change proxy” modal with Smart Proxy select + bulk change dispatch.
webpack/src/Extends/Hosts/BulkActions/BulkChangeProxyCommon/actions.js	Redux API actions for fetching proxies + bulk change endpoint.
webpack/src/Extends/Hosts/BulkActions/BulkChangeProxyCommon/tests/actions.test.js	Unit tests validating APIActions.get/put usage.
webpack/src/Extends/Hosts/ActionsBar/index.js	Adds “Puppet” flyout under Hosts index kebab with 4 actions.
webpack/src/Extends/Hosts/ActionsBar/ActionsBar.scss	Styles intended for disabled menu description content.
webpack/global_index.js	Registers fills: Hosts index kebab section + modal fills.
test/services/foreman_puppet/bulk_hosts_manager_test.rb	Unit tests for BulkHostsManager puppet proxy updates.
test/controllers/foreman_puppet/api/v2/hosts_bulk_actions_controller_test.rb	Controller tests for change/remove endpoints and error cases.
lib/foreman_puppet/register.rb	Adds RBAC permission mappings for the new API actions.
lib/foreman_puppet/engine.rb	Includes the BulkHostsManager extension concern into core manager.
config/api_routes.rb	Adds new API routes for bulk change/remove puppet proxy.
app/services/concerns/foreman_puppet/extensions/bulk_hosts_manager.rb	Implements the bulk host update logic for puppet proxy fields.
app/controllers/foreman_puppet/api/v2/hosts_bulk_actions_controller.rb	Implements the bulk API endpoints and response handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-24T15:10:17Z

+      included: {
+        search: fetchBulkParams(),
+      },
+      proxy_id: smartProxyId.split('-')[0],
+      ca_proxy: isCAProxy,
+    };


smartProxyId is stored as a combined string ("<id>-<name>") and later parsed via split('-')[0]. This is brittle (depends on a delimiter and forces string parsing). Prefer using the proxy id itself as the Select value (and keep the label separately) so proxy_id can be sent without parsing.

Copilot · 2026-04-24T15:10:18Z

+            host.puppet_ca_proxy = proxy
+          else
+            host.puppet_proxy = proxy
+          end
+          host.save(validate: false)
+        rescue StandardError => e
+          message = format(_('Failed to set proxy for %{host}.'), host: host)
+          Foreman::Logging.exception(message, e)
+          error_hosts << host.id


host.save(validate: false) can return false (e.g., aborted callbacks) without raising, so failures may be silently treated as success and never added to error_hosts. Consider using save! (and rescuing) or explicitly checking the boolean return value and pushing the host id into error_hosts when the save fails.

One could argue that with validate: false this shouldn't really happen?

I would assume it just works. Not sure what would happen if you e.g. assign a proxy and it is deleted between the check in the API and call the to host.save.

@adamruzicka Any preference what to do with this code? The validation is in line with the original implementation: https://github.com/theforeman/foreman_puppet/blob/master/app/controllers/concerns/foreman_puppet/extensions/hosts_controller_extensions.rb#L73

Not sure what would happen if we change the behavior now. I agree that having the error catching does not make much sense from this perspective. However, removing it also feels wrong somehow.

Can you please take another look? I would like to finish this up and it just keeps growing and growing :)

I'd be fine with keeping it as it was. Sometimes copilot gets overly zealous

adamruzicka

I currently don't have an environment to test this, but code wise it looks good.

Assisted-by: OpenAI Codex <codex@openai.com>

nadjaheitmann · 2026-05-06T09:48:49Z

environ

Thanks @adamruzicka . I accidentally requested another review from you, feel free to ignore that.

pr-processor Bot added the Not yet reviewed label Sep 24, 2025

adamruzicka reviewed Sep 24, 2025

View reviewed changes

pr-processor Bot removed the Not yet reviewed label Sep 24, 2025

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from e2b1c7f to c7dd83c Compare September 24, 2025 12:15

nadjaheitmann commented Sep 24, 2025

View reviewed changes

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 2 times, most recently from ac62ab7 to e980546 Compare September 24, 2025 12:48

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 3 times, most recently from 11c8919 to 4512e86 Compare October 13, 2025 07:52

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from 4512e86 to 2eb2f1a Compare January 21, 2026 17:09

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 3 times, most recently from 4bb7ef6 to 0faf7d6 Compare February 10, 2026 10:44

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 3 times, most recently from 75e0f00 to 7d7c810 Compare February 18, 2026 13:40

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from d68caaf to 0caaa68 Compare March 2, 2026 09:21

jeremylenz reviewed Mar 2, 2026

View reviewed changes

Comment thread webpack/src/Extends/Hosts/ActionsBar/index.js Outdated

Comment thread webpack/src/Extends/Hosts/ActionsBar/index.js Outdated

Comment thread webpack/src/Extends/Hosts/ActionsBar/index.js Outdated

Comment thread webpack/src/Extends/Hosts/ActionsBar/index.js Outdated

nadjaheitmann mentioned this pull request Mar 2, 2026

Fixes #39047 - replace useForemanModal in BulkActions Katello/katello#11626

Merged

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 9 times, most recently from 98fdd79 to dfb9239 Compare April 10, 2026 12:35

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from dfb9239 to 8bc4dc2 Compare April 10, 2026 13:32

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 7 times, most recently from 9208a4c to d6fd7f8 Compare April 24, 2026 12:39

jeremylenz approved these changes Apr 24, 2026

View reviewed changes

adamruzicka requested changes Apr 24, 2026

View reviewed changes

pr-processor Bot added the Waiting on contributor label Apr 24, 2026

adamruzicka requested a review from Copilot April 24, 2026 15:03

Copilot started reviewing on behalf of adamruzicka April 24, 2026 15:03 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

pr-processor Bot added Needs re-review and removed Waiting on contributor labels Apr 27, 2026

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 3 times, most recently from 1268415 to 7db72d3 Compare April 27, 2026 12:06

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 2 times, most recently from 0c74440 to 44484fc Compare May 6, 2026 08:38

pr-processor Bot removed the Needs re-review label May 6, 2026

adamruzicka reviewed May 6, 2026

View reviewed changes

Comment thread app/controllers/foreman_puppet/api/v2/hosts_bulk_actions_controller.rb

Comment thread config/api_routes.rb

Add Puppet proxy (CA) bulk actions

f3e13d0

Assisted-by: OpenAI Codex <codex@openai.com>

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from 44484fc to f3e13d0 Compare May 6, 2026 09:25

nadjaheitmann requested a review from adamruzicka May 6, 2026 09:37

nadjaheitmann changed the title ~~Draft: Add Puppet proxy (CA) bulk actions~~ Add Puppet proxy (CA) bulk actions May 7, 2026

Conversation

nadjaheitmann commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nadjaheitmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeremylenz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nadjaheitmann commented Mar 9, 2026

Uh oh!

nadjaheitmann commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremylenz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nadjaheitmann May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nadjaheitmann commented Sep 24, 2025 •

edited

Loading

nadjaheitmann commented Apr 24, 2026 •

edited

Loading

nadjaheitmann May 6, 2026 •

edited

Loading