CC-30971: update state with allowlist api responses #258
Conversation
| Description: "Name of this allowlist entry. If not set explicitly, this value does not sync with the server.", | ||
| Description: "Name of this allowlist entry. If left unset, it will inherit a server side default.", |
There was a problem hiding this comment.
@kathancox just a little bit of docs change here if you could take a peak! ty
| return fmt.Sprintf(allowlistIDFmt, clusterID, cidrIP, cidrMask) | ||
| } | ||
|
|
||
| func loadAllowlistEntryToTerraformState(clusterID string, entry *client.AllowlistEntry, state *AllowlistEntry) { |
There was a problem hiding this comment.
Curious why this isn't more like a "transformToState" function with a signature like
(string, *client.AllowlistEntry) -> (*AllowlistEntry)
?
I might be missing it but do we get something from passing in the state param instead of making this a pure function?
There was a problem hiding this comment.
Ah because other uses besides create already come with a state I guess. In those cases though it seems like we throw away the old state anyway, so we could always do something like
*state = *transformToState(clusterID, entry)
Maybe this is just following a pattern we use all over even if in a vacuum there may be a better solution. That seems fine to me.
There was a problem hiding this comment.
I've generally just been following the pattern that was existing but I think you're probably right about why this pattern started. I think it would be cleaner to do as you suggest. It would also be nice to separate each resource into it's own package or create an interface which allows us to stop fully qualifying all the function names. i.e loadAllowlistEntryToTerraformState -> r.transformToState
docs/resources/allow_list.md
Outdated
| @@ -36,7 +36,7 @@ resource "cockroach_allow_list" "vpn" { | |||
|
|
|||
| ### Optional | |||
|
|
|||
| - `name` (String) Name of this allowlist entry. If not set explicitly, this value does not sync with the server. | |||
| - `name` (String) Name of this allowlist entry. If left unset, it will inherit a server side default. | |||
There was a problem hiding this comment.
| - `name` (String) Name of this allowlist entry. If left unset, it will inherit a server side default. | |
| - `name` (String) Name of this allowlist entry. If left unset, it will inherit a server-side default. |
fantapop
force-pushed
the
CC-30971-unnamed-allowlist-causes-terraform-state-inconsistency-error
branch
2 times, most recently
from
fe70889 to
084151e
Compare
Previously the object the api returned for created and updated allowlist entries was ignored when updating the terraform state. This meant that defaults set on the server side and returned were not correctly being set and could cause state inconsistencies in case they were different. Specifically there was a case where not setting an initial value for the create would return an empty string. The empty string caused a state consistency error with the unset value. To fix this we now set the state based on the response. As part of this the code was reworked to be more similar to how other resources are structured. Tests were added to show that creating an allowlist without a name now succeeds. Also, as part of this change, instances of allowList were renamed to allowlist for consistency.
The image in use for our ci actions, ubuntu-latest is now defaulting to ubuntu-24.04. In the previous version, terraform was installed by default but has now been removed so we install it manually for each job that relies on it. actions/runner-images#10636 We choose not to pin the version of terraform that is installed so we are always testing against the the latest CLI version. If there is a discrepancy between versions that only affects tests, developers can update to the latest version and ensure the tests work for that.
fantapop
force-pushed
the
CC-30971-unnamed-allowlist-causes-terraform-state-inconsistency-error
branch
from
084151e to
81f2de2
Compare
fantapop
deleted the
CC-30971-unnamed-allowlist-causes-terraform-state-inconsistency-error
branch
Previously the object the api returned for created and updated allowlist entries was ignored when updating the terraform state. This meant that defaults set on the server side and returned were not correctly being set and could cause state inconsistencies in case they were different. Specifically there was a case where not setting an initial value for the create would return an empty string. The empty string caused a state consistency error with the unset value.
To fix this we now set the state based on the response. Also, part of this the code was reworked to be more similar to how other resources are structured.
Tests were added to show that creating an allowlist without a name now succeeds.
Also, as part of this change, instances of allowList were renamed to allowlist for consistency.
Commit checklist
make generate)