Fixes #37395 - New Hosts page perform build actions #10146

parthaa · 2024-04-29T20:13:47Z

Added build actions to work with the new hosts page.

Steps to verify

Have a foreman with multiple hosts
Check out this PR
go to http:///new/hosts
Select 1 host and choose Build Hosts from the kebab
Play with different options

app/controllers/hosts_controller.rb

ekohl · 2024-05-03T09:27:51Z

app/controllers/hosts_controller.rb

@@ -533,20 +548,39 @@ def submit_multiple_build
        message = _('Failed to redeploy %s.') % host
        Foreman::Logging.exception(message, error)
        success = false
+        message = _('Failed to reboot %s.') % @host
+        warning(message)
+        Foreman::Logging.exception(message, error)


Logging 2 messages for 1 exception feels redundant. Was this intentional?

ianballou · 2024-05-03T19:07:47Z

Trying to re-run redmine issues seems to not be working.

ekohl · 2024-05-10T10:49:32Z

app/controllers/api/v2/hosts_bulk_actions_controller.rb

+          manager = BulkHostsManager.new(hosts: @hosts)
+          missed_hosts = manager.build(reboot: reboot)
+          if missed_hosts.empty?
+            process_response(true, { :message => _("Built %{host_count} %{hosts}") % { :host_count => @hosts.count, :hosts => 'host'.pluralize(@hosts.count) }})


This breaks translations because host or hosts will not be correctly translated. There are also languages which have a more complex plural system and n_() is the correct function to use. See https://projects.theforeman.org/projects/foreman/wiki/Translating#Extracting-strings as well:

Suggested change

process_response(true, { :message => _("Built %{host_count} %{hosts}") % { :host_count => @hosts.count, :hosts => 'host'.pluralize(@hosts.count) }})

process_response(true, { :message => n_("Built %s host", "Built %s hosts", @hosts.count) % @hosts.count })

I don't know if @hosts.count will be cached or result in 2 queries, but the previous code already had that too.

ekohl · 2024-05-10T10:50:03Z

app/controllers/api/v2/hosts_bulk_actions_controller.rb

+          if missed_hosts.empty?
+            process_response(true, { :message => _("Built %{host_count} %{hosts}") % { :host_count => @hosts.count, :hosts => 'host'.pluralize(@hosts.count) }})
+          else
+            process_response(false, { :message => _("The following hosts failed the build operation: %s") % missed_hosts.map(&:name).to_sentence})


Does to_sentence actually work with translations?

we are not doing to_sentence anymore

ekohl · 2024-05-10T10:50:27Z

app/controllers/api/v2/hosts_bulk_actions_controller.rb

+        end
+
+        if message.blank?
+          process_response(true, { :message => _("Rebuilt configuration for %{host_count} %{hosts}") % { :host_count => @hosts.count, :hosts => 'host'.pluralize(@hosts.count) }})


Same problem as above, so please use n_()

jeremylenz

Thanks for re-implementing again @parthaa

I like the BulkHostsManager approach muuuuuch better than the previous tries! 🚀

A few code comments here; I'll retest once ewoud's and my comments are addressed

app/controllers/api/v2/hosts_bulk_actions_controller.rb

app/controllers/hosts_controller.rb

app/services/bulk_hosts_manager.rb

...ets/javascripts/react_app/components/HostsIndex/BulkActions/buildHosts/BulkBuildHostModal.js

jeremylenz

Just a couple more minor comments :)

jeremylenz · 2024-05-20T20:13:49Z

webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/buildHosts/actions.js

+    handleSuccess: response => {
+      if (handleSuccess) handleSuccess(response);
+    },


Any reason this won't work?

Suggested change

handleSuccess: response => {

if (handleSuccess) handleSuccess(response);

},

handleSuccess,

APIRequest.js already handles it for us if you don't pass handleSuccess: https://github.com/theforeman/foreman/blob/develop/webpack/assets/javascripts/react_app/redux/API/APIRequest.js#L20

jeremylenz · 2024-05-20T20:14:55Z

webpack/assets/javascripts/react_app/components/HostsIndex/index.js

+      onClick={setModalOpen}
+      isDisabled={selectedCount === 0}
+    >
+      {__('Build Hosts')}


As per the mockup:

Suggested change

{__('Build Hosts')}

{__('Build management')}

