New data source: `azurerm_dynatrace_monitor` by jiaweitao001 · Pull Request #28381 · hashicorp/terraform-provider-azurerm

jiaweitao001 · 2024-12-25T07:25:35Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

New Data Source: azurerm_dynatrace_monitor [GH-00000]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #0000

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

jackofallops

Hi @jiaweitao001 - Thanks for the PR. Some comments to take a look at below if you could please?

jackofallops · 2025-01-13T12:14:58Z

internal/services/dynatrace/dynatrace_monitors_data_source.go

+		"environment_properties": {
+			Type:     pluginsdk.TypeList,
+			Computed: true,
+			Elem: &pluginsdk.Resource{
+				Schema: map[string]*pluginsdk.Schema{
+					"environment_info": {
+						Type:     pluginsdk.TypeList,
+						Computed: true,
+						Elem: &pluginsdk.Resource{
+							Schema: map[string]*pluginsdk.Schema{
+								"environment_id": {
+									Type:     pluginsdk.TypeString,
+									Computed: true,
+								},
+							},
+						},
+					},
+				},
+			},
+		},


Is there a reason we're doing 2 levels of nesting here for a single value? Can this just be flattened to the top level?

Also, this property doesn't appear to be supported by the related resource, should it even be included here?

We'd better not flatten this because according to the service team, there are a bunch of other attributes under environment_properties and environment_info, but they do not want them on TF right now. We will have to onboard them once they request in the future.

This property is not supported by the related resource is because it's an optional + computed attribute and its value comes from the API response only, that's why we will have to onboard the data source first. Then I'll add it to the related resource.

jackofallops · 2025-01-13T12:17:09Z

internal/services/dynatrace/dynatrace_monitors_data_source.go

+					Identity:                      identityProps,
+					EnvironmentProperties:         FlattenDynatraceEnvironmentProperties(props.DynatraceEnvironmentProperties),
+					PlanData:                      FlattenDynatracePlanData(props.PlanData),
+					UserInfo:                      FlattenDynatraceUserInfo(metadata.ResourceData.Get("user").([]interface{})),


This is computed only, so we cannot "Get" it here, if this is not returned by the API, it cannot be included in the Data Source as it will always be empty.

jackofallops · 2025-01-13T12:19:08Z

internal/services/dynatrace/dynatrace_monitors_resource_test.go

+    first_name   = "Alice"
+    last_name    = "Bobab"
+    email        = "agarwald@microsoft.com"
+    phone_number = "123456"
+    country      = "westus"


Why are we now supplying data directly here? This looks like it might actually be a real email (or could be in future?) Is this a mistake from local testing that should be removed?

Right, it was for local testing, will remove them.

jackofallops · 2025-01-13T12:19:40Z

website/docs/d/dynatrace_monitors.html.markdown

+
+---
+
+A `user` block supports the following:


and here

Suggested change

A `user` block supports the following:

A `user` block exports the following:

jackofallops · 2025-01-13T12:19:47Z

website/docs/d/dynatrace_monitors.html.markdown

+
+---
+
+A `plan` block supports the following:


same here

Suggested change

A `plan` block supports the following:

A `plan` block exports the following:

jackofallops · 2025-01-13T12:20:33Z

website/docs/d/dynatrace_monitors.html.markdown

+
+---
+
+A `identity` block supports the following:


Since this block is computed only, it's exports rather than supports.

Suggested change

A `identity` block supports the following:

An `identity` block exports the following:

jackofallops · 2025-01-13T12:21:05Z

website/docs/d/dynatrace_monitors.html.markdown

+
+A `identity` block supports the following:
+
+* `type` - The type of identity used for the resource. Only possible value is `SystemAssigned`.


Since this is computed only, we don't specify possible values.

Suggested change

* `type` - The type of identity used for the resource. Only possible value is `SystemAssigned`.

* `type` - The type of identity used for the resource.

@jackofallops / @jiaweitao001, should we even expose type if the only valid value is SystemAssigned?

WodansSon

@jiaweitao001, Thank you for this PR! I have given it a look, while it mostly LGTM I did leave a comment with some questions about the implementation. If you can take a look at that, I will be happy to look again once that is addressed. Thanks! 🚀

WodansSon · 2025-02-14T07:20:07Z

internal/services/dynatrace/helper.go

+func FlattenDynatraceEnvironmentProperties(input *monitors.DynatraceEnvironmentProperties) []EnvironmentProperties {
+	if input == nil {
+		return []EnvironmentProperties{}
+	}
+
+	environmentInfo := FlattenDynatraceEnvironmentInfo(input.EnvironmentInfo)
+
+	return []EnvironmentProperties{
+		{
+			EnvironmentInfo: environmentInfo,
+		},
+	}
+}
+
+func FlattenDynatraceEnvironmentInfo(input *monitors.EnvironmentInfo) []EnvironmentInfo {
+	if input == nil {
+		return []EnvironmentInfo{}
+	}
+
+	return []EnvironmentInfo{
+		{
+			EnvironmentId: pointer.From(input.EnvironmentId),
+		},
+	}
+}


