Link 'miq_servers' and 'miq_tasks' tables (make miq_task belongs_to miq_server) #14701

yrudman · 2017-04-08T00:22:13Z

Part of #14698 - adding former Job specific columns to Task view

We linked miq_server and jobs table in #14385 in order to show server name as Owner column on jobs UI. After merging Job and MiqTask layouts in ManageIQ/manageiq-ui-classic#242 UI we will need to show Owner column (which is server name) on tasks UI

Another benefit of having miq_server linked to task (in addition of providing the same UI for job management screens) : in multy-server environment it would be clear on which server we need to check logs if something wrong with task

Column miq_task.miq_server_id already exists but was not in use.

This PR i:

make miq_task belongs_to :miq_server
assign miq_server_id to task when task status changed to Active
data migration jobs.miq_server_id to miq_tasks.miq_server_id

@miq-bot add-label wip, core, refactoring

yrudman · 2017-04-11T14:40:27Z

@gtanzillo @Fryguy could you review

Fryguy · 2017-05-09T20:18:38Z

app/models/miq_task.rb

@@ -295,6 +296,17 @@ def process_cancel
    end
  end

+  def attributes_hash_when_state_change(state, attributes = {})
+    case attributes[:state] = state


I'm not a fan of mixing an assignment and a case at the same time. These can just be two separate lines and it would be clear.

ok, will change

Fryguy · 2017-05-09T20:19:53Z

app/models/miq_task.rb

+      attributes[:started_on] = Time.now.utc if started_on.nil?
+      attributes[:miq_server_id] = MiqServer.my_server.try(:id)
+    when STATE_FINISHED
+      attributes[:miq_server_id] = MiqServer.my_server.try(:id)


What do you do in the else case? Is there even an else case?

