-
Notifications
You must be signed in to change notification settings - Fork 5k
data_factory_pipeline - rename moniter_metrics_after_duration property to monitor_metrics_after_duration
#32177
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
62de371
a5a557d
2c332a5
1ff3ac3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import ( | |
| "github.com/hashicorp/go-azure-sdk/resource-manager/datafactory/2018-06-01/pipelines" | ||
| "github.com/hashicorp/terraform-provider-azurerm/helpers/tf" | ||
| "github.com/hashicorp/terraform-provider-azurerm/internal/clients" | ||
| "github.com/hashicorp/terraform-provider-azurerm/internal/features" | ||
| "github.com/hashicorp/terraform-provider-azurerm/internal/services/datafactory/validate" | ||
| "github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk" | ||
| "github.com/hashicorp/terraform-provider-azurerm/internal/tf/validation" | ||
|
|
@@ -23,7 +24,7 @@ import ( | |
| ) | ||
|
|
||
| func resourceDataFactoryPipeline() *pluginsdk.Resource { | ||
| return &pluginsdk.Resource{ | ||
| resource := &pluginsdk.Resource{ | ||
| Create: resourceDataFactoryPipelineCreateUpdate, | ||
| Read: resourceDataFactoryPipelineRead, | ||
| Update: resourceDataFactoryPipelineCreateUpdate, | ||
|
|
@@ -104,12 +105,33 @@ func resourceDataFactoryPipeline() *pluginsdk.Resource { | |
| ValidateFunc: validation.StringIsNotEmpty, | ||
| }, | ||
|
|
||
| "moniter_metrics_after_duration": { | ||
| Type: pluginsdk.TypeString, | ||
| Optional: true, | ||
| "monitor_metrics_after_duration": { | ||
| Type: pluginsdk.TypeString, | ||
| Optional: true, | ||
| ValidateFunc: validation.StringIsNotEmpty, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| if !features.FivePointOh() { | ||
| resource.Schema["moniter_metrics_after_duration"] = &pluginsdk.Schema{ | ||
| Type: pluginsdk.TypeString, | ||
| Optional: true, | ||
| Computed: true, | ||
| ValidateFunc: validation.StringIsNotEmpty, | ||
| ConflictsWith: []string{"monitor_metrics_after_duration"}, | ||
| Deprecated: "`moniter_metrics_after_duration` has been deprecated in favour of `monitor_metrics_after_duration` and will be removed in v5.0 of the AzureRM Provider", | ||
| } | ||
| resource.Schema["monitor_metrics_after_duration"] = &pluginsdk.Schema{ | ||
| Type: pluginsdk.TypeString, | ||
| Optional: true, | ||
| Computed: true, | ||
| ValidateFunc: validation.StringIsNotEmpty, | ||
| ConflictsWith: []string{"moniter_metrics_after_duration"}, | ||
| } | ||
| } | ||
|
|
||
| return resource | ||
| } | ||
|
|
||
| func resourceDataFactoryPipelineCreateUpdate(d *pluginsdk.ResourceData, meta interface{}) error { | ||
|
|
@@ -187,14 +209,24 @@ func resourceDataFactoryPipelineCreateUpdate(d *pluginsdk.ResourceData, meta int | |
| payload.Properties.Concurrency = pointer.To(int64(v.(int))) | ||
| } | ||
|
|
||
| if v, ok := d.GetOk("moniter_metrics_after_duration"); ok { | ||
| if v, ok := d.GetOk("monitor_metrics_after_duration"); ok { | ||
| payload.Properties.Policy = &pipelines.PipelinePolicy{ | ||
| ElapsedTimeMetric: &pipelines.PipelineElapsedTimeMetricPolicy{ | ||
| Duration: pointer.To(v), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| if !features.FivePointOh() { | ||
| if !pluginsdk.IsExplicitlyNullInConfig(d, "moniter_metrics_after_duration") { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| payload.Properties.Policy = &pipelines.PipelinePolicy{ | ||
| ElapsedTimeMetric: &pipelines.PipelineElapsedTimeMetricPolicy{ | ||
| Duration: pointer.To(d.Get("moniter_metrics_after_duration")), | ||
| }, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if v, ok := d.GetOk("folder"); ok { | ||
| payload.Properties.Folder = &pipelines.PipelineFolder{ | ||
| Name: pointer.To(v.(string)), | ||
|
|
@@ -258,7 +290,11 @@ func resourceDataFactoryPipelineRead(d *pluginsdk.ResourceData, meta interface{} | |
| elapsedTimeMetricDuration = v | ||
| } | ||
| } | ||
| d.Set("moniter_metrics_after_duration", elapsedTimeMetricDuration) | ||
|
|
||
| d.Set("monitor_metrics_after_duration", elapsedTimeMetricDuration) | ||
| if !features.FivePointOh() { | ||
| d.Set("moniter_metrics_after_duration", elapsedTimeMetricDuration) | ||
| } | ||
|
|
||
| if folder := props.Folder; folder != nil { | ||
| d.Set("folder", pointer.From(folder.Name)) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the CreateUpdate function, the deprecated field check will always overwrite the new field's policy value in pre-5.0 mode:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @liuwuliuyun thanks for the review and pointing out the situation. I checked the
IsExplicitlyNullInConfigfunction, 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....