Couldn't these two functions be collapsed into a single function (e.g., FlattenDynatraceEnvironmentProperties)? I don't understand why we need the second FlattenDynatraceEnvironmentInfo function unless it is called from another resource? Also, should these be public functions vs. a private function (e.g., FlattenDynatraceEnvironmentProperties vs. flattenDynatraceEnvironmentProperties)?

Suggested change

func FlattenDynatraceEnvironmentProperties(input *monitors.DynatraceEnvironmentProperties) []EnvironmentProperties {

if input == nil {

return []EnvironmentProperties{}

}

environmentInfo := FlattenDynatraceEnvironmentInfo(input.EnvironmentInfo)

return []EnvironmentProperties{

{

EnvironmentInfo: environmentInfo,

},

}

}

func FlattenDynatraceEnvironmentInfo(input *monitors.EnvironmentInfo) []EnvironmentInfo {

if input == nil {

return []EnvironmentInfo{}

}

return []EnvironmentInfo{

{

EnvironmentId: pointer.From(input.EnvironmentId),

},

}

}

func FlattenDynatraceEnvironmentProperties(input *monitors.DynatraceEnvironmentProperties) []EnvironmentProperties {

result := []EnvironmentProperties{

EnvironmentInfo: pointer.To([]EnvironmentInfo{}),

}

if input == nil {

return result

}

if input.EnvironmentInfo.EnvironmentId != nil {

result.EnvironmentInfo.EnvironmentId = pointer.From(input.EnvironmentInfo.EnvironmentId)

}

return result

}

@WodansSon , I'm not combining these 2 functions because there're other attributes under this EnvironmentProperties as well as EnvironmentInfo, the service said they will give us more details on how to configure them recently so I'll leave the functions uncollapsed.

@jiaweitao001, I understand that, however, as more and more child EnvironmentProperties are exposed by the service team we would just need to inline those blocks into the one flatten function (e.g, flattenDynatraceEnvironmentsProperties), making the code much easier to read/understand and maintain moving forward for repo maintainers. If we end up with 12 flatten functions that get called from the parent flatten function it becomes exponentially more complicated to understand what the code is doing and review.

Make sense. I'll combine these functions.

WodansSon

@jiaweitao001, thanks for pushing those changes, this LGTM now! 🚀

WodansSon · 2025-02-15T05:03:28Z

Tests Pass?:

* New data source: azurerm_dynatrace_monitor * Remove deprecated method * address comments * Address comment on collapsing flatten func

github-actions · 2025-03-18T02:16:23Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

New data source: azurerm_dynatrace_monitor

30066ee

jiaweitao001 requested a review from a team as a code owner December 25, 2024 07:25

github-actions bot added documentation service/dynatrace size/XL waiting-response labels Dec 25, 2024

Remove deprecated method

36e729f

github-actions bot removed the waiting-response label Dec 25, 2024

rcskosir added the new-data-source label Jan 8, 2025

jackofallops requested changes Jan 13, 2025

View reviewed changes

github-actions bot assigned jackofallops Jan 13, 2025

github-actions bot added the waiting-response label Jan 13, 2025

address comments

2e1095c

github-actions bot removed the waiting-response label Jan 14, 2025

jiaweitao001 requested a review from jackofallops February 7, 2025 09:06

WodansSon requested changes Feb 14, 2025

View reviewed changes

github-actions bot assigned WodansSon Feb 14, 2025

github-actions bot added the waiting-response label Feb 14, 2025

Address comment on collapsing flatten func

72f582e

github-actions bot removed the waiting-response label Feb 14, 2025

WodansSon approved these changes Feb 15, 2025

View reviewed changes

WodansSon merged commit 3cd63a3 into hashicorp:main Feb 15, 2025
36 checks passed

github-actions bot added this to the v4.20.0 milestone Feb 15, 2025

WodansSon added a commit that referenced this pull request Feb 15, 2025

Update CHANGELOG.md #28381

553f96b

github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2025

	A `user` block supports the following:
	A `user` block exports the following:

	A `plan` block supports the following:
	A `plan` block exports the following:

	A `identity` block supports the following:
	An `identity` block exports the following:


		A `identity` block supports the following:

		* `type` - The type of identity used for the resource. Only possible value is `SystemAssigned`.

Conversation

jiaweitao001 commented Dec 25, 2024 • edited by WodansSon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Uh oh!

jackofallops left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WodansSon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WodansSon Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WodansSon left a comment

Choose a reason for hiding this comment

Uh oh!

WodansSon commented Feb 15, 2025

Uh oh!

Uh oh!

github-actions bot commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jiaweitao001 commented Dec 25, 2024 •

edited by WodansSon

Loading

WodansSon left a comment •

edited

Loading

WodansSon Feb 14, 2025 •

edited

Loading