started_on is new column for MiqTask and idea here is to record timestamp when task become active for the first time. I think going forward there may be more states (in addition to active which could be added to case.
I think we do not need else case here at this time.

gtanzillo · 2017-05-09T20:20:54Z

app/models/miq_task.rb

+    case attributes[:state] = state
+    when STATE_ACTIVE
+      attributes[:started_on] = Time.now.utc if started_on.nil?
+      attributes[:miq_server_id] = MiqServer.my_server.try(:id)


I think this can just be attributes[:miq_server] = MiqServer.my_server. Also, it can be moved out of the case statement.

@gtanzillo ok, will change

Fryguy · 2017-05-09T20:21:52Z

app/models/miq_task.rb

-    attributes = {:state => state, :status => status, :message => message}
-    attributes[:started_on] = Time.now.utc if state == STATE_ACTIVE && started_on.nil?
+    attributes = {:status => status, :message => message}
+    attributes = attributes_hash_when_state_change(state, attributes)


Why build up a hash like this for update? You can just do

self.status = status self.message = message ... save!

@Fryguy agreed, it does look artificial and reasons for that: attempt to avoid repeating the same code

It looks like it would be more simple if I would follow you advice and remove new method

Fryguy · 2017-05-09T20:23:05Z

app/models/miq_task.rb

+  def attributes_hash_when_state_change(state, attributes = {})
+    case attributes[:state] = state
+    when STATE_ACTIVE
+      attributes[:started_on] = Time.now.utc if started_on.nil?


If you go with just settings methods, then you do self.started_on ||= Time.now.utc

Fryguy · 2017-05-09T20:23:57Z

db/migrate/20170410130134_copy_server_id_from_jobs_to_miq_tasks.rb

+  end
+
+  class MiqTask < ActiveRecord::Base
+    has_one :job, :class_name => 'CopyServerIdFromJobsToMiqTasks::MiqTask'


There's no need to link these together because you don't actually use the relationship below. Same goes for Job#miq_task belongs_to up above.

Fryguy · 2017-05-09T20:24:29Z

spec/migrations/20170410130134_copy_server_id_from_jobs_to_miq_tasks_spec.rb

+    it "copies data from 'jobs.miq_server_id' to 'miq_tasks.miq_server_id'" do
+      task = task_stub.create!(:name => task_name)
+      migration_stub(:Job).create!(:miq_server_id => server_id, :miq_task_id => task.id)
+      migrate


Same comments from the other PR

Blank lines around migrate

create a let for the migration_stub(:Job)

ok, will do

Fryguy · 2017-05-09T20:26:59Z

spec/migrations/20170410130134_copy_server_id_from_jobs_to_miq_tasks_spec.rb

+describe CopyServerIdFromJobsToMiqTasks do
+  let(:task_name) { "Hello Test Task" }
+  let(:task_stub) { migration_stub(:MiqTask) }
+  let(:server_id) { 1111 }


Don't hardcode a server id. You can generate one by using ArRegion.anonymous_class_with_ar_region.rails_sequence_start

Fryguy · 2017-05-09T20:27:37Z

spec/models/miq_task_spec.rb

+
+  describe "#attributes_hash_when_state_change" do
+    let(:miq_task) { FactoryGirl.create(:miq_task_plain) }
+    before(:each) do


Prefer before over before(:each) as each is the default.

Fryguy · 2017-05-09T20:28:20Z

spec/models/miq_task_spec.rb

+  describe "#attributes_hash_when_state_change" do
+    let(:miq_task) { FactoryGirl.create(:miq_task_plain) }
+    before(:each) do
+      @miq_task = FactoryGirl.create(:miq_task_plain)


Why are you creating two miq_tasks (one via a let, and one via an instance var)?

It looks like @miq_task is not used

forgot to remove

Fryguy · 2017-05-09T20:31:16Z

app/models/miq_task.rb

@@ -295,6 +296,17 @@ def process_cancel
    end
  end

+  def attributes_hash_when_state_change(state, attributes = {})


Should this be a private method? You can still run specs on it, just mark the specs as describe "#attributes_hash_when_state_change (private)" do

as discussed above - this method does look confusing, I will add direct update to appropriate methods.

…ted_on attribute when task become active

miq-bot · 2017-05-10T21:27:40Z

Checked commits yrudman/manageiq@2c6d405~...3036171 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 1 offense detected

db/migrate/20170410130134_copy_server_id_from_jobs_to_miq_tasks.rb

❗ - Line 18, Col 15 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.

gtanzillo

👍 LGTM

Fryguy · 2017-05-17T20:21:00Z

db/migrate/20170410130134_copy_server_id_from_jobs_to_miq_tasks.rb

+  def up
+    say_with_time("Copying miq_server_id from jobs table to miq_tasks") do
+      Job.where.not(:miq_task_id => nil).find_each do |job|
+        MiqTask.find(job.miq_task_id).update_attributes!(:miq_server_id => job.miq_server_id)


This might an expensive N+1 during a migration if there are a lot of jobs x tasks. .includes would solve the N+1 on the .find at least.

miq-bot changed the title ~~Link 'miq_servers' and 'miq_tasks' tables (make miq_task belongs_to miq_server)~~ [WIP] Link 'miq_servers' and 'miq_tasks' tables (make miq_task belongs_to miq_server) Apr 8, 2017

miq-bot added core refactoring wip labels Apr 8, 2017

yrudman mentioned this pull request Apr 8, 2017

Merge 'MiqTask' and 'Job' views (add former job specific columns to task view) #14698

Closed

5 tasks

yrudman force-pushed the link-miq_tasks-and-miq-servers-tables branch 2 times, most recently from 0b84b4b to 604e2a8 Compare April 10, 2017 20:06

miq-bot added the sql migration label Apr 10, 2017

yrudman force-pushed the link-miq_tasks-and-miq-servers-tables branch 4 times, most recently from febccc8 to 90d1889 Compare April 11, 2017 14:06

yrudman changed the title ~~[WIP] Link 'miq_servers' and 'miq_tasks' tables (make miq_task belongs_to miq_server)~~ Link 'miq_servers' and 'miq_tasks' tables (make miq_task belongs_to miq_server) Apr 11, 2017

miq-bot removed the wip label Apr 11, 2017

yrudman force-pushed the link-miq_tasks-and-miq-servers-tables branch from 90d1889 to afab251 Compare April 11, 2017 14:32

miq-bot added the fine/no label Apr 11, 2017

miq-bot assigned Fryguy Apr 17, 2017

Fryguy reviewed May 9, 2017

View reviewed changes

gtanzillo reviewed May 9, 2017

View reviewed changes

Fryguy reviewed May 9, 2017

View reviewed changes

link miq_server and miq_task tables and update miq_server_id and star…

2c6d405

…ted_on attribute when task become active

yrudman added 2 commits May 10, 2017 17:23

added migration jobs.miq_server_id to miq_tasks.miq_server.id

eceac9b

added test coverage for MiqTask#state_active and #update_status"

3036171

yrudman force-pushed the link-miq_tasks-and-miq-servers-tables branch from afab251 to 3036171 Compare May 10, 2017 21:23

gtanzillo approved these changes May 12, 2017

View reviewed changes

Fryguy reviewed May 17, 2017

View reviewed changes

Fryguy merged commit dced078 into ManageIQ:master May 17, 2017

Fryguy added this to the Sprint 61 Ending May 22, 2017 milestone May 17, 2017

yrudman deleted the link-miq_tasks-and-miq-servers-tables branch May 18, 2017 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Link 'miq_servers' and 'miq_tasks' tables (make miq_task belongs_to miq_server) #14701

Link 'miq_servers' and 'miq_tasks' tables (make miq_task belongs_to miq_server) #14701

yrudman commented Apr 8, 2017 •

edited

yrudman commented Apr 11, 2017

Fryguy May 9, 2017

yrudman May 10, 2017

Fryguy May 9, 2017

yrudman May 10, 2017

gtanzillo May 9, 2017

yrudman May 10, 2017

Fryguy May 9, 2017

yrudman May 10, 2017

yrudman May 10, 2017

Fryguy May 9, 2017

yrudman May 10, 2017

Fryguy May 9, 2017 •

edited

yrudman May 10, 2017

Fryguy May 9, 2017

yrudman May 10, 2017

Fryguy May 9, 2017

yrudman May 11, 2017

Fryguy May 9, 2017

Fryguy May 9, 2017

gtanzillo May 9, 2017

yrudman May 10, 2017

Fryguy May 9, 2017

yrudman May 10, 2017

miq-bot commented May 10, 2017

gtanzillo left a comment

Fryguy May 17, 2017

Link 'miq_servers' and 'miq_tasks' tables (make miq_task belongs_to miq_server) #14701

Link 'miq_servers' and 'miq_tasks' tables (make miq_task belongs_to miq_server) #14701

Conversation

yrudman commented Apr 8, 2017 • edited

yrudman commented Apr 11, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy May 9, 2017 • edited

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

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

miq-bot commented May 10, 2017

gtanzillo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrudman commented Apr 8, 2017 •

edited

Fryguy May 9, 2017 •

edited