Fixes #746 - Generate all the Host template when click on Build to avoid... #1753

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
@shlomizadok
Member

shlomizadok commented Sep 11, 2014

Generate all the Host template when click on Build to avoid errors during installation.
Includes moving of template_used to Managed model so it can be used elsewhere.

app/models/host/managed.rb
@@ -786,6 +787,46 @@ def validate_media?
managed && pxe_build? && build?
end
+ def template_used(provisioning = nil)
+ kinds = if provisioning == 'image'

This comment has been minimized.

@dLobatog

dLobatog Sep 12, 2014

Member

Please refactor into a separate method available_template_kinds or something like that.

@dLobatog

dLobatog Sep 12, 2014

Member

Please refactor into a separate method available_template_kinds or something like that.

app/helpers/hosts_helper.rb
@@ -223,24 +223,28 @@ def possible_images cr, arch = nil, os = nil
cr.images.where(:architecture_id => arch, :operatingsystem_id => os)
end
- def state s
- s ? ' ' + _("Off") : ' ' + _("On")
+ def state(status)

This comment has been minimized.

@dLobatog

dLobatog Sep 12, 2014

Member

😄

app/models/host/managed.rb
+ @host = self
+ valid_template = unattended_render(template.template)
+ return false if valid_template.blank?
+ rescue

This comment has been minimized.

@dLobatog

dLobatog Sep 12, 2014

Member

Try to rescue the rendering error you are expecting, this will catch all exceptions and we might miss some valuable stack traces.

@dLobatog

dLobatog Sep 12, 2014

Member

Try to rescue the rendering error you are expecting, this will catch all exceptions and we might miss some valuable stack traces.

app/models/host/managed.rb
+
+ def host_templates_valid?
+ return false if self.template_used.empty?
+ self.template_used.each do | template |

This comment has been minimized.

@dLobatog

dLobatog Sep 12, 2014

Member

|template| , actually |used_template| might be better, as we are calling template.template later and it reads weird, used_template.template doesn't look good either but it's a tad more readable

@dLobatog

dLobatog Sep 12, 2014

Member

|template| , actually |used_template| might be better, as we are calling template.template later and it reads weird, used_template.template doesn't look good either but it's a tad more readable

app/models/host/managed.rb
+ end
+ end
+ # returns true if none of the templates failed to render
+ return true

This comment has been minimized.

@dLobatog

dLobatog Sep 12, 2014

Member

No need for a return statement here, just true.

@dLobatog

dLobatog Sep 12, 2014

Member

No need for a return statement here, just true.

app/models/host/managed.rb
+ return false
+ end
+ end
+ # returns true if none of the templates failed to render

This comment has been minimized.

@dLobatog

dLobatog Sep 12, 2014

Member

Write comments in the positive form, e.g: true # if all templates rendered successfully.

@dLobatog

dLobatog Sep 12, 2014

Member

Write comments in the positive form, e.g: true # if all templates rendered successfully.

test/unit/host_test.rb
+ valid_template_host = hosts(:one)
+ assert valid_template_host.host_templates_valid?
+
+ invalid_template_host = hosts(:fault_template)

This comment has been minimized.

@dLobatog

dLobatog Sep 12, 2014

Member

This should be a separate test, also, please use FactoryGirl if the object is not very widely used across the test base. hosts(:one) is, but putting hosts(:fault_template) in the fixtures loads an object for the whole test run when it's used just once.

@dLobatog

dLobatog Sep 12, 2014

Member

This should be a separate test, also, please use FactoryGirl if the object is not very widely used across the test base. hosts(:one) is, but putting hosts(:fault_template) in the fixtures loads an object for the whole test run when it's used just once.

@@ -508,28 +516,8 @@ def process_taxonomy
end
def template_used
- host = params[:host]

This comment has been minimized.

@dLobatog

dLobatog Sep 12, 2014

Member

Now that template_used is a method in the model, I think it'll be good to write an unit test for it.

@dLobatog

dLobatog Sep 12, 2014

Member

