-
Notifications
You must be signed in to change notification settings - Fork 10
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
Allow enabling log export for serverless clusters. #201
base: cloud-20
Are you sure you want to change the base?
Conversation
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? |
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
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. |
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. |
But does this mean we have not tests for this feature, if there are none for Serverless and Dedicated tests are disabled? |
No description provided.