fix: config merging in source/destination updates

[leggetter]

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

• Modified Update methods in both destination/sdk.go and source/sdk.go to accept the prior configuration state as a parameter
• Added toUpdatePayload methods to both destination and source models that:
  • Build the update payload using the existing toPayload() method
  • Parse the prior configuration state
  • Explicitly set removed config keys to null in the update payload to ensure they are deleted on the API side
• Updated resource update handlers in destination/resource.go and source/resource.go to:
  • Retrieve the current state before calling Update()
  • Pass the prior configuration (as a JSON string) to the Update() method

Implementation 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 null in 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

[Copilot]

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 config JSON from state and build update payloads that set removed keys to null
• 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.

[leggetter]

Source acceptance coverage was added in 1a17fb7: TestAccSourceResource_RemoveCustomResponse (removes a nullable object and asserts the key is absent via API read) and TestAccSourceResource_RemoveAllowedHTTPMethodsResetsToDefault (non-nullable array resets to the documented default). Both read back from the API.

---

Generated by Claude Code

[alexluong]

LGTM, left a few comments! Appreciate the testing setup as well

[alexluong]

This feels a bit off because it requires us maintaining this mapping. Right now we have many sources but this only applies for HTTP. What about other cases?

That said, I don't have a suggestion here yet, just want to flag this and see if there's something else we can consider?

[leggetter]

Agreed it's not ideal. The API marks array/boolean config fields non-nullable, so null returns 422 — we can't clear them like nullable fields, so the map resets them to their documented defaults (audited against the 2025-07-01 spec; rationale in 82234fc). You're right it's a maintained mapping, and unmapped fields fall back to merge. The clean fix is API-side — accept null and reset server-side — which removes the map. I'll raise it with the team.

[alexluong]

We seem to only check top-level keys then. Is that the expected behavior?

[leggetter]

Yes, top-level only is intentional. Nested sub-key removal (e.g. dropping a field inside custom_response while keeping the object) isn't handled — whether that's a gap depends on whether the API replaces nested objects or deep-merges them, which I haven't confirmed. I'll add a probe test against custom_response to settle it, and recurse the diff if needed.

[leggetter]

Worth grounding first: source and destination updates both use PUT /sources/{id} and PUT /destinations/{id} (sdk.go:187 / sdk.go:180) — there's no PATCH on either endpoint in the spec. So "partial nested update" here means PUT-ing a config body with a nested object whose inner fields are omitted, against an endpoint that already (unusually) does top-level merge on the PUT body — the behavior this PR fixes.

Spot-checked the spec to be sure: SourceCustomResponse does declare required: [content_type, body], so the 422 there when dropping either field is consistent with the spec — that was a poor probe choice on my part, the spec already forbade the shape I was sending.

The real finding is HMAC auth: SourceConfigHTTPAuthHMAC declares no required array, yet the API still rejects with config.auth.header_key is required when header_key is dropped. So the OpenAPI spec under-declares required fields on HMAC (and probably the other HTTP auth variants, though I didn't probe those) — worth flagging with the API team separately.

In the two nested objects probed (custom_response and HMAC auth), the API rejects any PUT body where a nested object is missing inner fields — so we never actually observe replace-vs-deep-merge, because the API short-circuits before merge semantics matter. That's a pattern across what I tested, not a proof across the whole schema, but it's enough to say the top-level-only diff in toUpdatePayload can't be reached with valid input that would clear a sub-key incorrectly. Leaving the recursion out on that basis.

Added TestAccSourceResource_NestedConfigObjectsValidatedWhole (ac81ff8) to lock the HMAC case in: it applies a full HMAC auth block, then attempts to update with header_key removed and expects a 422. If the API ever loosens that validation and step 2 starts succeeding, the test fails and that's the signal to revisit whether the diff needs to recurse into nested objects.

[leggetter]

Followup: checked the API source to settle the replace-vs-deep-merge question instead of leaving it inferred from the 422s.

In server/controllers/Source.ts:58-87, the merge is purely top-level on config. For each legacy field (allowed_http_methods, custom_response), the controller does: if the key is present in the request, take the new value as-is; if omitted, preserve the existing value; otherwise null. auth follows the same pattern — the entire new auth object replaces the prior verification.configs with no merging.

So nested objects are replaced wholesale, not deep-merged. The 422s we observed earlier were schema validation (IntegrationService rejecting HMAC configs missing header_key; Joi rejecting custom_response missing body) — both run before the merge logic and aren't related to merge semantics.

Net: the top-level-only diff in toUpdatePayload is correct by construction, not just empirically defensible. If a nested object somewhere has truly optional inner fields, a user dropping one would just work — the new (smaller) object lands as-is and the inner key is gone. No recursion needed in the provider.

[alexluong]

I think we can use a better data structure for priorConfig here instead of string. Why not destinationResourceModel for example?

[leggetter]

Done in 5726e6c — Update/toUpdatePayload now take the typed prior model instead of a config string.