-
Notifications
You must be signed in to change notification settings - Fork 33
Move eips and bandwidths to networking/v1 #5
Conversation
In case of collisions with vpc resources, move eips and bandwidths to networking from vpc.
Pull Request Test Coverage Report for Build 26
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 26
💛 - Coveralls |
freesky-edward
left a comment
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.
@Savasw could you please take a look at the inline comments.
| func NewVpcServiceV1(client *golangsdk.ProviderClient, eo golangsdk.EndpointOpts) (*golangsdk.ServiceClient, error) { | ||
| // NewNetworkV1 creates a ServiceClient that may be used with the v1 network | ||
| // package. | ||
| func NewNetworkV1(client *golangsdk.ProviderClient, eo golangsdk.EndpointOpts) (*golangsdk.ServiceClient, error) { |
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'd better bring this into correspondence with other VPC resource e.g. https://github.com/gator1/gophercloud/blob/master/openstack/client.go#L330:6
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.
sure, as we agreed to place all such resources under networking/v1 and the endpoint type is named 'network', so seems the client name here is better?
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.
client name looks good to me, there are some difference of ResourceBase. how to deal with project id here?
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.
If not all resources require a project id with the endpoint, we should just let themselves deal with it in their urls.go like https://github.com/huaweicloud/golangsdk/blob/master/openstack/vpc/v1/eips/urls.go#L9
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.
agree
freesky-edward
left a comment
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.
LGTM
| func NewVpcServiceV1(client *golangsdk.ProviderClient, eo golangsdk.EndpointOpts) (*golangsdk.ServiceClient, error) { | ||
| // NewNetworkV1 creates a ServiceClient that may be used with the v1 network | ||
| // package. | ||
| func NewNetworkV1(client *golangsdk.ProviderClient, eo golangsdk.EndpointOpts) (*golangsdk.ServiceClient, error) { |
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.
agree
What this PR does / why we need it:
In case of collisions with vpc resources, move eips and bandwidths
to networking from vpc.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Partially address #4
Special notes for your reviewer:
Release note: