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

Conversation

Savasw
Copy link
Contributor

@Savasw Savasw commented Mar 28, 2018

What this PR does / why we need it: This PR adds VPC, Subnet, Route, and Peering Connection interfaces

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #4

Special notes for your reviewer: The acceptance tests for peering connection and routes require Peer's tenant id or name (OS_Peer_Tenant_ID or OS_Peer_Tenant_Name), hence acceptance tests for these two interfaces fail if peer tenant details not provided. Also they required Peer Network Client separately, hence two new client functions are part of acceptance clients.go file where Peer tenant id is validated individually with no impact on other modules.

Release note:

None

Copy link
Contributor

@freesky-edward freesky-edward left a comment

Choose a reason for hiding this comment

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

it is pretty good, except some format issues. could you please fix the CI failure?

"github.com/huaweicloud/golangsdk/acceptance/clients"
"github.com/huaweicloud/golangsdk/acceptance/tools"
"github.com/huaweicloud/golangsdk/openstack/networking/v1/subnets"
"testing"
Copy link
Contributor

@freesky-edward freesky-edward Mar 28, 2018

Choose a reason for hiding this comment

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

format the import order, it should be :

import (
	"testing"

 	"github.com/huaweicloud/golangsdk"
 	"github.com/huaweicloud/golangsdk/openstack/networking/v2/peerings"
 	"github.com/huaweicloud/golangsdk/openstack/networking/v2/routes"
 )

"github.com/huaweicloud/golangsdk/acceptance/clients"
"github.com/huaweicloud/golangsdk/acceptance/tools"
"github.com/huaweicloud/golangsdk/openstack/networking/v1/vpcs"
"testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

fix the order issue the same as above

"github.com/huaweicloud/golangsdk/acceptance/tools"
"github.com/huaweicloud/golangsdk/openstack/networking/v1/vpcs"
"github.com/huaweicloud/golangsdk/openstack/networking/v2/peerings"
"testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

order issue

"github.com/huaweicloud/golangsdk/acceptance/clients"
"github.com/huaweicloud/golangsdk/acceptance/tools"
"github.com/huaweicloud/golangsdk/openstack/networking/v2/peerings"
"testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

order issue

@coveralls
Copy link

coveralls commented Mar 28, 2018

Pull Request Test Coverage Report for Build 58

  • 360 of 483 (74.53%) changed or added relevant lines in 12 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.4%) to 63.593%

Changes Missing Coverage Covered Lines Changed/Added Lines %
openstack/networking/v2/routes/results.go 25 27 92.59%
openstack/networking/v1/subnets/results.go 25 27 92.59%
openstack/networking/v2/peerings/results.go 45 47 95.74%
openstack/networking/v1/vpcs/results.go 25 27 92.59%
openstack/networking/v2/routes/requests.go 31 38 81.58%
openstack/networking/v1/vpcs/requests.go 50 81 61.73%
openstack/networking/v2/peerings/requests.go 71 107 66.36%
openstack/networking/v1/subnets/requests.go 55 96 57.29%
Totals Coverage Status
Change from base Build 36: 1.4%
Covered Lines: 2772
Relevant Lines: 4359

💛 - Coveralls

Copy link
Contributor

@freesky-edward freesky-edward left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -0,0 +1,57 @@
/*
Package Subnets enables management and retrieval of Subnets from the Open Telekom Cloud VPC service.
Copy link
Member

Choose a reason for hiding this comment

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

Nits: not only Open Telekom Cloud uses this golangsdk, better to not mention it here.

@@ -0,0 +1,52 @@
/*
Package vpcs enables management and retrieval of Vpcs from the Open Telekom Cloud
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -0,0 +1,87 @@
/*
Package peerings enables management and retrieval of vpc peering connections from the Open Telekom Cloud VPC service.

Copy link
Member

Choose a reason for hiding this comment

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

ditto

if matched {
refinedPeerings = append(refinedPeerings, peering)
}
}*/
Copy link
Member

Choose a reason for hiding this comment

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

Is this still useful?

return c.ServiceURL(rootPath, resourcePath, id)
}

func deleteURL(c *golangsdk.ServiceClient, id string) string {
Copy link
Member

Choose a reason for hiding this comment

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

seems there's no difference between deleteURL and resourceURL, how about just using one resourceURL?

@Savasw
Copy link
Contributor Author

Savasw commented Mar 29, 2018

@niuzhenguo Updated comments and removed redundant code. Let me know if there is more feedback.

@niuzhenguo
Copy link
Member

@Savasw Thanks for the update, LGTM now!

@niuzhenguo niuzhenguo merged commit 904d4b2 into huaweicloud:master Mar 29, 2018
@simrannagra simrannagra deleted the integration branch August 6, 2018 06:46
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.

Directory structure to follow for VPC Sub-services sdk development

4 participants