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

Only create new Public IP if one cannot be found #195

Merged
merged 1 commit into from
Jan 26, 2018
Merged

Only create new Public IP if one cannot be found #195

merged 1 commit into from
Jan 26, 2018

Conversation

djberg96
Copy link
Collaborator

@djberg96 djberg96 commented Jan 9, 2018

At the moment, if you select a public IP address when provisioning, but that IP is not currently associated with a NIC, it will create a new IP instead of using the one selected.

This PR modifies the provisioning code so that when a new NIC is provisioned, it will attempt to use the selected public IP before resorting to creating a new one.

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

@djberg96 djberg96 requested a review from bronaghs January 9, 2018 17:37
@bronaghs
Copy link

Looks good @djberg96
Is there a spec that can be added?

Copy link

@bronaghs bronaghs left a comment

Choose a reason for hiding this comment

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

as mentioned inline, looks like a spec can be added for this.

@bronaghs bronaghs self-assigned this Jan 16, 2018
@djberg96
Copy link
Collaborator Author

@bronaghs I think I would have to create a new spec file, since provision_spec.rb stubs out the response for create_nic. I noticed that there's no provision/cloning_spec.rb, so I'll have to add one, which will take some time.

@bronaghs
Copy link

I am wondering if you can update provision_spec.rb to stub out NetworkInterfaceService and IpAddressService within the create_nic method?

@djberg96
Copy link
Collaborator Author

@bronaghs well, i'm not really testing the overall provisioning, just the method itself. Creating a separate spec file for cloning.rb would probably boost our coverage a bit, too.

Initial commit.

Add initial create_nic spec and suppupporting code.

Added more specs for create_nic.
@miq-bot
Copy link
Member

miq-bot commented Jan 25, 2018

Checked commit https://github.com/djberg96/manageiq-providers-azure/commit/ac78d353f62fbb8d8ec615484854773ddf77f972 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@djberg96
Copy link
Collaborator Author

@bronaghs I've added some specs. I created a separate cloning_spec.rb file that just tests the Cloning module methods in isolation instead of adding more provisioning specs since that's really the behavior we care about in this instance.

@bronaghs bronaghs merged commit d4ac3bf into ManageIQ:master Jan 26, 2018
@bronaghs bronaghs added the bug label Jan 26, 2018
@bronaghs bronaghs added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 26, 2018
@simaishi
Copy link
Contributor

gaprindashvili/yes ?

@djberg96
Copy link
Collaborator Author

djberg96 commented Apr 12, 2018

Well, target release was 5.9, but it should be ok. @bronaghs ?

Nevermind, set tag.

@simaishi
Copy link
Contributor

gaprindashvili is 5.9!

simaishi pushed a commit that referenced this pull request Apr 12, 2018
Only create new Public IP if one cannot be found
(cherry picked from commit d4ac3bf)

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

Gaprindashvili backport details:

$ git log -1
commit 7247d28184cefbdb498228574abaa4e1928af72d
Author: Bronagh Sorota <bsorota@redhat.com>
Date:   Fri Jan 26 12:40:49 2018 -0500

    Merge pull request #195 from djberg96/auto_provision_nic
    
    Only create new Public IP if one cannot be found
    (cherry picked from commit d4ac3bfb134aeaa1313a44e80299199e58cee230)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1566577

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.

4 participants