Skip to content

fix: Removes interval_min plan modifier to avoid error on update #3051

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 12, 2025
Merged

fix: Removes interval_min plan modifier to avoid error on update #3051

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
061b4c4
fix interval_min plan modifier
oarbusi Feb 11, 2025
b1cb8e1
changelog
oarbusi Feb 11, 2025
bb784da
Update internal/service/alertconfiguration/resource_alert_configurati…
oarbusi Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/3051.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
resource/mongodbatlas_alert_configuration: Removes UseStateForUnknown plan modifier for interval_min
```
3 changes: 0 additions & 3 deletions internal/service/alertconfiguration/resource_alert_configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,9 +283,6 @@ func (r *alertConfigurationRS) Schema(ctx context.Context, req resource.SchemaRe
"interval_min": schema.Int64Attribute{
Optional: true,
Computed: true,
PlanModifiers: []planmodifier.Int64{
Copy link
Collaborator

@EspenAlbert EspenAlbert Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative fix would be using requireReplace on changes to type_name that might handle more cases where UseStateForUnknown is not safe

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is the Update API available, I think it's better to follow standard update behavior

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any chance this UseStateForUnknown was needed for something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the PR where it was introduced I see that it was only to reduce plan verbosity

int64planmodifier.UseStateForUnknown(),
},
},
"mobile_number": schema.StringAttribute{
Optional: true,
Expand Down
57 changes: 56 additions & 1 deletion internal/service/alertconfiguration/resource_alert_configuration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,6 @@ func datadogTestCase(t *testing.T) *resource.TestCase {
},
}
}

func TestAccConfigRSAlertConfiguration_withPagerDuty(t *testing.T) {
proxyPort := replay.SetupReplayProxy(t)

Expand Down Expand Up @@ -464,6 +463,46 @@ func TestAccConfigRSAlertConfiguration_withPagerDuty(t *testing.T) {
})
}

func TestAccConfigRSAlertConfiguration_withEmailToPagerDuty(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test case!

proxyPort := replay.SetupReplayProxy(t)

var (
projectID = replay.ManageProjectID(t, acc.ProjectIDExecution)
serviceKey = dummy32CharKey
)

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acc.PreCheckBasic(t) },
ProtoV6ProviderFactories: acc.TestAccProviderV6FactoriesWithProxy(proxyPort),
CheckDestroy: checkDestroyUsingProxy(proxyPort),
Steps: []resource.TestStep{
{
Config: configWithEmail(projectID, true),
Check: resource.ComposeAggregateTestCheckFunc(
checkExistsUsingProxy(proxyPort, resourceName),
resource.TestCheckResourceAttr(resourceName, "project_id", projectID),
),
},
{
Config: configWithPagerDuty(projectID, serviceKey, true),
Check: resource.ComposeAggregateTestCheckFunc(
checkExistsUsingProxy(proxyPort, resourceName),
resource.TestCheckResourceAttr(resourceName, "project_id", projectID),
),
},
{
ResourceName: resourceName,
ImportStateIdFunc: importStateProjectIDFunc(resourceName),
ImportState: true,
ImportStateVerify: true,
// service key is not returned by api in import operation
// integration_id is not returned during Create
ImportStateVerifyIgnore: []string{"updated", "notification.0.service_key", "notification.0.integration_id"},
},
},
})
}

func TestAccConfigAlertConfiguration_PagerDutyUsingIntegrationID(t *testing.T) {
// create a new project as it need to ensure no third party integration has already been created
var (
Expand Down Expand Up @@ -848,6 +887,22 @@ func configWithPagerDuty(projectID, serviceKey string, enabled bool) string {
`, projectID, serviceKey, enabled)
}

func configWithEmail(projectID string, enabled bool) string {
return fmt.Sprintf(`
resource "mongodbatlas_alert_configuration" "test" {
project_id = %[1]q
enabled = %[2]t
event_type = "NO_PRIMARY"

notification {
type_name = "EMAIL"
interval_min = 60
email_address = "[email protected]"
}
}
`, projectID, enabled)
}

func configWithPagerDutyIntegrationID(orgID, projectName, serviceKey string) string {
return fmt.Sprintf(`
resource "mongodbatlas_project" "test" {
Expand Down