fix: preserve site coordinates and redacted secrets #2

graham · 2026-11-01T20:48:21Z

graham commented

2026-11-01 20:48:21 +00:00

Decode site responses from the API data envelope and preserve configured SNMP communities when the API redacts them. Document the drift-detection limitation.

Verification: Go tests, vet and module verification; site/organization Terraform acceptance tests; OpenTofu create, update and no-change refresh against an API-contract fixture.

Decode site responses from the API data envelope and preserve configured SNMP communities when the API redacts them. Document the drift-detection limitation. Verification: Go tests, vet and module verification; site/organization Terraform acceptance tests; OpenTofu create, update and no-change refresh against an API-contract fixture.

graham added 1 commit

2026-11-01 20:48:21 +00:00

fix: preserve site coordinates and redacted secrets

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

skippy-bot/autofix Repaired by the auto-fix in 91f884de

Details

9e55942828

skippy-bot reviewed

2026-11-01 20:51:23 +00:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity	Location	Issue
🟠 High	`internal/provider/client.go:153`	Envelope decode fixed for sites only: 24 other decodes still read bare structs

_{Reviewed 9e5594282850. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `internal/provider/client.go:153` | Envelope decode fixed for sites only: 24 other decodes still read bare structs | <sub>Reviewed `9e5594282850`. Comment `skippy review` to re-run.</sub>

internal/provider/client.go Outdated

					
				@ -151,2 +151,3 @@

					var result Site

					var result struct {

						Data Site `json:"data"`

🟠 High — Envelope decode fixed for sites only: 24 other decodes still read bare structs

The API wraps every JSON success in %{data: ...} (Resources.success_body/2, and the ResourceController moduledoc calls it the whole v1 contract), so these three site decodes are the only ones in this file that now match the wire format. The other 24 var result <Struct> unmarshals in this file (device, schedule, escalation policy, agent, integration, maintenance window, check, coverage) still decode the envelope into a bare struct, which succeeds silently and leaves every field zero. GetDevice is the sharp edge: DeviceResource.Create then writes data.IPAddress = types.StringValue(created.IPAddress), so a real apply plans ip_address = "192.0.2.10" and stores "", and Terraform rejects it with "Provider produced inconsistent result after apply". The device, check and coverage tests miss this because their httptest mocks still return bare structs, the same false negative client_test.go was fixed for here. Suggested fix: one type apiEnvelope[T any] struct { Data T json:"data" } (go 1.25), decode into it in every method in this file, decode into *T so a null or missing data errors instead of yielding a zero resource, and wrap the mocks in the affected _test.go files the same way.

**🟠 High — Envelope decode fixed for sites only: 24 other decodes still read bare structs** The API wraps every JSON success in `%{data: ...}` (`Resources.success_body/2`, and the `ResourceController` moduledoc calls it the whole v1 contract), so these three site decodes are the only ones in this file that now match the wire format. The other 24 `var result <Struct>` unmarshals in this file (device, schedule, escalation policy, agent, integration, maintenance window, check, coverage) still decode the envelope into a bare struct, which succeeds silently and leaves every field zero. `GetDevice` is the sharp edge: `DeviceResource.Create` then writes `data.IPAddress = types.StringValue(created.IPAddress)`, so a real apply plans `ip_address = "192.0.2.10"` and stores `""`, and Terraform rejects it with "Provider produced inconsistent result after apply". The device, check and coverage tests miss this because their httptest mocks still return bare structs, the same false negative `client_test.go` was fixed for here. Suggested fix: one `type apiEnvelope[T any] struct { Data T `json:"data"` }` (go 1.25), decode into it in every method in this file, decode into `*T` so a null or missing `data` errors instead of yielding a zero resource, and wrap the mocks in the affected `_test.go` files the same way.

skippy-bot marked this conversation as resolved

skippy-bot added 1 commit

2026-11-01 20:54:52 +00:00

fix(review): envelope decode fixed for sites only: 24 other decodes still

skippy-bot/review Skippy review: clean — no open findings

Details

skippy-bot/autofix Auto-fix in 91f884de: 1 finding(s) resolved, 4 file(s) changed. CI is the referee.

Details

91f884dea2

Skippy-Autofix: internal/provider/client.go:153 Envelope decode fixed for sites only: 24 other decodes still read bare structs

skippy-bot commented

2026-11-01 20:54:54 +00:00

First-time contributor

Auto-fix pushed by skippy-bot in 91f884de.

4 files changed

Still 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.

Auto-fix pushed by skippy-bot in `91f884de`. - 4 files changed Still 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.

skippy-bot/review Skippy review: clean — no open findings

Details

skippy-bot/autofix Auto-fix in 91f884de: 1 finding(s) resolved, 4 file(s) changed. CI is the referee.

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/site-organization-apply:fix/site-organization-apply

git switch fix/site-organization-apply

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.

git switch main

git merge --no-ff fix/site-organization-apply

git switch fix/site-organization-apply

git rebase main

git switch main

git merge --ff-only fix/site-organization-apply

git switch fix/site-organization-apply

git rebase main

git switch main

git merge --no-ff fix/site-organization-apply

git switch main

git merge --squash fix/site-organization-apply

git switch main

git merge --ff-only fix/site-organization-apply

git switch main

git merge fix/site-organization-apply

git push origin main

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

graham/terraform-provider-towerops!2

Rows
Columns