-
Notifications
You must be signed in to change notification settings - Fork 208
feat: Adds support for alertConfiguration.severityOverride attribute #3795
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
base: master
Are you sure you want to change the base?
Conversation
|
APIx bot: a message has been sent to Docs Slack channel |
| - `Basic import tests` are done as the last step in the `basic acceptance tests`, not as a different test, e.g. [basicTestCase](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/searchindex/resource_search_index_test.go#L211). Exceptions apply for more specific import tests, e.g. testing with incorrect IDs. [Import tests](https://developer.hashicorp.com/terraform/plugin/sdkv2/resources/import#resource-acceptance-testing-implementation) verify that the [Terraform Import](https://developer.hashicorp.com/terraform/cli/import) functionality is working fine. | ||
| - Data sources are tested in the same tests as the resources, e.g. [commonChecks](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/searchindex/resource_search_index_test.go#L262-L263). | ||
| - Helper functions such as `resource.TestCheckTypeSetElemNestedAttrs` or `resource.TestCheckTypeSetElemAttr` can be used to check resource and data source attributes more easily, e.g. [resource_serverless_instance_test.go](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/66c44e62c9afe04ffe8be0dbccaec682bab830e6/internal/service/serverlessinstance/resource_serverless_instance_test.go#L61). | ||
| - Note: before running the acceptance tests, make sure you set the environment variable `TF_ACC=1`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: my recommendation is to use the Make target that already adds it for you, e.g.:
ACCTEST_PACKAGES=./internal/service/databaseuser ACCTEST_REGEX_RUN='TestAccDatabaseUser_basic' make testacc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my note is based on the VS Code run/debug experience, which is most likely what most repo contributors will use, wdyt?
| Enabled: types.BoolValue(true), | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's previous to this PR but consider to have the testCase as a map[string]struct... with test name as keys, we're using this format in most tests
| @@ -0,0 +1,7 @@ | |||
| ```release-note:enhancement | |||
| data-source/mongodbatlas_alert_configuration: Adds `severity_override` attribute | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also in plural data source?
| Config: configWithSeverityOverride(projectID, "WARNING"), | ||
| Check: resource.ComposeAggregateTestCheckFunc( | ||
| checkExists(resourceName), | ||
| resource.TestCheckResourceAttr(resourceName, "project_id", projectID), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in TPF (terraform-plugin-framework) resources, only computed attributes are interested to be checked, values provided in the config don't need to be checked because that's a TF guarantee.
it's especially useful for data source where most attributes are computed, and for computed attributes in resources
(we still have these checks in many TPF tests, for SDKv2 they're needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed these checks added by this PR, but would prefer to defer the cleanup for previous tests.
| data-source/mongodbatlas_alert_configuration: Adds `severity_override` attribute | ||
| ``` | ||
|
|
||
| ```release-note:enhancement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documentation for the new attribute is not updated, or will be in a follow-up PR? (update is small so could go here)
|
if branch name is correct, CLOUDP-31301 in PR description is missing the last number, should be CLOUDP-313016 (which is an epic instead of story) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Leo brings up a couple of good points, good to address those first
| }, | ||
| }, | ||
| "severity_override": schema.StringAttribute{ | ||
| Optional: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to double check: This is never returned by the API unless defined by the user? How do the user reset the attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is never returned by the API unless defined by the user?
correct
How do the user reset the attribute?
as of now, this flag cannot be cleared (if not set in the PUT request body it won't be reset). Same happens from UI too, I will open a ticket with the owning team.
I added a note in the docs about explicitly setting it to ERROR (the default).
|
Related issue is unrelated? |
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: configWithSeverityOverride(projectID, "WARNING"), | ||
| Check: resource.ComposeAggregateTestCheckFunc( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add an additional step modifying the value, and also capture behaviour of what happens when attribute is removed (related to Espen thread).
Description
CLOUDP-313016: add support for alertConfiguration.severityOverride attribute
Type of change:
Required Checklist:
Further comments