Fix API response handling for staging resources #3
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/api-response-envelope"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Decode site, device, and check API response envelopes and preserve the redacted organization SNMP community. Verified with go test, go vet, and a staging apply followed by a no-change plan.
🤖 Skippy PR review
3 findings — 3 blocking before merge.
internal/provider/site_resource_test.go:377internal/provider/client.go:235internal/provider/client.go:159Reviewed
e414395e3c3f. Commentskippy reviewto re-run.@ -155,3 +157,3 @@}return &result, nilreturn &result.Data, nil🟡 Warning — Six of ten registered resources still decode the envelope into a bare struct
This PR converts the site, device and check clients to a wrapper struct with a
dataJSON tag, but the contract holds for every API v1 endpoint (ToweropsWeb.Api.V1.ResourceControllermoduledoc: every JSON success is adataenvelope) and 18 functions still unmarshal into a bare struct: schedules (:279,:294,:310), escalation policies (:341,:356,:372), agents (:409,:424), integrations (:464,:479,:495), maintenance windows (:530,:545,:561), coverages (:736,:751,:769,:791). Each decodes to zero values, so create stores an emptyidand the following refresh or destroy requests the collection path with no id. All six resources are registered inprovider.go.This is a scope call rather than a repair, so it is not auto-fixed: either extend this PR with the same wrapper or split a follow-up before calling response handling fixed.
@ -223,3 +233,3 @@}return &result, nilreturn &result.Data, nil🟠 High — Decoding the device envelope exposes the same wiped write-only secret: SNMPv3 passwords
Now that
GetDevice/CreateDevice/UpdateDevicedecode the payload, the read path really runs - andDeviceSerializerreturns onlysnmpv3_auth_password_set/snmpv3_priv_password_set, never the passwords.device_resource.go:402and:414(Read) assigntypes.StringNull()when the response omits them, and:626/:638(Update) do the same, so a device configured withsnmpv3_auth_passwordorsnmpv3_priv_passwordloses both from state on the first refresh and the next update apply fails with "Provider produced inconsistent result after apply" (both attributes are Optional, Sensitive, not Computed). No test sets either password, which is why the suite is green.Same fix as the organization: leave the planned value in
datawhen the API omits the field.@ -402,3 +376,2 @@w.WriteHeader(http.StatusCreated)json.NewEncoder(w).Encode(Site{ID: siteID,json.NewEncoder(w).Encode(map[string]Site{"data": {ID: siteID,🟠 High — Site SNMP community is wiped on create because the fake API invents a field the real one redacts
This test only passes because its handler returns
snmp_community, which the API never sends:ToweropsWeb.Api.SiteSerializerexposessnmp_community_setand no credential value. Against the real APIcreated.SNMPCommunityis nil,setSiteOptionalFields(site_resource.go:270) assignstypes.StringNull(), and sincesnmp_communityis Optional and not Computed, Terraform aborts the create with "Provider produced inconsistent result after apply"; on refresh the value is dropped from state and every later plan re-proposes it.This PR already gives
towerops_organizationthe right treatment (keep the planned value, never overwrite it from the response). Do the same here: drop theelse { types.StringNull() }branch forSNMPCommunityinsetSiteOptionalFields, and make this fake returnsnmp_community_setinstead of the secret, the wayTestAccOrganizationResource_redactedCommunitydoes.Auto-fix pushed by skippy-bot in
6774d081.else { data.SNMPCommunity = types.StringNull() }branch insetSiteOptionalFieldsso the plannedsnmp_communityis kept when the API redacts itsnmp_community_set: true(viamap[string]any) instead of echoing the secret the real API never sendsSNMPv3AuthPasswordinReadSNMPv3PrivPasswordinReadSNMPv3AuthPasswordinUpdateSNMPv3PrivPasswordinUpdateStill open, not mechanical: none.
Not re-reviewed: this repair came from the reviewer's own findings, so the fix pass closes what it fixed and CI is the referee.
Correction to the auto-fix note above (it said "Still open, not mechanical: none"):
6774d081and resolved there, so it is closed, not open.dataenvelope; schedules, escalation policies, agents, integrations, maintenance windows and coverages still read bare structs. Your call whether that belongs in this PR or a follow-up.Two things worth knowing before you merge: this repo's only workflow is Release on tags, so no CI grades the push; and
helper/resource.Testskips unlessTF_ACCis set (no test case setsIsUnitTest), sogo test ./internal/provideralone skips everyTestAcc*fake, including the new organization redaction test.TF_ACC=1 go test ./internal/provideris the run that exercises them.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.