Allow duplicate nil pxe_image_types during seed (for the current region) #17544

bdunne · 2018-06-06T21:51:20Z

DEPENDS on #16775 for Gaprindashvili backport

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

We want to disallow the same name template in different regions with the
same pxe_image_type. Normally, region id sequences prevent this from
happening but we were treating nil pxe_image_types as the same.
Therefore you could have the "same" pxe_image_type in different regions
for the same name template.

jrafanie · 2018-06-07T11:26:20Z

@bdunne I wonder if we want to have a BZ for this one so we can backport this.

jrafanie

LGTM, maybe just add a BZ link

carbonin · 2018-06-07T13:43:57Z

@jrafanie Should we wait and see if the BZ you're working on would be fixed by this change rather than opening a new one?

jrafanie · 2018-06-07T14:52:57Z

@jrafanie Should we wait and see if the BZ you're working on would be fixed by this change rather than opening a new one?

yeah, good point...let's hold off on merging until we have a possibility of recreating it

jrafanie · 2018-06-07T22:10:20Z

@carbonin @bdunne @Fryguy Here's the changes @d-m-u and I did on top of Brandon's in_my_region change.

carbonin · 2018-06-08T12:53:36Z

spec/models/customization_template_spec.rb

+    let(:customization_template)        { FactoryGirl.create(:customization_template, :pxe_image_type => pxe_image_type) }
+    let(:system_customization_template) { described_class.create!(:system => true, :name => "nick") }
+
+    it "doesn't allow same name in same region" do


Is this true regardless of pxe image type? Should we also test for that?

Yes. Note, we had a test for that but it was merely an extension of this test so we removed it. The table @Fryguy wrote on the whiteboard had two customization templates with the same name in the same region could have any pxe image type. Maybe @bdunne @Fryguy know the use cases better to know if that still makes sense?

Wow, I wrote that completely wrong.

@Fryguy wrote on the whiteboard that two customization templates with the same name in the same region was OUT regardless of it's pxe image type. Right?

@d-m-u feel free to tell me I'm wrong or misremembering, I don't have the whiteboard in front of me.

@Fryguy wrote on the whiteboard that two customization templates with the same name in the same region was OUT regardless of it's pxe image type. Right?

Yeah, that's pretty much exactly what I was asking "Is this true regardless of pxe image type?" 😆

carbonin · 2018-06-08T12:55:15Z

spec/models/customization_template_spec.rb

+
+    it "allow same name, nil pxe_image_types in different regions (for system seeding)" do
+      ctid = ApplicationRecord.id_in_region(system_customization_template.id, ApplicationRecord.my_region_number + 1)
+      expect { described_class.create!(:system => true, :name => "nick", :id => ctid) }.to change(described_class, :count).by(1)


I feel like using the to change() syntax here would allow this test to succeed even if the first template wasn't created. Can we be more explicit and assert that there are two templates with the same name directly?

Also the fact that the first creation is done in a (lazily evaluated) let makes it less clear that it is actually being created.

Yeah, that makes sense. We had this testing the two had the same name originally and I had the brilliant idea of changing it. @d-m-u was right but didn't stop me. 😉

carbonin · 2018-06-08T12:58:05Z

spec/models/customization_template_spec.rb

+      pid =  ApplicationRecord.id_in_region(pxe_image_type.id, MiqRegion.my_region_number + 1)
+      ctid = ApplicationRecord.id_in_region(customization_template.id, MiqRegion.my_region_number + 1)
+      pt2 =  FactoryGirl.create(:pxe_image_type, :id => pid)
+      expect { FactoryGirl.create(:customization_template, :pxe_image_type => pt2, :id => ctid) }.to change(described_class, :count).by(1)


A name needs to be specified in this create call and in the let for customization_template. name is a sequence in the factory so the name of this record won't be the same as customization_template.name

Interesting, let me see if the test fails if I check that correctly. Good 👀

jrafanie · 2018-06-08T21:02:03Z

@carbonin @d-m-u I corrected the tests and hopefully made them easier to read at the expense of more lines added 😱

bdunne

Do we also need a test for "same name, same region, different pxe_image_type"?

jrafanie · 2018-06-11T14:21:33Z

Do we also need a test for "same name, same region, different pxe_image_type"?

Sure, we already have it but it's not clear so I'll rename the test...

    it "doesn't allow same name in same region" do
      FactoryGirl.create(:customization_template, :name => "fred")
      expect { FactoryGirl.create(:customization_template, :name => "fred") }.to raise_error(ActiveRecord::RecordInvalid)
    end

bdunne · 2018-06-11T19:54:48Z

spec/models/customization_template_spec.rb

