search

cockroachdb / terraform-provider-cockroach

Terraform provider for CockroachDB Cloud
Apache License 2.0
55 stars 10 forks source link

Allow enabling log export for serverless clusters. #201

Open alyshanjahani-crl opened 2 months ago

fantapop commented 2 months ago

Are there any test updates we can make for this change?

alyshanjahani-crl commented 2 months ago

Are there any test updates we can make for this change?

I didn't take a look at the tests here, tbh i was kinda surprised we're doing this checking here in terraform provider. Because the actual console/intrusion endpoints perform all this validation I.e previously, enabling log export for serverless was not supported, console/intrusion would return an error. After i implemented it, it is supported and i was surprised that i even had to make this PR :/

Is the validation done here so that the error message is user readable? Does marking an error from console/intrusion as a UserError not bubble up through the terraform provider well?

fantapop commented 2 months ago

Are there any test updates we can make for this change?

I didn't take a look at the tests here, tbh i was kinda surprised we're doing this checking here in terraform provider. Because the actual console/intrusion endpoints perform all this validation I.e previously, enabling log export for serverless was not supported, console/intrusion would return an error. After i implemented it, it is supported and i was surprised that i even had to make this PR :/

If there are no differences between the tf resources or expected differences from the perspective of the api, I think its fine to test with either serverless or dedicated. We do have an integration test for each resource and the current one for log_export_config is spinning up a dedicated AWS cluster (here). I think we should swap it to use a serverless cluster instead so its quicker and doesn't use up an AWS account every time we run to the acceptance tests.

Is the validation done here so that the error message is user readable? Does marking an error from console/intrusion as a UserError not bubble up through the terraform provider well?

Sometimes we do some validation up front to provide a quicker or better message to user. In general I'm a fan of relying on the api to provide those errors and I think that generally works.

fantapop commented 2 months ago

I just noticed that the acceptance tests for this are actually marked as skipped for now. So no need to update the test for now.

andy-kimball commented 2 months ago

But does this mean we have not tests for this feature, if there are none for Serverless and Dedicated tests are disabled?