Fixes #23699 - Remediation Support for Host Status #6135

akshay-mendhe · 2018-10-12T22:27:47Z

Hello, I am just confused about what text should we display when the status is "pending installation" or "installation error", as the case may have variety of solutions. Also in case of "Token expired", we can show cancel build and regenerate token again but if same host renders Token expired for many times, then we can tell user to create a new host instead, isn't it?

houndci-bot · 2018-10-12T22:27:57Z

app/helpers/hosts_helper.rb

+      N_("Unknown status")
+    end
+  end
+


Layout/EmptyLinesAroundModuleBody: Extra empty line detected at module body end.

theforeman-bot · 2018-10-12T22:28:02Z

Issues: #23699

ares

I love the idea. Could it be adfed in a more gemeric way so any host status can define it's help texts? Basically the helper would mostly be moved to HostStatus::Base class.

akshay-mendhe · 2018-10-13T14:41:36Z

Hello, @ares Updated the changes.

ares

Thanks, I left a comment with more details, this is going the right direction, just needs few more steps.

ares · 2018-10-13T17:39:28Z

app/models/host_status/build_status.rb

    def self.status_name
      N_("Build")
    end

    def to_label(options = {})
      case to_status
        when PENDING
+          @@build_status_help_text["Pending installation"] = "Wait for sometime and if build fails, rebuild again"


How about adding a method to to_label_help that would have the same case, but instead of label would return the full help. Then we wouldn't need @@ var nor the helper method. In the view we'd check if this methods returns nil or string, in case of string, we'd use it as help.

I guess that means changing the current host page a bit more, but I believe, that would be cleaner. Let me know, if you need help or I should explain more.

app/models/host_status/build_status.rb

+          N_("Check audit log")
+        when "Unknown build status"
+          N_("Unknown status")
+        else


akshay-mendhe · 2018-10-14T09:39:25Z

Hi, @ares updated the changes, but sorry, I didn't get the point related to checking whether the function returns string or nil. As, in view, we have to show help text on only one row i.e. build status, that is why i am using that if condition in view. can you please explain it more.

lzap

Nicely done, couple of comments.

app/models/host_status/build_status.rb

houndci-bot · 2018-10-16T09:20:04Z

app/models/host_status/build_status.rb

@@ -64,7 +64,7 @@ def token_expired?
    def build_errors?
      host && host.build_errors.present?
    end
-  end
+   end


Layout/EndAlignment: end at 67, 3 is not aligned with class at 2, 2.

houndci-bot · 2018-10-16T09:20:04Z

app/helpers/hosts_helper.rb

@@ -517,4 +517,19 @@ def power_status_visible?
  def host_breadcrumb
    breadcrumbs(resource_url: "/api/v2/hosts?thin=true'")
  end
+
+  def remediation_help_text
+      case @host.build_status_label


Layout/IndentationWidth: Use 2 (not 4) spaces for indentation.

houndci-bot · 2018-10-16T09:20:05Z

app/helpers/hosts_helper.rb

@@ -517,4 +517,19 @@ def power_status_visible?
  def host_breadcrumb
    breadcrumbs(resource_url: "/api/v2/hosts?thin=true'")
  end
+


Layout/TrailingWhitespace: Trailing whitespace detected.

lzap · 2018-10-16T11:42:00Z

app/helpers/hosts_helper.rb

+      when "Token expired"
+        N_("Cancel your build and regenerate token by initiating build again")
+      when "Installed"
+        N_("Done!")


Probably: OS installer reported end of installation and rebooted the system

lzap · 2018-10-16T11:42:47Z

app/helpers/hosts_helper.rb

+      when "Installed"
+        N_("Done!")
+      when "Installation error"
+        N_("Check audit log")


