Allow OrchestrationTemplate subclass to customize md5 calculation #17126

miha-plesko · 2018-03-09T10:55:37Z

Currently, md5 checksum is forcibly calculated on entire orchestration template text content:

OrchestrationTemplate.calc_md5(text)

But this is not always feasible. VMware vCloud, for example, yields randomly ordered elements in XML even when describing the very same orchestration template, hence the MD5 checksum differs. Therefore we need to be able to customize the way the MD5 checksum is calculated.

With this commit we make sure that calc_md5 function from the subclass is used:

ManageIQ::Providers::Vmware::CloudManager::OrchestrationTemplate.calc_md5(text)

Only this way we're able to customize what part of the XML will actually be used to calculate MD5 checksum.

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

@miq-bot assign @Ladas
@miq-bot add_label enhancement,gaprindashvili/yes

/cc @bzwei @agrare @gberginc

miha-plesko · 2018-03-09T12:15:39Z

@Ladas I assigned you since you're usually involved in refresher stuff. Please let me know if I'm wrong.

bzwei · 2018-03-09T15:18:12Z

spec/models/orchestration_template_spec.rb

+
+      it "uses parent if type is not present" do
+        expect(ManageIQ::Providers::Vmware::CloudManager::OrchestrationTemplate).not_to receive(:calc_md5)
+        expect(described_class).not_to receive(:calc_md5).at_least(:once)


why not_to? shouldn't it call the parent to calculate the md5?

Ha! Typo! But it seems like .at_least(:once) makes it ignore the not_to since it works both with

.not_to receive(:calc_md5).at_least(:once)

or with

.to receive(:calc_md5).at_least(:once)

Thanks for noticing, I've updated the code.

It actually freaks me out that it works either way, is this something to be reported to rspec guys? I mean, when both .to and .not_to are passing at the same time, it's kind of breaking the core principal of computer science 😄

bzwei · 2018-03-09T15:18:54Z

spec/models/orchestration_template_spec.rb

@@ -36,6 +36,24 @@
        expect(@existing_record).not_to eq(OrchestrationTemplate.find_or_create_by_contents(@query_hash)[0])
        expect(OrchestrationTemplate.count).to eq(2)
      end
+
+      it "uses subclass if type is present" do
+        expect(ManageIQ::Providers::Vmware::CloudManager::OrchestrationTemplate).to receive(:calc_md5).at_least(:once)


why at_lease(:once)? can it be called more than once?

In fact it gets called twice:

1st time in L46 md5s << klass.calc_md5(with_universal_newline(hash[:content]))

2nd time in L56 template = create!(hash.except(:md5)) because of this hook https://github.com/manageiq/manageiq/blob/master/app/models/orchestration_template.rb#L64

Theoretically I could test for :twice but would prefer to keep it more general, should you agree.

Oh, yes, it is called twice. Then I think at_least(:once) is ok.

miha-plesko

Thanks for a review @bzwei , I've updated the code accordingly. Please let me know if you prefer me to use :twice instead of .at_least(:once).

miha-plesko · 2018-03-09T15:30:44Z

spec/models/orchestration_template_spec.rb

@@ -36,6 +36,24 @@
        expect(@existing_record).not_to eq(OrchestrationTemplate.find_or_create_by_contents(@query_hash)[0])
        expect(OrchestrationTemplate.count).to eq(2)
      end
+
+      it "uses subclass if type is present" do
+        expect(ManageIQ::Providers::Vmware::CloudManager::OrchestrationTemplate).to receive(:calc_md5).at_least(:once)


In fact it gets called twice:

1st time in L46 md5s << klass.calc_md5(with_universal_newline(hash[:content]))

2nd time in L56 template = create!(hash.except(:md5)) because of this hook https://github.com/manageiq/manageiq/blob/master/app/models/orchestration_template.rb#L64

Theoretically I could test for :twice but would prefer to keep it more general, should you agree.

miha-plesko · 2018-03-09T15:33:05Z

spec/models/orchestration_template_spec.rb

+
+      it "uses parent if type is not present" do
+        expect(ManageIQ::Providers::Vmware::CloudManager::OrchestrationTemplate).not_to receive(:calc_md5)
+        expect(described_class).not_to receive(:calc_md5).at_least(:once)


Ha! Typo! But it seems like .at_least(:once) makes it ignore the not_to since it works both with

.not_to receive(:calc_md5).at_least(:once)

or with

.to receive(:calc_md5).at_least(:once)

Thanks for noticing, I've updated the code.

miha-plesko · 2018-03-12T07:13:17Z

