From 17d606943c11930db4a6ed3a78e1d6c154a4e7b2 Mon Sep 17 00:00:00 2001 From: caballeto Date: Tue, 5 May 2026 22:18:44 +0200 Subject: [PATCH] fix(monitor): correct status_code assertion docs + orphan-cleanup safety net MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 DevEx surfaced two issues with `assertions { type = "status_code" }`: 1. **Docs example used `expected = 200`** (number). The API contract is `String expected` (so it can carry "200", "2xx", "200-299"), and Jackson silently coerces the number to "200" on store. The round-trip then trips Terraform's "Provider produced inconsistent result after apply" check AND, because the API call had already succeeded by then, leaves an orphaned monitor server-side that no Terraform state file references. Fix: example now uses `expected = "200"` with an inline comment explaining why. Schema description for `assertions[].config` calls out the type-strictness rule explicitly (status_code.expected is STRING, response_time.thresholdMs is NUMBER). 2. **Create-flow had no orphan rollback.** If `mapToState` or `resp.State.Set` failed AFTER the API create, the monitor leaked. Fix: thread a `cleanupOrphan` closure through Create and call DELETE if either downstream step returns an error. Logged at Debug on success and Warn on failure (so the leaked-id is visible to operators in either case). This does NOT cover the framework's post-Create consistency check (which runs after our function returns and is what produced the round-2 reports). The fix for that path is the doc/example change above — once user values match the API's wire format, the matching logic in `mapToState` produces consistent state and the framework never errors. The cleanup closure protects against any future internal failure in the same window. Round-2 DevEx friction P0 #1 + P0 #3 (see /tmp/devex-cli-test/FRICTION-TABLE.md). Co-authored-by: Cursor --- docs/resources/monitor.md | 11 ++++-- .../resources/devhelm_monitor/resource.tf | 9 +++-- internal/provider/resources/monitor.go | 34 ++++++++++++++++++- 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/docs/resources/monitor.md b/docs/resources/monitor.md index d4ff0a9..84dcce5 100644 --- a/docs/resources/monitor.md +++ b/docs/resources/monitor.md @@ -27,8 +27,13 @@ resource "devhelm_monitor" "api" { }) assertions { - type = "status_code" - config = jsonencode({ expected = 200, operator = "equals" }) + type = "status_code" + # `expected` is a STRING in the API contract — it can hold "200", "2xx", + # or "200-299". Always quote the value, even for plain numeric codes; + # `jsonencode({ expected = 200, ... })` (number) plans cleanly but + # apply fails with "Provider produced inconsistent result" because the + # API normalizes the value to "200" (string) on the round-trip. + config = jsonencode({ expected = "200", operator = "equals" }) } assertions { @@ -147,7 +152,7 @@ resource "devhelm_monitor" "basic_auth" { Required: -- `config` (String) Assertion configuration as JSON; the inner `type` field is omitted (set via the sibling `type` attribute) and the rest of the shape depends on the assertion kind. Field names inside the JSON are camelCase (the API wire format), e.g. `jsonencode({expected = 200, operator = "equals"})` for `status_code` or `jsonencode({thresholdMs = 500})` for `response_time`. +- `config` (String) Assertion configuration as JSON; the inner `type` field is omitted (set via the sibling `type` attribute) and the rest of the shape depends on the assertion kind. Field names inside the JSON are camelCase (the API wire format) and JSON value types must match the API contract exactly — e.g. `status_code.expected` is a STRING (`jsonencode({expected = "200", operator = "equals"})`, not `expected = 200`), while `response_time.thresholdMs` is a NUMBER (`jsonencode({thresholdMs = 500})`). Type-mismatched values plan cleanly but fail apply with "Provider produced inconsistent result" because the API normalizes them on the round-trip. - `type` (String) Assertion type discriminator in snake_case wire format (e.g. `status_code`, `response_time`, `body_contains`, `header_value`, `dns_resolves`, `ssl_expiry`, `tcp_connects`). Must match an AssertionType enum value as serialized by the API. Optional: diff --git a/examples/resources/devhelm_monitor/resource.tf b/examples/resources/devhelm_monitor/resource.tf index 5f47d7f..76ee2de 100644 --- a/examples/resources/devhelm_monitor/resource.tf +++ b/examples/resources/devhelm_monitor/resource.tf @@ -12,8 +12,13 @@ resource "devhelm_monitor" "api" { }) assertions { - type = "status_code" - config = jsonencode({ expected = 200, operator = "equals" }) + type = "status_code" + # `expected` is a STRING in the API contract — it can hold "200", "2xx", + # or "200-299". Always quote the value, even for plain numeric codes; + # `jsonencode({ expected = 200, ... })` (number) plans cleanly but + # apply fails with "Provider produced inconsistent result" because the + # API normalizes the value to "200" (string) on the round-trip. + config = jsonencode({ expected = "200", operator = "equals" }) } assertions { diff --git a/internal/provider/resources/monitor.go b/internal/provider/resources/monitor.go index fe25592..3be71c7 100644 --- a/internal/provider/resources/monitor.go +++ b/internal/provider/resources/monitor.go @@ -26,6 +26,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/schema/validator" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-framework/types/basetypes" + "github.com/hashicorp/terraform-plugin-log/tflog" openapi_types "github.com/oapi-codegen/runtime/types" ) @@ -360,7 +361,7 @@ func (r *MonitorResource) Schema(_ context.Context, _ resource.SchemaRequest, re }, "config": schema.StringAttribute{ Required: true, - Description: "Assertion configuration as JSON; the inner `type` field is omitted (set via the sibling `type` attribute) and the rest of the shape depends on the assertion kind. Field names inside the JSON are camelCase (the API wire format), e.g. `jsonencode({expected = 200, operator = \"equals\"})` for `status_code` or `jsonencode({thresholdMs = 500})` for `response_time`.", + Description: "Assertion configuration as JSON; the inner `type` field is omitted (set via the sibling `type` attribute) and the rest of the shape depends on the assertion kind. Field names inside the JSON are camelCase (the API wire format) and JSON value types must match the API contract exactly — e.g. `status_code.expected` is a STRING (`jsonencode({expected = \"200\", operator = \"equals\"})`, not `expected = 200`), while `response_time.thresholdMs` is a NUMBER (`jsonencode({thresholdMs = 500})`). Type-mismatched values plan cleanly but fail apply with \"Provider produced inconsistent result\" because the API normalizes them on the round-trip.", }, "severity": schema.StringAttribute{ Optional: true, @@ -1231,11 +1232,42 @@ func (r *MonitorResource) Create(ctx context.Context, req resource.CreateRequest return } + // Orphan-cleanup safety net: from this point on, the monitor exists + // server-side. If anything below fails (state mapping or state-set), the + // resource will not enter Terraform state — and Terraform has no hook for + // "delete what you just created" in that case. We DELETE the orphan + // ourselves so future plans don't show silent server-side drift. + // + // NOTE: this does NOT catch the framework's post-Create consistency check + // ("Provider produced inconsistent result after apply") — that runs after + // this function returns and is what produced the original orphan reports + // in DevEx round 2. The fix for that path is to keep state in sync with + // the plan inside `mapToState`, plus the doc/example fix that prevents + // the value-type drift in the first place. + cleanupOrphan := func(reason string) { + if monitor == nil { + return + } + id := monitor.Id.String() + if err := api.Delete(ctx, r.client, api.MonitorPath(id)); err != nil { + tflog.Warn(ctx, "orphan monitor cleanup failed; resource may be leaked", + map[string]any{"id": id, "reason": reason, "delete_error": err.Error()}) + return + } + tflog.Debug(ctx, "deleted orphan monitor after create-time error", + map[string]any{"id": id, "reason": reason}) + } + resp.Diagnostics.Append(r.mapToState(ctx, &plan, monitor)...) if resp.Diagnostics.HasError() { + cleanupOrphan("mapToState returned errors") return } resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...) + if resp.Diagnostics.HasError() { + cleanupOrphan("State.Set returned errors") + return + } } func (r *MonitorResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {