Create migrations to add resource_type row #302

juliancheal · 2018-11-08T20:26:19Z

Adds new row
Migrates existing data to the new row

This PR needs to be merged in order see issue for ordering ManageIQ/manageiq#18222

jrafanie · 2018-11-14T23:10:03Z

@juliancheal how hard is it to get all the callers to use the new column? If we merge this, they'll be looking for towhat column, which no longer exists. It feels like we might need to:

add the new column, resource_type
override towhat to read_attribute(:towhat) || read_attribute(:resource_type)
migrate the data from towhat to resource_type
change callers to use resource_type
Drop the override for towhat in step 2

Thoughts?

juliancheal · 2018-11-15T14:09:34Z

@jrafanie

override towhat to read_attribute(:towhat) || read_attribute(:resource_type)

Is a great idea. Apart from tests for this PR and adding that above line, I think in the core and ui repos i've removed all the callers (that i know of) to towhat.

This sounds like a good way to proceed.

jrafanie · 2018-11-16T18:34:34Z

@juliancheal just remind people on the PRs, the order of the PRs and why you're doing it in that order. It's an abnormal procedure so forgetful me or others might question it.

juliancheal · 2018-11-20T16:26:16Z

@juliancheal just remind people on the PRs, the order of the PRs and why you're doing it in that order. It's an abnormal procedure so forgetful me or others might question it.

Created an issue to list the order ManageIQ/manageiq#18222

jrafanie

I only have a few minor comments. 🇺🇸

jrafanie · 2018-11-21T16:46:11Z

db/migrate/20181108195546_migrate_miq_policy_towhat_to_resource_type.rb

+
+  def up
+    say_with_time("Moving MiqPolicy towhat to resource type") do
+      MiqPolicy.all.each do |policy|


I dont think there's too many, but maybe we want find_each

or eventually one sql query MiqPolicy.update_all('resource_type = resource_type')

jrafanie · 2018-11-21T16:47:38Z

db/migrate/20181108195546_migrate_miq_policy_towhat_to_resource_type.rb

+    say_with_time("Moving MiqPolicy towhat to resource type") do
+      MiqPolicy.all.each do |policy|
+        policy.resource_type = policy.towhat
+        policy.save


policy.update(:resource_type => policy.towhat)

jrafanie · 2018-11-21T16:47:49Z

db/migrate/20181108195546_migrate_miq_policy_towhat_to_resource_type.rb

+    say_with_time("Moving MiqPolicy resource type to towhat") do
+      MiqPolicy.all.each do |policy|
+        policy.towhat = policy.resource_type
+        policy.save


same as above

jrafanie · 2018-11-21T16:48:13Z

db/migrate/20181108195945_migrate_conditions_tow_hat_to_resource_type.rb

+    say_with_time("Moving Condition towhat to resource type") do
+      Condition.all.each do |condition|
+        condition.resource_type = condition.towhat
+        condition.save


same as above

jrafanie · 2018-11-21T16:48:20Z

db/migrate/20181108195945_migrate_conditions_tow_hat_to_resource_type.rb

+    say_with_time("Moving Condition resource type to towhat") do
+      Condition.all.each do |condition|
+        condition.towhat = condition.resource_type
+        condition.save


jrafanie · 2018-11-21T16:50:18Z

spec/migrations/20181108195546_migrate_miq_policy_towhat_to_resource_type_spec.rb

+
+      migrate
+
+      expect(miq_policy.reload).to eq(:resource_type => "ContainerImage")


expect(miq_policy.reload.resource_type).to eq("ContainerImage")

jrafanie · 2018-11-21T16:50:31Z

spec/migrations/20181108195546_migrate_miq_policy_towhat_to_resource_type_spec.rb

+
+      migrate
+
+      expect(miq_policy.reload).to eq(:resource_type => "ContainerImage")


same as above

jrafanie · 2018-11-21T16:50:43Z

spec/migrations/20181108195945_migrate_conditions_tow_hat_to_resource_type_spec.rb

+
+      migrate
+
+      expect(condition.reload).to eq(:resource_type => "ContainerImage")


