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

Add support_physical_cluster_replication field to cluster creation #261

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davidwding
Copy link
Contributor

Commit checklist

  • Changelog
  • Doc gen (make generate)
  • Integration test(s)
  • Acceptance test(s)
  • Example(s)

@davidwding davidwding requested a review from fantapop January 28, 2025 20:46
"support_physical_cluster_replication": schema.BoolAttribute{
Optional: true,
Computed: true,
Description: "Specifies whether a cluster should be started using an architecture that supports physical cluster replication.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have any documentation to suggest why a customer wouldn't want this set to true? Otherwise why wouldn't everyone set it to true? @kathancox this is probably more of a docs concern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we should also say it's defaulting to false.

Comment on lines +567 to +569
if cfg.SupportPhysicalClusterReplication.ValueBool() {
dedicated.SupportPhysicalClusterReplication = ptr(cfg.SupportPhysicalClusterReplication.ValueBool())
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like if someone were to actually set the value to false, SupportPhysicalClusterReplication would be omitted from the request. I think we want it to to pass an explicit false in that case. Otherwise, once we start defaulting to true on the serverside the false pass via terraform will be ignored.

Comment on lines +261 to +262
Optional: true,
Computed: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Normally when we have optional values, we'll let the server choose the default. computed: true, would normally allow the value in the state to be set by what the server chose. That way from provider version to version we could change the default on the server and it would remain working with our serverside default. However, I suggested we not add the field to the cluster response in that recent PR. I think that is a bit problematic with the current implementation here. I see 2 options:

  1. we have a hard default of false that's built into the provider for this version. That won't feel very ideal when we change the serverside default to true because people will probably forget to pass this optional parameter and end up with false. We could change the default in a later terraform version but it would be a breaking change to the provider.
  2. we follow through with adding a value to the cluster response that indicates which value was chosen.

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