Skip to content

Add data_region to the recognized fields#28

Merged
PetrHeinz merged 3 commits intomainfrom
th/add-data-region
Jan 7, 2025
Merged

Add data_region to the recognized fields#28
PetrHeinz merged 3 commits intomainfrom
th/add-data-region

Conversation

@gyfis
Copy link
Contributor

@gyfis gyfis commented Jan 7, 2025

No description provided.

@gyfis gyfis requested a review from PetrHeinz January 7, 2025 08:54
Copy link
Member

@PetrHeinz PetrHeinz left a comment

Choose a reason for hiding this comment

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

Thanks for adding the new parameter!

Comment on lines +192 to +199
"data_region": {
Description: "Region where we store your data.",
Type: schema.TypeString,
Optional: true,
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
return d.Id() != ""
},
},
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure whether the DiffSuppressFunc is the way to go here 🤔

If I understand it correctly, we want the data_region to be readable from the API response if not provided (this should be optional + computed), and it cannot be changed during the resource existence - this can be done via force new, would recreate the source on region change.

Suggested change
"data_region": {
Description: "Region where we store your data.",
Type: schema.TypeString,
Optional: true,
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
return d.Id() != ""
},
},
"data_region": {
Description: "Region where we store your data.",
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
},

With this config it should be possible to add data_region to sourceRef without any special handling. WDYT?


In the current implementation I would worry about people changing the data_region and nothing happening. Not sure how exactly would it behave if no region was provided - would it be null in the terraform state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Petr! I didn't want ForceNew since I didn't want accidental new sources created just by changing this one field

Copy link
Member

@PetrHeinz PetrHeinz Jan 7, 2025

Choose a reason for hiding this comment

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

But then you could intentionally rewrite the field, think you have your source in a different region, but nothing would happen. If we don't want to support force new, we should throw an error in that case, ideally in the plan phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error when you try to change the data_region sounds good to me, can you take over please @PetrHeinz? 🙏

Copy link
Member

@PetrHeinz PetrHeinz Jan 7, 2025

Choose a reason for hiding this comment

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

@gyfis Just added ea4be5e that should work just nicely, could you take a look and thumb? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amazing, thank you Petr!

@PetrHeinz PetrHeinz merged commit f7ea69d into main Jan 7, 2025
17 checks passed
@PetrHeinz PetrHeinz deleted the th/add-data-region branch January 7, 2025 15:05
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.

2 participants