Update default of internal attribute in service template #251

hsong-rh · 2018-08-10T15:24:58Z

Change default value of internal attribute in service template to false.

hsong-rh · 2018-08-10T16:58:15Z

@bzwei @gmcculloug @bdunne @carbonin Please review.

bdunne · 2018-08-10T17:17:06Z

db/migrate/20180810144738_update_default_internal_attribute.rb

+  end
+
+  def up
+    change_column :service_templates, :internal, :boolean


In this case you would add_column

@bdunne the column already exists

I don't fully understand. We may not need this line at all. We don't change the column property, but only migrate the values.

The column is already a boolean type. Why are we doing this change_column at all?

We only change the default value here. I will remove change_column

bdunne · 2018-08-10T17:18:59Z

db/migrate/20180810144738_update_default_internal_attribute.rb

+  end
+
+  def down
+    change_column :service_templates, :internal, :boolean


In this case you would use remove_column.

bdunne · 2018-08-10T17:20:02Z

db/migrate/20180810144738_update_default_internal_attribute.rb

+
+  def down
+    change_column :service_templates, :internal, :boolean
+    say_with_time("Set ServiceTemplate internal") do


Since the column is being removed, there is no data migration required.

bzwei · 2018-08-10T17:20:48Z

spec/migrations/20180810144738_update_default_internal_attribute_spec.rb

+
+  migration_context :up do
+    it "sets internal to false when type is not ServiceTemplateTransformationPlan" do
+      st = service_template_stub.create!(:type => 'OtherServiceTemplateTransformationPlan')


you need to create two service templates, one with type ServiceTemplateTransformationPlan, the other not.

Like @gmcculloug mentioned, the ServiceTemplateTransformationPlan type has been tested in PR #238. I'll only keep the test for up migration.

You'll still need to test for ServiceTemplateTransformationPlan to prove it does not get modified by this migration.

bzwei · 2018-08-10T17:32:51Z

db/migrate/20180810144738_update_default_internal_attribute.rb

+    end
+  end
+
+  def down


We don't need down. It was a bug. We should set the column to nil at all.

+1 I don't think this migration should have a down because we don't want to put the data back into a bad state.

gmcculloug · 2018-08-10T17:37:41Z

PR #238 is already doing the work to set the internal column for ServiceTemplateTransformationPlan types.

Can this just be an up migration that does: ServiceTemplate.where(:internal => nil).update_all(:internal => false)

carbonin · 2018-08-10T17:39:46Z

db/migrate/20180810144738_update_default_internal_attribute.rb

+  class ServiceTemplate < ActiveRecord::Base
+    self.inheritance_column = :_type_disabled
+
+    include MigrationStubHelper


I think this is only needed if you're using relations in the stub. Since you're only editing rows in the service_templates table I don't think this is needed.

@Fryguy @bdunne keep me honest here.

bdunne · 2018-08-10T18:02:50Z

This is getting confusing. Are you trying to:
A: set column defaults in Postgres
-or-
B: update existing records from nil to false?

If A, prefer default_value_for in the model.
If B, I think that should just be an edit to the migration added in #238

Fryguy · 2018-08-10T18:06:00Z

This seems unnecessary as nil is falsey...existing record will be internal => nil, which is falsey.

bdunne · 2018-08-10T18:10:17Z

This seems unnecessary as nil is falsey...existing record will be internal => nil, which is falsey.

The only problem is on the model side to query for non-internal records you need ServiceTemplate.where(:internal => [nil, false]) rather than ServiceTemplate.where(:internal => false)

gmcculloug · 2018-08-10T18:11:14Z

The public_service_templates scope is not returning any records where internal = nil. And since we are using default_value_for on the base ServiceTemplate model it seemed better to update the nil records instead of adjusting the scope to compensate for nil.

hsong-rh · 2018-08-10T18:20:07Z

@bdunne We are in the case of B: update existing records. So you want me reopen #238 to add this?

bdunne · 2018-08-10T18:25:49Z

Since the migration in #238 hasn't been released yet and it feels like it was missed in the original migration, I think that's the right direction. @carbonin @Fryguy any objections to that?

hsong-rh · 2018-08-10T20:14:17Z

@carbonin @Fryguy Do you agreed with @bdunne? If you do, I will reopen #238 to add this fix.

Fryguy · 2018-08-10T20:43:41Z

Oh, that's annoying :/

Fryguy · 2018-08-10T20:45:35Z

Ok, then this PR is correct. We should not modify existing migrations that change their functionality after they've been merged, because it's likely they've already been executed by developers. (modifying for performance or bug fixes doesn't change what the original migration did, so those are ok, but this doesn't fall into that category, IMO).

Fryguy · 2018-08-10T20:46:48Z

spec/migrations/20180810144738_update_default_internal_attribute_spec.rb

+
+      migrate
+
+      expect(st.reload.internal).to be_falsey


