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

Jump to bottom

New Resource: azurerm_datadog_monitor #16131

Merged
merged 18 commits into from
Jul 28, 2022
Merged

New Resource: azurerm_datadog_monitor #16131

merged 18 commits into from
Jul 28, 2022

Conversation

vikotha
Copy link
Contributor

@vikotha vikotha commented Mar 29, 2022

No description provided.

ms-henglu
ms-henglu suggested changes Mar 31, 2022
View reviewed changes
Copy link
Contributor

@ms-henglu ms-henglu left a comment

Choose a reason for hiding this comment

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

Thank you for opening this PR, it looks great!

I left some suggestions, please take a look.

internal/services/datadog/datadog_monitors_resource.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource_test.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource_test.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Outdated Show resolved Hide resolved
katbyte
katbyte requested changes Apr 5, 2022
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @vikotha - left some comments while passing thorugh inline

website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
@vikotha vikotha requested a review from katbyte April 6, 2022 05:41
@vikotha
Copy link
Contributor Author

vikotha commented Apr 13, 2022

@katbyte can you please review the updated changes.

@vikotha
Copy link
Contributor Author

vikotha commented May 20, 2022

@koikonom Can you please review this PR or assign someone

@katbyte katbyte requested a review from mbfrahry May 30, 2022 15:22
@vikotha
Copy link
Contributor Author

vikotha commented May 31, 2022

Hey @mbfrahry

In our scenario, there exists a RT which has child resources with 1:1 mapping. So should it be part of same schema or we can have separate schema for child RTs?

@WodansSon WodansSon added this to the v3.9.0 milestone May 31, 2022
mbfrahry
mbfrahry requested changes Jun 2, 2022
View reviewed changes
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @vikotha, thanks for opening this PR! Datadog Monitor was a doozy to figure out but this PR does a good job of mapping that functionality into Terraform. I've called out quite a few things and they're mostly gotchas from the api not returning things the way we expected.

Unfortunately, the acceptance tests don't work because our acceptance test framework only uses service principal auth. I don't have a solution for that yet, but I'll report back once I do

internal/services/datadog/datadog_monitors_resource.go Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource_test.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource_test.go Show resolved Hide resolved
@tombuildsstuff tombuildsstuff modified the milestones: v3.9.0, v3.10.0 Jun 3, 2022
@vikotha
Copy link
Contributor Author

vikotha commented Jun 8, 2022

Hey @mbfrahry

In our scenario, there exists a RT which has child resources with 1:1 mapping. So should it be part of same schema or we can have separate schema for child RTs?

@mbfrahry Any update on this?

@tombuildsstuff tombuildsstuff removed this from the v3.10.0 milestone Jun 10, 2022
@tombuildsstuff tombuildsstuff modified the milestones: v3.14.0, v3.15.0 Jul 15, 2022
@vikotha
Copy link
Contributor Author

vikotha commented Jul 18, 2022

@mbfrahry, can you please review the changes.

mbfrahry
mbfrahry requested changes Jul 18, 2022
View reviewed changes
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @vikotha, I think we're nearly there. The big asks are to remove the d.Set for users and a big pass through the documentation. All throughout the doc,datadog Monitor needs to be cased to Datadog Monitor

internal/services/datadog/datadog_monitors_resource.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource.go Outdated Show resolved Hide resolved
internal/services/datadog/datadog_monitors_resource_test.go Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
@vikotha vikotha requested a review from mbfrahry July 20, 2022 05:42
mbfrahry
mbfrahry requested changes Jul 20, 2022
View reviewed changes
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Looks like a couple things were still missed and we need to update the tests to skip them if those environments variables haven't been set.

website/docs/r/datadog_monitors.html.markdown Show resolved Hide resolved
website/docs/r/datadog_monitors.html.markdown Outdated Show resolved Hide resolved
@vikotha
Copy link
Contributor Author

vikotha commented Jul 23, 2022

Looks like a couple things were still missed and we need to update the tests to skip them if those environments variables haven't been set.

Can you point out those parameters.

@tombuildsstuff tombuildsstuff modified the milestones: v3.15.0, v3.16.0 Jul 25, 2022
@mbfrahry
Copy link
Member

Hey @vikotha, this ask around tests that needs to be looked at #16131 (comment)

@vikotha vikotha requested a review from mbfrahry July 28, 2022 19:55
@vikotha
Copy link
Contributor Author

vikotha commented Jul 28, 2022

Hey @vikotha, this ask around tests that needs to be looked at #16131 (comment)

@mbfrahry Added the check

mbfrahry
mbfrahry approved these changes Jul 28, 2022
View reviewed changes
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for getting everything addressed in the reviews

@mbfrahry mbfrahry changed the title Add Datadog plugin for monitor resource New Resource: azurerm_datadog_monitor Jul 28, 2022
@mbfrahry mbfrahry merged commit 9e76a20 into hashicorp:main Jul 28, 2022
mbfrahry added a commit that referenced this pull request Jul 28, 2022
@mbfrahry
Update changelog for #16131
9e87070
@vikotha vikotha mentioned this pull request Aug 1, 2022
Merged
@github-actions
Copy link

github-actions bot commented Aug 1, 2022

This functionality has been released in v3.16.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

github-actions bot commented Sep 1, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ms-henglu ms-henglu ms-henglu requested changes

@tombuildsstuff tombuildsstuff tombuildsstuff left review comments

@mbfrahry mbfrahry mbfrahry approved these changes

@katbyte katbyte Awaiting requested review from katbyte

Assignees

@mbfrahry mbfrahry

Labels
dependencies documentation size/XL
Projects
None yet
Milestone
v3.16.0
Development

Successfully merging this pull request may close these issues.
9 participants
@vikotha @mbfrahry @tombuildsstuff @katbyte @ms-henglu @manicminer @jackofallops @WodansSon @magodo