chore: Generate alert configuration resource and add acc tests #3880
Conversation
manupedrozo
force-pushed
the
CLOUDP-356406-autogen-alert-configuration
branch
from
56a1171 to
43664fd
Compare
manupedrozo
force-pushed
the
CLOUDP-356406-autogen-alert-configuration
branch
from
43664fd to
0d1c5ca
Compare
| ImportStateIdFunc: importStateProjectIDFunc(resourceName), | ||
| ImportState: true, | ||
| ImportStateVerify: true, | ||
| ImportStateVerifyIgnore: []string{"group_id", "updated"}, |
There was a problem hiding this comment.
why do we need to ignore group_id? shouldn't change
There was a problem hiding this comment.
Good catch! Removing the project_id ignores from the handwritten resource tests as well. Not sure why they were added. 006b01d
| go install github.com/terraform-linters/tflint@v0.52.0 | ||
| go install github.com/rhysd/actionlint/cmd/actionlint@latest | ||
| go install golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment@latest | ||
| go install golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment@v0.38.0 # Pinning version since v0.39.0 fails to fix files with large structs. See versions at https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/fieldalignment?tab=versions |
There was a problem hiding this comment.
don't see a concern but curious was this a bug on their latest version (0.39.0)?
There was a problem hiding this comment.
It only started happening today, checked versions and 0.39 was released yesterday.
Can change back to latest when fixed, haven't looked if there is an associated bug.
| @@ -0,0 +1,747 @@ | |||
| package alertconfigurationapi_test | |||
There was a problem hiding this comment.
nice, great to see coverage over the full API and handling redacted sensitive values in response
tools/codegen/config.yml
Outdated
| notifications.delay_min: | ||
| computability: | ||
| optional: true | ||
| computed: true | ||
| notifications.interval_min: | ||
| computability: | ||
| optional: true | ||
| computed: true |
There was a problem hiding this comment.
so these are both getting default values returned from the API right?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks!
Adjusting the existing alert_configuration resource schema in this case could risk a breaking change. Optional+Computed implies users can potentially not have the attribute in the config but have a value in the state. As soon as the attribute changes from Optional+Computed to Optional this would trigger a plan change in which terraform tries to unset the attribute. We could create a ticket and flag as candidate for including in a 3.0 release.
There was a problem hiding this comment.
Got it, thanks! To clarify, a user having the value in the state but not config is not expected, correct? Or is there a common flow where this may happen?
There was a problem hiding this comment.
Given API behaviour should not be common scenario: Potentially a user that defined the attribute and then removes it, leaving the config with no attribute but the state will still have it present.
|
Tests failing due to S3 issue. Unrelated to PR changes. Oncall looking into it with upstream team. |
Description
Generate
alert_configuration_apiand add acc tests based on the handwrittenalert_configuration.Link to any related issue(s): CLOUDP-356406
Type of change:
Required Checklist:
Further comments