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

Add auto peering for Consul Federation #154

Merged
merged 15 commits into from
Jun 29, 2021
Merged

Conversation

hanshasselberg
Copy link
Member

@hanshasselberg hanshasselberg commented Jun 23, 2021

🛠️ Description

This PR adds the new parameter auto_hvn_to_hvn_peering to consul clusters. It enables the user to decided if they want to use automatic peering during federation or not. Needs https://github.com/hashicorp/cloud-consul-service/pull/1253.

This PR also semi depends on #156 because right now there is no way to create a manual peering.

🏗️ Acceptance tests

  • Are there any feature flags that are required to use this functionality?
  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccConsul'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/... -v -run=TestAccConsul -timeout 120m
?   	github.com/hashicorp/terraform-provider-hcp/internal/clients	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-hcp/internal/consul	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-hcp/internal/input	(cached) [no tests to run]


=== RUN   TestAccConsulCluster
--- PASS: TestAccConsulCluster (553.00s)
=== RUN   TestAccConsulSnapshot
--- PASS: TestAccConsulSnapshot (591.61s)
PASS
ok  	github.com/hashicorp/terraform-provider-hcp/internal/provider	1144.969s

@hanshasselberg hanshasselberg requested a review from a team as a code owner June 23, 2021 12:06
@hanshasselberg hanshasselberg requested a review from a team June 23, 2021 12:06
@hanshasselberg
Copy link
Member Author

I still need to generate the docs!

Copy link
Contributor

@bcmdarroch bcmdarroch left a comment

Choose a reason for hiding this comment

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

Looks good so far! I'd echo all of @riddhi89's comments, and I'd like to get some more product/design input on how updates to auto_peering should be handled.

docs/resources/consul_cluster.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
internal/provider/resource_consul_cluster.go Show resolved Hide resolved
internal/provider/resource_consul_cluster.go Show resolved Hide resolved
@hanshasselberg hanshasselberg force-pushed the add_auto_peering branch 2 times, most recently from aef2a5c to 3e6e80b Compare June 25, 2021 08:31
@hanshasselberg
Copy link
Member Author

I removed the default, and re-added ForceNew.

@hanshasselberg
Copy link
Member Author

Running integration tests right now.

Copy link
Contributor

@bcmdarroch bcmdarroch left a comment

Choose a reason for hiding this comment

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

This is close! I have a few more questions, and some doc suggestions. I don't have all the feature flag set up for testing this locally, so I'll leave that to you to verify :)

internal/provider/resource_consul_cluster.go Show resolved Hide resolved
templates/resources/consul_cluster.md.tmpl Outdated Show resolved Hide resolved
examples/resources/hcp_consul_cluster/federation.tf Outdated Show resolved Hide resolved
docs/guides/consul-federation.md Show resolved Hide resolved
examples/resources/hcp_consul_cluster/federation.tf Outdated Show resolved Hide resolved
@bcmdarroch
Copy link
Contributor

bcmdarroch commented Jun 25, 2021

Ah I did see a panic on my run of the acceptance test, looks like on the data source step:

panic: Invalid address to set: []string{"auto_hvn_to_hvn_peering"}

goroutine 915 [running]:
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*ResourceData).Set(0xc000c42380, 0x1ee0956, 0x17, 0x1ce92a0, 0x278c6e0, 0x0, 0x0)
	$GOPATH/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource_data.go:230 +0x371
github.com/hashicorp/terraform-provider-hcp/internal/provider.setConsulClusterResourceData(0xc000c42380, 0xc000b99380, 0xc0007aab90, 0xc00094fdd0, 0xc000b94450)
	$GOPATH/src/github.com/hashicorp/terraform-provider-hcp/internal/provider/resource_consul_cluster.go:462 +0x85b
github.com/hashicorp/terraform-provider-hcp/internal/provider.dataSourceConsulClusterRead(0x20ebd98, 0xc000b98240, 0xc000c42380, 0x1cbd9a0, 0xc000d6e0b0, 0xc000667f40, 0xc00076f948, 0x100fff8)
	$GOPATH/src/github.com/hashicorp/terraform-provider-hcp/internal/provider/data_source_consul_cluster.go:171 +0x5d6
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).read(0xc000148000, 0x20ebd28, 0xc000d14280, 0xc000c42380, 0x1cbd9a0, 0xc000d6e0b0, 0x0, 0x0, 0x0)

docs/guides/consul-federation.md Outdated Show resolved Hide resolved
examples/guides/consul_cluster_federation/main.tf Outdated Show resolved Hide resolved
internal/provider/resource_consul_cluster.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bcmdarroch bcmdarroch left a comment

Choose a reason for hiding this comment

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

Nice work! Just two last few nits but otherwise this looks good to go! And acceptance tests are passing now 👍 👍

templates/resources/consul_cluster.md.tmpl Outdated Show resolved Hide resolved
templates/guides/consul-federation.md.tmpl Show resolved Hide resolved
@hanshasselberg
Copy link
Member Author

@bcmdarroch yeah the panic was caused by the missing changes related to the data-source.

@hanshasselberg
Copy link
Member Author

@bcmdarroch @riddhi89 thank you both for your reviews! I think it is done.

Copy link
Contributor

@riddhi89 riddhi89 left a comment

Choose a reason for hiding this comment

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

Tested it out manually. LGTM :shipit:

@hanshasselberg hanshasselberg merged commit e476502 into main Jun 29, 2021
@hanshasselberg hanshasselberg deleted the add_auto_peering branch June 29, 2021 17:06
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.

3 participants