Fixes #38456 - Hosts bulk action: Disassociate hosts by lfu · Pull Request #10560 · theforeman/foreman

lfu · 2025-06-03T17:25:37Z

Add Disassociate hosts for new hosts page.

To test
Select hosts - Disassociate hosts from Kebab menu.

lfu · 2025-06-03T17:26:02Z

lfu · 2025-06-03T17:26:17Z

ekohl

I like the bulk action, but I have questions about the preview. What if the user selects all entries, also those on other pages. Will it render potentially 100s or 1000s of hosts?

ekohl · 2025-06-03T18:00:19Z

No branded naming please

This is the message from mock page.
How about change it to server?

If you change it to Foreman, the branding plugin will take care of changing it to Satellite downstream.

ekohl · 2025-06-03T18:01:34Z

We also support VMs that aren't associated so I don't think you should explicitly mention physical.

Also, the title needs to be translatable.

Physical hosts is what is used in the existing old page and in the mock page.
Any suggestion if we do need to change it?

ekohl · 2025-06-03T18:02:12Z

Why a magic 7 here?

Suggested change

apiUrl: `${HOSTS_API_PATH}?per_page=7`,

apiUrl: HOSTS_API_PATH,

Or did you intend per_page=all to avoid pagination? If so, I think that should be passed via defaultParams and not as part of the URL.

I'll take responsibility for this, as I assume it was copied and pasted from one of the wizards on the same page. A per_page = 7 was the starting page size that looks the best with these modals. It doesn't make sense to start with the default per_page of 20 in this case, because they don't all fit on the screen. However, the per_page may not be relevant here, if there's no table involved.

It may be. On the non-react all host page you can also select all items. Also, the user may have changed the page size on the overview to be more than the default. So I'd suggest per_page=all to avoid pagination.

ekohl · 2025-06-03T18:07:38Z

Does it make sense to also list the name of the compute resource and possibly the type of the compute resource? If so, a table view already makes more sense.

I added information about compute resource when it's available.

Removed host details. Only host count would be displayed.

ekohl · 2025-06-03T18:12:15Z

This doesn't work with translations. Please use n_() to translate with respecting plurals. See https://projects.theforeman.org/projects/foreman/wiki/Translating#Extracting-strings for more information.

lfu · 2025-06-03T19:13:11Z

I like the bulk action, but I have questions about the preview. What if the user selects all entries, also those on other pages. Will it render potentially 100s or 1000s of hosts?

You asked many questions that I have kept asking myself when working on this.
Could we restrict it to 5 entries in the preview list?

lfu · 2025-06-05T17:23:29Z

... Could we restrict it to 5 entries in the preview list?

TreeView would display a scrollbar if the list is long.

lfu · 2025-06-13T14:20:51Z

Could we have a review here?

jeremylenz · 2025-06-13T15:05:46Z

You asked many questions that I have kept asking myself when working on this.
Could we restrict it to 5 entries in the preview list?

On the Packages wizards (and also Errata I think) we only show the badge count for hosts, and don't let you expand the tree or see the individual hosts at all. If we do that here, it'd be consistent so I'd be fine with that.

lfu · 2025-06-13T20:36:38Z

Updated.

ekohl

Naming wise I think we should avoid physical vs virtual. We have many people who have virtual machines but don't use compute resources.

It would be really good if we had some automated tests for this. Could you have a look at creating those so we build up a regression test suite?

ekohl · 2025-06-25T16:28:55Z

I think this text could be improved on what it actually does. The "To avoid deleting" part is really confusing because we already have a setting destroy_vm_on_host_delete that already prevents deletion of the VM on deletion from Foreman.

I'm not sure what a good text actually is though. Perhaps ask our writers for input?

Repeating my suggestion from offline discussion:

This will disassociate the host in Foreman from its compute resource. After disassociating, a host can be deleted from Foreman without affecting its virtual machine. Hosts without a compute resource will be excluded.

ekohl · 2025-06-25T17:10:17Z

Thinking more: is this a good moment to consider the naming? I know it was already there, but perhaps this is clearer what it actually does:

Suggested change

api :PUT, "/hosts/bulk/disassociate", N_("Disassociate hosts")

api :PUT, "/hosts/bulk/disassociate", N_("Disassociate compute resources")

My reasoning is that the hosts part doesn't add anything because we're already on the hosts overview. The new subject makes it clear what will happen.

Does this change apply only here? Or all places where Disassociate hosts is displayed?

Looks like there is only one place now:

foreman/app/helpers/host_description_helper.rb

Line 18 in 9ff8631

{ :action => [_('Disassociate Hosts'), multiple_disassociate_hosts_path], :priority => 600 },

I lean to not updating the old texts because I'm not sure where else that's referred to.

Thanks for looking into this @ekohl 👍

I think this task really means disassociate hosts from their compute resources.
If that is correct, then disassociate hosts makes more sense than disassociate compute resources.

