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

Service#my_zone should only reference a VM associated to a provider. #14696

Merged
merged 1 commit into from
Apr 10, 2017

Conversation

gmcculloug
Copy link
Member

Services do not have an inherent zone association. When one is desired, for example when running a custom button, we look at the associated VMs to select a zone. The current logic always uses the first VM from the list of vms. If that VM happens to be archived/orphaned (not associated with a provider) the logic raises an error. The new logic checks that the VM has a valid provider before accessing the zone name.

Note: The Vm model contains a my_zone method which handles a nil provider, but in that scenario it returns the current MiqServer zone name which is not the desired result.

Links [Optional]

Steps for Testing/QA

Options 1: Follow step list in BZ to recreate issue.
Options 2:

Run the following steps in the rails console (before changes):

s = Service.create(:name => "Zone Test")
s.my_zone    # => nil

v = Vm.create!(:name => "Zone test", :vendor => "vmware", :location => "cannot_be_blank")
s.add_resource!(v)

s.my_zone    # => NoMethodError: undefined method `zone' for nil:NilClass

After applying the changes s.my_zone will return nil for the error case above.

@gmcculloug gmcculloug force-pushed the service_my_zone_with_archived_vm branch from 198242b to 08a473d Compare April 7, 2017 20:08
@gmcculloug gmcculloug requested a review from bzwei April 7, 2017 20:09
@gmcculloug gmcculloug assigned gmcculloug and bdunne and unassigned gmcculloug Apr 7, 2017
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.

Changes look good, but naming is hard :)

end

it 'returns the provider zone when the VM is connected to a provider' do
provider = FactoryGirl.create(:ext_management_system, :zone => FactoryGirl.create(:zone))
Copy link
Member

Choose a reason for hiding this comment

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

Can this be manager or ems instead since it's not a Provider?

it 'returns the provider zone with one VM connected to a provider and one archived' do
service.add_resource!(FactoryGirl.build(:vm_vmware))

provider = FactoryGirl.create(:ext_management_system, :zone => FactoryGirl.create(:zone))
Copy link
Member

Choose a reason for hiding this comment

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

Same

@gmcculloug gmcculloug force-pushed the service_my_zone_with_archived_vm branch from 08a473d to 46b7f9f Compare April 10, 2017 14:15
@miq-bot
Copy link
Member

miq-bot commented Apr 10, 2017

Checked commit gmcculloug@46b7f9f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. ⭐

@bdunne bdunne merged commit 6a23f36 into ManageIQ:master Apr 10, 2017
@bdunne bdunne added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 10, 2017
@bdunne bdunne added the euwe/no label Apr 10, 2017
simaishi pushed a commit that referenced this pull request Apr 11, 2017
…ed_vm

Service#my_zone should only reference a VM associated to a provider.
(cherry picked from commit 6a23f36)

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

Fine backport details:

$ git log -1
commit a507c03a0a5fd31bb2e8bd307d472b0eea0a1ceb
Author: Brandon Dunne <[email protected]>
Date:   Mon Apr 10 11:27:09 2017 -0400

    Merge pull request #14696 from gmcculloug/service_my_zone_with_archived_vm
    
    Service#my_zone should only reference a VM associated to a provider.
    (cherry picked from commit 6a23f369e4147a0afc6519f8541f44fdaf67960d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1441249

simaishi pushed a commit that referenced this pull request Apr 17, 2017
…ed_vm

Service#my_zone should only reference a VM associated to a provider.
(cherry picked from commit 6a23f36)

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

Euwe backport details:

$ git log -1
commit 1482ed559ee4d3a7890392c55608a838f0656b6d
Author: Brandon Dunne <[email protected]>
Date:   Mon Apr 10 11:27:09 2017 -0400

    Merge pull request #14696 from gmcculloug/service_my_zone_with_archived_vm
    
    Service#my_zone should only reference a VM associated to a provider.
    (cherry picked from commit 6a23f369e4147a0afc6519f8541f44fdaf67960d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1441251

@gmcculloug gmcculloug deleted the service_my_zone_with_archived_vm branch January 1, 2020 23:50
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