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

Regions to be an unordered TypeSet #315

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

JanBerktold
Copy link
Contributor

Closes #314

@fformica fformica marked this pull request as ready for review June 14, 2024 11:13
Copy link
Contributor

@fformica fformica left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the contribution!

@fformica fformica merged commit 74bfe46 into ns1-terraform:master Jun 14, 2024
2 checks passed
@JanBerktold
Copy link
Contributor Author

My apologies @fformica - I thought this was a draft and wasn't done testing it.

Thanks for catching the array/Set mishap. My remaining concern is that this code path sets a []map[string]interface{} instead of a Set for `regions. I am not familiar enough with TF to know whether it magically translates those under the hood.

		regions := make([]map[string]interface{}, 0, len(r.Regions))
		for _, k := range keys {
		for name, region := range r.Regions {
			newRegion := make(map[string]interface{})
			region := r.Regions[k]
			newRegion["name"] = k
			newRegion["name"] = name
			newRegion["meta"] = region.Meta.StringMap()
			regions = append(regions, newRegion)
		}
		log.Printf("Setting regions %+v", regions)
		err := d.Set("regions", regions)
		if err != nil {
			return fmt.Errorf("[DEBUG] Error setting regions for: %s, error: %#v", r.Domain, err)
		}

@fformica
Copy link
Contributor

Ah, sorry, we should have a new release soon so I hurried to get this in.
Anyway, it's ok, the slice is converted to a Set internally: https://github.com/hashicorp/terraform-plugin-sdk/blob/v2.24.1/helper/schema/field_writer_map.go#L297

@JanBerktold
Copy link
Contributor Author

Awesome, ty! Thanks for getting this out quickly.

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.

Regions in record resource are modified on every plan/apply
2 participants