+      expect { FactoryGirl.create(:customization_template, :name => "fred", :pxe_image_type => pit) }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+
+    it "doesn't allow same name in same region with the different pxe_image_type" do


I think this should be allowed. cc @Fryguy to verify that

jrafanie · 2018-06-11T22:18:53Z

bdunne
I think this should be allowed. cc @Fryguy to verify that

I have validated that @d-m-u knows her validations. We did lots of typos until we got it right but reversed the logic so now you can have the same name customization template with different pxe_image_types regardless of the region.

jrafanie · 2018-06-11T22:20:22Z

app/models/customization_template.rb


  scope :with_pxe_image_type_id, ->(pxe_image_type_id) { where(:pxe_image_type_id => pxe_image_type_id) }
  scope :with_system,            ->(bool = true)       { where(:system => bool) }

+  def not_seeding?
+    !system? || pxe_image_type.present?


Checking both system and presence is overkill since we have a pxe_image_type presence validation unless system? above... I guess it could just check !system

carbonin · 2018-06-12T13:36:54Z

app/models/customization_template.rb


  scope :with_pxe_image_type_id, ->(pxe_image_type_id) { where(:pxe_image_type_id => pxe_image_type_id) }
  scope :with_system,            ->(bool = true)       { where(:system => bool) }

+  def not_seeding?


Can we make the method name not a negative? 😉

Sure, give me a good name for it.

I honestly have no idea why we're seeding customization templates with no pxe_image_type if pxe_image_type is required for validation of a customization template. This whole thing would be so much easier if seeding behaved properly. @bdunne do you know why seeding wants to be different?

Ignoring the validity of creating templates without a pxe_image_type for a second I'd be fine with just seeded_template?. I really just wanted to drop the not_ from the method.

yeah, there were reasons we went negative, but we tried many things so maybe @d-m-u's last magic try will allow us to use if instead of unless...I'll give it a go

@jrafanie The system images are not usable by customers. They're just there as examples that the user can copy. If you want, you can change seeding so that they get seeded into the system customization templates PxeImageType.

https://bugzilla.redhat.com/show_bug.cgi?id=1588417 We want to disallow the same name template in different regions with the same pxe_image_type. Normally, region id sequences prevent this from happening but we were treating nil pxe_image_types as the same. Therefore you could have the "same" pxe_image_type in different regions for the same name template.

jrafanie · 2018-06-12T14:29:50Z

I have no idea... @d-m-u was there and we could only get it to work with if and a negative condition. I was able to mostly reverse the logic and the tests still pass. 🤷‍♂️ I blame the fresh air and pollen during our hike.

carbonin · 2018-06-12T15:38:57Z

Strange, looks like the unique_within_region validator spec failed because we were relying on it being used in this model (which has been removed).

jrafanie · 2018-06-12T17:09:26Z

Strange, looks like the unique_within_region validator spec failed because we were relying on it being used in this model (which has been removed).

Yeah, I was hoping I can just delete it but the test is trying to test STI with two subclass instances with the same template name. I will have to see if I can test that in a class agnostic way.

Since CustomizationTemplate no longer uses the unique within region validation, change the test to use anonymous STI classes hardcoded to use the vms table.

jrafanie · 2018-06-12T20:26:22Z

@carbonin I changed the unique within region spec to use anonymous classes instead of CustomizationTemplate since the latter no longer uses unique within region.

miq-bot · 2018-06-12T20:27:20Z

Checked commits bdunne/manageiq@63f07d5~...4644f3e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

carbonin

Spec failures look unrelated.

jrafanie · 2018-06-12T22:31:38Z

Yeah, I think #17562 broke master but have no idea why

carbonin · 2018-06-13T13:26:18Z

ManageIQ/manageiq-providers-ansible_tower#81 was the issue @jrafanie a fix has been merged so I restarted the tests.

bdunne

👍 LGTM

jrafanie · 2018-06-13T15:08:24Z

ManageIQ/manageiq-providers-ansible_tower#81 was the issue @jrafanie a fix has been merged so I restarted the tests.

😅 🙇 🥇

jrafanie · 2018-06-13T15:12:13Z

I believe this won't backport directly so I'll open a PR for gaprindashvili

jrafanie · 2018-06-13T15:58:24Z

Ah, the gaprindashvili cherry-pick is clean (and tests pass!) if we backport the dependent PR from @d-m-u first.

git cherry-pick -x -m1 c992bac
git cherry-pick -x -m1 e8b923a

Updating this PR and the other one.

Allow duplicate nil pxe_image_types during seed (for the current region) (cherry picked from commit e8b923a) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1591450

