Feature/systemstatus 1291 #3549
Conversation
|
I know we talked on Friday - but I'm going to leave the same comments here too. |
| end | ||
|
|
||
| def node_status(cluster) | ||
| c_info = cluster_info cluster |
There was a problem hiding this comment.
Can we use parentheses on method calls? I know Ruby allows this, but I feel like it's more readable with the enclosing ().
c_info = cluster_info(cluster)
| url = ""; | ||
| fetch(url, { headers: { Accept: "text/vnd.turbo-stream.html" } }) |
There was a problem hiding this comment.
I think this URL set to "" is a bit dangerous as this may be used in other places. I'd suggest adding the route to the app/views/layouts/_config.html.erb and then adding the corresponding method to the config.js file to extract the correct URL.
| .then(response => response.ok ? Promise.resolve(response) : Promise.reject(response.text())) | ||
| .then((r) => r.text()) | ||
| .then((html) => replaceHTML("system-status", html)) | ||
| .then(setTimeout(poll, bcPollDelay())) |
There was a problem hiding this comment.
We probably want a new configuration for this, but let's hard code it to 30000 (30 seconds) for now.
| <div class="col-sm-6 col-xs-12 p-2"> | ||
| <div class="col border rounded p-2 text-center"> | ||
| <h4><strong><%= title c %></strong></h4> | ||
| <div>Hello world at <%= Time.now %></div> |
There was a problem hiding this comment.
I don't think there's any need for this div anymore.
| <% @job_clusters.each do |c| %> | ||
| <div class="col-sm-6 col-xs-12 p-2"> | ||
| <div class="col border rounded p-2 text-center"> | ||
| <h4><strong><%= title c %></strong></h4> |
There was a problem hiding this comment.
Be careful with headings. Semantically, they only kinda make sense if there are also h1-h3's on the page. an h4 by itself is going to throw screen reader users off if they don't see others.
If it's a sizing thing you can directly use the h4 bootstrap class to get the appropriate text size.
| active_count = 0 | ||
| job_adapter(cluster).info_all_each do |i| | ||
| if i.status.running? | ||
| active_count += 1 | ||
| end | ||
| end | ||
| active_count |
There was a problem hiding this comment.
Does this work as well? If you find yourself creating/defining variables outside of loops like active_count = 0, you'll often find there's a nicer way to do it in ruby.
job_adapter(cluster).info_all_each.select do |info|
info.status.running?
end.length
| "#{cluster.metadata.title.titleize} Cluster Status" | ||
| end | ||
|
|
||
| def generic_pct(value, total) |
There was a problem hiding this comment.
We can probably just call this percent right?
| @job_adapter = cluster.job_adapter | ||
| @cluster_info = @job_adapter.cluster_info | ||
| @cluster = cluster |
There was a problem hiding this comment.
We don't want member variables in helpers. They should be treated as functional methods - i.e., they don't hold any state, are idempotent and so on.
| @job_clusters = OodAppkit.clusters | ||
| .select(&:job_allow?) | ||
| .reject { |c| c.metadata.hidden } |
There was a problem hiding this comment.
Looks like you pulled this from here? Seems like we shouldn't reimplement it or in fact even need it here - the view can directly use Configuration.job_clusters.each.
ondemand/apps/dashboard/config/configuration_singleton.rb
Lines 115 to 121 in 2121695
Work on #1291