-
Notifications
You must be signed in to change notification settings - Fork 180
[wip] ip addresses (neutron extension) #477
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't we embed os.Server
here? (haven't worked in go in a while 😁 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but I don't think we gain anything here by doing it. Specifically, if the OpenStack server has fields that Rackspace doesn't have (now or in the future), we'll end up copy-and-pasting like this anyhow. Only if done that way, it would potentially break code that initializes a Rackspace server (since it'll no longer embed an OpenStack server):
server := rsServers.Server{
osServers.Server{
},
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I agree actually. I think we should decouple the providers as much as possible and not assume openstack is a superset
I don't see anything wrong with copy and pasting the Rackspace Server struct into a more distinct thing. We'll likely be adding extra fields in the future, so it's probably best we decouple Rackspace's implementation. The only question I have is whether all Rackspace API users will have this added field (my guess is yes). If this is not the case, the only other option would be to embed the Also, I know it's a pain, but do we have acceptance tests for this? |
Yeah, that field gets added to all of the responses from Ah, quite right. All that copy-and-paste was to get that field I needed to create the acceptance tests in the first place (to share a public IP, the servers have to have the same value for that field.) I'll add those. |
Not ready to merge. Permissions issues (likely limited to me) arose while running the acceptance tests. |
Closing. This will go in the Rackspace extension to |
No description provided.