Vpnaas: Create VPN service #760

simonre · 2018-02-13T17:02:55Z

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

API:
https://developer.openstack.org/api-ref/network/v2/#create-vpn-service

theopenlab-ci · 2018-02-13T18:38:43Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 46s
gophercloud-acceptance-test : SUCCESS in 1h 35m 11s

coveralls · 2018-02-14T12:23:10Z

Coverage increased (+0.003%) to 73.362% when pulling fe9da82 on simonre:vpnaas-service-create into 6da026c on gophercloud:master.

coveralls · 2018-02-14T12:23:10Z

Coverage increased (+0.003%) to 73.362% when pulling 2dc358c on simonre:vpnaas-service-create into 6da026c on gophercloud:master.

theopenlab-ci · 2018-02-14T12:49:23Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 27s
gophercloud-acceptance-test : SUCCESS in 27m 17s

simonre · 2018-02-14T12:54:50Z

@jtopjian This is ready for review

jtopjian

@simonre Thank you for working on this. This is a really good start.

In general, everything looks good. I've left some comments for some minor changes.

This PR will need two additional items before it can be merged:

A doc.go file similar to this.
An acceptance test. Documentation for acceptance tests can be found here. You can use the existing networking tests as an example.

Please let me know if you have any questions or need any help.

jtopjian · 2018-02-15T02:17:54Z

openstack/networking/v2/extensions/vpnaas/services/requests.go

