feat(env_aws): support clickhouse clusters and keepers - #283
Draft
ianaya89 wants to merge 8 commits into
Draft
Conversation
ianaya89
marked this pull request as draft
August 25, 2026 12:31
The input types were already generated; only the read fragments were missing the fields, so reads never returned them.
Clusters and Keepers are part of the environment spec in the public API, with no standalone mutation, so they are exposed as `clickhouse_clusters` and `clickhouse_keepers` attributes on the environment resource. Unlike the rest of the spec, which is sent wholesale under UpdateStrategy REPLACE, these entries are patched by name and removal is explicit: Update diffs plan against state to fill the API's *ToDelete lists, at both the top level and inside each cluster. The shared mapping lives in env/common and works on plain spec structs; each env only lowers its own gqlgenc fragment types into them, since those are generated per env. Refresh semantics worth noting: - Passwords are never returned by the API, so `password_value` is kept from the configuration and never refreshed. `password_type` follows it, because the API can report PLAIN_TEXT, which is not a valid input. - Empty lists and empty strings coming back from the API map to null, so an omitted attribute does not drift to `[]` or `""`. - A null keeper in a read means detached, not absent: the configured name is preserved since the API ignores it while disabled. Immutability is checked in ModifyPlan by pairing entries by name rather than with the ImmutableString modifier, which compares by index and would report a false positive whenever an entry is removed from the middle of the list.
Adds a full example and a section covering the behaviors that are not obvious from the schema: removal deletes, the node group reservations a cluster and a Keeper need, which attributes are immutable, and that passwords are never returned. `make docs` now passes --provider-name explicitly, since tfplugindocs otherwise derives it from the directory name and fails in a git worktree.
Five defects found reviewing the initial implementation: - Entries added to an existing environment go through the update API, whose input carries no mode, zones or storage class, so they would silently come up with the defaults and fail the post-apply consistency check. Setting any of the three on a newly added cluster, Keeper or volume is now rejected at plan time. An explicit STANDARD mode is still allowed, since that is what would be created anyway. - Zones were stored in the order the API returned them, which diffs forever against an immutable list attribute. They now follow the configured order, the same way env and node group zones already do. - Unknown values were read as empty strings, so two entries whose names are only known at apply time collided as duplicates, an unknown mode failed the SWARM check, and an unknown zone element shortened the list and read as a removed zone. Each rule now defers instead. - The name pattern made its trailing group optional, so a one-character name passed a validator documented as 2-15. - Documentation claimed renaming an entry was rejected at plan time. It never was, and it cannot be: a rename is a delete and a create. The plan now warns about every entry that is about to be deleted, and the docs say so. Also cross-validates instance types against node group reservations, which is knowable from the configuration and otherwise only surfaces as a failed environment reconcile.
…butes Two defects that between them broke the documented example. ValidateConfig reads the raw configuration, before schema defaults are applied, so an omitted `keeper.enabled` arrives null rather than as its Default(true). ValueBool() reports false for a null Bool, so every cluster that left the attribute out was rejected as a STANDARD cluster running without a Keeper. The pointer accessors only map null to nil: on an unknown value they hand back a pointer to the zero value. Every Optional+Computed attribute without a schema default is unknown in the plan, so create sent `mode: ""` — not a valid enum literal — along with an empty storage class and zeroed iops, throughput, profile and quota. They are now omitted, and `mode` carries a STANDARD default like the other Optional+Computed enums in the provider. Both paths were invisible to the tests, which only ever built models out of known values and never exercised an omitted attribute.
ianaya89
force-pushed
the
feat/clickhouse-clusters-aws
branch
from
August 25, 2026 14:25
e420241 to
c1f4b35
Compare
Closes the rest of the review findings. A refresh used to copy every cluster and Keeper the API returned into state, including ones created outside Terraform. The configuration declares none, so the next plan proposed removing them and the update deleted them along with their data. State now only tracks what prior state already managed, the way custom domains do; the data source keeps reporting everything. Reserved platform users are dropped for the same reason: the configuration can never declare them, so keeping them made the applied state differ from the plan on an attribute that is not Computed. The scoped read only rejected wholly-unknown list elements, so an unknown nested `disk` or `users` still reached reflection into a Go struct and failed as a provider error. It now walks nested objects, lists, sets and maps. When values are that unsettled the plan-time checks cannot run at all, which used to happen silently even though the deletion warning is the only notice before data goes away; the plan now says so. Also: - `keeper.name` is optional, so a SWARM cluster running without a Keeper no longer has to invent a name the API documents as ignored. It is still required, and still checked against `clickhouse_keepers`, whenever the Keeper is enabled. - `password_value_from_secret` requires `password_type`, which the API needs whenever a password value is set in either form. The example set neither. - Comments in the new files are back to one line, per the project convention.
ValidateClickHousePlan walked only the planned `additional_disks`, so a volume present in state and absent from the plan was never looked at. It still landed in `AdditionalDisksToDelete`, so dropping or renaming one destroyed the volume and its data with no plan-time diagnostic at all — clusters and Keepers already warned, volumes did not. The three `name` attributes described themselves as "Immutable", which reads as a promise that the plan rejects a change. It does not: the entry is deleted and a new, empty one is created. They now say so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Exposes the ClickHouse clusters and Keepers that the public API now carries in the env spec. AWS only; the other five envs have the same fields and are still unwired.
clickhouse_clusters/clickhouse_keepersonaltinitycloud_env_aws— they live in the env spec, there is no standalone mutationUpdatediffs plan vs state to fill the API's*ToDeletelists. Renaming an entry deletes it and creates a new one, so the plan warns before every deletionpassword_valueis sensitive and never refreshed — the API never returns passwordsThe e2e config exercises the new fields but has not been run against the dev control plane.