This is wrong, because without your PR, this test would pass anyway (because nil is falsey). This should say .to be false to show you want the literal false value

https://bugzilla.redhat.com/show_bug.cgi?id=1610729

JPrause · 2018-08-13T13:42:41Z

@miq-bot add_label blocker

JPrause · 2018-08-13T13:43:16Z

@hsong-rh if this can be backported, can you add the gaprindashvili/yes label.

miq-bot · 2018-08-13T13:43:32Z

Checked commit hsong-rh@296d944 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

db/migrate/20180810144738_update_default_internal_attribute.rb

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

hsong-rh · 2018-08-13T13:48:50Z

@bdunne @Fryguy Please review and merge. Today is the last day for v2v build. Thanks.

Fryguy · 2018-08-13T13:51:55Z

@JPrause we don't backport migrations

JPrause · 2018-08-13T14:21:18Z

@Fryguy thanks,..that was my bad. I overlooked what repo this was. 😢

hsong-rh · 2018-08-15T14:14:01Z

@Fryguy Can you approve and merge this? thanks.

hsong-rh force-pushed the update_internal_default branch from 6aaf80a to bacf917 Compare August 10, 2018 16:45

bdunne requested changes Aug 10, 2018

View reviewed changes

bzwei reviewed Aug 10, 2018

View reviewed changes

carbonin reviewed Aug 10, 2018

View reviewed changes

hsong-rh force-pushed the update_internal_default branch from bacf917 to c0d9f20 Compare August 10, 2018 18:02

Fryguy requested changes Aug 10, 2018

View reviewed changes

hsong-rh force-pushed the update_internal_default branch 2 times, most recently from fdd3301 to b1bbdbc Compare August 12, 2018 19:24

Update default of internal attribute in service template

296d944

https://bugzilla.redhat.com/show_bug.cgi?id=1610729

hsong-rh force-pushed the update_internal_default branch from b1bbdbc to 296d944 Compare August 13, 2018 13:39

miq-bot added the blocker label Aug 13, 2018

bdunne added enhancement gaprindashvili/no and removed blocker labels Aug 13, 2018

bdunne added data transformation labels Aug 13, 2018

bdunne approved these changes Aug 13, 2018

View reviewed changes

Fryguy merged commit ffe25c1 into ManageIQ:master Aug 21, 2018

Fryguy added this to the Sprint 93 Ending Aug 27, 2018 milestone Aug 21, 2018

hsong-rh deleted the update_internal_default branch August 21, 2018 15:30

Fryguy added v2v and removed transformation labels Feb 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update default of internal attribute in service template #251

Update default of internal attribute in service template #251

hsong-rh commented Aug 10, 2018

hsong-rh commented Aug 10, 2018

bdunne Aug 10, 2018

bzwei Aug 10, 2018

bzwei Aug 10, 2018

carbonin Aug 10, 2018

hsong-rh Aug 10, 2018

bdunne Aug 10, 2018

bdunne Aug 10, 2018

bzwei Aug 10, 2018

hsong-rh Aug 10, 2018

bzwei Aug 10, 2018

bzwei Aug 10, 2018

carbonin Aug 10, 2018

gmcculloug commented Aug 10, 2018

carbonin Aug 10, 2018

bdunne commented Aug 10, 2018

Fryguy commented Aug 10, 2018

bdunne commented Aug 10, 2018

gmcculloug commented Aug 10, 2018

hsong-rh commented Aug 10, 2018

bdunne commented Aug 10, 2018

hsong-rh commented Aug 10, 2018

Fryguy commented Aug 10, 2018

Fryguy commented Aug 10, 2018

Fryguy Aug 10, 2018

JPrause commented Aug 13, 2018

JPrause commented Aug 13, 2018

miq-bot commented Aug 13, 2018

hsong-rh commented Aug 13, 2018

Fryguy commented Aug 13, 2018

JPrause commented Aug 13, 2018

hsong-rh commented Aug 15, 2018

Update default of internal attribute in service template #251

Update default of internal attribute in service template #251

Conversation

hsong-rh commented Aug 10, 2018

hsong-rh commented Aug 10, 2018

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

gmcculloug commented Aug 10, 2018

Choose a reason for hiding this comment

bdunne commented Aug 10, 2018

Fryguy commented Aug 10, 2018

bdunne commented Aug 10, 2018

gmcculloug commented Aug 10, 2018

hsong-rh commented Aug 10, 2018

bdunne commented Aug 10, 2018

hsong-rh commented Aug 10, 2018

Fryguy commented Aug 10, 2018

Fryguy commented Aug 10, 2018

Choose a reason for hiding this comment

JPrause commented Aug 13, 2018

JPrause commented Aug 13, 2018

miq-bot commented Aug 13, 2018

hsong-rh commented Aug 13, 2018

Fryguy commented Aug 13, 2018

JPrause commented Aug 13, 2018

hsong-rh commented Aug 15, 2018