v0.2.0: Refactor cluster resources

[erademacher]

• Consolidated create specs and update specs into configs
• Added some basic validation to Create and Update
• Removed the cluster data source in favor of the resource
• Updated and pruned examples
• Reran go generate to format examples and update docs
• Fixed debug address in main.go
• Bumped version to 0.2.0

The TF design principles document says, "A Terraform resource and associated schema should follow the naming and structure of the API, unless the it degrades the user experience or works in a way counter to a user's expectations of Terraform". The cluster resource doesn't need to include both the create and upgrade specs, plus config, in a single object. It's cleaner and more idiomatic to combine these fields, which this PR attempts to do.

The most important changes are in cluster_resource.go.

Acceptance tests pass, but are very basic. I've also got some manual testing underway, but that involves creating and modifying dedicated clusters, which takes a while. Serverless seems to work as expected.

Checklist

• [ ] I have added these changes to the changelog (or it's not applicable).

---

This change is 

[erademacher]

@marksoper The commit template has a reference to a changelog, but I don't see one anywhere. Am I just missing it, or should I update the template?

[marksoper]

I think the changelog reference is just boilerplate that can be removed

[marksoper]

main.go line 55 at r1 (raw file):

registry.terraform.io/providers/cockroachdb/cockroach This looks like the correct URL, the new value as of last week's update. It does resolve for me, not sure why you're getting a 404 @fantapop

[marksoper]

main.go line 55 at r1 (raw file):

Previously, marksoper (Mark Soper) wrote…

> registry.terraform.io/providers/cockroachdb/cockroach This looks like the correct URL, the new value as of last week's update. It does resolve for me, not sure why you're getting a 404 @fantapop

https://github.com/cockroachdb/terraform-provider-cockroach/pull/31/files

[marksoper]

@erademacher By removing the data source are you thinking that terraform import will be the way to work with resources originally provisioned outside of Terraform?

[marksoper]

examples/workflows/cockroach_serverless_cluster/main.tf line 56 at r1 (raw file):

Previously, fantapop (Christopher Fitzner) wrote…

Would it be a better example if we transformed all of cloud_provider_regions into a list of regions instead of just picking the first one?

It's Serverless so only a single region is supported.

[marksoper]

Previously, marksoper (Mark Soper) wrote…

@erademacher By removing the data source are you thinking that `terraform import` will be the way to work with resources originally provisioned outside of Terraform?

The pattern I'm seeing with other providers is that a given resource has both a TF Resource and a TF Data Source - e.g. MongoDB Atlas https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/data-sources/advanced_cluster

@erademacher are you concerned about the reliability of the Data Source defs we have in place - if not we could leave them in place as a way to access CC resources in a read-only way if they're sure they don't want to bring the resource under TF management but do want to be able to refer to it.

[fantapop]

main.go line 55 at r1 (raw file):

Previously, erademacher (Evan Rademacher) wrote…

That's very weird, because unless I remove the "/providers", I get this: ``` unable to validate ServeOpts: unable to validate Address: expected hostname/namespace/type format, got: registry.terraform.io/providers/cockroachdb/cockroach ```

I agree its very weird. It seems like maybe its not intended to be a routable address. I've looked at a few other examples and none of them resolve. Including the aws provider, vercel and planetscale.

Here is the description of that field. It seems to indicate that its used only for matching like an ID

https://github.com/hashicorp/terraform-plugin-sdk/blob/main/plugin/serve.go#L83-L88

[fantapop]

examples/workflows/cockroach_serverless_cluster/main.tf line 56 at r1 (raw file):

Previously, erademacher (Evan Rademacher) wrote…

>It's Serverless so only a single region is supported. That was my thinking, but the API spec does take a list of regions, with the goal of supporting multiregion serverless when it does roll out. I'll change it back to a list, but leave it with only one value.

sounds good. FWIW, I think its nice to provide this example using a list even though we only support one currently. This is probably the code that someone wants to use in their own templates since that field is a list.

[marksoper]

Provider produced inconsistent result after apply I can help look into this @jenngeorge - I've been looking for sort requirements suggested for Terraform config arrays. I don't think that order-is-important matching could work because users wouldn't know how to pre-order their arrays in their configs not knowing what the sort function is in the provider.

[marksoper]

Makefile line 3 at r5 (raw file):

Previously, jenngeorge (Jennifer Georgevich) wrote…

Is the namespace for provider address "registry.terraform.io/cockroachdb/cockroach" "cockroachdb" instead of "hashicorp"? I have a `.terraformrc` file with `plugin_cache_dir = "$HOME/.terraform.d/plugins"` and `terraform init` did not find the new binary from `make install` until I changed `NAMESPACE=cockroachdb` here.

If I recall this is configuration for the destination of the local dev build, doesn't apply to the registered provider. But the naming of the NAMESPACE in the Makefile may need to correspond to the actual registry URL - i.e. the dir structure locally has to have the same path as the registry for terraform to find it locally. I changed the registry setting in order to make registration work, but never changed the NAMESPACE value. So this would explain why it didn't work for you @jenngeorge and why your change fixed it.

[marksoper]

main.go line 55 at r1 (raw file):

Previously, erademacher (Evan Rademacher) wrote…

Thanks for looking that up. I'm going to leave it as-is then. I guess a URN doesn't have to be a URL.

Agreed that the correct value should be "registry.terraform.io/cockroachdb/cockroach" (without the /providers/). This is an "id" not a url. Btw our currently published provider has a totally incorrect value. The value in the main branch is a fix I applied only 9 days ago, and we have re-published since then. https://github.com/cockroachdb/terraform-provider-cockroach/pull/31/files

[marksoper]

From @jlinder on how to publish a new version of the provider:

If you push a new tag for the version number you want to publish, it will automatically build and publish the code at the SHA that was tagged. So if you push the tag v0.2.0 or v0.1.1, a GitHub Actions workflow will automatically start and do the publishing. You’ll be able to watch the logs from the running workflow on this page.

https://github.com/cockroachdb/terraform-provider-cockroach/actions/workflows/release.yml

Cc: @erademacher @jenngeorge @fantapop

[marksoper]

docs/resources/cluster.md line 46 at r5 (raw file):

- `name` (String)
- `node_count` (Number)
- `sql_dns` (String)

should sql_dns and ui_dns be "Computed" instead of "Required"?

[marksoper]

examples/workflows/cockroach_serverless_cluster/main.tf line 56 at r1 (raw file):

Previously, fantapop (Christopher Fitzner) wrote…

sounds good. FWIW, I think its nice to provide this example using a list even though we only support one currently. This is probably the code that someone wants to use in their own templates since that field is a list.

good point @fantapop I agree

[marksoper]

internal/provider/cluster_resource.go line 108 at r5 (raw file):

                      },
                  },
                  "memory_gib": {

@erademacher should this also have the "Optional: true" line?

[marksoper]

internal/provider/cockroach_cluster_data_source.go line 45 at r5 (raw file):

Previously, jenngeorge (Jennifer Georgevich) wrote…

Did removing `tfsdk.UseStateForUnknown()` result in the all the attribute values reading `(known after apply)` in the plan? It's bit long and looks like more changes than will happen. Why not use the state values for most of these attributes? ``` # data.cockroach_cluster.cockroach will be read during apply # (depends on a resource or a module with changes pending) <= data "cockroach_cluster" "cockroach" { + account_id = (known after apply) + cloud_provider = (known after apply) + cockroach_version = (known after apply) + creator_id = (known after apply) + dedicated = { + disk_iops = (known after apply) + machine_type = (known after apply) + memory_gib = (known after apply) + num_virtual_cpus = (known after apply) + storage_gib = (known after apply) } + id = "[redacted]" + name = (known after apply) + operation_status = (known after apply) + plan = (known after apply) + regions = [ ] -> (known after apply) + serverless = { + routing_id = (known after apply) + spend_limit = (known after apply) } + state = (known after apply) } ```

In the "data source", as opposed to the "resource", everything except the id is (known after apply) by definition because data sources are strictly read-only representations of the resource.

[erademacher]

Hey @jlinder, I added an env variable to point the tests to a different server. I'd like to point CI at staging, but I'm not sure where to manage secrets (i.e. the API key). Is that something you could help with? Or is that something @marksoper manages?

[jlinder]

Hi @erademacher, Up til now it's probably been something Mark has managed.

Is this a change to https://github.com/cockroachdb/terraform-provider-cockroach/blob/main/.github/workflows/ci.yaml? If so, do you want CI to always run against staging or will it run against two different servers for every run? If it's always against the staging env and the code is using the COCKROACH_API_KEY secret (like in this line), then the value of the GitHub Actions secret can be updated with the new value. And if you need another secret added, we another one can be added to the GitHub Actions secrets for the repository.

[prafull01]

@erademacher Sorry for the late review. I was on leave. Added few comments, please take a look.