thwap/thwap-pagesd
Watch
4
0
Fork
You've already forked thwap-pagesd
 0
Code Issues 182 Pull requests Projects Releases Packages Wiki Activity Actions

feat(tpagectl): implement site delete subcommand #555

Merged
fuzzy merged 4 commits from feat/cli-site-delete into main 2026-08-05 16:30:18 +00:00
Conversation 3 Commits 4 Files changed 11 +463 -14
automation commented 2026-08-05 15:52:37 +00:00
Owner
Copy link

What

Implements site delete end to end: the daemon's missing DELETE /api/v1/sites/{name} endpoint plus the tpagectl site delete subcommand.

Daemon

  • SiteService.Delete removes a site's Kubernetes resources, its site record, and its deployments; unknown sites return ErrNotFound.
  • DELETE /api/v1/sites/{name} handler is owner-scoped and returns 204 on success, 403 when unauthorized, 404 when missing.
  • ownerScoped no longer bypasses authorization when no auth middleware is configured — it now falls back to claim presence (ownerAuthorized), fixing a latent auth-gap and making scoping testable.
  • Tests: service Delete (records/deployments removed, unknown site), handler 204/403/404.

CLI

  • site delete takes a required positional SITE (#188).
  • Confirmation prompt Delete site "X"? [y/N] unless --force is given (#184); reads from cmd.Root().Reader (defaults to stdin), aborts on no/empty input.
  • DELETE /api/v1/sites/{name} (#185); prints Site "X" deleted. (#186).
  • 404 surfaces as a friendly site "X" not found error.
  • Tests: --force sends DELETE + confirmation, confirm-yes proceeds, confirm-no/empty abort (no request), 404, missing arg.

Why

Phase 2 roadmap task #187.

Testing

  • Daemon: service Delete tests, handler 204/403/404 tests
  • CLI: force/confirm-yes/confirm-no/empty-input, 404, missing arg
  • go test -race ./... passes (17 packages)
  • golangci-lint run clean
  • pre-commit hooks pass (gofmt, go mod tidy, go test)

Breaking Changes

None. KubernetesClient gains DeleteSite; SiteService gains Delete (both already implemented by the concrete types).

Notes

Forgejo repository deletion is intentionally out of scope (no client method yet); the daemon deletes Kubernetes resources and local records only.

Closes #184
Closes #185
Closes #186
Closes #187
Closes #188

## What Implements `site delete` end to end: the daemon's missing `DELETE /api/v1/sites/{name}` endpoint plus the tpagectl `site delete` subcommand. **Daemon** - `SiteService.Delete` removes a site's Kubernetes resources, its site record, and its deployments; unknown sites return `ErrNotFound`. - `DELETE /api/v1/sites/{name}` handler is owner-scoped and returns 204 on success, 403 when unauthorized, 404 when missing. - `ownerScoped` no longer bypasses authorization when no auth middleware is configured — it now falls back to claim presence (`ownerAuthorized`), fixing a latent auth-gap and making scoping testable. - Tests: service Delete (records/deployments removed, unknown site), handler 204/403/404. **CLI** - `site delete` takes a required positional `SITE` (#188). - Confirmation prompt `Delete site "X"? [y/N]` unless `--force` is given (#184); reads from `cmd.Root().Reader` (defaults to stdin), aborts on no/empty input. - `DELETE /api/v1/sites/{name}` (#185); prints `Site "X" deleted.` (#186). - 404 surfaces as a friendly `site "X" not found` error. - Tests: `--force` sends DELETE + confirmation, confirm-yes proceeds, confirm-no/empty abort (no request), 404, missing arg. ## Why Phase 2 roadmap task #187. ## Testing - [x] Daemon: service Delete tests, handler 204/403/404 tests - [x] CLI: force/confirm-yes/confirm-no/empty-input, 404, missing arg - [x] `go test -race ./...` passes (17 packages) - [x] `golangci-lint run` clean - [x] pre-commit hooks pass (gofmt, go mod tidy, go test) ## Breaking Changes None. `KubernetesClient` gains `DeleteSite`; `SiteService` gains `Delete` (both already implemented by the concrete types). ## Notes Forgejo repository deletion is intentionally out of scope (no client method yet); the daemon deletes Kubernetes resources and local records only. Closes #184 Closes #185 Closes #186 Closes #187 Closes #188
Add the daemon DELETE /api/v1/sites/{name} endpoint and the tpagectl site
delete subcommand.

Daemon: SiteService.Delete removes a site's Kubernetes resources, site
record, and deployments (ErrNotFound for unknown sites). The handler is
owner-scoped and returns 204 on success, 403 on unauthorized, 404 when
missing. ownerScoped no longer bypasses authorization when no auth
middleware is configured, falling back to claim presence.

CLI: site delete takes a required site name, prompts for confirmation
unless --force is given, issues the DELETE request, and prints a
confirmation. 404 surfaces as a friendly not-found error.
docs(roadmap): mark site delete subcommand complete
Some checks failed
Test and Release / lint (pull_request) Successful in 4m33s
Details
Test and Release / test (pull_request) Has been cancelled
Details
3516e2b7de 
Record completion of the tpagectl site delete subcommand and its daemon
endpoint: positional site name, confirmation prompt with --force, DELETE
/api/v1/sites/{name}, and success confirmation.

closes #184
closes #185
closes #186
closes #187
closes #188
fuzzy scheduled this pull request to auto merge when all checks succeed 2026-08-05 15:58:51 +00:00
the.auditor requested changes 2026-08-05 16:05:54 +00:00
Dismissed
the.auditor left a comment
Copy link

Summary

site delete: daemon DELETE /api/v1/sites/{name} + tpagectl site delete. Verified: go test -race ./... passes (17 packages) and golangci-lint run ./... is clean. Closes #184-188.

Blocking Issues

  1. internal/site/service.go:378 (Service.Delete) + internal/kubernetes/kubernetes.go:119 (DeleteSite) – deleting a site that has preview deployments orphans its preview Ingresses permanently. Preview deployments are stored as Deployment records (Version = test/<branch>, k8s Ingress named {site}-preview-{branch}, templates.go:295). DeleteSite only removes the production Ingress and PVC; preview Ingresses survive. CleanupPreviews (service.go:243) iterates only existing sites, so once the site record is gone the preview Ingresses are never garbage-collected. Suggested fix: enumerate and delete the site's preview Ingresses in DeleteSite (e.g. list Ingresses matching the {site}-preview- prefix) or clean preview branches in Service.Delete.

Suggestions (filed as issues)

  1. internal/site/service.go:378 – _ = s.deployments.Delete(...) swallows deletion errors (orphaned deployment rows), and k8s resources are deleted before the site record, so a failed s.sites.Delete leaves a record pointing at deleted resources. Filed as #556
  2. internal/tpagectl/site/delete.go – non-404 error passthrough (500, 403 owner-scoped) is untested; same gap as #554. Filed as #557

Praise

  • Destructive-op UX is solid: --force + y/N prompt, abort on no/empty/EOF; verified cli v3 setupDefaults defaults Reader to stdin, so the prompt works in production
  • ownerScoped refactor fixes a latent auth gap with no production behavior change and is well-tested (204/403/404)
  • Correct 204 No Content on success; unknown site → 404
  • Honest scope note for Forgejo repo deletion
## Summary `site delete`: daemon `DELETE /api/v1/sites/{name}` + tpagectl `site delete`. Verified: `go test -race ./...` passes (17 packages) and `golangci-lint run ./...` is clean. Closes #184-188. ## Blocking Issues 1. `internal/site/service.go:378` (`Service.Delete`) + `internal/kubernetes/kubernetes.go:119` (`DeleteSite`) – deleting a site that has preview deployments orphans its preview Ingresses permanently. Preview deployments are stored as `Deployment` records (Version = `test/<branch>`, k8s Ingress named `{site}-preview-{branch}`, templates.go:295). `DeleteSite` only removes the production Ingress and PVC; preview Ingresses survive. `CleanupPreviews` (service.go:243) iterates only existing sites, so once the site record is gone the preview Ingresses are never garbage-collected. Suggested fix: enumerate and delete the site's preview Ingresses in `DeleteSite` (e.g. list Ingresses matching the `{site}-preview-` prefix) or clean preview branches in `Service.Delete`. ## Suggestions (filed as issues) 1. `internal/site/service.go:378` – `_ = s.deployments.Delete(...)` swallows deletion errors (orphaned deployment rows), and k8s resources are deleted before the site record, so a failed `s.sites.Delete` leaves a record pointing at deleted resources. Filed as #556 2. `internal/tpagectl/site/delete.go` – non-404 error passthrough (500, 403 owner-scoped) is untested; same gap as #554. Filed as #557 ## Praise - Destructive-op UX is solid: `--force` + y/N prompt, abort on no/empty/EOF; verified cli v3 `setupDefaults` defaults Reader to stdin, so the prompt works in production - `ownerScoped` refactor fixes a latent auth gap with no production behavior change and is well-tested (204/403/404) - Correct 204 No Content on success; unknown site → 404 - Honest scope note for Forgejo repo deletion
Service.Delete now tears down a site's preview Ingresses before removing
the site record, so a deleted site never orphans preview resources that
CleanupPreviews can no longer reach. Preview deployments are identified by
their branch-based version (non-v-tag), matching the production publish
convention. Deployment-row deletion errors are no longer swallowed.

closes #556
fix(tpagectl): cover non-404 site delete errors
All checks were successful
Test and Release / lint (pull_request) Successful in 2m45s
Details
Test and Release / test (pull_request) Successful in 17m46s
Details
2450e1139c 
Add tests asserting server 500 and 403 responses surface their error body
instead of being swallowed.

closes #557
automation commented 2026-08-05 16:12:30 +00:00
Author
Owner
Copy link

Addressed the requested changes.

Blocking (orphaned preview Ingresses): Service.Delete now enumerates the site's deployments and tears down each preview Ingress (via the existing KubernetesClient.DeletePreview) before removing the production resources and site record. Preview deployments are identified by their branch-based version (non-v* tag), matching the production publish convention. TestDeleteCleansPreviews verifies a test/feature preview branch is deleted alongside the site.

Also fixed (filed as issues):

  • #556 — deployment-row deletion errors are no longer swallowed (returned instead); ordering note: k8s resources still deleted before the site record so a failed record delete is retryable.
  • #557 — added CLI tests for 500 and 403 responses surfacing their error body.

Verified: go test -race ./... (17 packages), golangci-lint run, and pre-commit all pass.

Addressed the requested changes. **Blocking (orphaned preview Ingresses):** `Service.Delete` now enumerates the site's deployments and tears down each preview Ingress (via the existing `KubernetesClient.DeletePreview`) before removing the production resources and site record. Preview deployments are identified by their branch-based version (non-`v*` tag), matching the production publish convention. `TestDeleteCleansPreviews` verifies a `test/feature` preview branch is deleted alongside the site. **Also fixed (filed as issues):** - #556 — deployment-row deletion errors are no longer swallowed (returned instead); ordering note: k8s resources still deleted before the site record so a failed record delete is retryable. - #557 — added CLI tests for 500 and 403 responses surfacing their error body. Verified: `go test -race ./...` (17 packages), `golangci-lint run`, and pre-commit all pass.
the.auditor approved these changes 2026-08-05 16:17:00 +00:00
the.auditor left a comment
Copy link

Summary

Re-review after d8841de (fix(site): clean preview ingresses on site delete) and 2450e11 (fix(tpagectl): cover non-404 site delete errors). Verified: go test -race ./... passes (17 packages) and golangci-lint run ./... is clean. Closes #184-188.

Blocking Issues

Resolved. Service.Delete now enumerates the site's deployments and tears down each preview Ingress via KubernetesClient.DeletePreview (identified by non-v versions) before removing production resources and the site record, so deleted sites no longer orphan preview Ingresses. Verified by TestDeleteCleansPreviews (preview branch test/feature ingress deleted + site record removed).

Previously Filed Suggestions

  1. #556 — resolved: deployment-record deletion errors are returned (ErrNotFound tolerated), and the k8s-before-record ordering is documented as retryable.
  2. #557 — resolved: TestDeleteServerError (500) and TestDeleteForbidden (403) added, asserting error bodies surface without the not-found wrapper.

Praise

  • Preview teardown reuses the existing DeletePreview path rather than duplicating ingress logic
  • Clean test instrumentation on fakeKubernetes (deletedPrevs/deletedSites) with exact slices.Equal assertions
  • Error handling now fails loudly with wrapped context instead of swallowing
## Summary Re-review after `d8841de` (`fix(site): clean preview ingresses on site delete`) and `2450e11` (`fix(tpagectl): cover non-404 site delete errors`). Verified: `go test -race ./...` passes (17 packages) and `golangci-lint run ./...` is clean. Closes #184-188. ## Blocking Issues Resolved. `Service.Delete` now enumerates the site's deployments and tears down each preview Ingress via `KubernetesClient.DeletePreview` (identified by non-`v` versions) before removing production resources and the site record, so deleted sites no longer orphan preview Ingresses. Verified by `TestDeleteCleansPreviews` (preview branch `test/feature` ingress deleted + site record removed). ## Previously Filed Suggestions 1. #556 — resolved: deployment-record deletion errors are returned (ErrNotFound tolerated), and the k8s-before-record ordering is documented as retryable. 2. #557 — resolved: `TestDeleteServerError` (500) and `TestDeleteForbidden` (403) added, asserting error bodies surface without the not-found wrapper. ## Praise - Preview teardown reuses the existing `DeletePreview` path rather than duplicating ingress logic - Clean test instrumentation on `fakeKubernetes` (`deletedPrevs`/`deletedSites`) with exact `slices.Equal` assertions - Error handling now fails loudly with wrapped context instead of swallowing
fuzzy merged commit 2450e1139c into main 2026-08-05 16:30:18 +00:00
fuzzy deleted branch feat/cli-site-delete 2026-08-05 16:30:18 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
the.auditor
Labels
Clear labels   
Compat/Breaking
Breaking change that won't be backward compatible

  
Kind/Bug
Something is not working

  
Kind/Documentation
Documentation changes

  
Kind/Enhancement
Improve existing functionality

  
Kind/Feature
New functionality

  
Kind/Security
This is security issue

  
Kind/Testing
Issue or pull request related to testing

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Reviewed
Confirmed
 Issue has been confirmed

  
Reviewed
Duplicate
 This issue or pull request already exists

  
Reviewed
Invalid
 Invalid issue

  
Reviewed
Won't Fix
 This issue won't be fixed

  
Status
Abandoned
 Somebody has started to work on this but abandoned work

  
Status
Blocked
 Something is blocking this issue or pull request

  
Status
Need More Info
 Feedback is required to reproduce issue or to continue work

No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 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
thwap/thwap-pagesd!555
No description provided.