simaishi · 2018-06-14T18:22:16Z

Gaprindashvili backport details:

$ git log -1
commit 9deace37264d31721f58a22f464e1a757571807c
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Wed Jun 13 10:35:48 2018 -0400

    Merge pull request #17544 from bdunne/scope_seeding
    
    Allow duplicate nil pxe_image_types during seed (for the current region)
    (cherry picked from commit e8b923a569b443ed250ed47f86e60bccd78b17af)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1591450

Only seed stuff in the current region

63f07d5

bdunne added the bug label Jun 6, 2018

jrafanie requested changes Jun 7, 2018

View reviewed changes

jrafanie approved these changes Jun 7, 2018

View reviewed changes

jrafanie changed the title ~~Only seed stuff in the current region~~ Allow non-unique nil pxe_image_types during seed (for the current region) Jun 7, 2018

jrafanie force-pushed the scope_seeding branch from c05b994 to b7fd5d8 Compare June 8, 2018 02:01

carbonin suggested changes Jun 8, 2018

View reviewed changes

jrafanie force-pushed the scope_seeding branch from b7fd5d8 to 9dcba1a Compare June 8, 2018 21:00

jrafanie force-pushed the scope_seeding branch from 9dcba1a to 49801f2 Compare June 8, 2018 21:06

jrafanie changed the title ~~Allow non-unique nil pxe_image_types during seed (for the current region)~~ Allow duplicate nil pxe_image_types during seed (for the current region) Jun 8, 2018

bdunne commented Jun 11, 2018

View reviewed changes

jrafanie force-pushed the scope_seeding branch from 49801f2 to d379623 Compare June 11, 2018 15:09

bdunne commented Jun 11, 2018

View reviewed changes

jrafanie force-pushed the scope_seeding branch from d379623 to 97a800d Compare June 11, 2018 22:15

jrafanie reviewed Jun 11, 2018

View reviewed changes

jrafanie force-pushed the scope_seeding branch from 97a800d to 9dfe08a Compare June 11, 2018 22:22

carbonin reviewed Jun 12, 2018

View reviewed changes

jrafanie force-pushed the scope_seeding branch from 9dfe08a to 0c2bf68 Compare June 12, 2018 14:28

Use anonymous class in unique_within_region spec

4644f3e

Since CustomizationTemplate no longer uses the unique within region validation, change the test to use anonymous STI classes hardcoded to use the vms table.

gtanzillo approved these changes Jun 12, 2018

View reviewed changes

carbonin approved these changes Jun 12, 2018

View reviewed changes

bdunne commented Jun 13, 2018

View reviewed changes

gtanzillo self-assigned this Jun 13, 2018

gtanzillo added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 13, 2018

gtanzillo merged commit e8b923a into ManageIQ:master Jun 13, 2018

bdunne deleted the scope_seeding branch June 13, 2018 14:47

jrafanie added gaprindashvili/yes core labels Jun 13, 2018

jrafanie mentioned this pull request Jun 13, 2018

Fix for STI scoping across leaves #16775

Merged

simaishi added blocker gaprindashvili/backported and removed gaprindashvili/yes labels Jun 14, 2018

Allow duplicate nil pxe_image_types during seed (for the current region) #17544

Allow duplicate nil pxe_image_types during seed (for the current region) #17544

Conversation

bdunne commented Jun 6, 2018 • edited by jrafanie Loading

jrafanie commented Jun 7, 2018

jrafanie left a comment

Choose a reason for hiding this comment

carbonin commented Jun 7, 2018

jrafanie commented Jun 7, 2018

jrafanie commented Jun 7, 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

jrafanie commented Jun 8, 2018

bdunne left a comment

Choose a reason for hiding this comment

jrafanie commented Jun 11, 2018

Choose a reason for hiding this comment

jrafanie commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie Jun 12, 2018 • edited Loading

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 commented Jun 12, 2018

carbonin commented Jun 12, 2018

jrafanie commented Jun 12, 2018

jrafanie commented Jun 12, 2018

miq-bot commented Jun 12, 2018

carbonin left a comment

Choose a reason for hiding this comment

jrafanie commented Jun 12, 2018

carbonin commented Jun 13, 2018

bdunne left a comment

Choose a reason for hiding this comment

jrafanie commented Jun 13, 2018

jrafanie commented Jun 13, 2018

jrafanie commented Jun 13, 2018 • edited Loading

simaishi commented Jun 14, 2018

bdunne commented Jun 6, 2018 •

edited by jrafanie

Loading

jrafanie Jun 12, 2018 •

edited

Loading

jrafanie commented Jun 13, 2018 •

edited

Loading