Now that template_used is a method in the model, I think it'll be good to write an unit test for it.

app/controllers/hosts_controller.rb
@@ -195,6 +195,14 @@ def puppetrun
redirect_to host_path(@host)
end
+ def review_before_build
+ @host_templates_used = @host.template_used

This comment has been minimized.

@dLobatog

dLobatog Sep 12, 2014

Member

There is no need to declare an instance variable to be maintained, call @host.template_used in the view.

@dLobatog

dLobatog Sep 12, 2014

Member

There is no need to declare an instance variable to be maintained, call @host.template_used in the view.

@@ -195,6 +195,14 @@ def puppetrun
redirect_to host_path(@host)
end
+ def review_before_build

This comment has been minimized.

@dLobatog

dLobatog Sep 12, 2014

Member

This method should have a functional test.

@dLobatog

dLobatog Sep 12, 2014

Member

This method should have a functional test.

This comment has been minimized.

@@ -24,6 +24,7 @@
<%= textarea_f f, :disk, :size => "col-md-8", :rows => "4",
:help_block => _("What ever text(or ERB template) you use in here, would be used as your OS disk layout options If you want to use the partition table option, delete all of the text from this field") %>
+ <%= password_field_tag(:fakepasswordremembered)%>

This comment has been minimized.

@dLobatog

dLobatog Sep 12, 2014

Member

Why?

This comment has been minimized.

@shlomizadok

shlomizadok Sep 12, 2014

Member

A hack for chrome not to autofill password

@shlomizadok

shlomizadok Sep 12, 2014

Member

A hack for chrome not to autofill password

@dLobatog

This comment has been minimized.

Show comment
Hide comment
@dLobatog

dLobatog Sep 12, 2014

Member

@shlomizadok There is some test that needs to be fixed and also the stuff I commented about.
Thanks for taking care of this one, we've been lacking this for 3 years no less. 😑

Member

dLobatog commented Sep 12, 2014

@shlomizadok There is some test that needs to be fixed and also the stuff I commented about.
Thanks for taking care of this one, we've been lacking this for 3 years no less. 😑

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Sep 12, 2014

Member

@elobato Thanks for the review. I will fix your comments and fix the tests. I am also extending the 'review before build' to: a) return a better explanation of what failed (as you have also commented) and b) to test if the smart proxy is available before building.

Member

shlomizadok commented Sep 12, 2014

@elobato Thanks for the review. I will fix your comments and fix the tests. I am also extending the 'review before build' to: a) return a better explanation of what failed (as you have also commented) and b) to test if the smart proxy is available before building.

