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.

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

fix: avoid diff in state after import for undefined optional attribute in alert config notification #1412

Merged
merged 1 commit into from
Aug 25, 2023
Merged

fix: avoid diff in state after import for undefined optional attribute in alert config notification #1412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions mongodbatlas/fw_resource_mongodbatlas_alert_configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,10 @@ func (r *AlertConfigurationRS) Schema(ctx context.Context, req resource.SchemaRe
},
"interval_min": schema.Int64Attribute{
Optional: true,
Computed: true,
PlanModifiers: []planmodifier.Int64{
int64planmodifier.UseStateForUnknown(),
},
Comment on lines +295 to +297
Copy link
Collaborator

Choose a reason for hiding this comment

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

UseStateForUnknown(): Copies the prior state value, if not null. This is useful for reducing (known after apply) plan outputs for computed attributes which are known to not change over time.

Why is this modifier needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Essentially to simplify the terraform plan output. The interval_min attribute will only change its value when modified by the user.
As a counter example, alert configuration has an updated attribute that is computed and is not set with this modifier. In this case is seems appropriate to have the message (known after apply) shown on the terraform plan because on every apply it will be modified.

},
"mobile_number": schema.StringAttribute{
Optional: true,
Expand Down Expand Up @@ -709,9 +713,6 @@ func newTFNotificationModelList(matlasSlice []matlas.Notification, currStateNoti
if !currState.EmailAddress.IsNull() {
newState.EmailAddress = conversion.StringNullIfEmpty(value.EmailAddress)
}
if !currState.IntervalMin.IsNull() {
newState.IntervalMin = types.Int64Value(int64(value.IntervalMin))
}
Comment on lines -712 to -714
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was reading this plan-modification#plan-modification-process

Plan Modification Process
When the provider receives a request to generate the plan for a resource change via the framework, the following occurs:

Set any attributes with a null configuration value to the default value.
If the plan differs from the current resource state, the framework marks computed attributes that are null in the configuration as unknown in the plan. This is intended to prevent unexpected Terraform errors. Providers can later enter any values that may be known.
Run attribute plan modifiers.
Run resource plan modifiers.

I am wondering if currState.IntervalMin.isNill is false but currState.IntervalMin.IsUnknown() is true when calling terraform plan. Could you verify? if that is the case we should add currState.IntervalMin.IsUnknown() in all of this ifs

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes your statement is correct, I could verify that the attribute in currState comes as unknown as it is an undefined computed attribute.

In this case, since it is a value that is always defined in the api request/response (defined as an int in manual sdk), we can simply define the value in the model. This is what solves the inconsistency issue when doing the import operation, because when importing you have no state/config to verify if field was defined by the user, so value coming from the api is set directly.

if !currState.MobileNumber.IsNull() {
newState.MobileNumber = conversion.StringNullIfEmpty(value.MobileNumber)
}
Expand All @@ -728,6 +729,7 @@ func newTFNotificationModelList(matlasSlice []matlas.Notification, currStateNoti
newState.Username = conversion.StringNullIfEmpty(value.Username)
}

newState.IntervalMin = types.Int64Value(int64(value.IntervalMin))
newState.DelayMin = types.Int64Value(int64(*value.DelayMin))
newState.EmailEnabled = types.BoolValue(value.EmailEnabled != nil && *value.EmailEnabled)
newState.SMSEnabled = types.BoolValue(value.SMSEnabled != nil && *value.SMSEnabled)
Expand Down
33 changes: 33 additions & 0 deletions mongodbatlas/fw_resource_mongodbatlas_alert_configuration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,39 @@ func TestAccConfigRSAlertConfiguration_importConfigNotifications(t *testing.T) {
})
}

// used for testing notification that does not define interval_min attribute
func TestAccConfigRSAlertConfiguration_importPagerDuty(t *testing.T) {
SkipTestExtCred(t) // Will skip because requires external credentials aka api key
var (
resourceName = "mongodbatlas_alert_configuration.test"
projectID = os.Getenv("MONGODB_ATLAS_PROJECT_ID")
serviceKey = os.Getenv("PAGER_DUTY_SERVICE_KEY")
alert = &matlas.AlertConfiguration{}
)

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheckBasic(t) },
ProtoV6ProviderFactories: testAccProviderV6Factories,
CheckDestroy: testAccCheckMongoDBAtlasAlertConfigurationDestroy,
Steps: []resource.TestStep{
{
Config: testAccMongoDBAtlasAlertConfigurationPagerDutyConfig(projectID, serviceKey, true),
Check: resource.ComposeTestCheckFunc(
testAccCheckMongoDBAtlasAlertConfigurationExists(resourceName, alert),
resource.TestCheckResourceAttrSet(resourceName, "project_id"),
),
},
{
ResourceName: resourceName,
ImportStateIdFunc: testAccCheckMongoDBAtlasAlertConfigurationImportStateIDFunc(resourceName),
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"notification.0.service_key"}, // service key is not returned by api in import operation
},
},
})
}

func TestAccConfigRSAlertConfiguration_DataDog(t *testing.T) {
SkipTestExtCred(t) // Will skip because requires external credentials aka api key
SkipTest(t) // Will force skip if enabled
Expand Down