fix: config merging in source/destination updates#211
Open
leggetter wants to merge 11 commits into
Open
Conversation
The Hookdeck API performs a partial (merge) update on the destination and source `config` object: keys omitted from the request body retain their existing values. Removing fields like `rate_limit` / `rate_limit_period` from a Terraform config therefore left them set on the resource and required manual cleanup in the dashboard. Diff the prior state's config against the new plan and explicitly send `null` for any top-level keys that were removed, so the API clears them. https://claude.ai/code/session_0157WHRYs4bD92XG8vmmh1vz
Verifies that removing rate_limit and rate_limit_period from a
destination's config actually clears them on the Hookdeck API.
Because the config attribute is not Computed (Terraform state stores the
user's input string, not the API's actual config), the assertion calls
GET /destinations/{id} directly and inspects the returned config object.
Run with:
TEST=./internal/provider/destination \
RUN=TestAccDestinationResource_RemoveRateLimit \
make testacc
https://claude.ai/code/session_0157WHRYs4bD92XG8vmmh1vz
The Hookdeck API resets rate_limit_period to its default ("second") when
rate_limit is cleared, rather than nulling it. The user-facing fix is
proven by rate_limit itself being null — rate limiting is disabled
regardless of what rate_limit_period reads.
https://claude.ai/code/session_0157WHRYs4bD92XG8vmmh1vz
Four new acceptance tests for destination config key removal: - AddRemoveReaddRateLimit: exercises the diff logic across multiple Update cycles (add, remove, remove, re-add) to catch state issues. - NoDriftAfterRateLimitRemoval: re-applies the same config after removal and asserts an empty plan, guarding against the null-out baseline drifting between applies. - RemoveHTTPMethod: proves the fix generalizes beyond rate_limit. http_method documented to default to null, so removal should produce a true null on the API (unlike rate_limit_period, which has a non-null default of "second"). - RateLimitPeriodMergeBehavior: investigates whether the API auto-resets rate_limit_period whenever rate_limit becomes null, or only when we explicitly send null for the period field. Sends null for rate_limit while keeping period in the config; assertion failure would indicate API-side auto-reset behavior, not a provider bug. https://claude.ai/code/session_0157WHRYs4bD92XG8vmmh1vz
The previous test job targeted ./internal/provider/ (non-recursive), which has no _test.go files — so it passed by doing nothing. The existing acceptance tests in connection/ and transformation/, and the new destination/ tests, weren't actually running in CI. Changes: - Recurse into ./internal/provider/... - Inject HOOKDECK_API_KEY from repo secrets so TF_ACC=1 tests can run. - Skip the test job on fork PRs (where secrets aren't available); they re-run after merge. - Limit push trigger to main to avoid duplicate runs on PR branches. - Refresh TF version matrix from 1.0-1.4 (unsupported) to 1.9 and 1.10. - Serialize acceptance runs (job concurrency + matrix max-parallel: 1) so the 240 req/min Hookdeck API limit isn't exceeded across parallel runs — the provider doesn't retry on 429. - Bump timeouts from 15/10m to 30/25m for headroom as the suite grows. Requires HOOKDECK_API_KEY to be set as a repo secret pointing at a dedicated test workspace (not production). https://claude.ai/code/session_0157WHRYs4bD92XG8vmmh1vz
Go modules (bundles #197, #201, #202, #203, #204): - github.com/hashicorp/terraform-plugin-testing 1.13.3 → 1.14.0 - github.com/hashicorp/terraform-plugin-framework 1.17.0 → 1.19.0 - github.com/hashicorp/terraform-plugin-go 0.29.0 → 0.31.0 - golang.org/x/sys 0.37.0 → 0.43.0 (>= #201's 0.42.0; supersedes #198) - golang.org/x/time 0.14.0 → 0.15.0 Transitive cascade required by plugin-go 0.31.0 (which makes GenerateResourceConfig required on tfprotov5/6.ResourceServer): - github.com/hashicorp/terraform-plugin-sdk/v2 2.37.0 → 2.40.1 - Go toolchain 1.24 → 1.25.8 GitHub Actions (bundles #206, #207, #208, #209, #210): - hashicorp/setup-terraform 3 → 4 - golangci/golangci-lint-action 8 → 9 - crazy-max/ghaction-import-gpg 6.2.0 → 7.0.0 - actions/setup-go 5 → 6 - goreleaser/goreleaser-action 6 → 7 CI guard: - Skip the acceptance test job for actor dependabot[bot]. Dependabot runs with a restricted GITHUB_TOKEN and can't access repo secrets, so HOOKDECK_API_KEY would be empty and the tests would t.Fatal. Tests re-run on push to main once dep PRs are merged. #198 superseded by #201 (same library, lower version). https://claude.ai/code/session_0157WHRYs4bD92XG8vmmh1vz
leggetter
changed the title
golangci-lint v2.1.6 was built with Go 1.24 and refuses to lint code
targeting Go 1.25+ ("the Go language version used to build golangci-lint
is lower than the targeted Go version"). The go.mod bump to 1.25.8 in
the prior dependabot consolidation tripped this. v2.5.0 is built with
Go 1.25 and is what's been verified locally.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 20, 2026
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes Hookdeck API “merge” semantics during Terraform updates by explicitly nulling removed config keys so they are deleted server-side, and adds acceptance coverage for destination config key removals.
Changes:
- Update source/destination update flows to pass prior
configJSON from state and build update payloads that set removed keys tonull - Add destination acceptance tests and HCL fixtures exercising key removal / drift behavior
- Bump Go / Terraform plugin dependencies and modernize CI workflows (Go setup, lint, acceptance job gating/serialization)
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/provider/source/sdk.go | Build update payload using prior config to null removed keys |
| internal/provider/source/resource.go | Fetch prior state config and pass into Update |
| internal/provider/destination/sdk.go | Same null-out update payload logic for destinations |
| internal/provider/destination/resource.go | Fetch prior state config and pass into Update |
| internal/provider/destination/resource_test.go | New destination acceptance tests validating API-side key deletion |
| internal/provider/destination/testdata/*.tf | Test fixtures for destination config variations |
| go.mod | Go version + dependency updates |
| go.sum | Dependency checksum updates |
| .github/workflows/test.yml | CI updates + acceptance test gating/serialization |
| .github/workflows/release.yml | Workflow action version bumps |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+105
to
+111
| var state *sourceResourceModel | ||
| resp.Diagnostics.Append(req.State.Get(ctx, &state)...) | ||
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } | ||
|
|
||
| diags := data.Update(ctx, &r.client, state.Config.ValueString()) |
Comment on lines
+105
to
+111
| var state *destinationResourceModel | ||
| resp.Diagnostics.Append(req.State.Get(ctx, &state)...) | ||
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } | ||
|
|
||
| diags := data.Update(ctx, &r.client, state.Config.ValueString()) |
| go 1.24.0 | ||
|
|
||
| toolchain go1.24.7 | ||
| go 1.25.8 |
Comment on lines
+285
to
+290
| var priorPayloadConfig map[string]interface{} | ||
| if err := json.Unmarshal([]byte(priorConfig), &priorPayloadConfig); err != nil { | ||
| diags.AddError("Error parsing prior source config", err.Error()) | ||
| return nil, diags | ||
| } | ||
|
|
Comment on lines
+278
to
+283
| var priorPayloadConfig map[string]interface{} | ||
| if err := json.Unmarshal([]byte(priorConfig), &priorPayloadConfig); err != nil { | ||
| diags.AddError("Error parsing prior destination config", err.Error()) | ||
| return nil, diags | ||
| } | ||
|
|
Comment on lines
+275
to
+283
| func (m *sourceResourceModel) toUpdatePayload(priorConfig string) (map[string]interface{}, diag.Diagnostics) { | ||
| payload, diags := m.toPayload() | ||
| if diags.HasError() { | ||
| return nil, diags | ||
| } | ||
|
|
||
| if priorConfig == "" { | ||
| return payload, diags | ||
| } |
Comment on lines
+32
to
+36
| func loadTestConfigFormatted(filename string, args ...interface{}) string { | ||
| path := filepath.Join("testdata", filename) | ||
| content, err := os.ReadFile(path) | ||
| if err != nil { | ||
| panic(err) |
Mirrors TestAccDestinationResource_RemoveRateLimit for sources: applies an HTTP source with allowed_http_methods set, removes it, and verifies via direct API read that the key is cleared. Exercises the same null-out fix in source/sdk.go that resource_test.go covers for destinations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Hookdeck OpenAPI spec marks every array- and boolean-typed top-level config property as non-nullable on both source and destination. Sending null for those fields produces a 422 "must be an array" / type error, turning the original silent merge bug into an apply-time failure. Type-switch on the prior value's runtime type and skip arrays and booleans in the diff loop. Scalars (string/number) and nullable objects continue to be cleared via null, which is what the destination test suite already verifies. For non-nullable types we fall back to the original merge behavior (key omitted from the payload); the provider can't reset such fields to their API default, but the apply succeeds rather than erroring. Tests: - Replace the source `allowed_http_methods` test (was exercising the bug introduced by the previous commit) with a `custom_response` test using a nullable object — actually exercises the fix on the source side, mirroring how destination tests exercise it on nullable scalars. - Add a regression test that removing `allowed_http_methods` succeeds without 422, locking in the array-skip behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously, removing a non-nullable config field (e.g. array `allowed_http_methods`, boolean `path_forwarding_disabled`) from a Terraform config left a half-fixed state: sending null returned 422 and broke the apply, while skipping the field left the original merge bug in place. Neither honors the user's intent of "no longer manage this field". Add a small per-type map of known documented defaults for non-nullable fields (audited against the OpenAPI spec at api.hookdeck.com/2025-07-01): - HTTP source `allowed_http_methods` → ["PUT","POST","PATCH","DELETE"] (spec description documents this exact default) - HTTP / CLI / MOCK_API destination `path_forwarding_disabled` → false (response examples + docs: "By default, Hookdeck preserves any path appended to the Hookdeck URL") When a known non-nullable field is removed, send the documented default explicitly so the API resets the field rather than retaining it. For non-nullable fields without a known default (none today, but possible for new source/destination types), fall back to omitting — apply still succeeds, but the merge bug remains for that field until we add it to the map. Tests: - Replace the source `DoesNotError` regression test with `ResetsToDefault`, asserting the API now has the documented default array after removal. - Add a parallel destination test for `path_forwarding_disabled` removal, asserting the API resets to false. Test rate-limit handling: in TF_ACC=1 mode, share a single rate limiter across every RawClient created in the process. Each acceptance step spins up a fresh provider, otherwise each gets its own 5-token burst, and stacked bursts across a large test suite were tripping the API's 240 req/min sliding window (visible as a 429 on the connection test after destinations + sources had run). Production behavior is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The shared rate limiter prevents the 5-token-burst-per-provider stacking issue within a single test process, but cross-process API budget can still occasionally trip the API's 240 req/min sliding window: each go-test-binary boundary resets the local limiter, and cumulative requests across the suite can still 429 even when each binary's own rate is conservative. Wrap the HTTP client under TF_ACC=1 to retry 429 responses with the Retry-After header respected (falling back to exponential backoff: 2s, 4s, 8s, 16s, 32s, capped at 60s, max 5 retries). Production behavior is unchanged — the sdkclient's existing comment explicitly rejects retry-on-429 there to avoid masking systemic issues and consuming rate-limit tokens; the test wrapper exists only to absorb transient bursts driven by the test framework's provider lifecycle. Includes a unit-test scripted-HTTPDoer covering: success-first-attempt, retry-then-success, max-retries-exhausted (returns final 429), exponential backoff when Retry-After absent, non-429 passthrough, transport-error passthrough, 60s wait cap, and HTTP-date Retry-After parsing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
This PR fixes an issue where removed configuration keys were not being properly deleted when updating sources and destinations. The Hookdeck API merges configuration updates, so omitted keys would persist on the resource even after being removed from the Terraform configuration.
Key Changes
Updatemethods in bothdestination/sdk.goandsource/sdk.goto accept the prior configuration state as a parametertoUpdatePayloadmethods to both destination and source models that:toPayload()methodnullin the update payload to ensure they are deleted on the API sidedestination/resource.goandsource/resource.goto:Update()Update()methodImplementation Details
The solution works by comparing the prior configuration keys with the new configuration keys. Any keys that existed before but are absent in the new configuration are explicitly set to
nullin the update payload. This ensures the Hookdeck API removes those configuration options rather than retaining them due to its merge behavior.https://claude.ai/code/session_0157WHRYs4bD92XG8vmmh1vz