I lean to not updating the old texts because I'm not sure where else that's referred to.

I mean Disassociate hosts in the action menu and the modal.
But Disassociate hosts seems right to me at this moment.

My point is that you're on the hosts page, so that bit should be implicit. But disassociate from what? Disassociate from compute resources is the answer. I left out the word from for brevity.

Perhaps Maria can weigh in on the UX aspect of it. Or @jeremylenz cpuld be a tie breaker

api :PUT, "/hosts/bulk/disassociate" implies that hosts are already involved, so IMO "Disassociate compute resources" is sufficient. It could be short for any number of things ('disassociate compute resources from their hosts?' 'disassociate [hosts] from compute resources?') but we don't need all of those words.

ekohl · 2025-06-30T08:52:56Z

Should we keep this consistent with the action description?

Suggested change

process_response(true, { :message => n_("Updated %{count} host: Disassociated from VM",

"Updated %{count} hosts: Disassociated from VM",

process_response(true, { :message => n_("Updated %{count} host: Disassociated from compute resource",

"Updated %{count} hosts: Disassociated from compute resource",

ekohl · 2025-06-30T09:41:16Z

Perhaps also avoid virtual vs physical in variable naming?

Suggested change

const virtualHosts = selectedResults?.filter(

const hostsWithComputeResource = selectedResults?.filter(

ekohl · 2025-06-30T09:41:51Z

I think Applicable could be confusing. Perhaps write out what it actually is?

Suggested change

name: __('Applicable hosts'),

name: __('Hosts associated to compute resources'),

ekohl · 2025-06-30T09:49:55Z

Can we keep these sorted alphabetically?

I also wonder why it doesn't use the easier way. https://guides.rubyonrails.org/routing.html#http-verb-constraints says match is only useful if you want to match multiple methods:

In general, you should use the get, post, put, patch, and delete methods to constrain a route to a particular verb. There is a match method that you could use with the :via option to match multiple verbs at once:

So you could simplify this to:

Suggested change

match 'hosts/bulk/disassociate', :to => 'hosts_bulk_actions#disassociate', :via => [:put]

put 'hosts/bulk/disassociate', :to => 'hosts_bulk_actions#disassociate'

ekohl · 2025-06-30T16:53:29Z

I'd be tempted to BulkHostsManager.new(hosts: @hosts).disassociate return the count and then use:

Suggested change

BulkHostsManager.new(hosts: @hosts).disassociate

process_response(true, { :message => n_("Updated %{count} host: Disassociated from compute resource",

"Updated %{count} hosts: Disassociated from compute resource",

@hosts.count) % {count: @hosts.count}})

count = BulkHostsManager.new(hosts: @hosts).disassociate

process_response(true, { :message => n_("Updated %{count} host: Disassociated from compute resource",

"Updated %{count} hosts: Disassociated from compute resource",

count) % {count: count}})

Mostly because it's neatly separating the concern.

ekohl · 2025-06-30T17:17:34Z

I'm going to note this down for my own sanity, because I've been looking at it multiple times.

The method disassociate! is defined here:

foreman/app/models/host/managed.rb

Lines 589 to 593 in f4246cd

def disassociate!

self.uuid = nil

self.compute_resource_id = nil

save!(:validate => false) # don't want to trigger callbacks

end

Now the comment says it doesn't trigger callbacks, but not what https://api.rubyonrails.org/v8.0.2/classes/ActiveRecord/Persistence.html#method-i-save-21 states it does run callbacks. Callbacks triggers audits and hooks, possibly more. That means you can't replace it with update_all since that uses direct SQL to update them all in bulk.

If we already accept that we need to save each individual object then there's still the question of counting the affected rows.

save!() will always return true or raise an exception so there's no way of knowing if it actually modified any records. That means you need to count.

Last note: that I found while looking this up: there is MyClass.in_batches.each_record to avoid instantiating all objects. For large collections of records this can save with memory consumption. That's probably something more for a general BulkHostsManager optimization.

Short summary is that for now this is probably the best we can do.

ekohl · 2025-06-30T17:23:54Z

In my previous message it was implied to also move away from the "physical" naming, just like with virtual.

LadislavVasina1 · 2025-07-01T13:32:45Z

I have tested the basic functionality of this PR here SatelliteQE/robottelo#18859, it passes the test.

lfu · 2025-07-02T13:48:16Z

Could we have another review?

LadislavVasina1 · 2025-07-07T06:12:31Z

Note: also tested with the newest commit, and it still works :)

lfu · 2025-07-08T15:28:54Z

Failed test seems not related.

lfu · 2025-07-11T21:07:52Z

Any update?

ekohl

Untested, but API wise and translation wise I think this is correct.

lfu · 2025-07-16T20:48:03Z

Could we have some care here? Thanks!

jeremylenz

One small comment, just to be consistent with @chris1984 's #10589

Other than that, ACK after tests are green. Perhaps a rebase will help.

jeremylenz · 2025-07-16T21:17:53Z

Suggested change

{__('Confirm')}

{__('Disassociate')}

lfu · 2025-07-17T14:02:51Z

Updated. Waiting...

pr-processor Bot added the Not yet reviewed label Jun 3, 2025

github-actions Bot added the UI label Jun 3, 2025

ekohl reviewed Jun 3, 2025

View reviewed changes

pr-processor Bot removed the Not yet reviewed label Jun 3, 2025

ekohl reviewed Jun 3, 2025

View reviewed changes

lfu force-pushed the 31029_disassociate_hosts branch 4 times, most recently from c077386 to e0979c4 Compare June 5, 2025 17:16

lfu force-pushed the 31029_disassociate_hosts branch 2 times, most recently from 4e68cae to 9fcaad6 Compare June 5, 2025 19:36

lfu force-pushed the 31029_disassociate_hosts branch from 9fcaad6 to 991e777 Compare June 13, 2025 20:30

ekohl requested changes Jun 25, 2025

View reviewed changes

pr-processor Bot added the Waiting on contributor label Jun 25, 2025

ekohl reviewed Jun 25, 2025

View reviewed changes

lfu force-pushed the 31029_disassociate_hosts branch from 991e777 to 096fd52 Compare June 27, 2025 15:27

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

ekohl reviewed Jun 30, 2025

View reviewed changes

lfu force-pushed the 31029_disassociate_hosts branch from 096fd52 to 07834c2 Compare June 30, 2025 15:49

ekohl reviewed Jun 30, 2025

View reviewed changes

lfu force-pushed the 31029_disassociate_hosts branch from 07834c2 to 05cceb6 Compare June 30, 2025 21:47

LadislavVasina1 mentioned this pull request Jul 1, 2025

Disassociate Hosts Coverage [SAT-31029] SatelliteQE/robottelo#18859

Merged

lfu force-pushed the 31029_disassociate_hosts branch 2 times, most recently from 3486310 to 579dd14 Compare July 2, 2025 13:46

lfu mentioned this pull request Jul 11, 2025

Fixes #38416 - Hosts bulk action: Assign organization and Assign location #10591

Merged

ekohl previously approved these changes Jul 14, 2025

View reviewed changes

ekohl requested a review from jeremylenz July 14, 2025 09:58

jeremylenz reviewed Jul 16, 2025

View reviewed changes

lfu dismissed ekohl’s stale review via 72de859 July 16, 2025 21:51

lfu force-pushed the 31029_disassociate_hosts branch 2 times, most recently from 72de859 to 9926688 Compare July 16, 2025 21:53

Fixes #38456 - Hosts bulk action: Disassociate hosts

09b6a9a

lfu force-pushed the 31029_disassociate_hosts branch from 9926688 to 09b6a9a Compare July 17, 2025 12:02

jeremylenz approved these changes Jul 17, 2025

View reviewed changes

jeremylenz merged commit b6097db into theforeman:develop Jul 17, 2025
38 of 39 checks passed

	apiUrl: `${HOSTS_API_PATH}?per_page=7`,
	apiUrl: HOSTS_API_PATH,

	api :PUT, "/hosts/bulk/disassociate", N_("Disassociate hosts")
	api :PUT, "/hosts/bulk/disassociate", N_("Disassociate compute resources")

-        process_response(true, { :message => n_("Updated %{count} host: Disassociated from VM",
-          "Updated %{count} hosts: Disassociated from VM",
+        process_response(true, { :message => n_("Updated %{count} host: Disassociated from compute resource",
+          "Updated %{count} hosts: Disassociated from compute resource",

	const virtualHosts = selectedResults?.filter(
	const hostsWithComputeResource = selectedResults?.filter(

	name: __('Applicable hosts'),
	name: __('Hosts associated to compute resources'),

	match 'hosts/bulk/disassociate', :to => 'hosts_bulk_actions#disassociate', :via => [:put]
	put 'hosts/bulk/disassociate', :to => 'hosts_bulk_actions#disassociate'

	def disassociate!
	self.uuid = nil
	self.compute_resource_id = nil
	save!(:validate => false) # don't want to trigger callbacks
	end

Uh oh!

Conversation

lfu commented Jun 3, 2025

Uh oh!

lfu commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lfu commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekohl left a comment

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!

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!

jeremylenz Jun 3, 2025 • 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!

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!

lfu commented Jun 3, 2025

Uh oh!

lfu commented Jun 5, 2025

Uh oh!

lfu commented Jun 13, 2025

Uh oh!

jeremylenz commented Jun 13, 2025

Uh oh!

lfu commented Jun 13, 2025

Uh oh!

ekohl left a comment

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!

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!

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!

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

lfu commented Jun 3, 2025 •

edited

Loading

lfu commented Jun 3, 2025 •

edited

Loading

jeremylenz Jun 3, 2025 •

edited

Loading

LadislavVasina1 commented Jul 7, 2025 •

edited

Loading