fix(clickpipes): change excluded columns type from list to set to avoid ordering issue#560
Open
tpanetti wants to merge 4 commits into
Open
fix(clickpipes): change excluded columns type from list to set to avoid ordering issue#560tpanetti wants to merge 4 commits into
tpanetti wants to merge 4 commits into
Conversation
…id ordering issue
There was a problem hiding this comment.
Pull request overview
This PR addresses ClickPipe plan/refresh instability caused by backend-returned excluded_columns ordering by modeling excluded_columns as an unordered Set, aligning provider semantics with the API behavior (Issue #558).
Changes:
- Convert
excluded_columnsin ClickPipe table-mapping models and schema fromListtoSetto make comparisons order-insensitive. - Update state sync logic to write
excluded_columnsas atypes.Set(instead oftypes.List) when reading from the API. - Add a regression test ensuring
ModifyPlandoes not reject reorderedexcluded_columns.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/resource/models/clickpipe_resource.go | Switches table-mapping ExcludedColumns model fields and object types from List to Set. |
| pkg/resource/clickpipe.go | Updates resource schema and API↔state sync to use Set for excluded_columns. |
| pkg/resource/clickpipe_test.go | Updates existing test fixture state to represent excluded_columns as SetNull. |
| pkg/resource/clickpipe_bug_fixes_test.go | Adds/updates regression coverage for Issue #558 and updates fixtures for Set-typed excluded_columns. |
| docs/resources/clickpipe.md | Updates documentation to reflect excluded_columns as a Set of String. |
Comments suppressed due to low confidence (1)
pkg/resource/clickpipe.go:1049
excluded_columnschanged from List to Set in the schema, but this PR description mentions a state migration path andClickPipeResourcedoes not implementUpgradeState(...)(no state upgrader is present in this file). If Terraform/plugin-framework cannot automatically coerce the prior List state to the new Set type, existing states may fail to decode after upgrade. Either add an explicit state upgrader (List->Set) forexcluded_columnsin the ClickPipe resource or update the PR description to reflect the actual migration behavior.
"excluded_columns": schema.SetAttribute{
Description: "Columns to exclude from replication.",
Optional: true,
ElementType: types.StringType,
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Why
excluded_columnswas modeled as aListrather than aSet. The backend returns excluded columns in an unguaranteed order, so a refresh could rewrite state in a different order than the config. TheModifyPlan"table mappings are immutable" guard compared them with order-sensitiveList.Equal, so every subsequent plan then hard-failed withCannot modify excluded_columns ...— even though the set of excluded columns was unchanged.What
excluded_columnsfromListtoSet(Postgres, MySQL, BigQuery) so comparisons are order-insensitive.sorting_keysis intentionally left aList(ClickHouse sort-key order is semantically meaningful).excluded_columnsas atypes.Set, and brings the BigQuery branch to parity with Postgres/MySQL by preserving an empty set from prior state (avoids null↔empty churn on refresh).ModifyPlanregression test: a reorderedexcluded_columnsmust not be flagged as a modification, while a realtarget_tablechange still is.State migration
No
StateUpgraderis needed (the earlier description's "state migration path" wording was inaccurate). AList→Setchange is wire-compatible in terraform-plugin-framework: both serialize as JSON arrays and are disambiguated only by the schema type. When the stored state's schema version matches the current one, the framework'sUpgradeResourceStatepassthrough re-decodes the existing array against the newSettype, so existing resources migrate automatically with a clean, no-op plan. No schemaVersionbump or upgrader is required.Implements #558