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

resource/cloudflare_list: Do not reapply changes if only list order changed #2063

Merged
merged 4 commits into from Nov 29, 2022

Conversation

cjolowicz
Copy link
Contributor

Fixes #1827

  • resource/cloudflare_list: add test that dynamic lists apply cleanly

Add a failing test with the example from #1827.

The test framework checks that a subsequent terraform plan does not produce a
diff. This check fails since the list is reordered on each terraform apply.

  • resource/cloudflare_list: use schema.TypeSet instead of schema.TypeList

Lists are unordered so using schema.TypeList leads to spurious updates.
Use schema.TypeSet instead to avoid these.

Refactor buildListItemsCreateRequest to traverse the Go data structure instead
of using resource.GetOk. We can't easily use "item.%d.value" with TypeSet
because its items are indexed by hash instead of position.

  • add changelog

Add a failing test with the example from cloudflare#1827.

The test framework checks that a subsequent terraform plan does not produce a
diff. This check fails since the list is reordered on each terraform apply.
Lists are unordered so using schema.TypeList leads to spurious updates.
Use schema.TypeSet instead to avoid these.

Refactor `buildListItemsCreateRequest` to traverse the Go data structure instead
of using `resource.GetOk`. We can't easily use "item.%d.value" with TypeSet
because its items are indexed by hash instead of position.
@github-actions
Copy link
Contributor

github-actions bot commented Nov 29, 2022

changelog detected ✅

@jacobbednarz
Copy link
Member

thanks for this one @cjolowicz. i went ahead and simplified the test case and removed the reliance on dynamic. we avoid using dynamic and HCL expressions in tests as it creates another potential point for bugs to creep in. here, we can achieve the same reproduction case without it.

acceptance tests all look good

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareList_" -count 1 -parallel 1 -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareList_Exists
--- PASS: TestAccCloudflareList_Exists (11.03s)
=== RUN   TestAccCloudflareList_UpdateDescription
--- PASS: TestAccCloudflareList_UpdateDescription (20.13s)
=== RUN   TestAccCloudflareList_Update
--- PASS: TestAccCloudflareList_Update (53.51s)
=== RUN   TestAccCloudflareList_UpdateIgnoreIPOrdering
--- PASS: TestAccCloudflareList_UpdateIgnoreIPOrdering (18.60s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/internal/provider	103.710s

@jacobbednarz jacobbednarz merged commit dfdff4b into cloudflare:master Nov 29, 2022
@github-actions github-actions bot added this to the v3.29.0 milestone Nov 29, 2022
github-actions bot pushed a commit that referenced this pull request Nov 29, 2022
@github-actions
Copy link
Contributor

This functionality has been released in v3.29.0 of the Terraform Cloudflare 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!

@cjolowicz cjolowicz deleted the claudio/FW-5366 branch November 30, 2022 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloudflare_list always seen as a change when using dynamic lists
2 participants