Skip to content
This repository has been archived by the owner on Aug 1, 2023. It is now read-only.

[rfr] Adds Fixed IP support to os-floating-ips #403

Merged
merged 2 commits into from
Jan 5, 2016
Merged

[rfr] Adds Fixed IP support to os-floating-ips #403

merged 2 commits into from
Jan 5, 2016

Conversation

jtopjian
Copy link
Contributor

This commit enables the ability to specify a fixed IP when associating a
floating IP to an instance. If a fixed IP is not specified, Nova will
attempt to associate the floating IP to the first detected fixed IP, as it
did prior to this patch.

@jtopjian
Copy link
Contributor Author

Any update on this one? It changes the existing code quite a bit, so is there a better way to add this functionality without having to change the existing code so much?

@jrperritt
Copy link
Contributor

Sorry for the delay; I didn't know if this was ready for review. The only problem right now is backward compatibility. Maybe you could create new functions named AssociateInstance and DisassociateInstance (or something of the like) and then leave Associate and Disassociate as they are but add comments about them being deprecated in favor of the new functions. Does that make sense?

@jtopjian
Copy link
Contributor Author

Oops - my bad. I'll note if PRs are ready to go in the future. :)

Yes, that does make sense. I'll re-work this going that route. Thanks!

@jrperritt
Copy link
Contributor

👍

This commit enables the ability to specify a fixed IP when associating a
floating IP to an instance. If a fixed IP is not specified, Nova will
attempt to associate the floating IP to the first detected fixed IP, as it
did prior to this patch.
@jtopjian
Copy link
Contributor Author

@jrperritt Finally getting around to this!

Clean commit with a new approach. The existing functions, Associate and Disassociate, have been left alone. Two new functions, AssociateFloatingIP and DisassociateFloatingIP, have been created with the ability to specify a Fixed IP To pair the Floating IP with.

@jtopjian jtopjian changed the title Adds Fixed IP support to os-floating-ips [rfr] Adds Fixed IP support to os-floating-ips Sep 17, 2015
@@ -79,7 +112,33 @@ func Associate(client *gophercloud.ServiceClient, serverId, fip string) Associat
return res
}

// AssociateFloatingIP pairs an allocated floating IP with an instance.
func AssociateFloatingIP(client *gophercloud.ServiceClient, opts AssociateOpts) AssociateResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to name this AssociateInstance so that when it called it would be floatingip.AssociateInstance instead of floatingip.AssociateFloatingIP? Similar proposal for DisassociateFloatingIP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I debated this when I originally wrote it and decided to wait and see if someone else flagged it :)

@jrperritt
Copy link
Contributor

Thanks for sticking with this; I'm sorry it took me so long to circle back around to it. +2

jrperritt added a commit that referenced this pull request Jan 5, 2016
[rfr] Adds Fixed IP support to os-floating-ips
@jrperritt jrperritt merged commit 9c901fb into rackspace:master Jan 5, 2016
@jtopjian
Copy link
Contributor Author

jtopjian commented Jan 5, 2016

No problem at all! Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants