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.

Sign up for GitHub

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

Jump to bottom

Unique Consul Cluster ID #437

Merged
merged 4 commits into from
Jan 10, 2023
Merged

Unique Consul Cluster ID #437

merged 4 commits into from
Jan 10, 2023

Conversation

JolisaBrownHashiCorp
Copy link
Contributor

@JolisaBrownHashiCorp JolisaBrownHashiCorp commented Jan 6, 2023 •
edited
Loading

🛠️ Description

This change adds unique cluster IDs to prevent reuse of identifiers and cluster name collision on test retries.

🏗️ 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=TestAccXXX'
==> Checking that code complies with gofmt requirements...
golangci-lint run --config ./golangci-config.yml 
TF_ACC=1 go test ./internal/... -v -run=TestAccConsulCluster -timeout 210m
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-hcp/internal/clients	(cached) [no tests to run]
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 (1792.53s)
PASS
ok  	github.com/hashicorp/terraform-provider-hcp/internal/provider	1793.055s
...

@JolisaBrownHashiCorp JolisaBrownHashiCorp requested a review from a team as a code owner January 6, 2023 21:10
@JolisaBrownHashiCorp JolisaBrownHashiCorp requested a review from a team January 6, 2023 21:10
bcmdarroch
bcmdarroch reviewed Jan 6, 2023
View reviewed changes
go.mod Outdated Show resolved Hide resolved
bcmdarroch
bcmdarroch reviewed Jan 6, 2023
View reviewed changes
internal/provider/resource_consul_cluster_test.go Outdated

data "hcp_consul_cluster" "test" {
cluster_id = hcp_consul_cluster.test.cluster_id
data "hcp_consul_cluster" "%[2]s" {
Copy link
Contributor

Choose a reason for hiding this comment

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

The resource name should stay the same.

Suggested change
data "hcp_consul_cluster" "%[2]s" {
data "hcp_consul_cluster" "test" {

bcmdarroch
bcmdarroch reviewed Jan 6, 2023
View reviewed changes
internal/provider/resource_consul_cluster_test.go Outdated
cluster_id = "test-consul-cluster"
var updatedConsulCluster = fmt.Sprintf(`
resource "hcp_consul_cluster" "%[1]s" {
cluster_id = "test-consul-cluster-updated"
Copy link
Contributor

Choose a reason for hiding this comment

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

We want the cluster ID to be unique. The resource names should stay the same test. The issue is not with Terraform having resource name collisions, it's with the Consul clusters having cluster ID collisions.

Suggested change
cluster_id = "test-consul-cluster-updated"
cluster_id = "%[1]s"

bcmdarroch
bcmdarroch reviewed Jan 6, 2023
View reviewed changes
internal/provider/resource_consul_cluster_test.go Outdated
resource "hcp_consul_cluster" "test" {
cluster_id = "test-consul-cluster"
var updatedConsulCluster = fmt.Sprintf(`
resource "hcp_consul_cluster" "%[1]s" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resource "hcp_consul_cluster" "%[1]s" {
resource "hcp_consul_cluster" "test" {

bcmdarroch
bcmdarroch reviewed Jan 6, 2023
View reviewed changes
internal/provider/resource_consul_cluster_test.go Outdated
}
`, consulCluster)
`, consulCluster, consulClusterUniqueResourceName)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this variable here.

Suggested change
`, consulCluster, consulClusterUniqueResourceName)
`, consulCluster)

bcmdarroch
bcmdarroch reviewed Jan 6, 2023
View reviewed changes
internal/provider/resource_consul_cluster_test.go Outdated
@@ -69,13 +72,13 @@ func TestAccConsulCluster(t *testing.T) {
Config: testConfig(setTestAccConsulClusterConfig(consulCluster)),
Check: resource.ComposeTestCheckFunc(
testAccCheckConsulClusterExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "cluster_id", "test-consul-cluster"),
resource.TestCheckResourceAttr(resourceName, "cluster_id", "test-consul-cluster-original"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resource.TestCheckResourceAttr(resourceName, "cluster_id", "test-consul-cluster-original"),
resource.TestCheckResourceAttr(resourceName, "cluster_id", consulClusterUniqueResourceName),

bcmdarroch
bcmdarroch reviewed Jan 6, 2023
View reviewed changes
internal/provider/resource_consul_cluster_test.go Outdated
}

// This includes tests against both the resource, the corresponding datasource,
// and creation of the Consul cluster root token resource in order to shorten
// testing time.
func TestAccConsulCluster(t *testing.T) {
resourceName := "hcp_consul_cluster.test"
dataSourceName := "data.hcp_consul_cluster.test"
resourceName := fmt.Sprintf("hcp_consul_cluster.%s", consulClusterUniqueResourceName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert these two lines.

@JolisaBrownHashiCorp JolisaBrownHashiCorp force-pushed the unique-consul-cluster-name branch from b385c7d to 4974dff Compare January 6, 2023 21:25
bcmdarroch
bcmdarroch reviewed Jan 9, 2023
View reviewed changes
.changelog/437.txt Outdated
@@ -0,0 +1,3 @@
```release-note:improvement
Enable unique clusterIDs
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Enable unique clusterIDs
Use unique cluster IDs in acceptance test

bcmdarroch
bcmdarroch reviewed Jan 9, 2023
View reviewed changes
internal/provider/resource_consul_cluster_test.go Outdated
@@ -38,7 +41,7 @@ func setTestAccConsulClusterConfig(consulCluster string) string {

data "hcp_consul_versions" "test" {}

%s
%[1]s
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
%[1]s
%s

bcmdarroch
bcmdarroch approved these changes Jan 9, 2023
View reviewed changes
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.

🎉

@JolisaBrownHashiCorp JolisaBrownHashiCorp changed the title Unique consul cluster name Unique Consul Cluster ID Jan 9, 2023
@JolisaBrownHashiCorp JolisaBrownHashiCorp merged commit 8929114 into main Jan 10, 2023
@JolisaBrownHashiCorp JolisaBrownHashiCorp deleted the unique-consul-cluster-name branch January 10, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcmdarroch bcmdarroch bcmdarroch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JolisaBrownHashiCorp @bcmdarroch