Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vpnaas: List Endpoint groups #813

Merged
merged 4 commits into from
Mar 9, 2018

Conversation

simonre
Copy link
Contributor

@simonre simonre commented Mar 7, 2018

For #723

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

https://github.com/openstack/neutron-vpnaas/blob/058469e1b99b647537a5228c6a384d93df5484df/neutron_vpnaas/db/vpn/vpn_db.py#L626

@coveralls
Copy link

coveralls commented Mar 7, 2018

Coverage Status

Coverage increased (+0.04%) to 73.772% when pulling c6e404e on simonre:vpnaas-epgroup-list into c3bc092 on gophercloud:master.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 7, 2018

Build succeeded.

@simonre
Copy link
Contributor Author

simonre commented Mar 7, 2018

@jtopjian This is ready for review.

I'm not sure if this is the right place to ask this but I noticed that you are a maintainer for terraform-provider-openstack as well. I'm planning to implement the VPNaas functionality in the openstack provider as well but I couldn't find any contributing guide. Anything I need to know before making a pull request?

@simonre simonre changed the title [WIP] Vpnaas: List Endpoint groups Vpnaas: List Endpoint groups Mar 7, 2018
Copy link
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

@simonre Looks good - just one minor change request.

// the API. Filtering is achieved by passing in struct field values that map to
// the Endpoint group attributes you want to see returned.
type ListOpts struct {
TenantID string `q:"tenant_id"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add ProjectID as well.

@jtopjian
Copy link
Contributor

jtopjian commented Mar 8, 2018

@simonre

I'm not sure if this is the right place to ask this but I noticed that you are a maintainer for terraform-provider-openstack as well. I'm planning to implement the VPNaas functionality in the openstack provider as well but I couldn't find any contributing guide. Anything I need to know before making a pull request?

Adding VPNaaS to Terraform would be much appreciated! Creating a Terraform resource is pretty straightforward, but let me know if you need help.

As far as contributing guidelines/rules:

  1. One resource or data source per PR, please. For example one PR would contain the implementation of openstack_vpnaas_ipsec_policy_v2. The PR can contain a fully functional resource (so full CRUD).

  2. It works best if you can do two commits: one with the vendoring (Terraform uses govendor) and one with the squashed commits of the resource (so the entire commit would be "adding openstack_vpnaas_ipsec_policy_v2"). The same rule applies about only amending commits if changes are needed, though, and if that happens, don't worry about having more than two commits.

  3. Acceptance tests are important but we won't be able to run them in the CI system, so two things to note:

    a. We'll trust that you've run the tests and they're successful.
    b. Restrict the tests to be triggered via an environment variable like here:

  4. Docs are also important, too. They are housed under the website directory of the repo.

  5. If you don't plan on implementing the ability to import the resource, make sure you omit these lines:

    https://github.com/terraform-providers/terraform-provider-openstack/blob/master/openstack/resource_openstack_dns_recordset_v2.go#L22-L24

    If you do want to implement importing, make sure to include a separate file like this:

    https://github.com/terraform-providers/terraform-provider-openstack/blob/master/openstack/import_openstack_dns_recordset_v2_test.go

The best way to get started is to copy an existing resource and start renaming things. Terraform resources are pretty easy to create since the functions you need to implement match very well with CRUD.

Implementing Terraform resources can sometimes uncover bugs we didn't catch in Gophercloud. That's totally OK - just open a Gophercloud Issue and PR with a patch.

In fact, Terraform is sometimes a great way to debug Gophercloud because you can do:

$ OS_DEBUG=1 TF_LOG=DEBUG terraform apply

And see the full API interaction. I've been meaning to add this functionality to the Gophercloud acceptance tests.

Feel free to open a PR and we'll work on it together. :)

@simonre
Copy link
Contributor Author

simonre commented Mar 8, 2018

@jtopjian I've implemented the change so this is ready for review again.

Once again thank you for the in-depth answer. It's a pleasure to work with you on this.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 8, 2018

Build succeeded.

@jtopjian
Copy link
Contributor

jtopjian commented Mar 9, 2018

LGTM!

@jtopjian jtopjian merged commit d2fe5bf into gophercloud:master Mar 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants