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 HCP Consul Plus Tier #148

Merged
merged 1 commit into from
Jun 18, 2021
Merged

Add HCP Consul Plus Tier #148

merged 1 commit into from
Jun 18, 2021

Conversation

hanshasselberg
Copy link
Member

@hanshasselberg hanshasselberg commented Jun 16, 2021

🛠️ Description

Pulling in hashicorp/hcp-sdk-go#24 to enable using private beta plus tier. Use go generate to regenerate docs.

🏗️ Acceptance tests

Output from acceptance testing:

I had to change the tier and scale in the test before this was passing.

$ make testacc TESTARGS='-run=TestAccConsulCluster'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/... -v -run=TestAccConsulCluster -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 (1178.55s)
PASS
ok  	github.com/hashicorp/terraform-provider-hcp/internal/provider	1179.231s
...

@hanshasselberg hanshasselberg requested a review from a team June 16, 2021 08:43
"development": {
input: "development",
expected: nil,
},
Copy link
Contributor

@riddhi89 riddhi89 Jun 16, 2021

Choose a reason for hiding this comment

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

looks like the valid tier lowercase testcase below can be removed now, with this test case doing the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like having the additional test case. I know it is duplicated, but makes sure all tiers are covered and the different ways to spell them.

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! 🎉 Just had some documentation nits and the new go SDK version that can go in go.mod.

go.mod Outdated
@@ -10,7 +10,7 @@ require (
github.com/google/uuid v1.2.0
github.com/hashicorp/go-cty v1.4.1-0.20200414143053-d3edf31b6320
github.com/hashicorp/hcl/v2 v2.8.2 // indirect
github.com/hashicorp/hcp-sdk-go v0.8.0
github.com/hashicorp/hcp-sdk-go v0.8.1-0.20210615232900-f12e2d231a28
Copy link
Contributor

Choose a reason for hiding this comment

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

I just cut the release so you can bump this to v0.9.0

GNUmakefile Outdated
@@ -55,6 +55,7 @@ depscheck:
gencheck:
@echo "==> Checking generated source code..."
go generate
@git diff
Copy link
Contributor

Choose a reason for hiding this comment

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

is this just to show the diff?

@@ -14,6 +14,7 @@ description: |-
{{ tffile "examples/resources/hcp_consul_cluster/resource.tf" }}

-> **Note:** The `primary_link` attribute is related to federation, a feature that is currently in private beta.
-> **Note:** The `plus` tier is currently in private beta.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double-checked this with the doc preview tool and looks like there needs to be a blank line between the notes for them to render separately.

Also, could you move the banners below # {{.Type}} ({{.Name}})? I just realized we don't have a consistent pattern across our pages. 😔

@@ -69,7 +69,7 @@ func resourceConsulCluster() *schema.Resource {
},
"tier": {
// TODO: link to HCP Consul feature tier page when it is available
Description: "The tier that the HCP Consul cluster will be provisioned as. Only `development` and `standard` are available at this time.",
Description: "The tier that the HCP Consul cluster will be provisioned as. Only `development`, `standard` and `plus` are available at this time.",
Copy link
Contributor

@bcmdarroch bcmdarroch Jun 16, 2021

Choose a reason for hiding this comment

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

Lookin at that TODO, looks like the pricing page exists now if you want to add the link here.

Suggested change
Description: "The tier that the HCP Consul cluster will be provisioned as. Only `development`, `standard` and `plus` are available at this time.",
Description: "The tier that the HCP Consul cluster will be provisioned as. Only `development`, `standard` and `plus` are available at this time. See [pricing information](https://cloud.hashicorp.com/pricing/consul).",

@hanshasselberg hanshasselberg requested a review from a team as a code owner June 17, 2021 11:52
<!-- schema generated by tfplugindocs -->
## Schema

### Required

- **cluster_id** (String) The ID of the HCP Consul cluster.
- **hvn_id** (String) The ID of the HVN this HCP Consul cluster is associated to.
- **tier** (String) The tier that the HCP Consul cluster will be provisioned as. Only `development` and `standard` are available at this time.
- **tier** (String) The tier that the HCP Consul cluster will be provisioned as. Only `development`, `standard` and `plus` are available at this time.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you might need to regen the docs to include the added link to the pricing page.

@hanshasselberg
Copy link
Member Author

hanshasselberg commented Jun 17, 2021

@bcmdarroch I addressed your feedback. I was just wondering if I need to add any tests? And I guess I have to test this locally!

@bcmdarroch
Copy link
Contributor

I think the validator test update is sufficient as far as unit tests go. I would do a local run of the Consul cluster acceptance test swapping development for plus and add the results to your PR description - but probably best to leave the acceptance tests on development since that doesn't require billing set up on the test environment.

@hanshasselberg
Copy link
Member Author

Put the output in the PR description.

@bcmdarroch bcmdarroch merged commit 70febf9 into main Jun 18, 2021
@bcmdarroch bcmdarroch deleted the add_plus branch June 18, 2021 19:59
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