[INF-5286] Write reconciliation functions that pave over inconsistencies in API responses#233
Open
martin-ably wants to merge 1 commit intomainfrom
Open
[INF-5286] Write reconciliation functions that pave over inconsistencies in API responses#233martin-ably wants to merge 1 commit intomainfrom
martin-ably wants to merge 1 commit intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (28)
WalkthroughAdds reconciliation helpers to merge Terraform plan/state with API responses, unit tests for those helpers, shared minimal test HCL helpers, many minimal acceptance tests for Ably resources, and refactors resource CRUD mappings to use the reconciler when building Terraform state. Changes
Sequence Diagram(s)sequenceDiagram
participant Terraform as Terraform (plan/state)
participant Provider as Provider (reconciler)
participant API as Ably API
participant State as Terraform State
Terraform->>Provider: Create/Read/Update request with plan/state
Provider->>API: Call Ably API (Create/Read/Update)
API-->>Provider: API response (resource fields)
Provider->>Provider: reconciler compares plan/state vs API (reconcileString/Bool/Int64)
Provider-->>State: Set reconciled Terraform types and diagnostics
State-->>Terraform: Apply/Plan result (includes computed values)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
martin-ably
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/provider/resource_ably_rule_lambda_test.go (1)
109-113: Prefer neutral credential placeholders over AWS-looking key patterns.Even in tests, key-shaped literals can trip secret scanners and create avoidable compliance noise. Use clearly non-secret test values instead.
Proposed adjustment
authentication = { mode = "credentials" - access_key_id = "AKIAIOSFODNN7EXAMPLE" - secret_access_key = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" + access_key_id = "test-access-key" + secret_access_key = "test-secret-key" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/resource_ably_rule_lambda_test.go` around lines 109 - 113, The test's authentication block uses AWS-shaped literal values for access_key_id and secret_access_key which can trigger secret scanners; update the authentication = { ... } block (specifically the access_key_id and secret_access_key fields) to use clearly non-secret, neutral placeholders (e.g., "TEST_ACCESS_KEY_ID", "TEST_SECRET_KEY") or similar descriptive test constants so the values no longer match real AWS key patterns.internal/provider/reconcile_test.go (1)
55-68: Add a regression assertion that error text does not include raw field values.Current negative tests only check
err != nil; adding a redaction assertion will prevent future sensitive-data leakage regressions.✅ Example assertion addition
func TestReconcileString_InputNull_OutputNonEmpty_NotComputed(t *testing.T) { t.Parallel() - _, err := reconcileString("my_field", types.StringNull(), types.StringValue("surprise"), false) + _, err := reconcileString("my_field", types.StringNull(), types.StringValue("surprise"), false) if err == nil { t.Fatal("expected error for unexpected API value on non-computed field") } + if got := err.Error(); got == "" || strings.Contains(got, "surprise") { + t.Fatalf("error should be redacted, got %q", got) + } }// add import import "strings"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/reconcile_test.go` around lines 55 - 68, Update the two tests (TestReconcileString_InputNull_OutputNonEmpty_NotComputed and TestReconcileString_InputEmpty_OutputNonEmpty_NotComputed) to assert the returned error text does not contain the raw API input value: import "strings", capture the input value used for the test (e.g., "surprise" for the first test, and the empty string for the second) and after verifying err != nil add a check like if inputValue != "" { if strings.Contains(err.Error(), inputValue) { t.Fatalf("error must not contain raw input value") } } so the tests fail if the error leaks sensitive/raw field data; reference reconcileString and the two test functions when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/provider/reconcile.go`:
- Around line 35-38: The reconcile error currently includes the raw API-returned
string via output.ValueString(), which may leak sensitive data; in the function
that returns types.StringNull() and the fmt.Errorf (the reconcile error path in
internal/provider/reconcile.go), replace the direct output.ValueString()
inclusion with a redacted placeholder (e.g. "<redacted>" or omit the value) so
the error message preserves context ("reconcile %q: API returned value but field
was not set...") without printing the actual API value; update the fmt.Errorf
call in that block accordingly.
---
Nitpick comments:
In `@internal/provider/reconcile_test.go`:
- Around line 55-68: Update the two tests
(TestReconcileString_InputNull_OutputNonEmpty_NotComputed and
TestReconcileString_InputEmpty_OutputNonEmpty_NotComputed) to assert the
returned error text does not contain the raw API input value: import "strings",
capture the input value used for the test (e.g., "surprise" for the first test,
and the empty string for the second) and after verifying err != nil add a check
like if inputValue != "" { if strings.Contains(err.Error(), inputValue) {
t.Fatalf("error must not contain raw input value") } } so the tests fail if the
error leaks sensitive/raw field data; reference reconcileString and the two test
functions when making the change.
In `@internal/provider/resource_ably_rule_lambda_test.go`:
- Around line 109-113: The test's authentication block uses AWS-shaped literal
values for access_key_id and secret_access_key which can trigger secret
scanners; update the authentication = { ... } block (specifically the
access_key_id and secret_access_key fields) to use clearly non-secret, neutral
placeholders (e.g., "TEST_ACCESS_KEY_ID", "TEST_SECRET_KEY") or similar
descriptive test constants so the values no longer match real AWS key patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7d7a4a9-6bbb-48f1-94d6-c303e52b73a2
📒 Files selected for processing (22)
internal/provider/provider_test.gointernal/provider/reconcile.gointernal/provider/reconcile_test.gointernal/provider/resource_ably_app_test.gointernal/provider/resource_ably_ingress_rule_mongo_test.gointernal/provider/resource_ably_ingress_rule_postgres_outbox_test.gointernal/provider/resource_ably_key_test.gointernal/provider/resource_ably_namespace_test.gointernal/provider/resource_ably_queue_test.gointernal/provider/resource_ably_rule_amqp_external_test.gointernal/provider/resource_ably_rule_amqp_test.gointernal/provider/resource_ably_rule_azure_function_test.gointernal/provider/resource_ably_rule_http_cloudflare_worker_test.gointernal/provider/resource_ably_rule_http_google_cloud_function_test.gointernal/provider/resource_ably_rule_http_test.gointernal/provider/resource_ably_rule_ifttt_test.gointernal/provider/resource_ably_rule_kafka_test.gointernal/provider/resource_ably_rule_kinesis_test.gointernal/provider/resource_ably_rule_lambda_test.gointernal/provider/resource_ably_rule_pulsar_test.gointernal/provider/resource_ably_rule_sqs_test.gointernal/provider/resource_ably_rule_zapier_test.go
Comment on lines
+35
to
+38
| return types.StringNull(), fmt.Errorf( | ||
| "reconcile %q: API returned %q but field was not set in config and is not computed", | ||
| field, output.ValueString(), | ||
| ) |
There was a problem hiding this comment.
Avoid leaking API-returned string values in reconcile errors.
Including raw output.ValueString() can expose sensitive values in logs/diagnostics when optional-only fields unexpectedly come back from API.
🔒 Proposed fix (redact value in error)
- return types.StringNull(), fmt.Errorf(
- "reconcile %q: API returned %q but field was not set in config and is not computed",
- field, output.ValueString(),
- )
+ return types.StringNull(), fmt.Errorf(
+ "reconcile %q: API returned a value but field was not set in config and is not computed",
+ field,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return types.StringNull(), fmt.Errorf( | |
| "reconcile %q: API returned %q but field was not set in config and is not computed", | |
| field, output.ValueString(), | |
| ) | |
| return types.StringNull(), fmt.Errorf( | |
| "reconcile %q: API returned a value but field was not set in config and is not computed", | |
| field, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/provider/reconcile.go` around lines 35 - 38, The reconcile error
currently includes the raw API-returned string via output.ValueString(), which
may leak sensitive data; in the function that returns types.StringNull() and the
fmt.Errorf (the reconcile error path in internal/provider/reconcile.go), replace
the direct output.ValueString() inclusion with a redacted placeholder (e.g.
"<redacted>" or omit the value) so the error message preserves context
("reconcile %q: API returned value but field was not set...") without printing
the actual API value; update the fmt.Errorf call in that block accordingly.
surminus
reviewed
Apr 13, 2026
Comment on lines
+9
to
+19
| // Reconcile functions handle the four-way matrix of input (plan/state) vs | ||
| // output (API response) emptiness: | ||
| // | ||
| // 1. Input non-empty, Output non-empty → return output (API is authoritative) | ||
| // 2. Input non-empty, Output empty → return input (write-only / sensitive field) | ||
| // 3. Input empty, Output non-empty: | ||
| // - computed=true → return output (server-owned or defaulted field) | ||
| // - computed=false → return error (unexpected value for optional-only field) | ||
| // 4. Input empty, Output empty → return null | ||
| // | ||
| // "Empty" means null, unknown, or (for strings) the zero-length string "". |
Contributor
There was a problem hiding this comment.
Agree with this approach, although I can't see the functions actually being used anywhere?
Collaborator
Author
There was a problem hiding this comment.
Argh... I forgot to merge in the files where I changed it... My working tree is a mess right now
martin-ably
force-pushed
the
martin/qos/write-reconciliation-function
branch
from
5db2c4a to
4d0f9c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/provider/resource_ably_rule_pulsar_test.go (2)
116-116: Use an obviously dummy token literal to reduce secret-scan noiseLine 116 currently looks token-like and may create recurring false positives in security tooling. Consider replacing it with a clearly synthetic value (e.g.,
token = "test-token-not-a-secret").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/resource_ably_rule_pulsar_test.go` at line 116, The literal assigned to the variable token in the test (variable name "token" in internal/provider/resource_ably_rule_pulsar_test.go) appears secret-like; replace it with an obviously synthetic value such as "test-token-not-a-secret" (or similar clearly non-secret string) so security scanners don't flag it; update any assertions or mocks that depend on the exact token value to use the new synthetic value.
126-130: Strengthen minimal-contract assertions for unexpected fieldsCurrent checks on Lines 127-130 validate defaults, but they don’t explicitly prove optional fields are absent. Since this PR’s goal includes catching unexpected API-returned fields, add negative assertions for non-required attributes (e.g.,
target.enveloped,target.format,target.tls_trust_certs).Proposed test-hardening diff
Check: resource.ComposeAggregateTestCheckFunc( resource.TestCheckResourceAttrSet("ably_rule_pulsar.rule0", "id"), resource.TestCheckResourceAttr("ably_rule_pulsar.rule0", "status", "enabled"), resource.TestCheckResourceAttr("ably_rule_pulsar.rule0", "request_mode", "single"), + resource.TestCheckNoResourceAttr("ably_rule_pulsar.rule0", "target.enveloped"), + resource.TestCheckNoResourceAttr("ably_rule_pulsar.rule0", "target.format"), + resource.TestCheckNoResourceAttr("ably_rule_pulsar.rule0", "target.tls_trust_certs.#"), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/resource_ably_rule_pulsar_test.go` around lines 126 - 130, Add explicit negative assertions to the test's aggregate check so optional/returned-only fields are proven absent: update the resource.ComposeAggregateTestCheckFunc block (where resource.TestCheckResourceAttrSet("ably_rule_pulsar.rule0", "id") and resource.TestCheckResourceAttr(... "status"/"request_mode") are declared) to include checks that "target.enveloped", "target.format", and "target.tls_trust_certs" are not set (use the appropriate TestCheckNoResourceAttr / equivalent helper or assert empty/unset attribute checks for the "ably_rule_pulsar.rule0" resource) so the test fails if the API returns those unexpected fields.internal/provider/resource_ably_rule_amqp_test.go (1)
114-138: Consider moving the inline minimal HCL into a dedicated helper.This block is fairly large for an in-test literal. Extracting it (for example,
testAccAblyRuleAMQPMinimalConfig(appName string)) would make the test intent easier to scan and reduce config drift across similar minimal tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/provider/resource_ably_rule_amqp_test.go` around lines 114 - 138, Extract the large inline HCL into a helper function (e.g., testAccAblyRuleAMQPMinimalConfig(appName string) string) and replace the current fmt.Sprintf block that assigns config with a call to that helper, passing appName (and using tfProvider inside the helper or as an argument). The helper should return the full HCL string (including tfProvider and the ably_app/ably_queue/ably_rule_amqp blocks) so tests remain concise and the same config can be reused across other minimal tests; update resource_ably_rule_amqp_test.go to call testAccAblyRuleAMQPMinimalConfig(appName) where the current fmt.Sprintf is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/provider/resource_ably_key.go`:
- Around line 97-110: In buildKeyState, avoid unconditionally using
api.Capability: if api.Capability is nil or empty, fall back to the planned
value input.Capability so we don't collapse a user-configured required
attribute; replace the current Capability: mapToTypedSet(api.Capability) usage
with logic that selects api.Capability when it is present/has entries, otherwise
uses input.Capability, and pass that chosen map into mapToTypedSet (reference
symbols: buildKeyState, api.Capability, input.Capability, mapToTypedSet).
In `@internal/provider/resource_ably_namespace.go`:
- Around line 174-181: The reconciler currently skips setting
ns.BatchingInterval, ns.ConflationInterval and ns.ConflationKey when
api.BatchingEnabled or api.ConflationEnabled is nil, which treats nil as “false”
and clears prior values; change the logic to treat nil as “field omitted” and
preserve input values unless the API explicitly returns false. Specifically, in
the block using api.BatchingEnabled and api.ConflationEnabled, call
rc.int64val/rc.str for batching_interval, conflation_interval and conflation_key
when the API pointer is nil or true (i.e., only clear/omit when the pointer
exists and is false), using existing helpers optIntValue(optStringValue) and the
input.* fields (input.BatchingInterval, input.ConflationInterval,
input.ConflationKey) to keep prior values. Ensure ns.BatchingInterval,
ns.ConflationInterval and ns.ConflationKey are only set to null/cleared when
api.BatchingEnabled or api.ConflationEnabled is non-nil and false.
---
Nitpick comments:
In `@internal/provider/resource_ably_rule_amqp_test.go`:
- Around line 114-138: Extract the large inline HCL into a helper function
(e.g., testAccAblyRuleAMQPMinimalConfig(appName string) string) and replace the
current fmt.Sprintf block that assigns config with a call to that helper,
passing appName (and using tfProvider inside the helper or as an argument). The
helper should return the full HCL string (including tfProvider and the
ably_app/ably_queue/ably_rule_amqp blocks) so tests remain concise and the same
config can be reused across other minimal tests; update
resource_ably_rule_amqp_test.go to call
testAccAblyRuleAMQPMinimalConfig(appName) where the current fmt.Sprintf is used.
In `@internal/provider/resource_ably_rule_pulsar_test.go`:
- Line 116: The literal assigned to the variable token in the test (variable
name "token" in internal/provider/resource_ably_rule_pulsar_test.go) appears
secret-like; replace it with an obviously synthetic value such as
"test-token-not-a-secret" (or similar clearly non-secret string) so security
scanners don't flag it; update any assertions or mocks that depend on the exact
token value to use the new synthetic value.
- Around line 126-130: Add explicit negative assertions to the test's aggregate
check so optional/returned-only fields are proven absent: update the
resource.ComposeAggregateTestCheckFunc block (where
resource.TestCheckResourceAttrSet("ably_rule_pulsar.rule0", "id") and
resource.TestCheckResourceAttr(... "status"/"request_mode") are declared) to
include checks that "target.enveloped", "target.format", and
"target.tls_trust_certs" are not set (use the appropriate
TestCheckNoResourceAttr / equivalent helper or assert empty/unset attribute
checks for the "ably_rule_pulsar.rule0" resource) so the test fails if the API
returns those unexpected fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0878042-fda8-4bcf-8ee6-e6dca8d5d1d5
📒 Files selected for processing (28)
internal/provider/ingress_rules.gointernal/provider/provider_test.gointernal/provider/reconcile.gointernal/provider/reconcile_test.gointernal/provider/resource_ably_app.gointernal/provider/resource_ably_app_test.gointernal/provider/resource_ably_ingress_rule_mongo_test.gointernal/provider/resource_ably_ingress_rule_postgres_outbox_test.gointernal/provider/resource_ably_key.gointernal/provider/resource_ably_key_test.gointernal/provider/resource_ably_namespace.gointernal/provider/resource_ably_namespace_test.gointernal/provider/resource_ably_queue.gointernal/provider/resource_ably_queue_test.gointernal/provider/resource_ably_rule_amqp_external_test.gointernal/provider/resource_ably_rule_amqp_test.gointernal/provider/resource_ably_rule_azure_function_test.gointernal/provider/resource_ably_rule_http_cloudflare_worker_test.gointernal/provider/resource_ably_rule_http_google_cloud_function_test.gointernal/provider/resource_ably_rule_http_test.gointernal/provider/resource_ably_rule_ifttt_test.gointernal/provider/resource_ably_rule_kafka_test.gointernal/provider/resource_ably_rule_kinesis_test.gointernal/provider/resource_ably_rule_lambda_test.gointernal/provider/resource_ably_rule_pulsar_test.gointernal/provider/resource_ably_rule_sqs_test.gointernal/provider/resource_ably_rule_zapier_test.gointernal/provider/rules.go
✅ Files skipped from review due to trivial changes (3)
- internal/provider/resource_ably_rule_azure_function_test.go
- internal/provider/resource_ably_rule_http_google_cloud_function_test.go
- internal/provider/reconcile_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/provider/resource_ably_ingress_rule_mongo_test.go
- internal/provider/resource_ably_rule_amqp_external_test.go
- internal/provider/resource_ably_ingress_rule_postgres_outbox_test.go
- internal/provider/resource_ably_rule_lambda_test.go
- internal/provider/resource_ably_rule_ifttt_test.go
- internal/provider/resource_ably_rule_sqs_test.go
- internal/provider/resource_ably_rule_kinesis_test.go
- internal/provider/resource_ably_queue_test.go
- internal/provider/resource_ably_rule_kafka_test.go
- internal/provider/resource_ably_rule_http_cloudflare_worker_test.go
- internal/provider/resource_ably_app_test.go
- internal/provider/provider_test.go
- internal/provider/reconcile.go
Comment on lines
+97
to
+110
| // buildKeyState reconciles plan/state input with an API response. | ||
| func buildKeyState(rc *reconciler, input AblyKey, api control.KeyResponse) AblyKey { | ||
| return AblyKey{ | ||
| ID: rc.str("id", input.ID, types.StringValue(api.ID), true), | ||
| AppID: rc.str("app_id", input.AppID, types.StringValue(api.AppID), false), | ||
| Name: rc.str("name", input.Name, types.StringValue(api.Name), false), | ||
| Key: rc.str("key", input.Key, types.StringValue(api.Key), true), | ||
| RevocableTokens: rc.boolean("revocable_tokens", input.RevocableTokens, optBoolValue(api.RevocableTokens), true), | ||
| Capability: mapToTypedSet(api.Capability), | ||
| Status: rc.int64val("status", input.Status, types.Int64Value(int64(api.Status)), true), | ||
| Created: rc.int64val("created", input.Created, types.Int64Value(api.Created), true), | ||
| Modified: rc.int64val("modified", input.Modified, types.Int64Value(api.Modified), true), | ||
| } | ||
| } |
There was a problem hiding this comment.
Preserve capabilities when the response omits it.
Line 105 still trusts api.Capability unconditionally. If a create/read/update response comes back with that field omitted or empty, this helper collapses a required user-configured attribute to {}, and the next plan will drift forever. buildKeyState should fall back to input.Capability when the API payload does not include a usable capability map.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/provider/resource_ably_key.go` around lines 97 - 110, In
buildKeyState, avoid unconditionally using api.Capability: if api.Capability is
nil or empty, fall back to the planned value input.Capability so we don't
collapse a user-configured required attribute; replace the current Capability:
mapToTypedSet(api.Capability) usage with logic that selects api.Capability when
it is present/has entries, otherwise uses input.Capability, and pass that chosen
map into mapToTypedSet (reference symbols: buildKeyState, api.Capability,
input.Capability, mapToTypedSet).
…I responses This should result in the TF provider being a little more resistant to upstream field changes.
martin-ably
force-pushed
the
martin/qos/write-reconciliation-function
branch
from
4d0f9c7 to
6955671
Compare
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
2 participants
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.
This should result in the TF provider being a little more resistant to upstream field changes.
Additionally, adds tests that check if any unexpected fields are returned when a minimal (required-only) config is sent.
Summary by CodeRabbit
Tests
Refactor
Chores