(and don't forget sentence case, not title case)

jeremylenz · 2024-05-20T20:15:47Z

webpack/assets/javascripts/react_app/components/HostsIndex/BulkActions/buildHosts/actions.js

+    successToast: () => {
+      if (params.rebuild_configuration)
+        return __('Rebuilt the configuration for hosts.');
+      return __('Built hosts.');


Would it be helpful to have different wording here if you checked reboot?
maybe

Hosts set to build and rebooting.

ekohl · 2024-05-21T08:44:19Z

app/controllers/api/v2/hosts_bulk_actions_controller.rb

+        message = ''
+        all_fails.each_pair do |key, values|
+          unless values.empty?
+            message << ((N_("%{config_type} rebuild failed for host: %{host_names}.",


N_() is for "mark to translate" while n_() is plural form

Suggested change

message << ((N_("%{config_type} rebuild failed for host: %{host_names}.",

message << ((n_("%{config_type} rebuild failed for host: %{host_names}.",

@evgeni and I were talking about this the other day and looking at https://projects.theforeman.org/projects/foreman/wiki/Translating, and it was unclear which one is correct for "I want to use the percent thingies, but not necessarily pluralize any words." The examples that use %s all use N_().

ekohl · 2024-05-21T08:45:47Z

app/controllers/api/v2/hosts_bulk_actions_controller.rb

+        end
+
+        if message.blank?
+          process_response(true, { :message => N_("Rebuilt configuration for %{host_count} %{hosts}") % { :host_count => @hosts.count, :hosts => 'host'.pluralize(@hosts.count) }})


Please also correct the translations here:

Suggested change

process_response(true, { :message => N_("Rebuilt configuration for %{host_count} %{hosts}") % { :host_count => @hosts.count, :hosts => 'host'.pluralize(@hosts.count) }})

process_response(true, { :message => n_("Rebuilt configuration for %s host", "Rebuilt configuration for %s hosts, @hosts.count) % @hosts.count })

parthaa · 2024-05-28T21:26:23Z

Should be addressed. Also added some toast magic for handling failed hosts

When you click on the failed hosts you get

parthaa · 2024-05-28T21:28:07Z

To fail I changed => bulk_hosts_manager with something like

      begin
        host.built(false) if host.build? && host.token_expired?
        host.setBuild
        host.power.reset if reboot && host.supports_power_and_running?
      raise "Life ...." if host.id.even?
      rescue => error

jeremylenz

LGTM, tested again and works well 👍

APGHA

ekohl · 2024-05-30T11:26:00Z

app/controllers/api/v2/hosts_bulk_actions_controller.rb

+        failed_host_ids = []
+        all_fails.each_pair do |key, values|
+          failed_host_ids.concat(values.map(&:id)) unless values.empty?
+        end


Ruby 2.7 introduced flat_map which can greatly simplify this code:

Suggested change

failed_host_ids = []

all_fails.each_pair do |key, values|

failed_host_ids.concat(values.map(&:id)) unless values.empty?

end

failed_host_ids = all_fails.flat_map { |_key, value| values.map(&:id) }

updated. @ekohl also let me know if you are ok with the changes. Trying to get this merged.

ekohl

Not a complete review, but I think at least the translation part (that I focused on) is correct now. However, I am concerned about the lack of tests for this.

ekohl · 2024-05-30T17:56:39Z

app/controllers/api/v2/hosts_bulk_actions_controller.rb

+        failed_host_ids.compact!
+        failed_host_ids.uniq!
+
+        if failed_host_ids.blank?


In normal Ruby blank? is not defined. Is that a rails extension? I'd still prefer the native empty?:

Suggested change

if failed_host_ids.blank?

if failed_host_ids.empty?

blank is from Rails and seems more useful to me, since it handles more than .empty. It's used all over Foreman so I think it's fine here.

I prefer blank? over .empty? to handle nil cases but that being said in this case if failed_host_ids are nil we have other problems. So going to commit this change

app/services/bulk_hosts_manager.rb

ekohl

Not a complete review: I focused on the translation part and that now looks correct. I'll leave the JS and other parts to people who know that stuff.

Refs #37395 - Made the hosts controller handle both json and html Refs #37395 - Fails correctly with url Refs #37395 - Moved error notifications to a separate file

jeremylenz

Per offline discussion: ACK so this can be merged now and it will unblock Samir. Please add test coverage in a followup PR 👍

thanks @parthaa !

pr-processor bot added Not yet reviewed Waiting on contributor labels Apr 29, 2024

github-actions bot added the UI label Apr 29, 2024

parthaa force-pushed the build-host branch 2 times, most recently from f5abb42 to 966eebb Compare May 2, 2024 19:16

ekohl reviewed May 3, 2024

View reviewed changes

parthaa force-pushed the build-host branch from 966eebb to 20c30b1 Compare May 3, 2024 18:15

pr-processor bot removed the Waiting on contributor label May 3, 2024

parthaa force-pushed the build-host branch 2 times, most recently from 1f9bea4 to 5cc9598 Compare May 7, 2024 20:28

ekohl requested changes May 10, 2024

View reviewed changes

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

jeremylenz reviewed May 14, 2024

View reviewed changes

parthaa force-pushed the build-host branch from 5cc9598 to 4e7b158 Compare May 17, 2024 03:34

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

jeremylenz reviewed May 20, 2024

View reviewed changes

ekohl reviewed May 21, 2024

View reviewed changes

stejskalleos assigned jeremylenz and MariaAga May 28, 2024

jeremylenz previously approved these changes May 29, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label May 29, 2024

parthaa force-pushed the build-host branch from f36a426 to 7ecad2c Compare May 29, 2024 19:06

ekohl reviewed May 30, 2024

View reviewed changes

parthaa dismissed jeremylenz’s stale review via baee838 May 30, 2024 15:00

parthaa force-pushed the build-host branch from 7ecad2c to baee838 Compare May 30, 2024 15:00

ekohl reviewed May 30, 2024

View reviewed changes

parthaa force-pushed the build-host branch 2 times, most recently from 39eaac7 to 29dc337 Compare June 4, 2024 20:35

parthaa requested a review from ekohl June 5, 2024 14:16

ekohl previously approved these changes Jun 6, 2024

View reviewed changes

Fixes #37395 - New Hosts page perform build actions

ac34b4c

Refs #37395 - Made the hosts controller handle both json and html Refs #37395 - Fails correctly with url Refs #37395 - Moved error notifications to a separate file

parthaa dismissed ekohl’s stale review via ac34b4c June 6, 2024 14:02

parthaa force-pushed the build-host branch from 29dc337 to ac34b4c Compare June 6, 2024 14:02

jeremylenz approved these changes Jun 6, 2024

View reviewed changes

jeremylenz merged commit 0057f0a into theforeman:develop Jun 6, 2024
67 checks passed

parthaa deleted the build-host branch June 6, 2024 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37395 - New Hosts page perform build actions #10146

Fixes #37395 - New Hosts page perform build actions #10146

parthaa commented Apr 29, 2024 •

edited

ekohl May 3, 2024

ianballou commented May 3, 2024

ekohl May 10, 2024

parthaa May 30, 2024

ekohl May 10, 2024

parthaa May 30, 2024

ekohl May 10, 2024

parthaa May 30, 2024

jeremylenz left a comment

jeremylenz left a comment

jeremylenz May 20, 2024

parthaa May 30, 2024

jeremylenz May 20, 2024

jeremylenz May 20, 2024 •

edited

ekohl May 21, 2024

jeremylenz May 21, 2024 •

edited

ekohl May 21, 2024

parthaa commented May 28, 2024

parthaa commented May 28, 2024

jeremylenz left a comment

ekohl May 30, 2024

parthaa May 30, 2024

ekohl left a comment

ekohl May 30, 2024

jeremylenz May 30, 2024

parthaa Jun 4, 2024 •

edited

ekohl left a comment

jeremylenz left a comment

	process_response(true, { :message => _("Built %{host_count} %{hosts}") % { :host_count => @hosts.count, :hosts => 'host'.pluralize(@hosts.count) }})
	process_response(true, { :message => n_("Built %s host", "Built %s hosts", @hosts.count) % @hosts.count })

	message << ((N_("%{config_type} rebuild failed for host: %{host_names}.",
	message << ((n_("%{config_type} rebuild failed for host: %{host_names}.",

Fixes #37395 - New Hosts page perform build actions #10146

Fixes #37395 - New Hosts page perform build actions #10146

Conversation

parthaa commented Apr 29, 2024 • edited

Choose a reason for hiding this comment

ianballou commented May 3, 2024

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

Choose a reason for hiding this comment

jeremylenz left a comment

Choose a reason for hiding this comment

jeremylenz left a 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

jeremylenz May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylenz May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthaa commented May 28, 2024

parthaa commented May 28, 2024

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthaa Jun 4, 2024 • edited

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

jeremylenz left a comment

Choose a reason for hiding this comment

parthaa commented Apr 29, 2024 •

edited

jeremylenz May 20, 2024 •

edited

jeremylenz May 21, 2024 •

edited

parthaa Jun 4, 2024 •

edited