data_factory_pipeline - rename moniter_metrics_after_duration property to monitor_metrics_after_duration #32177
data_factory_pipeline - rename moniter_metrics_after_duration property to monitor_metrics_after_duration #32177LingyuTang wants to merge 4 commits intohashicorp:mainfrom
data_factory_pipeline - rename moniter_metrics_after_duration property to monitor_metrics_after_duration #32177Conversation
liuwuliuyun
left a comment
There was a problem hiding this comment.
Left some comments, thanks~
| } | ||
| } | ||
|
|
||
| if !features.FivePointOh() { |
There was a problem hiding this comment.
In the CreateUpdate function, the deprecated field check will always overwrite the new field's policy value in pre-5.0 mode:
// First: sets policy from the NEW field ✅
if v, ok := d.GetOk("monitor_metrics_after_duration"); ok {
payload.Properties.Policy = &pipelines.PipelinePolicy{...}
}
// Second: OVERWRITES policy from the deprecated field ❌
if !features.FivePointOh() {
if !pluginsdk.IsExplicitlyNullInConfig(d, "moniter_metrics_after_duration") {
payload.Properties.Policy = &pipelines.PipelinePolicy{
Duration: pointer.To(d.Get("moniter_metrics_after_duration")), // empty/zero value
}
}
}
IsExplicitlyNullInConfig returns true only when a field is explicitly set to null in HCL. When the field is simply absent from config, it returns false, so !false = true — the block executes and overwrites the policy with d.Get("moniter_metrics_after_duration") which returns the zero/computed value.
Result: When a user sets monitor_metrics_after_duration = "00:01:00", the policy is correctly set, then immediately overwritten with an empty value from the deprecated field.
There was a problem hiding this comment.
Hey @liuwuliuyun thanks for the review and pointing out the situation. I checked the IsExplicitlyNullInConfig function, from the comment above the function, seems like it will return true when the field is absent. Source code here.
So, I did some test with debug log on, it did return true when the field is not presents. I guess the function name is kind of misleading....
| } | ||
|
|
||
| if !features.FivePointOh() { | ||
| if !pluginsdk.IsExplicitlyNullInConfig(d, "moniter_metrics_after_duration") { |
There was a problem hiding this comment.
pluginsdk.IsExplicitlyNullInConfig is intended for CustomizeDiff scenarios where distinguishing "not set" vs "explicitly null" matters. The standard pattern for checking whether an optional field was provided in Create/Update functions is d.GetOk(). Using IsExplicitlyNullInConfig here deviates from established provider patterns
There was a problem hiding this comment.
Thanks for the review! I’m using this approach because it’s recommended in the https://hashicorp.github.io/terraform-provider-azurerm/topics/guide-breaking-changes/ in the contribution docs.
When drafting the change, I initially tried GetOk, but it failed the update test because GetOk can’t distinguish whether the user explicitly set the value in the config. In this case, if a user had previously set moniter_metrics_after_duration, that value would already exist in state. Later, when the user switches to monitor_metrics_after_duration (both map to the same state field), the old moniter_metrics_after_duration value would overwrite the user’s new change.
| @@ -184,6 +185,94 @@ resource "azurerm_data_factory_pipeline" "test" { | |||
| } | |||
|
|
|||
| func (PipelineResource) complete(data acceptance.TestData) string { | |||
There was a problem hiding this comment.
The complete test config correctly branches on !features.FivePointOh() to use the deprecated field name. However, the update config always uses monitor_metrics_after_duration without a pre-5.0 branch. This means:
Pre-5.0 test flow: create with moniter_metrics_after_duration → update with monitor_metrics_after_duration
This implicitly tests a field migration rather than a pure update of the deprecated field
The update config should also have a !features.FivePointOh() branch using moniter_metrics_after_duration to properly test the deprecated field's update path.
There was a problem hiding this comment.
Changed the update test to have !features.FivePointOh()
| } | ||
|
|
||
| func (PipelineResource) complete(data acceptance.TestData) string { | ||
| if !features.FivePointOh() { |
There was a problem hiding this comment.
Could we also include this in a template?
There was a problem hiding this comment.
Done, use the template for duplicate test string
|
Hey @liuwuliuyun thanks I have made the changes based on your review. Could you check my comment and the changed code when you have the time? Let me know if you have any questions! |
Community Note
Description
This PR is to resolve #32137. Rename the
moniter_metrics_after_durationtomonitor_metrics_after_durationPR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
data_factory_pipeline- renamemoniter_metrics_after_durationtomonitor_metrics_after_duration[azurerm_data_factory_pipeline: typo in a parameter #32137]This is a (please select all that apply):
Related Issue(s)
Fixes #32137
AI Assistance Disclosure
Rollback Plan
If a change needs to be reverted, we will publish an updated version of the provider.
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
Note
If this PR changes meaningfully during the course of review please update the title and description as required.