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

Allow enabling log export for serverless clusters. #201

Open
wants to merge 1 commit into
base: cloud-20
Choose a base branch
from

Conversation

alyshanjahani-crl
Copy link

No description provided.

@fantapop
Copy link
Collaborator

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

@alyshanjahani-crl
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

@fantapop fantapop self-requested a review April 24, 2024 23:03
@andy-kimball
Copy link
Contributor

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

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.

None yet

3 participants