Probably: OS installer post script reported failure, check logs (I don't think this can be found in audit but in normal log)

lzap · 2018-10-16T11:43:58Z

app/helpers/hosts_helper.rb

@@ -517,4 +517,19 @@ def power_status_visible?
  def host_breadcrumb
    breadcrumbs(resource_url: "/api/v2/hosts?thin=true'")
  end
+
+  def remediation_help_text
+    case @host.build_status_label


Please use build_status method instead and compare constants rather than text.

lzap · 2018-10-16T11:45:16Z

app/helpers/hosts_helper.rb

+      when "Installation error"
+        N_("Check audit log")
+      when "Unknown build status"
+        N_("Unknown status")


Probably: The host was not scheduled for build yet

lzap · 2018-10-16T11:45:57Z

Few more comments, looks good.

lzap

Apologies, missed two more strings which might be improved I think.

lzap · 2018-10-17T15:22:28Z

app/models/host_status/build_status.rb

+    def remediation_help_text
+      case host.build_status
+        when PENDING
+          N_("Install media and if build fails, rebuild again")


Suggested change

N_("Install media and if build fails, rebuild again")

N_("Installation haven't started yet or in progress")

lzap · 2018-10-17T15:23:11Z

app/models/host_status/build_status.rb

+        when PENDING
+          N_("Install media and if build fails, rebuild again")
+        when TOKEN_EXPIRED
+          N_("Cancel your build and regenerate token by initiating build again")


Suggested change

N_("Cancel your build and regenerate token by initiating build again")

N_("Build token is no longer valid, cancel build mode and enter it again to generate new")

app/models/host_status/build_status.rb

ares

Great, this is what I meant. Just one question, will this work in non build statuses? I think the method remediation_help_text is now defined only there. We should implement a default one in Base class, that would return nil or something. So in view layer, we'd only renser this if remediation_help_text returns some value.

akshay-mendhe · 2018-10-22T14:01:31Z

Hello, @ares and @lzap sorry for the delay, but if we move the method remediation_help_text() to main status file i.e. status.rb then, we will not be able to use build status labels like PENDING, BUILT,etc. or we have to check with strings , build_status_label. Also if I am not wrong, if we try to check whether this method returns string or nil in View, it will always return some or other status as it will always be set, resulting in showing hover text on other things too. Also we cannot pass "Value" from view as parameter to this method, as it is HTML tag and contains other html attributes, so is there any other alternative we can do?

ares · 2018-10-22T15:35:11Z

The point is, you query the status object itself. The method should live in status.rb returnil nil. Then you override the method in build status that looks at self.to_status. It's the same pattern as with to_label method. Any other substatus can override the method. That way all statuses automatically returns nil (inherited) by default and those that provide more details would return strings. Hence checking for nil in views.

houndci-bot reviewed Oct 12, 2018

View reviewed changes

theforeman-bot added Needs testing Not yet reviewed labels Oct 12, 2018

akshay-mendhe force-pushed the remediation_support_host_status branch from 322d862 to 682789d Compare October 12, 2018 22:30

ares reviewed Oct 13, 2018

View reviewed changes

akshay-mendhe force-pushed the remediation_support_host_status branch from 682789d to 57617ff Compare October 13, 2018 14:39

ares requested changes Oct 13, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Oct 13, 2018

akshay-mendhe force-pushed the remediation_support_host_status branch from 57617ff to 1bc8c0e Compare October 14, 2018 09:33

theforeman-bot added Needs re-review and removed Waiting on contributor labels Oct 14, 2018

houndci-bot reviewed Oct 14, 2018

View reviewed changes

app/models/host_status/build_status.rb

N_("Check audit log")

when "Unknown build status"

N_("Unknown status")

else

This comment was marked as resolved.

Sign in to view

akshay-mendhe force-pushed the remediation_support_host_status branch from 1bc8c0e to e21a6e9 Compare October 14, 2018 09:35

akshay-mendhe force-pushed the remediation_support_host_status branch from e21a6e9 to c118de8 Compare October 14, 2018 10:53

lzap requested changes Oct 15, 2018

View reviewed changes

app/models/host_status/build_status.rb Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Needs re-review labels Oct 15, 2018

akshay-mendhe force-pushed the remediation_support_host_status branch from c118de8 to c8004ee Compare October 16, 2018 09:19

theforeman-bot added Needs re-review and removed Waiting on contributor labels Oct 16, 2018

houndci-bot reviewed Oct 16, 2018

View reviewed changes

akshay-mendhe force-pushed the remediation_support_host_status branch 3 times, most recently from 7721a66 to 52702f4 Compare October 16, 2018 09:27

lzap reviewed Oct 16, 2018

View reviewed changes

akshay-mendhe force-pushed the remediation_support_host_status branch 3 times, most recently from 9c1f4fc to b3fed67 Compare October 16, 2018 18:23

lzap requested changes Oct 17, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Oct 17, 2018

akshay-mendhe force-pushed the remediation_support_host_status branch from b3fed67 to 4dff589 Compare October 17, 2018 17:39

theforeman-bot added Needs re-review and removed Waiting on contributor labels Oct 17, 2018

houndci-bot reviewed Oct 17, 2018

View reviewed changes

app/models/host_status/build_status.rb Show resolved Hide resolved

akshay-mendhe force-pushed the remediation_support_host_status branch from 4dff589 to a8b7b20 Compare October 17, 2018 17:42

Fixes #23699 - Remediation Support for Host Status

0e8df52

akshay-mendhe force-pushed the remediation_support_host_status branch from a8b7b20 to 0e8df52 Compare October 18, 2018 06:08

ares reviewed Oct 18, 2018

View reviewed changes

akshay-mendhe closed this Oct 22, 2018

akshay-mendhe mentioned this pull request Nov 27, 2018

Fixes #23699 - Remediation Support for Host Status #6167

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #23699 - Remediation Support for Host Status #6135

Fixes #23699 - Remediation Support for Host Status #6135

akshay-mendhe commented Oct 12, 2018

houndci-bot Oct 12, 2018

theforeman-bot commented Oct 12, 2018

ares left a comment

akshay-mendhe commented Oct 13, 2018

ares left a comment

ares Oct 13, 2018

This comment was marked as resolved.

akshay-mendhe commented Oct 14, 2018

lzap left a comment

houndci-bot Oct 16, 2018

houndci-bot Oct 16, 2018

houndci-bot Oct 16, 2018

lzap Oct 16, 2018

lzap Oct 16, 2018

lzap Oct 16, 2018

lzap Oct 16, 2018

lzap commented Oct 16, 2018

lzap left a comment

lzap Oct 17, 2018

lzap Oct 17, 2018

ares left a comment

akshay-mendhe commented Oct 22, 2018

ares commented Oct 22, 2018

	N_("Install media and if build fails, rebuild again")
	N_("Installation haven't started yet or in progress")

	N_("Cancel your build and regenerate token by initiating build again")
	N_("Build token is no longer valid, cancel build mode and enter it again to generate new")

Fixes #23699 - Remediation Support for Host Status #6135

Fixes #23699 - Remediation Support for Host Status #6135

Conversation

akshay-mendhe commented Oct 12, 2018

Choose a reason for hiding this comment

theforeman-bot commented Oct 12, 2018

ares left a comment

Choose a reason for hiding this comment

akshay-mendhe commented Oct 13, 2018

ares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

akshay-mendhe commented Oct 14, 2018

lzap 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

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

lzap commented Oct 16, 2018

lzap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ares left a comment

Choose a reason for hiding this comment

akshay-mendhe commented Oct 22, 2018

ares commented Oct 22, 2018