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.

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

[synthetics_private_location] Migrate synthetics resource to framework #2881

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

AntoineDona
Copy link
Contributor

@AntoineDona AntoineDona commented Mar 3, 2025

This PR migrates the synthetics_private_location resource to the new terraform framework.
It also fixes a bug where metadata was not removable.

@AntoineDona AntoineDona requested review from a team as code owners March 3, 2025 18:46
@AntoineDona AntoineDona changed the title Migrate synthetics_private_location to framework [synthetics_private_location] Migrate synthetics_private_location to framework Mar 3, 2025
@AntoineDona AntoineDona changed the title [synthetics_private_location] Migrate synthetics_private_location to framework [synthetics_private_location] Migrate synthetics resource to framework Mar 3, 2025
@AntoineDona AntoineDona force-pushed the antoine.donascimento/SYNTH-18766/migrate-PL-to-framework branch from b69defb to d542d75 Compare March 3, 2025 18:48
rtrieu
rtrieu previously approved these changes Mar 3, 2025
@AntoineDona AntoineDona marked this pull request as draft March 4, 2025 10:58
@AntoineDona AntoineDona marked this pull request as ready for review March 5, 2025 15:39
@AntoineDona AntoineDona requested a review from rtrieu March 5, 2025 15:39
rtrieu
rtrieu previously approved these changes Mar 5, 2025
transfer_encoding: []
trailer: {}
host: api.datadoghq.com
remote_addr: ""
request_uri: ""
body: |
{"description":"a private location","name":"tf-TestAccDatadogSyntheticsPrivateLocation_Basic-local-1714765065","tags":["foo:bar","baz"]}
{"description":"a private location","metadata":{},"name":"tf-TestAccDatadogSyntheticsPrivateLocation_Basic-local-1741182374","tags":["foo:bar","baz"]}
Copy link

Choose a reason for hiding this comment

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

Since there's a bug fix here, I suggest you either:

  • Split the PR so the bug fix is done in isolation (better)
  • Mark this PR as a bugfix (slightly worse because it makes it harder to read what the bug fix exactly is)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I see! As I don't want to fix it in the older sdkv2 resource and again in the new one, I will just undo the fix in this one, and create a fix pr afterwards!

Copy link

Choose a reason for hiding this comment

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

Thanks, this will help maintaining a clean commit history and changelog.

teodor2312
teodor2312 previously approved these changes Mar 7, 2025
Copy link
Contributor

@teodor2312 teodor2312 left a comment

Choose a reason for hiding this comment

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

I still don't fully understand the new syntax, but Antoine sat with me to explain things so the changes make sense 👍


if _, httpresp, err := apiInstances.GetSyntheticsApiV1().GetPrivateLocation(auth, r.Primary.ID); err != nil {
func SyntheticsPrivateLocationDestroyerHelper(auth context.Context, s *terraform.State, apiInstances *utils.ApiInstances) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: added this retry feature, as every migrated resources (see example) seem to be using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants