feat: sync preserve plan order for required list attrs provider-wide - #99
Conversation
📝 WalkthroughWalkthroughThe PR restricts POST retries for transport errors and HTTP 500 responses. Provider resources now preserve planned ordering for returned collections, persist cluster IDs before readiness polling, expose Sequence Diagram(s)sequenceDiagram
participant Terraform
participant dataQualityScheduleDataSource
participant GalaxyClient
participant GalaxyAPI
Terraform->>dataQualityScheduleDataSource: Read configuration
dataQualityScheduleDataSource->>GalaxyClient: Fetch data quality schedule
GalaxyClient->>GalaxyAPI: Send API request
GalaxyAPI-->>GalaxyClient: Return schedule response
GalaxyClient-->>dataQualityScheduleDataSource: Return response
dataQualityScheduleDataSource->>dataQualityScheduleDataSource: Map values and accumulate diagnostics
dataQualityScheduleDataSource-->>Terraform: Set state or return diagnostics
Possibly related PRs
🚥 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 (3)
internal/client/client_test.go (1)
92-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for POST transport-error exclusion.
The transport-error path in
doRequestWithRetry(line 241 ofclient.go) also excludes POST from retries, but only the HTTP 500 case is tested here. A transport-error test would guard that path against regressions, and a positive test confirming non-POST 500 is retried would protect against accidentally over-restricting the condition.♻️ Suggested additional tests
type mockRoundTripper struct { requestCount int statusCode int method string attempts []string + err error } func (m *mockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { m.requestCount++ m.method = req.Method m.attempts = append(m.attempts, req.Method) + if m.err != nil { + return nil, m.err + } return &http.Response{ StatusCode: m.statusCode, Header: http.Header{}, Body: io.NopCloser(strings.NewReader("{}")), Request: req, }, nil } +func TestPostTransportErrorNotRetried(t *testing.T) { + mock := &mockRoundTripper{err: fmt.Errorf("connection reset by peer")} + client := &GalaxyClient{ + BaseURL: "http://localhost", + ClientID: "test", + ClientSecret: "test", + ProviderVersion: "1.0.0", + HTTPClient: &http.Client{Transport: mock}, + accessToken: "test-token", + tokenExpiry: time.Now().Add(1 * time.Hour), + } + + ctx := context.Background() + var result map[string]interface{} + err := client.doRequestWithRetry(ctx, http.MethodPost, "/test", nil, &result, 3) + + if err == nil { + t.Fatalf("expected error for POST transport failure, got nil") + } + if mock.requestCount != 1 { + t.Fatalf("POST transport error should not be retried; expected 1 request, got %d", mock.requestCount) + } +} + +func TestGet500Retried(t *testing.T) { + mock := &mockRoundTripper{statusCode: http.StatusInternalServerError} + client := &GalaxyClient{ + BaseURL: "http://localhost", + ClientID: "test", + ClientSecret: "test", + ProviderVersion: "1.0.0", + HTTPClient: &http.Client{Transport: mock}, + accessToken: "test-token", + tokenExpiry: time.Now().Add(1 * time.Hour), + } + + ctx := context.Background() + var result map[string]interface{} + _ = client.doRequestWithRetry(ctx, http.MethodGet, "/test", nil, &result, 3) + + if mock.requestCount < 2 { + t.Fatalf("GET 500 should be retried; expected at least 2 requests, got %d", mock.requestCount) + } +}internal/provider/data_quality_schedule_data_source.go (1)
155-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd warn logging for unexpected nested entry types, consistent with
groups_data_source.go.
groups_data_source.gonow logstflog.Warnwhen nested array elements (roles, users) aren'tmap[string]interface{}(lines 208-210, 244-246). ThedataQualityChecksloop here silently skips non-map entries at line 158 without logging, creating an observability gap. Since this code is already being modified for diagnostic propagation, adding the same warn logging would be a low-effort consistency improvement.♻️ Suggested refactor for consistency
if checkMap, ok := checkInterface.(map[string]interface{}); ok { attributeTypes := datasource_data_quality_schedule.DataQualityChecksValue{}.AttributeTypes(ctx) attributes := map[string]attr.Value{} if dataQualityCheckId, ok := checkMap["dataQualityCheckId"].(string); ok { attributes["data_quality_check_id"] = types.StringValue(dataQualityCheckId) } else { attributes["data_quality_check_id"] = types.StringNull() } if name, ok := checkMap["name"].(string); ok { attributes["name"] = types.StringValue(name) } else { attributes["name"] = types.StringNull() } checkValue, d := datasource_data_quality_schedule.NewDataQualityChecksValue(attributeTypes, attributes) if d.HasError() { diags.Append(d...) continue } checksList = append(checksList, checkValue) + } else { + tflog.Warn(ctx, fmt.Sprintf("unexpected entry type in dataQualityChecks, skipping: %T", checkInterface)) }internal/provider/evaluation_data_source.go (1)
139-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd warn logging for unexpected nested entry types, consistent with
groups_data_source.go.Same as the
data_quality_schedule_data_source.goobservation: theevaluationsloop at line 142 silently skips non-map entries, whilegroups_data_source.gonow logs warnings for equivalent nested arrays (roles, users). Adding the sametflog.Warnhere would maintain observability consistency across the provider.♻️ Suggested refactor for consistency
if evalMap, ok := evalInterface.(map[string]interface{}); ok { attributeTypes := datasource_evaluation.EvaluationsValue{}.AttributeTypes(ctx) attributes := map[string]attr.Value{} if basedOnStatsAt, ok := evalMap["basedOnStatsAt"].(string); ok { attributes["based_on_stats_at"] = types.StringValue(basedOnStatsAt) } else { attributes["based_on_stats_at"] = types.StringNull() } if evaluatedAt, ok := evalMap["evaluatedAt"].(string); ok { attributes["evaluated_at"] = types.StringValue(evaluatedAt) } else { attributes["evaluated_at"] = types.StringNull() } if predicate, ok := evalMap["predicate"].(string); ok { attributes["predicate"] = types.StringValue(predicate) } else { attributes["predicate"] = types.StringNull() } if status, ok := evalMap["status"].(string); ok { attributes["status"] = types.StringValue(status) } else { attributes["status"] = types.StringNull() } evalValue, d := datasource_evaluation.NewEvaluationsValue(attributeTypes, attributes) if d.HasError() { diags.Append(d...) continue } evalsList = append(evalsList, evalValue) + } else { + tflog.Warn(ctx, fmt.Sprintf("unexpected entry type in evaluations, skipping: %T", evalInterface)) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 00705b75-efd8-40ab-8cff-a7b965dc5877
📒 Files selected for processing (21)
docs/data-sources/schema.mddocs/data-sources/table.mdinternal/client/client.gointernal/client/client_test.gointernal/provider/cluster_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/groups_data_source.gointernal/provider/list_ordering.gointernal/provider/mongodb_catalog_resource.gointernal/provider/policy_resource.gointernal/provider/role_privilege_grant_resource.gointernal/provider/service_account_password_resource.gointernal/provider/service_account_resource.gointernal/provider/usage_example_data_source.go
Automated sync from terraform-provider-galaxy-generation repository.
Source commit: b7f9cfce6f8e871d17ac94093eac9cd2f8a4d6ab
This PR contains the latest generated provider code from the generation repository.