feat: sync Address review findings across recent auto-sync PRs - #97
feat: sync Address review findings across recent auto-sync PRs #97starburstdata-automation wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe changes prevent POST requests from being retried, add coverage for HTTP 500 responses, and stabilize Terraform resource state with plan modifiers. Cluster state persistence and URI mapping were adjusted. Data sources now validate provider configuration, report malformed results, and propagate conversion diagnostics. Role privilege import scope validation is entity-specific, and owner descriptions now refer to schemas and tables accurately. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/provider/mongodb_catalog_resource.go (1)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent no-op risk if
catalog_idis renamed/retyped by codegen; consider a shared helper.The type assertion guards correctly today, but if a future schema regeneration renames
catalog_idor changes its attribute kind, this block becomes a silent no-op — quietly reintroducing the exact "known after apply" destroy/recreate bug this change is meant to fix, with no compiler or runtime signal. This same guarded-assignment pattern is duplicated verbatim across every catalog/resource file in this PR (mysql, postgresql, s3, sqlserver, service_account, service_account_password, etc.).Consider extracting a small helper, e.g.:
func applyUseStateForUnknown(attrs map[string]schema.Attribute, name string) { attr, ok := attrs[name].(schema.StringAttribute) if !ok { panic(fmt.Sprintf("expected %q to be a StringAttribute for UseStateForUnknown", name)) // or: log via tflog and skip, depending on desired failure mode } attr.PlanModifiers = append(attr.PlanModifiers, stringplanmodifier.UseStateForUnknown()) attrs[name] = attr }This both removes the duplication and turns future schema drift into a fast, loud failure instead of a silent regression.
Also applies to: 79-89
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: fbbe97f1-3c9f-47c3-b2f7-40db07246ee2
📒 Files selected for processing (34)
docs/data-sources/schema.mddocs/data-sources/table.mdinternal/client/client.gointernal/client/client_test.gointernal/provider/bigquery_catalog_resource.gointernal/provider/cassandra_catalog_resource.gointernal/provider/cluster_resource.gointernal/provider/column_mask_resource.gointernal/provider/data_product_resource.gointernal/provider/data_quality_check_data_source.gointernal/provider/data_quality_check_resource.gointernal/provider/data_quality_checks_data_source.gointernal/provider/data_quality_schedule_data_source.gointernal/provider/datasource_schema/schema_data_source_gen.gointernal/provider/datasource_table/table_data_source_gen.gointernal/provider/evaluation_data_source.gointernal/provider/gcs_catalog_resource.gointernal/provider/groups_data_source.gointernal/provider/mongodb_catalog_resource.gointernal/provider/mysql_catalog_resource.gointernal/provider/opensearch_catalog_resource.gointernal/provider/policy_resource.gointernal/provider/postgresql_catalog_resource.gointernal/provider/redshift_catalog_resource.gointernal/provider/role_privilege_grant_resource.gointernal/provider/role_resource.gointernal/provider/row_filter_resource.gointernal/provider/s3_catalog_resource.gointernal/provider/service_account_password_resource.gointernal/provider/service_account_resource.gointernal/provider/snowflake_catalog_resource.gointernal/provider/sqlserver_catalog_resource.gointernal/provider/tag_resource.gointernal/provider/usage_example_data_source.go
| attr.PlanModifiers = []planmodifier.String{ | ||
| stringplanmodifier.UseStateForUnknown(), | ||
| } | ||
| s.Attributes["data_product_id"] = attr | ||
| } | ||
|
|
||
| // created_on is assigned at creation and never changes. Without UseStateForUnknown, any update | ||
| // to the data product causes Terraform to mark created_on as "known after apply". | ||
| if attr, ok := s.Attributes["created_on"].(schema.StringAttribute); ok { | ||
| attr.PlanModifiers = []planmodifier.String{ | ||
| stringplanmodifier.UseStateForUnknown(), | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Add the missing planmodifier import.
planmodifier.String at Lines 63 and 72 is undefined, so CI cannot compile this package. Import github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier.
🧰 Tools
🪛 GitHub Actions: CI / 0_ci.txt
[error] 63-63: Go build failed: undefined: planmodifier (internal/provider/data_product_resource.go:63:26).
🪛 GitHub Actions: CI / ci
[error] 63-63: Go build failed: internal/provider/data_product_resource.go:63:26: undefined: planmodifier
🪛 GitHub Check: ci
[failure] 72-72:
undefined: planmodifier
[failure] 63-63:
undefined: planmodifier
Source: Linters/SAST tools
|
closing, build fix addressed in #98 |
Automated sync from terraform-provider-galaxy-generation repository.
Source commit: b8ab9f7badff321c109369a71a981375663175f7
This PR contains the latest generated provider code from the generation repository.