[CC-30766] disallow 0 as a value for disk_iops #254
Conversation
|
This looks big because it includes a vendor change that resulted in a lot of updates. The commit to review is this one: |
| @@ -139,7 +139,7 @@ Optional: | |||
| Optional: | |||
|
|
|||
| - `cidr_range` (String) The IPv4 range in CIDR format that will be used by the cluster. This is supported only on GCP, and must have a subnet mask no larger than /19. Defaults to "172.28.0.0/14". This cannot be changed after cluster creation. | |||
| - `disk_iops` (Number) Number of disk I/O operations per second that are permitted on each node in the cluster. Zero indicates the cloud provider-specific default. | |||
| - `disk_iops` (Number) Number of disk I/O operations per second that are permitted on each node in the cluster. Omitting this attribute will result in the cloud provider-specific default. | |||
There was a problem hiding this comment.
@kathancox here is the doc change I was hoping to get your eyes on.
| // If supplied this value must be non-zero. 0 is a | ||
| // valid api value indicating the default being | ||
| // returned but it causes a provider inconsistency. | ||
| int64validator.AtLeast(1), |
There was a problem hiding this comment.
Hmm this is a breaking change if anyone is actually setting this right? Is it really important to change this? It seems like anyone setting zero today knows what they're getting. I don't feel that strongly about it though so if you want to merge it then go for it.
There was a problem hiding this comment.
As discussed offline, I don't think this is a breaking change because using 0 as was previously mentioned in the doc causes an error. When 0 is written to the tf configuration, it is then passed via the api. The api returns the actual (non-zero) default depending on the disk size. The 0 in the configuration causes a state inconsistency error with the returned value. I just noticed the wrong commit message was added to this PR since I broke out some vendor additions to a separate commit. I'll fix the PR message. Hopefully that helps clear this point up.
This caused an update to the validation framework again. It has been pulled in a separate commit for clarity.
0 is a valid value to pass into the api for disk_iops. That, along with omitting the value in the api call results in the default iops value. However, the api returns the actual value for the iops that was chosen. This results in a configuration conflict with the set value of 0 and the actual value that was returned from the server. For this reason we now disallow 0 from being set and we clarify in the docs that omitting the value is the correct way to ask for the default.
fantapop
force-pushed
the
CC-30766-disk-iops-set-to-0-results-in-state-inconsistency
branch
from
b38c4ed to
7501b35
Compare
fantapop
deleted the
CC-30766-disk-iops-set-to-0-results-in-state-inconsistency
branch
0 is a valid value to pass into the api for disk_iops. That, along with omitting the value in the api call results in the default iops value. However, the api returns the actual value for the iops that was chosen. This results in a configuration conflict with the set value of 0 and the actual value that was returned from the server. For this reason we now disallow 0 from being set and we clarify in the docs that omitting the value is the correct way to ask for the default.
Commit checklist
make generate)