+}
+
+// CreateOpts contains all the values needed to create a new VPN service
+type CreateOpts struct {


doc nit: all fields should have a descriptive comment even if it's something simple like:

Name is the human-readable name of the VPN Service.

jtopjian · 2018-02-15T02:18:31Z

openstack/networking/v2/extensions/vpnaas/services/requests.go

+	RouterID     string `json:"router_id" required:"true"`
+	Description  string `json:"description,omitempty"`
+	AdminStateUp *bool  `json:"admin_state_up"`
+	ProjectID    string `json:"project_id"`


I don't see project_id defined anywhere here, so maybe it's an internal neutron field? What happens if TenantID and ProjectID are different UUIDs?

jtopjian · 2018-02-15T02:19:03Z

openstack/networking/v2/extensions/vpnaas/services/requests.go

+// CreateOptsBuilder allows extensions to add additional parameters to the
+// Create request.
+type CreateOptsBuilder interface {
+	ToVPNServiceCreateMap() (map[string]interface{}, error)


This should be ToServiceCreateMap.

jtopjian · 2018-02-15T02:19:14Z

openstack/networking/v2/extensions/vpnaas/services/requests.go

+}
+
+// ToVPNServiceCreateMap casts a CreateOpts struct to a map.
+func (opts CreateOpts) ToVPNServiceCreateMap() (map[string]interface{}, error) {


ToServiceCreateMap

jtopjian · 2018-02-15T02:22:49Z

openstack/networking/v2/extensions/vpnaas/services/results.go

+)
+
+// Service is a VPN Service
+type Service struct {


doc nit: these fields should have comments to describe what they are.

jtopjian · 2018-02-15T02:23:08Z

openstack/networking/v2/extensions/vpnaas/services/results.go

+// Service is a VPN Service
+type Service struct {
+	TenantID     string `json:"tenant_id"`
+	SubnetID     string `json:"subnet_id,omitempty"`


omitempty isn't valid for results. it can safely be removed.

theopenlab-ci · 2018-02-15T14:15:58Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 14s
gophercloud-acceptance-test : FAILURE in 12m 07s

jtopjian · 2018-02-16T15:45:56Z

@simonre Thank you for the updates. Travis is failing due to some gofmt issues. These can be fixed by manually running gofmt on the changed files or configuring your IDE/editor to run gofmt upon save.

The OpenLab failures were a temporary issue with the CI system. I wouldn't worry about those.

jtopjian · 2018-02-16T16:44:23Z

@simonre Thank you again for the updates.

This looks good to me. Unfortunately I can't test this myself since I don't have access to VPNaaS, but from reviewing the Neutron code, this looks correct. In addition, the acceptance test looks correct, too.

Can you verify that this code works? Meaning: have you tested it yourself to create an actual VPN service? If it works for you, I'll go ahead and merge this.

The inline comments/docs could use some formatting, but I don't consider doc updates to be blocking unless there are other changes required. If you plan on continuing to work on the other parts of the VPNaaS, then maybe fix them there. You can use this file for reference (or really any of the requests.go and results.go files). Note the formatting of the comments for the fields, particularly the space between // and the comment as well as a new line to separate the fields.

The reason we want to make sure formatting is correct is because the inline comments turn into the official godoc reference documentation.

theopenlab-ci · 2018-02-16T16:56:09Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 36s
gophercloud-acceptance-test : SUCCESS in 49m 20s

…me formatting on inline comments

simonre · 2018-02-16T17:40:08Z

@jtopjian Thanks for the quick feedback. I just updated the doc.go sample code to code that I can verify worked for me to create an actual VPN service. I also updated the inline formatting in requests.go, is that what you had in mind? If yes, I'll keep that in mind for the rest of the code and I'll update the other files as well!

theopenlab-ci · 2018-02-16T18:18:35Z

Build succeeded.

gophercloud-unittest : SUCCESS in 3m 03s
gophercloud-acceptance-test : SUCCESS in 39m 55s

jtopjian · 2018-02-16T18:24:51Z

@simonre

I also updated the inline formatting in requests.go, is that what you had in mind?

Yep - That's exactly what I had in mind. Though the same format should be applied to the Service result struct, too. Let me know if you want to fix that now or in another PR.

I can verify worked for me to create an actual VPN service

Perfect - thank you 😄

simonre · 2018-02-16T18:27:18Z

I'll fix it now. Just wanted to make sure I'm actually doing the right thing before doing it for all files.

jtopjian · 2018-02-16T19:01:45Z

@simonre I apologize - I missed one thing in the unit tests:

There should be a comparison to the struct that is returned from the Create / Extract call:

expected := services.Service{
  RouterID: "66e3b16c-8ce5-40fb-bb49-ab6d8dc3f2aa",
  ...
}

actual, err := services.Create(fake.ServiceClient(), options).Extract()
th.AssertNoErr(t, err)
th.AssertDeepEquals(t, expected, actual)

This same pattern will need to be in the other unit tests (where applicable), so best to get in the habit now :)

theopenlab-ci · 2018-02-16T19:08:16Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 24s
gophercloud-acceptance-test : SUCCESS in 37m 05s

simonre · 2018-02-16T21:58:05Z

@jtopjian
th.AssertDeepEquals(t, expected, actual)
doesn't seem to work because of the pointer to AdminStateUp. I added individual comparisons like the ones here instead which should do the same.

theopenlab-ci · 2018-02-16T22:28:22Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 09s
gophercloud-acceptance-test : SUCCESS in 31m 18s

jtopjian · 2018-02-16T23:40:59Z

@simonre We definitely want to ensure a struct can be compared successfully, and not just individual fields.

Using "networks" as an example, the AdminStateUp field for the result struct is bool rather than *bool:

https://github.com/gophercloud/gophercloud/blob/master/openstack/networking/v2/networks/results.go#L57

However, the CreateOpts AdminStateUp should probably remain *bool as this is the standard used for the networking services. For example:

https://github.com/gophercloud/gophercloud/blob/master/openstack/networking/v2/networks/requests.go#L69

Unless there's a good reason to keep the result field as *bool, let's change it to bool. The only reason I can think of using *bool is if there are three distinct states for AdminStateUp (true/false/nil). Given the prevalence of AdminStateUp already in Gophercloud, I can't see VPNaaS doing something different.

…rison. Also changed AdminStateUp type from *bool to bool

theopenlab-ci · 2018-02-17T20:12:12Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 14s
gophercloud-acceptance-test : SUCCESS in 28m 45s

simonre · 2018-02-17T20:29:48Z

@jtopjian You're right. I changed it again. I also did the same in the ipsecpolicy branch.

jtopjian · 2018-02-18T02:52:35Z

@simonre This looks good to me. Thank you very much for your help and patience.

sreinkemeier added 7 commits February 8, 2018 19:28

Added create method for vpn services

68aec77

Added urls to urls file

1eacf5a

Added results file for create method

c14e678

Created unit test for vpn service creation

16586dd

Added missing extract method to results file

9c6e4ca

Made parameter in create API compliant

e204816

Made flavorId parameter optional, fixed unit tests

19cfa7c

simonre changed the title ~~Vpnaas: Create VPN service~~ [wip]Vpnaas: Create VPN service Feb 13, 2018

sreinkemeier added 3 commits February 14, 2018 12:30

Removed space in struct definition

7cbe100

removed comma in struct

9df9d98

formatting

fe9da82

simonre changed the title ~~[wip]Vpnaas: Create VPN service~~ Vpnaas: Create VPN service Feb 14, 2018

jtopjian requested changes Feb 15, 2018

View reviewed changes

sreinkemeier added 7 commits February 15, 2018 12:31

added documentation file doc.go

1cf2dbb

added descriptive comments to fields

d4b3c20

renamed ToVPNServiceCreateMap to ToServiceCreateMap

acfd9fa

removed faulty description of subnet ID

644a052

removed field project_id

97f146c

Added acceptance test for vpn service creation

2c9df10

Removed unnecessary print statement

325cf51

Formatting

3ee1884

formatting doc.go file

ede2826

Changed example in doc.go to verified working example. Also edited so…

c8c578c

…me formatting on inline comments

Fixed inline formatting for results.go

c529c58

Added comparison to created object at the end

d5115ce

Changed individual attribute comparisons in unit test to struct compa…

2dc358c

…rison. Also changed AdminStateUp type from *bool to bool

jtopjian approved these changes Feb 18, 2018

View reviewed changes

jtopjian merged commit b922c6e into gophercloud:master Feb 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vpnaas: Create VPN service #760

Vpnaas: Create VPN service #760

simonre commented Feb 13, 2018 •

edited by jtopjian

theopenlab-ci bot commented Feb 13, 2018

coveralls commented Feb 14, 2018

coveralls commented Feb 14, 2018 •

edited

theopenlab-ci bot commented Feb 14, 2018

simonre commented Feb 14, 2018

jtopjian left a comment •

edited

jtopjian Feb 15, 2018

jtopjian Feb 15, 2018

jtopjian Feb 15, 2018

jtopjian Feb 15, 2018

jtopjian Feb 15, 2018

jtopjian Feb 15, 2018

theopenlab-ci bot commented Feb 15, 2018

jtopjian commented Feb 16, 2018

jtopjian commented Feb 16, 2018

theopenlab-ci bot commented Feb 16, 2018

simonre commented Feb 16, 2018 •

edited

theopenlab-ci bot commented Feb 16, 2018

jtopjian commented Feb 16, 2018

simonre commented Feb 16, 2018

jtopjian commented Feb 16, 2018

theopenlab-ci bot commented Feb 16, 2018

simonre commented Feb 16, 2018

theopenlab-ci bot commented Feb 16, 2018

jtopjian commented Feb 16, 2018

theopenlab-ci bot commented Feb 17, 2018

simonre commented Feb 17, 2018

jtopjian commented Feb 18, 2018

Vpnaas: Create VPN service #760

Vpnaas: Create VPN service #760

Conversation

simonre commented Feb 13, 2018 • edited by jtopjian

theopenlab-ci bot commented Feb 13, 2018

coveralls commented Feb 14, 2018

coveralls commented Feb 14, 2018 • edited

theopenlab-ci bot commented Feb 14, 2018

simonre commented Feb 14, 2018

jtopjian left a comment • edited

Choose a reason for hiding this comment

jtopjian Feb 15, 2018

Choose a reason for hiding this comment

jtopjian Feb 15, 2018

Choose a reason for hiding this comment

jtopjian Feb 15, 2018

Choose a reason for hiding this comment

jtopjian Feb 15, 2018

Choose a reason for hiding this comment

jtopjian Feb 15, 2018

Choose a reason for hiding this comment

jtopjian Feb 15, 2018

Choose a reason for hiding this comment

theopenlab-ci bot commented Feb 15, 2018

jtopjian commented Feb 16, 2018

jtopjian commented Feb 16, 2018

theopenlab-ci bot commented Feb 16, 2018

simonre commented Feb 16, 2018 • edited

theopenlab-ci bot commented Feb 16, 2018

jtopjian commented Feb 16, 2018

simonre commented Feb 16, 2018

jtopjian commented Feb 16, 2018

theopenlab-ci bot commented Feb 16, 2018

simonre commented Feb 16, 2018

theopenlab-ci bot commented Feb 16, 2018

jtopjian commented Feb 16, 2018

theopenlab-ci bot commented Feb 17, 2018

simonre commented Feb 17, 2018

jtopjian commented Feb 18, 2018

simonre commented Feb 13, 2018 •

edited by jtopjian

coveralls commented Feb 14, 2018 •

edited

jtopjian left a comment •

edited

simonre commented Feb 16, 2018 •

edited