app/assets/stylesheets/application.scss
@@ -350,3 +350,7 @@ table.table thead .sorting_desc_disabled { background: #f9f9f9 url('sort_desc_di
.config_group, .selected_config_group {
min-height: 30px;
}
+
+#fakepasswordremembered {
+ display: none;

This comment has been minimized.

@ohadlevy

ohadlevy Sep 14, 2014

Member

please extracte to a different PR

@ohadlevy

ohadlevy Sep 14, 2014

Member

please extracte to a different PR

+<div class="modal-dialog">
+ <div class="modal-content">
+ <div class="modal-header">
+ <button type="button" class="close" data-dismiss="modal" aria-hidden="true">&times;</button>

This comment has been minimized.

@ohadlevy

ohadlevy Sep 14, 2014

Member

we have a few helpers in layout_helper to manipulate html, usually its better to keep in a ruby helper as this tend to change across bootstrap versions.

@ohadlevy

ohadlevy Sep 14, 2014

Member

we have a few helpers in layout_helper to manipulate html, usually its better to keep in a ruby helper as this tend to change across bootstrap versions.

+ <div class="modal-body">
+ <div class="alert alert-<%= @host_templates_valid %>">
+ <% if @host_templates_valid == 'success' %>
+ <p><b><span class="glyphicon glyphicon-ok"></span> <%= _("Looking good to build!") %></b></p>

This comment has been minimized.

@ohadlevy

ohadlevy Sep 14, 2014

Member

ditto about icon helper

@ohadlevy

ohadlevy Sep 14, 2014

Member

ditto about icon helper

lib/foreman/renderer.rb
@@ -93,4 +93,4 @@ def unattended_render_to_temp_file content, prefix = id.to_s, options = {}
end
end
-end
+end

This comment has been minimized.

@lzap

lzap Sep 15, 2014

Member

Nitpick: No newline :-D

@lzap

lzap Sep 15, 2014

Member

Nitpick: No newline :-D

app/views/hosts/show.html.erb
@@ -76,3 +76,4 @@
</div>
</div>
</div>
+<div id="review_before_build" class="modal hide fade"></div>

This comment has been minimized.

@lzap

lzap Sep 15, 2014

Member

More newlines, vim users will fight this. If you can, setup your editor not to do this when possible.

@lzap

lzap Sep 15, 2014

Member

More newlines, vim users will fight this. If you can, setup your editor not to do this when possible.

This comment has been minimized.

@shlomizadok

shlomizadok Sep 15, 2014

Member

👍 Fixed on my editor.

@shlomizadok

shlomizadok Sep 15, 2014

Member

👍 Fixed on my editor.

@lzap

This comment has been minimized.

Show comment
Hide comment
@lzap

lzap Sep 15, 2014

Member

Nice, I like that a lot. Do folks think it is good time to add the rendered templates to audit now? Or would it make sense to add all template rendering to audit in more generic way?

Member

lzap commented Sep 15, 2014

Nice, I like that a lot. Do folks think it is good time to add the rendered templates to audit now? Or would it make sense to add all template rendering to audit in more generic way?

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Sep 16, 2014

Member

[test]

Member

shlomizadok commented Sep 16, 2014

[test]

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Sep 16, 2014

Member

@shlomizadok I assume you need to rebase too.

Member

ohadlevy commented Sep 16, 2014

@shlomizadok I assume you need to rebase too.

app/models/host/managed.rb
+ false if self.smart_proxies.empty?
+ self.smart_proxies.each do | smart_proxy |
+ begin
+ return false if Faraday.head("#{smart_proxy.url}/features").status != 200

This comment has been minimized.

@ohadlevy

ohadlevy Sep 16, 2014

Member

Faraday? Why not use the builtin ProxyAPI classes?

@ohadlevy

ohadlevy Sep 16, 2014

Member

Faraday? Why not use the builtin ProxyAPI classes?

This comment has been minimized.

@shlomizadok

shlomizadok Sep 16, 2014

Member

Used Faraday as I just wanted to fetch the head. Guess I can switch to

ProxyAPI::Features.new({:url => smart_proxy.url}).features
@shlomizadok

shlomizadok Sep 16, 2014

Member

Used Faraday as I just wanted to fetch the head. Guess I can switch to

ProxyAPI::Features.new({:url => smart_proxy.url}).features
@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Sep 16, 2014

Contributor

Strings need review before merging as they're not quite right, I can do this once you're closer to the finished code.

Contributor

domcleal commented Sep 16, 2014

Strings need review before merging as they're not quite right, I can do this once you're closer to the finished code.

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Sep 16, 2014

Member

@domcleal -- Rebased + tests have passed. Can you please review (+review the strings) ? Thanks.

Member

shlomizadok commented Sep 16, 2014

@domcleal -- Rebased + tests have passed. Can you please review (+review the strings) ? Thanks.

app/models/host/managed.rb
+ end
+
+ def smart_proxies_available?
+ false if self.smart_proxies.empty?

This comment has been minimized.

@ohadlevy

ohadlevy Sep 16, 2014

Member

missing return?

@ohadlevy

ohadlevy Sep 16, 2014

Member

missing return?

app/models/host/managed.rb
+ self.smart_proxies.each do | smart_proxy |
+ begin
+ return false if ProxyAPI::Features.new({:url => smart_proxy.url}).features.empty?
+ rescue

This comment has been minimized.

@ohadlevy

ohadlevy Sep 16, 2014

Member

should we detect the failures to report it?

@ohadlevy

ohadlevy Sep 16, 2014

Member

should we detect the failures to report it?

+ <% if @host.smart_proxies_available? %>
+ <%= icon_text('ok', _('Smart proxies passed')) %>
+ <% else %>
+ <%= icon_text('remove', _('Smart proxies failed')) %>

This comment has been minimized.

@ohadlevy

ohadlevy Sep 16, 2014

Member

which one failed?

@ohadlevy

ohadlevy Sep 16, 2014

Member

which one failed?

+ </small>
+ </p>
+ </div>
+ <div class="alert alert-warning">

This comment has been minimized.

@ohadlevy

ohadlevy Sep 16, 2014

Member

nitpick: use alert helper?

@ohadlevy

ohadlevy Sep 16, 2014

Member

nitpick: use alert helper?

+ <%= modal_close %>
+ <% if @host_build_status == 'success' %>
+ <%=
+ link_to_if_authorized(_("Build"), hash_for_setBuild_host_path(:id => @host).merge(:auth_object => @host, :permission => 'build_hosts'),

This comment has been minimized.

@ohadlevy

ohadlevy Sep 16, 2014

Member

since the link is the same, maybe extract the differences into a helper?

@ohadlevy

ohadlevy Sep 16, 2014

Member

since the link is the same, maybe extract the differences into a helper?

+ </div>
+ </div>
+</div>
+<script>

This comment has been minimized.

@ohadlevy

ohadlevy Sep 16, 2014

Member

inline scripts are discouraged normally, could you move it into specific js asset ?

@ohadlevy

ohadlevy Sep 16, 2014

Member

inline scripts are discouraged normally, could you move it into specific js asset ?

+ var originalUrl = $('.modal-footer a').attr('href');
+ $("#build_reboot").change(function() {
+ if (this.checked) {
+ $('.modal-footer a').text(__('Reboot and build')).attr("href", originalUrl + "?reboot=true");

This comment has been minimized.

@ohadlevy

ohadlevy Sep 16, 2014

Member

not sure if its in the scope of this PR, but it sounds like we need to change it from GET to PUT.

@ohadlevy

ohadlevy Sep 16, 2014

Member

not sure if its in the scope of this PR, but it sounds like we need to change it from GET to PUT.

lib/foreman/renderer.rb
- :media_path, :param_true?, :param_false?, :match, :indent ]
+ :snippet_if_exists, :ks_console, :root_pass,
+ :multiboot, :jumpstart_path, :install_path, :miniroot,
+ :media_path, :param_true?, :param_false?, :match, :indent ]

This comment has been minimized.

@ohadlevy

ohadlevy Sep 16, 2014

Member

not sure about all the whitespace / style changes, maybe best to separate into different PR, so we know no new functionality was added?

@ohadlevy

ohadlevy Sep 16, 2014

Member

not sure about all the whitespace / style changes, maybe best to separate into different PR, so we know no new functionality was added?

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Sep 17, 2014

Member

[test]

Member

shlomizadok commented Sep 17, 2014

[test]

@dLobatog

This comment has been minimized.

Show comment
Hide comment
@dLobatog

dLobatog Sep 29, 2014

Member

@shlomizadok This needs a rebase

Member

dLobatog commented Sep 29, 2014

@shlomizadok This needs a rebase

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Sep 29, 2014

Member

Rebased. Please note that the code of template_used, has been moved to the model -- with the recent changes of develop branch. see here

Member

shlomizadok commented Sep 29, 2014

Rebased. Please note that the code of template_used, has been moved to the model -- with the recent changes of develop branch. see here

app/assets/javascripts/hosts.js
@@ -0,0 +1,12 @@
+$(function () {
+ $('#review_before_build').on('change', "#build_reboot", function () {
+ var submit = $("#build_form").find("input");

This comment has been minimized.

@ohadlevy

ohadlevy Sep 29, 2014

Member

input matches too many things, try find(":submit") or add a class to it?

@ohadlevy

ohadlevy Sep 29, 2014

Member

input matches too many things, try find(":submit") or add a class to it?

This comment has been minimized.

@shlomizadok

shlomizadok Sep 29, 2014

Member

Added a class

@shlomizadok

shlomizadok Sep 29, 2014

Member

Added a class

+ </p>
+ </div>
+ <div class="alert alert-warning">
+ <b><%= _("Rebuild %s on next reboot?") % @host %></b><br/>

This comment has been minimized.

@ohadlevy

ohadlevy Sep 29, 2014

Member

this is not wrapped in if, and will be visible even if you dont have the ability to reboot

@ohadlevy

ohadlevy Sep 29, 2014

Member

this is not wrapped in if, and will be visible even if you dont have the ability to reboot

This comment has been minimized.

@shlomizadok

shlomizadok Sep 29, 2014

Member

This is the warning message which warns that all the data will be lost.
It appears anyhow, whether the host can reboot or cannot reboot.
The message asks: Rebuild on next reboot? -- same as it was on the alert message.

@shlomizadok

shlomizadok Sep 29, 2014

Member

This is the warning message which warns that all the data will be lost.
It appears anyhow, whether the host can reboot or cannot reboot.
The message asks: Rebuild on next reboot? -- same as it was on the alert message.

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Sep 30, 2014

Member

refactored, rebased and re[test]

Member

shlomizadok commented Sep 30, 2014

refactored, rebased and re[test]

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Oct 2, 2014

Member

[test]

Member

ohadlevy commented Oct 2, 2014

[test]

test/functional/hosts_controller_test.rb
assert_response :found
assert_redirected_to hosts_path
assert_not_nil flash[:error]
assert flash[:error] =~ /Failed to enable #{@host} for installation/
end
+ test 'when host is saved after setBuild and reboot was requested, the flash should inform it' do
+ Host.any_instance.stubs(:setBuild).returns(true)

This comment has been minimized.

@dLobatog

dLobatog Oct 6, 2014

Member

Please refactor this into a context, and use a setup method for the shared code

@dLobatog

dLobatog Oct 6, 2014

Member

Please refactor this into a context, and use a setup method for the shared code

test/functional/hosts_controller_test.rb
+ assert_response :found
+ assert_redirected_to hosts_path
+ assert_not_nil flash[:notice]
+ assert flash[:notice] == "Enabled #{@host} for reboot and rebuild"

This comment has been minimized.

@dLobatog

dLobatog Oct 6, 2014

Member

assert == -> assert_equals a,b same below

@dLobatog

dLobatog Oct 6, 2014

Member

assert == -> assert_equals a,b same below

test/functional/hosts_controller_test.rb
+ true
+ end
+ end
+ Host::Managed.any_instance.stubs(:power).returns(PowerShmocker.new())

This comment has been minimized.

@dLobatog

dLobatog Oct 6, 2014

Member

Expect (not stub) PowerManager::BMC.new(smart_proxies(:bmc)), and make any instance of PowerManager::BMC return true for reset (stubbing 🆗 here).

@dLobatog

dLobatog Oct 6, 2014

Member

Expect (not stub) PowerManager::BMC.new(smart_proxies(:bmc)), and make any instance of PowerManager::BMC return true for reset (stubbing 🆗 here).

This comment has been minimized.

@shlomizadok

shlomizadok Oct 7, 2014

Member

The problem is that PowerManager::BMC.new(smart_proxies(:bmc)) tries to reach the smart proxy and fails. Tried to stub that, without success, which is why I did the power mocker.

@shlomizadok

shlomizadok Oct 7, 2014

Member

The problem is that PowerManager::BMC.new(smart_proxies(:bmc)) tries to reach the smart proxy and fails. Tried to stub that, without success, which is why I did the power mocker.

+ delegate :available_template_kinds, :smart_proxies, :to => :host
+ VALIDATION_TYPES = [:host, :templates, :proxies]
+
+ def initialize(host)

This comment has been minimized.

@dLobatog

dLobatog Oct 6, 2014

Member

This constructor is a tad strange. I would recommend making host_status, templates_status, smart_proxies_status return the status instead of setting instance variables. It'd make it more usable by any controller and unit tests will test and check whether methods are correct, not the object as they do now.

@dLobatog

dLobatog Oct 6, 2014

Member

This constructor is a tad strange. I would recommend making host_status, templates_status, smart_proxies_status return the status instead of setting instance variables. It'd make it more usable by any controller and unit tests will test and check whether methods are correct, not the object as they do now.

+ host.errors.full_messages.each do |error|
+ fail!(:host, error.to_s, host.to_label)
+ end
+ rescue => error

This comment has been minimized.

@dLobatog

dLobatog Oct 6, 2014

Member

What are you rescuing here and in the exception in templates_status?

Maybe test what happens when the proxy is not available, or it doesn't have x or y feature and rescue for those here. For any other case raise a Foreman::Exception so we get an [ERF number](http://projects.theforeman.org/projects/foreman/wiki/ErrorCodes) and we can track the issue.

Same thing below for the template rendering (templates_status) and smart proxy refresh (smart_proxies_status)

@dLobatog

dLobatog Oct 6, 2014

Member

What are you rescuing here and in the exception in templates_status?

Maybe test what happens when the proxy is not available, or it doesn't have x or y feature and rescue for those here. For any other case raise a Foreman::Exception so we get an [ERF number](http://projects.theforeman.org/projects/foreman/wiki/ErrorCodes) and we can track the issue.

Same thing below for the template rendering (templates_status) and smart proxy refresh (smart_proxies_status)

This comment has been minimized.

@ohadlevy

ohadlevy Oct 22, 2014

Member

@elobato the idea here is to show the error in the UI, so we are catching the exception here which are generic (e.g. during host validation proxy command fail of some sort), since afaik we only reach out to proxies during validations, i think its fine to rescue here and raise other proxy errors in the proxy specific section?

@ohadlevy

ohadlevy Oct 22, 2014

Member

@elobato the idea here is to show the error in the UI, so we are catching the exception here which are generic (e.g. during host validation proxy command fail of some sort), since afaik we only reach out to proxies during validations, i think its fine to rescue here and raise other proxy errors in the proxy specific section?

+ end
+ rescue => error
+ fail!(:host, _('Failed to validate %s: %s') % [host, error.to_s], host.to_label)
+ end

This comment has been minimized.

@dLobatog

dLobatog Oct 6, 2014

Member

Missing space in between methods.

@dLobatog

dLobatog Oct 6, 2014

Member

Missing space in between methods.

@dLobatog

This comment has been minimized.

Show comment
Hide comment
@dLobatog

dLobatog Oct 6, 2014

Member

@shlomizadok thanks for your work in here, I've found it very useful. There are a few concerns in the code that need to be addressed before continuing though.

Member

dLobatog commented Oct 6, 2014

@shlomizadok thanks for your work in here, I've found it very useful. There are a few concerns in the code that need to be addressed before continuing though.

@domcleal

This comment has been minimized.

Show comment
Hide comment
@domcleal

domcleal Oct 7, 2014

Contributor

re large_spinner.gif, is that yours or Red Hat's to provide, or is it licensed from elsewhere? (It looks external, which is why I ask.)

Contributor

domcleal commented Oct 7, 2014

re large_spinner.gif, is that yours or Red Hat's to provide, or is it licensed from elsewhere? (It looks external, which is why I ask.)

@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Oct 8, 2014

Member

@domcleal removed large_spinner.gif, as wasn't sure about its license. Thanks for catching that.

Member

shlomizadok commented Oct 8, 2014

@domcleal removed large_spinner.gif, as wasn't sure about its license. Thanks for catching that.

app/services/host_build_status.rb
+ host_status
+ templates_status
+ smart_proxies_status
+ self

This comment has been minimized.

@ohadlevy

ohadlevy Oct 12, 2014

Member

why return self?

@ohadlevy

ohadlevy Oct 12, 2014

Member

why return self?

app/services/host_build_status.rb
+ private
+
+ def host_status
+ self if host.valid?

This comment has been minimized.

@ohadlevy

ohadlevy Oct 12, 2014

Member

ditto

@@ -25,8 +25,7 @@ def memory=(mem)
end
def reset
- poweroff
- start
+ service.vm_action(uuid, :reset)

This comment has been minimized.

@ohadlevy

ohadlevy Oct 12, 2014

Member

IMHO this is a separate bug in a different commit :)

@ohadlevy

ohadlevy Oct 12, 2014

Member

IMHO this is a separate bug in a different commit :)

app/controllers/hosts_controller.rb
@@ -187,10 +188,26 @@ def puppetrun
redirect_to host_path(@host)
end
+ def review_before_build
+ @build = HostBuildStatus.new(@host).host_build_status

This comment has been minimized.

@ohadlevy

ohadlevy Oct 12, 2014

Member

you can avoid returning self if you change this line to

(@build = HostBuildStatus.new(@host)).host_build_status

another simple way, is to add a method on the host object similar to

def build_status
  build_status = HostBuildStatus.new(self)
  build_status.host_build_status # I would rename to method but thats another story
  build_status
end
@ohadlevy

ohadlevy Oct 12, 2014

Member

you can avoid returning self if you change this line to

(@build = HostBuildStatus.new(@host)).host_build_status

another simple way, is to add a method on the host object similar to

def build_status
  build_status = HostBuildStatus.new(self)
  build_status.host_build_status # I would rename to method but thats another story
  build_status
end
app/models/concerns/host_common.rb
+ build_status.check_all_statuses
+ build_status
+ end
+

This comment has been minimized.

@ohadlevy

ohadlevy Oct 22, 2014

Member

why in host common? I don't think it is used in hostgroup?

@ohadlevy

ohadlevy Oct 22, 2014

Member

why in host common? I don't think it is used in hostgroup?

app/services/power_manager/bmc.rb
@@ -28,7 +32,8 @@ def action_map
:off => 'off',
:soft => 'soft',
:cycle => 'cycle',
- :status => 'status'
+ :status => 'status',
+ :ready? => 'ready?'

This comment has been minimized.

@ohadlevy

ohadlevy Oct 22, 2014

Member

do we need the ready? I think you just defined the method ^^

@ohadlevy

ohadlevy Oct 22, 2014

Member

do we need the ready? I think you just defined the method ^^

@mmoll

This comment has been minimized.

Show comment
Hide comment
@mmoll

mmoll Oct 22, 2014

Member

tested (without a Compute Resource), works for me.
One thing I noted, when a proxy is not reachable, the mesage in the UI is:

Proxies
  Error connecting to smart.pro.xy: undefined method `to_sentence' for true:TrueClass.
Member

mmoll commented Oct 22, 2014

tested (without a Compute Resource), works for me.
One thing I noted, when a proxy is not reachable, the mesage in the UI is:

Proxies
  Error connecting to smart.pro.xy: undefined method `to_sentence' for true:TrueClass.
@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Oct 23, 2014

Member

@mmoll - Thanks for testing. I have fixed the error message (though error handling on smart_proxy api should be re-factored too [but that is for another pr])

Member

shlomizadok commented Oct 23, 2014

@mmoll - Thanks for testing. I have fixed the error message (though error handling on smart_proxy api should be re-factored too [but that is for another pr])

app/helpers/hosts_helper.rb
+ def supports_power_and_running(host)
+ return false unless host.compute_resource_id || host.bmc_available?
+ host.power.ready?
+ rescue ProxyAPI::ProxyException

This comment has been minimized.

@ohadlevy

ohadlevy Oct 26, 2014

Member

add an explanation ?

@ohadlevy

ohadlevy Oct 26, 2014

Member

add an explanation ?

This comment has been minimized.

@shlomizadok

shlomizadok Oct 26, 2014

Member

Don't think so. Should return true or false. The rescue is in cases the Proxy is unavailable.

@shlomizadok

shlomizadok Oct 26, 2014

Member

Don't think so. Should return true or false. The rescue is in cases the Proxy is unavailable.

@@ -25,8 +25,8 @@ def memory=(mem)
end
def reset
- poweroff
- start
+ # @TODO: change to poweroff && start upon fix for LibVirt on Fog gem.

This comment has been minimized.

@ohadlevy

ohadlevy Oct 26, 2014

Member

did you add an issue on it / send the patch to fog already?

@ohadlevy

ohadlevy Oct 26, 2014

Member

did you add an issue on it / send the patch to fog already?

This comment has been minimized.

app/assets/javascripts/hosts.js
+ $('#review_before_build').on('click', "#recheck_review", function () {
+ $("#build-review").click();
+ });
+});

This comment has been minimized.

@ohadlevy

ohadlevy Oct 26, 2014

Member

a tidy bit more optimized jquery could be:

 $(function () {
-  $("#build-review").click(function() {
-    $("#review_before_build .modal-body #build_status").html('');
+  var dialog = $("#review_before_build");
+  $("#build-review").click(function () {
+    dialog.find(".modal-body #build_status").html('');
     $('.loading').addClass('visible');
     $.ajax({
-      type:'get',
+      type: 'get',
       url: $(this).attr('data-url'),
-      success: function(result){
-       $("#review_before_build .modal-body #build_status").html(result);
+      success: function (result) {
+        $("#review_before_build").find(".modal-body #build_status").html(result);
       },
-      complete: function(){
+      complete: function () {
         $('.loading').removeClass('visible');
       }
-    })
+    });
   });
-  $('#review_before_build').on('change', "#host_build", function () {
+  dialog.on('change', "#host_build", function () {
     $('#build_form').find('input.submit').val((this.checked) ? (__("Reboot and build")) : (__("Build")));
   });

-  $('#review_before_build').on('click', "#recheck_review", function () {
+  dialog.on('click', "#recheck_review", function () {
     $("#build-review").click();
   });
 });
@ohadlevy

ohadlevy Oct 26, 2014

Member

a tidy bit more optimized jquery could be:

 $(function () {
-  $("#build-review").click(function() {
-    $("#review_before_build .modal-body #build_status").html('');
+  var dialog = $("#review_before_build");
+  $("#build-review").click(function () {
+    dialog.find(".modal-body #build_status").html('');
     $('.loading').addClass('visible');
     $.ajax({
-      type:'get',
+      type: 'get',
       url: $(this).attr('data-url'),
-      success: function(result){
-       $("#review_before_build .modal-body #build_status").html(result);
+      success: function (result) {
+        $("#review_before_build").find(".modal-body #build_status").html(result);
       },
-      complete: function(){
+      complete: function () {
         $('.loading').removeClass('visible');
       }
-    })
+    });
   });
-  $('#review_before_build').on('change', "#host_build", function () {
+  dialog.on('change', "#host_build", function () {
     $('#build_form').find('input.submit').val((this.checked) ? (__("Reboot and build")) : (__("Build")));
   });

-  $('#review_before_build').on('click', "#recheck_review", function () {
+  dialog.on('click', "#recheck_review", function () {
     $("#build-review").click();
   });
 });
@shlomizadok

This comment has been minimized.

Show comment
Hide comment
@shlomizadok

shlomizadok Oct 26, 2014

Member

@ohadlevy tidied the js, rebased and tests are 💚

Member

shlomizadok commented Oct 26, 2014

@ohadlevy tidied the js, rebased and tests are 💚

@ohadlevy

This comment has been minimized.

Show comment
Hide comment
@ohadlevy

ohadlevy Oct 26, 2014

Member

merged as 6e916e5 thanks @shlomizadok!

Member

ohadlevy commented Oct 26, 2014

merged as 6e916e5 thanks @shlomizadok!

@ohadlevy ohadlevy closed this Oct 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment