Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Look for resources in the same region as the selected template during provisioning. #13045

Merged
merged 1 commit into from
Jan 18, 2017

Conversation

lfu
Copy link
Member

@lfu lfu commented Dec 7, 2016

Check tags, Pxe server and cloud init customization templates in the same region as the template.

Links

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

Steps for Testing/QA

  • Create two remote regions and one master region.
    Central admin should be enabled for the two remote regions.
    Don't create anything in the global region which should be just the replication of remotes.

cc @gmcculloug @bdunne

@@ -1328,8 +1328,12 @@ def get_pxe_server
PxeServer.find_by(:id => get_value(@values[:pxe_server_id]))
end

def get_template
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the get_source_vm method instead. Note that in the case of service catalog items this can return a nil value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_source_vm returns result in MiqHashStruct instead of AR Object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use load_ar_obj to convert MiqHashStruct or AR.

Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the approach looks great. Just have a few questions / comments.

:id => other_region_id,
:pxe_image_type => pxe_image_type)

allow(workflow).to receive(:supports_native_clone?).and_return(true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be an expect?

:pxe_image_type => pxe_image_type)

allow(workflow).to receive(:supports_native_clone?).and_return(true)
expect(workflow.allowed_customization_templates.size).to eq(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better for this to match_array to verify that we end up with the correct template.

@@ -1328,8 +1328,12 @@ def get_pxe_server
PxeServer.find_by(:id => get_value(@values[:pxe_server_id]))
end

def get_template
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call this source? In the subclasses it is not always a VmOrTemplate

@@ -1328,8 +1328,12 @@ def get_pxe_server
PxeServer.find_by(:id => get_value(@values[:pxe_server_id]))
end

def get_template
VmOrTemplate.find_by(:id => get_value(@values[:src_vm_id]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be hard coded to VmOrTemplate?

@lfu lfu force-pushed the in_region_1400991 branch 4 times, most recently from e887bbc to 57c3155 Compare December 8, 2016 16:55
@lfu lfu force-pushed the in_region_1400991 branch 2 times, most recently from ebd8e78 to 4b045b1 Compare December 9, 2016 23:20
…on during provisioning.

Only check for resources in the same region where the selected template locates.
https://bugzilla.redhat.com/show_bug.cgi?id=1400991
@lfu lfu force-pushed the in_region_1400991 branch from 4b045b1 to 6c9058a Compare December 13, 2016 14:28
@miq-bot
Copy link
Member

miq-bot commented Dec 13, 2016

Checked commit lfu@6c9058a with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
4 files checked, 0 offenses detected
Everything looks good. 🍰

@lfu
Copy link
Member Author

lfu commented Jan 18, 2017

@gmcculloug Can this PR be merged?

@bdunne bdunne merged commit a25bb6c into ManageIQ:master Jan 18, 2017
@bdunne bdunne added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 18, 2017
imtayadeway added a commit to imtayadeway/manageiq that referenced this pull request Jan 18, 2017
Changes brought about in ManageIQ#13045
prevented this value from populating in the way expected. It appears
that in order for the provision workflow to populate this value now, it
must find the source vm/template, which needs to be supplied in the
options when you create the provision request.
imtayadeway added a commit to imtayadeway/manageiq that referenced this pull request Jan 18, 2017
With the merging of ManageIQ#13045,
this spec was failing because the provision workflow failed to find the
source. Adding the id to the options when creating the provision request
appears to fix it.
simaishi pushed a commit that referenced this pull request Jan 19, 2017
Look for resources in the same region as the selected template during provisioning.
(cherry picked from commit a25bb6c)

https://bugzilla.redhat.com/show_bug.cgi?id=1414886
@simaishi
Copy link
Contributor

Euwe backport details:

$ git log -1
commit 4a41661f5bac25aa83ac920845d841b2b060a4a6
Author: Brandon Dunne <[email protected]>
Date:   Wed Jan 18 14:15:35 2017 -0500

    Merge pull request #13045 from lfu/in_region_1400991
    
    Look for resources in the same region as the selected template during provisioning.
    (cherry picked from commit a25bb6c7f828ec1e0c953fffa26a9e7297adbfa2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1414886

@lfu lfu deleted the in_region_1400991 branch October 16, 2017 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants