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

[AgustinBettati]

Description

Related ticket: case found when testing INTMDB-981

Summary: adjust optional attribute interval_min to handle default values the same way as delay_min to avoid non empty plan after using import operation.

For the following valid resource that does not define interval_min attribute:

resource "mongodbatlas_alert_configuration" "test" {
  project_id = "64c0f3f5ce752426ab9f506b"
  event_type = "NO_PRIMARY"
  enabled    = true

  notification {
    type_name    = "PAGER_DUTY"
    delay_min    = 4
    service_key = "<service key>"
  }
}

When import statement and HCL code is generated using the following data source for the created resource above:

data "mongodbatlas_alert_configuration" "test" {
  project_id             = "64c0f3f5ce752426ab9f506b"
  alert_configuration_id = "aWQ=:NjRlNzBmNDJjM2MyOTM2OGFiNWY4Mjk5-cHJvamVjdF9pZA==:NjRjMGYzZjVjZTc1MjQyNmFiOWY1MDZi"

  output {
    type = "resource_import"
  }
  output {
    type = "resource_hcl"
  }
}

[Previous] Terraform plan output on the generated resource code results in a non empty plan:

[New] Terraform plan output:

Type of change:

• Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
• New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
• Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
• This change requires a documentation update
• Documentation fix/enhancement

Required Checklist:

• I have signed the MongoDB CLA
• I have read the contribution guidelines
• I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
• I have added any necessary documentation (if appropriate)
• I have run make fmt and formatted my code

Further comments

Test that was defined needs credentials, so ran all resource and data source tests:

[andreaangiolillo]

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?

[AgustinBettati]

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.

[andreaangiolillo]

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

[AgustinBettati]

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.

[andreaangiolillo]

LGTM. Thanks for the discussion

[AgustinBettati]

@maastha small change here just in case you want to take a look

[maastha]

LGTM