jrafanie · 2018-11-21T16:50:48Z

spec/migrations/20181108195945_migrate_conditions_tow_hat_to_resource_type_spec.rb

+
+      migrate
+
+      expect(condition.reload).to eq(:resource_type => "ContainerImage")


jrafanie

LGTM.

lpichler · 2018-11-21T21:11:17Z

db/migrate/20181108195546_migrate_miq_policy_towhat_to_resource_type.rb

@@ -0,0 +1,21 @@
+class MigrateMiqPolicyTowhatToResourceType < ActiveRecord::Migration[5.0]
+  class MiqPolicy < ActiveRecord::Base
+    self.inheritance_column = :_type_disabled


it is not needed for MiqPolicy as there is no MiqPolicy#type column, see: #4 (comment)

lpichler · 2018-11-21T21:12:09Z

db/migrate/20181108195945_migrate_conditions_tow_hat_to_resource_type.rb

@@ -0,0 +1,21 @@
+class MigrateConditionsTowHatToResourceType < ActiveRecord::Migration[5.0]
+  class Condition < ActiveRecord::Base
+    self.inheritance_column = :_type_disabled


same here , it is not needed #4 (comment)

lpichler · 2018-11-21T21:33:25Z

LGTM and I had also some minor comments 🇨🇿 😄

Adds new row Migrates existing data to the new row

juliancheal · 2018-11-21T23:21:11Z

@jrafanie seeing as you're off next week, shall we put a pin in this until you return?

jrafanie · 2018-11-22T01:04:27Z

LGTM, I can't merge here but it looks good. @juliancheal you can work with @bdunne or @carbonin, I believe if they're good with the approach, it will be fine to do while I'm away.

miq-bot · 2018-11-22T16:24:45Z

Checked commit juliancheal@ae1f208 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. ⭐

Migration of data already happend in PR ManageIQ#302

juliancheal · 2018-12-11T17:26:40Z

@miq-bot add_label technical debt

bdunne · 2018-12-11T18:31:32Z

Any reason not to rename_column? It would be much faster and avoid the triple migration (create, migrate data, delete). We could deprecate_attribute on the model side as well.

juliancheal · 2018-12-11T18:41:39Z

Any reason not to rename_column? It would be much faster and avoid the triple migration (create, migrate data, delete). We could deprecate_attribute on the model side as well.

@bdunne I'm sure there is a reason @jrafanie?

jrafanie · 2018-12-11T18:54:55Z

Any reason not to rename_column? It would be much faster and avoid the triple migration (create, migrate data, delete). We could deprecate_attribute on the model side as well.

The only reason is that we can't rename the column and change the caller code in core and ui-classic all in one PR. The core and ui-classic PRs depend on the resource_type existing so they can't go green until the column is created.

jrafanie · 2018-12-11T18:57:54Z

The idea was to:

create the resource_type column (get it merged)
create the core, ui-classic and schema (data migration) PRs, get them all green, then merge together
then drop the column

bdunne · 2018-12-11T19:29:06Z

My concerns about doing it the proposed way are:

For a short time we'll have 2 columns with the same data (hopefully), if they're different which one is correct?
There's not just 2 repos (core and ui-classic), so merging schema and core (with the deprecation) at the same time would be fairly easy. Merging 3 or more repos at the same time will be much more complicated.
Touching every record seems unnecessary

jrafanie · 2018-12-11T20:11:33Z

The intent is to create the column, leave it unused, get the 3 co-dependent PRs to use/migrate to the new column merged together, then drop the column.

I'm not sure how to do rename column with two other repos needing to use the new or old column based on the schema version. How do you suggest we do that? The core and ui-classic PRs can't become green until the column is renamed so you can't time them to use the correct column until they're green and merged.

juliancheal · 2018-12-11T20:36:11Z

After speaking with @bdunne and @jrafanie we have a plan!

juliancheal · 2018-12-13T16:26:42Z

Closing in favour of #312

juliancheal force-pushed the create_resource_type_column branch from 29bfbe0 to a061d31 Compare November 8, 2018 20:38

