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

Conversation

trumant
Copy link

@trumant trumant commented Sep 16, 2015

This is my first contribution to the project so very open to and looking forward to any constructive feedback

DeviceID string
DeviceOwner string
SecurityGroups []string
AllowedAddressPairs interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

since the structure of AllowedAddressPairs is known, it would be better to specify this as type []map[string]string

Copy link
Author

Choose a reason for hiding this comment

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

I was just following the pattern currently in place for FixedIPs

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why we used an interface{} for the FixedIPs. We may not have found documentation for them when that was written (i.e. is it an array of objects, an array of strings, something else, etc). Regardless, let's use []AddressPair if we know that's the format. Same thing on line 107.

@trumant
Copy link
Author

trumant commented Sep 16, 2015

Assuming this looks good to merge, I'll add support for this option on Port creation as well. Just wanted to get a complete, working version of update through review before going too far down the rabbit hole on this.

@trumant trumant force-pushed the allowed_address_pairs branch from b9aa632 to 5629171 Compare October 1, 2015 14:43
@nmische
Copy link

nmische commented Nov 4, 2015

Any update on this? We would really like to see allowed address pairs added to gophercloud such that we can provision via Terraform.

@trumant
Copy link
Author

trumant commented Jan 12, 2016

@jrperritt - Any pointers you could give me about getting this to a mergeable point would be appreciated.

@jrperritt
Copy link
Contributor

It looks good aside from the 2 minor type changes. I'm not concerned with the unit tests failing on the Go tip branch.

@trumant
Copy link
Author

trumant commented Jan 12, 2016

Thanks for the feedback @jrperritt I'll work to address it.

@trumant trumant force-pushed the allowed_address_pairs branch from bac04f1 to a371c0e Compare January 12, 2016 21:06
@roobert
Copy link

roobert commented Feb 1, 2016

Hi, Like @nmische, I came here looking for this feature since it's required in order to update the Terraform OpenStack provider. Since this ticket hasn't been updated in 20 days I was wondering if there is anything I could do to help get this merged?

Cheers,

@trumant
Copy link
Author

trumant commented Feb 1, 2016

@nmische and @roobert and @jrperritt - Finally got around to making this change. Appreciate the reminder and feedback.

@roobert
Copy link

roobert commented Feb 1, 2016

Thanks @trumant! Hopefully @jrperritt will be able to merge this now.

@jrperritt
Copy link
Contributor

@trumant looks great. +2

jrperritt added a commit that referenced this pull request Feb 9, 2016
Allowed address pairs support for Neutron Port
@jrperritt jrperritt merged commit 7cfd38c into rackspace:master Feb 9, 2016
@trumant trumant deleted the allowed_address_pairs branch February 9, 2016 21:42
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.

5 participants