@bzwei if I understand correctly we only need to hit the green button here, right?

@miq-bot assign @bzwei

bzwei · 2018-03-12T13:37:07Z

spec/models/orchestration_template_spec.rb

+      end
+
+      it "uses parent if type is not present" do
+        expect(ManageIQ::Providers::Vmware::CloudManager::OrchestrationTemplate).not_to receive(:calc_md5)


Do not need this line. It is meaningless.

Currently, md5 checksum is forcibly calculated on entire orchestration template text content: ``` OrchestrationTemplate.calc_md5(text) ``` But this is not always feasible. VMware vCloud, for example, yields randomly ordered elements in XML even when describing the very same orchestration template, hence the MD5 checksum differs. Therefore we need to be able to customize the way the MD5 checksum is calculated. With this commit we make sure that calc_md5 function from the subclass is used: ``` ManageIQ::Providers::Vmware::CloudManager::OrchestrationTemplate.calc_md5(text) ``` Only this way, we're able to customize what part of the XML will actually be used to calculate MD5 checksum. Signed-off-by: Miha Pleško <[email protected]>

miq-bot · 2018-03-14T15:12:02Z

Checked commit miha-plesko@4df94a0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

bzwei · 2018-03-14T20:06:35Z

@gmcculloug can you merge it?

…-md5 Allow OrchestrationTemplate subclass to customize md5 calculation (cherry picked from commit fbdd937) https://bugzilla.redhat.com/show_bug.cgi?id=1559628

simaishi · 2018-03-22T22:22:15Z

Gaprindashvili backport details:

$ git log -1
commit 25209f34a9ea398c073aaea01099084323feb708
Author: Adam Grare <[email protected]>
Date:   Wed Mar 14 17:03:55 2018 -0400

    Merge pull request #17126 from miha-plesko/subclass-can-override-calc-md5
    
    Allow OrchestrationTemplate subclass to customize md5 calculation
    (cherry picked from commit fbdd937efefcbc407d32136183417b95de761f97)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1559628

miq-bot assigned Ladas Mar 9, 2018

miq-bot added enhancement gaprindashvili/yes labels Mar 9, 2018

miha-plesko changed the title ~~Allow OrchestraionTemplate subclass to customize md5 calculation~~ Allow OrchestrationTemplate subclass to customize md5 calculation Mar 9, 2018

miha-plesko mentioned this pull request Mar 9, 2018

Prevent vapp templates from being duplicated ManageIQ/manageiq-providers-vmware#209

Merged

bzwei reviewed Mar 9, 2018

View reviewed changes

miha-plesko force-pushed the subclass-can-override-calc-md5 branch from 928981a to b448e6c Compare March 9, 2018 15:34

miha-plesko commented Mar 9, 2018

View reviewed changes

miq-bot assigned bzwei and unassigned Ladas Mar 12, 2018

bzwei reviewed Mar 12, 2018

View reviewed changes

miha-plesko force-pushed the subclass-can-override-calc-md5 branch from b448e6c to 4df94a0 Compare March 14, 2018 15:09

bzwei approved these changes Mar 14, 2018

View reviewed changes

agrare self-assigned this Mar 14, 2018

agrare merged commit fbdd937 into ManageIQ:master Mar 14, 2018

agrare added this to the Sprint 82 Ending Mar 26, 2018 milestone Mar 14, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Mar 22, 2018

miha-plesko deleted the subclass-can-override-calc-md5 branch January 7, 2019 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow OrchestrationTemplate subclass to customize md5 calculation #17126

Allow OrchestrationTemplate subclass to customize md5 calculation #17126

miha-plesko commented Mar 9, 2018

miha-plesko commented Mar 9, 2018

bzwei Mar 9, 2018

miha-plesko Mar 9, 2018

miha-plesko Mar 9, 2018

bzwei Mar 9, 2018

miha-plesko Mar 9, 2018

bzwei Mar 9, 2018

miha-plesko left a comment

miha-plesko Mar 9, 2018

miha-plesko Mar 9, 2018

miha-plesko commented Mar 12, 2018

bzwei Mar 12, 2018

miha-plesko Mar 14, 2018

miq-bot commented Mar 14, 2018

bzwei commented Mar 14, 2018

simaishi commented Mar 22, 2018

Allow OrchestrationTemplate subclass to customize md5 calculation #17126

Allow OrchestrationTemplate subclass to customize md5 calculation #17126

Conversation

miha-plesko commented Mar 9, 2018

miha-plesko commented Mar 9, 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

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Mar 14, 2018

bzwei commented Mar 14, 2018

simaishi commented Mar 22, 2018