juliancheal mentioned this pull request Nov 20, 2018

Remove all instances of towhat ManageIQ/manageiq#18222

Closed

juliancheal force-pushed the create_resource_type_column branch 4 times, most recently from a81d116 to 2b9aca7 Compare November 21, 2018 16:46

jrafanie requested changes Nov 21, 2018

View reviewed changes

juliancheal force-pushed the create_resource_type_column branch 4 times, most recently from 9616d6a to fd8b0bf Compare November 21, 2018 17:38

jrafanie approved these changes Nov 21, 2018

View reviewed changes

lpichler reviewed Nov 21, 2018

View reviewed changes

Create migrations to add resource_type row

ae1f208

Adds new row Migrates existing data to the new row

juliancheal force-pushed the create_resource_type_column branch from fd8b0bf to ae1f208 Compare November 21, 2018 23:20

jrafanie approved these changes Nov 22, 2018

View reviewed changes

juliancheal pushed a commit to juliancheal/manageiq-schema that referenced this pull request Dec 11, 2018

Deletes towhat column

1886d7c

Migration of data already happend in PR ManageIQ#302

juliancheal mentioned this pull request Dec 11, 2018

Deletes towhat column #311

Closed

miq-bot added the technical debt label Dec 11, 2018

juliancheal closed this Dec 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create migrations to add resource_type row #302

Create migrations to add resource_type row #302

juliancheal commented Nov 8, 2018 •

edited

Loading

jrafanie commented Nov 14, 2018 •

edited

Loading

juliancheal commented Nov 15, 2018

jrafanie commented Nov 16, 2018

juliancheal commented Nov 20, 2018

jrafanie left a comment

jrafanie Nov 21, 2018

lpichler Nov 21, 2018

jrafanie Nov 21, 2018

jrafanie Nov 21, 2018

jrafanie Nov 21, 2018

jrafanie Nov 21, 2018

jrafanie Nov 21, 2018

jrafanie Nov 21, 2018

jrafanie Nov 21, 2018

jrafanie Nov 21, 2018

jrafanie left a comment

lpichler Nov 21, 2018 •

edited

Loading

lpichler Nov 21, 2018

lpichler commented Nov 21, 2018

juliancheal commented Nov 21, 2018

jrafanie commented Nov 22, 2018

miq-bot commented Nov 22, 2018

juliancheal commented Dec 11, 2018

bdunne commented Dec 11, 2018

juliancheal commented Dec 11, 2018

jrafanie commented Dec 11, 2018

jrafanie commented Dec 11, 2018

bdunne commented Dec 11, 2018

jrafanie commented Dec 11, 2018

juliancheal commented Dec 11, 2018

juliancheal commented Dec 13, 2018


		migrate

		expect(miq_policy.reload).to eq(:resource_type => "ContainerImage")


		migrate

		expect(condition.reload).to eq(:resource_type => "ContainerImage")

Create migrations to add resource_type row #302

Create migrations to add resource_type row #302

Conversation

juliancheal commented Nov 8, 2018 • edited Loading

jrafanie commented Nov 14, 2018 • edited Loading

juliancheal commented Nov 15, 2018

jrafanie commented Nov 16, 2018

juliancheal commented Nov 20, 2018

jrafanie 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie left a comment

Choose a reason for hiding this comment

lpichler Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpichler commented Nov 21, 2018

juliancheal commented Nov 21, 2018

jrafanie commented Nov 22, 2018

miq-bot commented Nov 22, 2018

juliancheal commented Dec 11, 2018

bdunne commented Dec 11, 2018

juliancheal commented Dec 11, 2018

jrafanie commented Dec 11, 2018

jrafanie commented Dec 11, 2018

bdunne commented Dec 11, 2018

jrafanie commented Dec 11, 2018

juliancheal commented Dec 11, 2018

juliancheal commented Dec 13, 2018

juliancheal commented Nov 8, 2018 •

edited

Loading

jrafanie commented Nov 14, 2018 •

edited

Loading

lpichler Nov 21, 2018 •

edited

Loading