Skip to content

fix(portal): forget tg in state so it can be re-imported as DCR - #1403

Merged
mashkovd merged 1 commit into
mainfrom
fix/portal-tg-forget
Sep 26, 2026
Merged

mashkovd merged 1 commit into
mainfrom
fix/portal-tg-forget

Conversation

@mashkovd

Copy link
Copy Markdown
Member

Why

The apply of #1402 (run 36226160494) imported api but failed on tg:

PUT .../mcp/servers/tg
400 {"code":7001,"message":"Expected string, received null","path":["body","auth_credentials"]}

With auth_credentials removed from config, provider 5.24 sends it as an explicit null, and the API refuses the whole PUT. Nothing was written. Read back after the failure, tg is still ready/connected with auth_config_summary: null.

The API does accept a PUT that omits the field; that is how the switch was done. But the provider can't be made to omit a value it holds in state.

What

  • removed { from = …tg; lifecycle { destroy = false } } drops tg from state with no API call. The plan should say forget, which is not delete, so the apply guards pass it without allow_destroy.
  • tg goes on UNMANAGED for this one apply, so the allowlist existence check passes.
  • The ignore_changes rationale moves from the removed tg block to projects.

Next

A follow-up PR re-adds import + resource "tg". The import reads the server from the API, which returns no auth_credentials, so tg lands in state as DCR, the same way seerrsense and api did. The same PR empties UNMANAGED and fixes the "Fourth upstream" description (the P3 from #1402).

Until then the nightly registration check may exit 2 for tg: state still holds the old blob while tg is on DCR_SERVERS. After this apply tg is out of state and the check is quiet.

Local checks: tofu validate, validate-portal-allowlists.py and its --selftest.

Refs #1363, #1402

🤖 Generated with Claude Code

https://claude.ai/code/session_01T5nLoYSquzMsGsdS64wvry

The apply of #1402 imported api but failed on tg: with auth_credentials
removed from config, provider 5.24 sends an explicit null and the API
refuses the PUT (400 7001 "Expected string, received null"). Nothing was
written; tg stays ready/connected on DCR.

A PUT that omits the field is accepted, but the provider cannot omit a
value it holds in state. So tg is dropped from state with a
`removed { lifecycle { destroy = false } }` block (no API call), and the
follow-up change imports it again; an import reads no auth_credentials,
leaving tg in state as DCR like seerrsense and api.

tg is on UNMANAGED for that one apply. The ignore_changes rationale moves
to the projects resource, since the tg block it lived in is gone.

Refs #1363, #1402
Claude-Session: https://claude.ai/code/session_01T5nLoYSquzMsGsdS64wvry
Copilot AI lite review requested due to automatic review settings September 26, 2026 07:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 2m 25s —— View job


Review complete ✅

  • Gather context (PR body, changed files)
  • Review diff ebba5aa5...5d7060e0
  • Post inline findings (if any)
  • Submit verdict

Verdict: APPROVE — no P1/P2 findings, 1 P3 nit.

Summary: Verified the mechanics end-to-end:

  • The removed { from = ...tg; lifecycle { destroy = false } } block is valid HCL (>= 1.9.0 required here) and correctly drops tg from state without an API call — the old import block and resource "tg" {...} are both fully removed, and no other .tf file in this root references cloudflare_zero_trust_access_ai_controls_mcp_server.tg anymore (only stale comments elsewhere mention tg by name, e.g. mcp-portal.tf, which is harmless).
  • scripts/validate-portal-allowlists.py's check_existence() (line 375) requires every mapped server to either have a literal resource "..." "<id>" block in mcp-servers.tf or be in UNMANAGED — adding "tg" to UNMANAGED correctly keeps this check green for the one apply where tg has neither. No selftest fixture references "tg", so this doesn't interact with existing fixtures.
  • The ignore_changes rationale comment was correctly moved to projects (the next resource in file order), and all downstream resources (alice, coolify, seerrsense, api) were correctly updated to point at "projects" instead of the now-removed "tg" block for that shared reasoning.
  • Note: I wasn't able to run tofu validate or the script's --selftest in this sandbox (no tofu/terraform binary available, and script execution required approval I couldn't obtain) — this is a static review based on reading the diff and cross-referencing the rest of the repo.

P3 (non-blocking): scripts/validate-portal-allowlists.py:178 — the module docstring a few lines above (line 47) still says the UNMANAGED list is "empty since every mapped server got its resource," which is now momentarily false. The inline comment right above the constant explains the temporary state correctly, so this is just a docstring nit, not a functional issue.

@github-actions

Copy link
Copy Markdown
Contributor

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit 5d7060e.

No significant issues found.

# `tg` is here for the one apply between forgetting it and importing it back
# (see the `removed` block in mcp-servers.tf); the change that re-imports it
# empties this again.
UNMANAGED: set[str] = {"tg"}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: the module docstring's existence bullet (line 47) still reads UNMANAGED list ... empty since every mapped server got its resource. That's now stale for the duration of this PR — UNMANAGEDis{"tg"}here, not empty. The inline comment right above this line explains the temporary state correctly; worth a one-line tweak to the docstring too (or leave as-is since the follow-up PR that emptiesUNMANAGED` again will make it briefly true again either way). Non-blocking.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right for the life of this PR only. The re-import follow-up empties UNMANAGED again, which makes the docstring true once more, so it is left alone here.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No P1/P2 findings (1 P3). Good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants