Skip to content

feat(operator): support 'oabctl delete -f <manifest>', mirroring apply -f#1296

Merged
thepagent merged 5 commits into
mainfrom
feat/oabctl-delete-file
Jul 5, 2026
Merged

feat(operator): support 'oabctl delete -f <manifest>', mirroring apply -f#1296
thepagent merged 5 commits into
mainfrom
feat/oabctl-delete-file

Conversation

@chaodu-agent

@chaodu-agent chaodu-agent commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

kubectl has a well-established convention: kubectl apply -f <file> and
kubectl delete -f <file> both accept the exact same manifest, so tearing
down what you deployed doesn't require separately remembering/typing the
resource type and name. oabctl apply already followed this pattern
(apply -f <file>), but oabctl delete only supported the imperative
<resource> <name> form — no file-based option at all, which is what
prompted this PR (came up while walking through manual teardown of a test
deployment).

Change

Adds -f/--file to oabctl delete, mirroring apply -f exactly:

oabctl delete -f /tmp/oab-telegram-ingress/manifest.yaml
# equivalent to:
oabctl delete oabservice telegram-ingress-demo --cluster oab --namespace prod
  • Mutually exclusive with <resource>/<name> (clap's conflicts_with_all) —
    clear error if both are given, or if neither is given.
  • Accepts a file or a directory (same as apply -f), and correctly expands
    an OABFleet manifest into multiple services, deleting each in turn.
  • --cluster/--namespace are ignored when using -f — consistent with
    apply, which never reads a cluster from the manifest either (hardcoded to
    "oab"); namespace comes from each manifest's metadata.namespace.
  • Continues past an individual service's delete failure (e.g. one already
    gone) instead of aborting the whole batch, but still reports an overall
    error at the end listing which ones failed.
  • No duplicated delete logic — reuses the existing load_manifests/
    parse_manifest_file parsing (now pub(crate), previously private to
    apply.rs) and the existing per-service delete::run().
Before:
  oabctl delete oabservice telegram-ingress-demo --cluster oab --namespace prod
  (must know/type the resource type, exact name, cluster, and namespace)

After:
  oabctl delete -f /tmp/oab-telegram-ingress/manifest.yaml
  (same file used for `apply -f` — name/namespace read straight from it,
   cluster is oab same as apply)

Testing done

cargo build --release                        # clean
cargo clippy --all-targets -- -D warnings    # clean
cargo test --release                         # 33 passed; 0 failed (no regressions)

No new pure logic to unit test here — this is CLI wiring plus reuse of
already-tested parsing/delete paths, matching the pattern of apply::run/
delete::run themselves (neither has direct unit tests either; only their
pure helper functions do).

Live E2E: ran oabctl delete -f /tmp/oab-telegram-ingress/manifest.yaml
against a real running service. Output was identical to the equivalent
oabctl delete oabservice <name> --cluster oab --namespace <ns> invocation
— scaled to 0, deleted the ECS service, tore down ingress wiring (Cloud Map
service, HTTP API route/integration), cleaned up S3 manifest/config
artifacts. Confirmed via aws ecs describe-services that the service
transitioned to DRAINING afterward. Also verified: the traditional
<resource> <name> form still works unchanged, and both new validation
error paths (missing args, -f combined with <resource>/<name>) produce
clear error messages.

Also updates docs/oabctl.md's commands table and the ingress teardown note
to mention the new flag.

Update: upfront resource plan before apply/delete -f

Added a second commit: both apply -f and this PR's new delete -f now
print an upfront Plan: listing every resource that manifest may touch, as
an unchecked [ ] list, before any AWS calls happen — then each step's
existing / confirmation line follows with the real detail (resource
ID, ARN, URL) as it actually completes:

Plan:
  telegram-ingress-demo:
    [ ] S3 manifest
    [ ] ECS task definition
    [ ] ECS service
    [ ] Cloud Map namespace + service
    [ ] VPC Link
    [ ] API Gateway HTTP API
    [ ] API Gateway integration
    [ ] API Gateway route: /webhook/telegram
    [ ] API Gateway stage
    [ ] Security group inbound rule
    [ ] Telegram webhook registration

  Applying telegram-ingress-demo (ECS)...
  ✓ Cloud Map namespace exists: oab-vpc-1f5b7e78
  ✓ Cloud Map service exists: oab-prod-telegram-ingress-demo.oab-vpc-1f5b7e78
  ...

This is the terraform-style "plan, then confirm as it happens" pattern —
deliberately not a true in-place terminal checklist redraw (which needs ANSI
cursor control and breaks when output is piped, redirected, or viewed in CI
logs). Stays plain sequential println!, safe everywhere.

The plan is derived purely from each manifest's own fields (spec.ingress
presence/paths, spec.secrets keys) — read-only, informational, never
blocks execution, and doesn't try to predict create-vs-update or
skip-vs-act decisions made at runtime by the existing logic. The already
existing / lines remain the single source of truth for real detail;
this only adds the upfront preview on top.

The imperative delete oabservice <name> form has no manifest to derive a
plan from, so it's unaffected — only the -f paths get a plan.

Re-verified live: ran both apply -f and delete -f again against the
real manifest with this change — plan prints correctly, followed by the
same detailed confirmations as before.

…y -f

kubectl has a well-established, documented convention: 'kubectl apply -f'
and 'kubectl delete -f' both accept the same manifest file, so the exact
same file that created a resource can delete it — no need to separately
remember/type the resource type and name. oabctl's apply already followed
this ('apply -f <file>'), but delete required the imperative
'<resource> <name>' form, with no file-based option at all.

Add '-f/--file' to 'oabctl delete', accepting the same manifest file or
directory apply -f does (including OABFleet, which expands to multiple
services - all get deleted in turn, continuing past individual failures
so one broken/already-gone service doesn't block cleanup of the rest).
Mutually exclusive with <resource>/<name> (clap's conflicts_with_all).
--cluster/--namespace are ignored when using -f, since apply itself
never reads cluster from a manifest either (it's hardcoded to "oab");
namespace comes from each manifest's metadata.namespace instead, same
as apply.

Reuses the existing load_manifests/parse_manifest_file parsing (now
pub(crate)) and the existing per-service run() logic - no duplicated
delete logic between the two entry points.

Testing: cargo build/clippy --all-targets -D warnings/test clean on
macmini (33/33 tests pass, no regressions - no new pure logic to unit
test here, this is CLI wiring + reuse of existing tested paths).
Live E2E: ran 'oabctl delete -f /tmp/oab-telegram-ingress/manifest.yaml'
against a real running service - correctly scaled to 0, deleted the
ECS service, tore down ingress wiring (Cloud Map, HTTP API route/
integration), and cleaned up S3 manifest/config artifacts, identical
to the equivalent 'oabctl delete oabservice <name> --cluster oab
--namespace <ns>' invocation. Confirmed the traditional <resource>
<name> form and the new mutual-exclusion validation both still work
correctly.
@chaodu-agent chaodu-agent requested a review from thepagent as a code owner July 4, 2026 21:42
@chaodu-agent

This comment has been minimized.

Mirrors the terraform-style 'plan then confirm as it happens' pattern
(not a true in-place terminal checklist redraw, which requires ANSI
cursor control and breaks when output is piped/redirected/viewed in CI
logs - this stays plain sequential println!, safe everywhere):

  Plan:
    telegram-ingress-demo:
      [ ] S3 manifest
      [ ] ECS task definition
      [ ] ECS service
      [ ] Cloud Map namespace + service
      [ ] VPC Link
      ...

  Applying telegram-ingress-demo (ECS)...
  ✓ Cloud Map namespace exists: oab-vpc-1f5b7e78
  ✓ Cloud Map service exists: oab-prod-telegram-ingress-demo.oab-vpc-1f5b7e78
  ...

The plan is derived purely from each manifest's own fields (spec.ingress
presence/paths, spec.secrets keys) — it's read-only and informational,
never blocks execution, and doesn't need to predict create-vs-update or
skip-vs-act decisions made at runtime. The existing ✓/⚠ lines already
printed by apply_ecs/ensure_gateway/delete::run as each step actually
completes remain the single source of truth for real detail (resource
IDs, ARNs, URLs) — this only adds the upfront preview on top.

Applies to both  and the new  (#1296) - the
imperative  form has no manifest to derive a
plan from, so it's unaffected.

Testing: cargo build/clippy --all-targets -D warnings/test clean on
macmini (33/33 tests pass, no regressions - purely additive println
output, no logic changed). Live E2E: ran both  and
 against the real manifest, confirmed the plan prints
correctly before each step, followed by the existing detailed
confirmation lines as each resource actually gets created/removed.
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

LGTM ✅ — Clean implementation of delete -f that correctly mirrors apply -f by reusing existing manifest parsing and per-service delete logic, now with an upfront resource plan for both commands.

What This PR Does

Adds -f/--file support to oabctl delete, allowing teardown of services using the same manifest file that deployed them — eliminating the need to manually specify resource type, name, cluster, and namespace. Also introduces a terraform-style "Plan:" preview for both apply -f and delete -f.

How It Works

  • Reuses apply::load_manifests (visibility widened to pub(crate)) to parse the manifest file/directory
  • New delete::run_from_file() iterates over parsed manifests, calling existing delete::run() for each service
  • conflicts_with_all on clap ensures mutual exclusivity with positional <resource> <name> form
  • Continues past individual delete failures (graceful fleet teardown), reports aggregate error at the end
  • Both apply -f and delete -f now print an upfront Plan: listing derived from manifest fields before any AWS calls
  • Cluster hardcoded to "oab" matching apply behavior; namespace read from each manifest

Findings

# Severity Finding Location
1 🟢 Code reuse via pub(crate) avoids duplicating manifest parsing logic apply.rs:103
2 🟢 Graceful fleet deletion — continues past failures, summarizes at end delete.rs:22-31
3 🟢 Upfront plan preview is informational-only, safe for pipes/CI (no ANSI) apply.rs:103-133, delete.rs:37-55
4 🟢 resource/name made Option<String> with .context() for clear errors main.rs:57-58,137-139
5 🟢 Docs updated consistently (commands table + ingress teardown note) docs/oabctl.md
What's Good (🟢)
  • No duplicated logic: leverages existing load_manifests + delete::run() rather than reimplementing parsing or delete orchestration
  • conflicts_with_all: clap enforces mutual exclusivity at parse time, producing a clear error message without custom validation code
  • Graceful batch semantics: one already-deleted service in a fleet doesn't abort cleanup of the rest
  • Consistent UX: -f accepts file or directory, same as apply; cluster/namespace semantics match exactly
  • Plan preview: terraform-style upfront listing gives the operator visibility into what's about to happen, without blocking execution or requiring terminal capabilities
  • Smart conditional plan items: telegram webhook only shown when path + secret both exist in manifest
  • Live E2E tested against a real deployment, verifying full teardown lifecycle
Baseline Check
  • PR opened: 2026-07-04
  • Main already has: oabctl delete oabservice <name> (imperative form only), load_manifests (private to apply.rs)
  • Net-new value: file-based delete (-f), fleet expansion support for delete, pub(crate) visibility for manifest loading, upfront resource plan for both apply -f and delete -f

CI note: The operator Rust build is still in-progress at review time. All other checks (changes, check, poll-and-review) pass. No code issues found — clean pass assuming CI completes green.

Per feedback: the upfront plan is meant to list AWS resources apply
will touch, not the best-effort Telegram setWebhook call after them -
drop it from print_plan. The actual registration logic and its
existing '✓ Telegram webhook registered: ...' confirmation line are
untouched.
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

LGTM ✅ — Clean, well-scoped feature addition that mirrors an established kubectl convention.

What This PR Does

oabctl delete previously only supported the imperative <resource> <name> form. This PR adds -f/--file to accept the same manifest file used by apply -f, so teardown doesn't require separately remembering resource types, names, clusters, and namespaces.

How It Works

  • Adds --file (short -f) to the Delete subcommand, mutually exclusive with positional resource/name via clap's conflicts_with_all
  • run_from_file() reuses the existing load_manifests/parse_manifest_file parsing (promoted to pub(crate)) and the existing per-service delete::run() — no duplicated delete logic
  • Continues past individual failures (e.g. already-deleted services) and reports aggregate error at the end
  • A second feature adds an upfront Plan: preview (terraform-style) to both apply -f and delete -f, derived purely from manifest fields — informational only, plain println!, safe in pipes/CI

Findings

# Severity Finding Location
1 🟢 Excellent code reuse — promotes existing parsing to pub(crate) rather than duplicating apply.rs:104
2 🟢 Graceful degradation — continue on individual delete failure with aggregate reporting delete.rs:33-36
3 🟢 Clear mutual exclusion via conflicts_with_all + .context() error messages main.rs:63, 141-143
4 🟢 Plan preview is additive-only — no logic changes, no blocking, safe everywhere apply.rs:103-130, delete.rs:40-60
5 🟢 Docs updated in the same PR with both the commands table and the ingress teardown note docs/oabctl.md
Baseline Check
  • PR opened: 2026-07-04
  • Main already has: delete oabservice <name> (imperative only), apply -f (file-based)
  • Net-new value: -f flag for delete (mirrors apply -f), upfront plan preview for both -f paths, load_manifests promoted to pub(crate) for cross-module reuse
What's Good (🟢)
  • Follows the kubectl convention exactly — users familiar with kubectl delete -f will have zero learning curve
  • No duplicated logic between the two delete entry points
  • OABFleet expansion handled correctly (same path as apply)
  • Error reporting is user-friendly: continues the batch, reports failures at end
  • Plan output is deliberately plain text (no ANSI cursor tricks) — works in pipes, CI logs, and redirected output
  • 4 clean commits with logical separation (feature → docs → enhancement → fix)

Per feedback across several iterations on the upfront plan/checklist
feature: removed the [+]/[~]/[-] plan preview and the ✅ per-item
confirmation lines entirely from both apply -f and delete -f — the
existing ✓-style lines already printed as apply_ecs/ensure_gateway
run were sufficient, and the plan/checklist added complexity without
enough payoff (an earlier in-place-terminal-redraw attempt was also
reverted after testing found it genuinely broken with pyte).

Instead, made --wait's stabilization polling actually informative:
previously it printed nothing between "Waiting for X to stabilize..."
and "X is stable" even on a multi-minute deploy. Now prints each
poll's real state as a composite status string, reusing the exact
vocabulary ecsctl itself already uses for get/alias ls
(github.com/oablab/ecsctl, src/alias.rs) - RUNNING, REPLACING(n->m)
(new deployment's tasks still coming up), DRAINING(n+m) (new
deployment up, old one still stopping), PENDING(n), PARTIAL(n/m), or
the raw ECS service status as a fallback - instead of raw
running_count/rollout_state fields.

Testing: cargo build/clippy --all-targets -D warnings/test clean on
macmini (33/33 tests pass, no regressions). Live E2E: ran apply -f
against the real manifest, observed the full real transition sequence
over a 160s rollout - DRAINING(0+1) -> REPLACING(0->1) ->
DRAINING(1+1) -> DRAINING(1+0) -> RUNNING - matching the actual old
task draining while the new one started, then the old task fully
stopping before settling.

Also documents (unrelated to this diff, discovered along the way): on
this machine, macOS 26.5.1 (25F80) kills freshly-scp'd ad-hoc-signed
binaries with SIGKILL (Code Signature Invalid) before they can even
run - `codesign --force --sign - <binary>` after copying resolves it.
Confirmed via crash report analysis (namespace CODESIGNING, dying in
dyld's mach_o::UnsafeHeader::isMachO before main()) and cross-checked
that the same binary runs fine directly on macmini - not a build or
code issue, a local re-signing step this specific machine now needs.
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

LGTM ✅ — Clean, well-structured addition of delete -f that correctly reuses existing infrastructure without duplication, plus a useful wait_for_stable UX improvement.

What This PR Does

Adds -f/--file to oabctl delete, mirroring the existing apply -f convention so teardown uses the same manifest that deployed a service — no need to manually recall resource types, names, clusters, and namespaces. Also enhances wait_for_stable to print real ECS deployment state transitions during --wait instead of silence.

How It Works

  • delete::run_from_file() reuses the existing load_manifests() parser (now pub(crate)) and delegates to the existing delete::run() per service — zero duplicated delete logic.
  • Clap's conflicts_with_all enforces mutual exclusivity between -f and positional <resource> <name> at the arg-parsing level; the else branch uses anyhow::Context on the Options for a clear runtime error if neither path is given.
  • Fleet manifests expand to multiple services, with continue-on-failure semantics: individual failures don't abort the batch, but all are reported at the end.
  • wait_for_stable now prints a composite status string (RUNNING, REPLACING, DRAINING, PENDING, PARTIAL) using the same vocabulary as ecsctl, providing visibility into multi-minute deploy rollouts.

Findings

# Severity Finding Location
1 🟢 Excellent reuse of existing load_manifests + delete::run() — no duplicated logic between the two entry points operator/src/delete.rs:16
2 🟢 Continue-on-failure with aggregate error reporting — correct UX for fleet teardown where one service being already-gone shouldn't block the rest operator/src/delete.rs:27-35
3 🟢 Status vocabulary matching ecsctl's own composite states makes the CLI consistent across tools operator/src/apply.rs:611-638
4 🟢 Good docs update — commands table and teardown section both updated to reflect the new flag docs/oabctl.md
What's Good (🟢)
  • Design discipline: the PR deliberately does not duplicate delete logic — both paths converge on the same delete::run(), keeping one source of truth for service teardown.
  • Error UX: mutual-exclusion is enforced at two levels (clap conflicts_with_all for direct conflict, anyhow::Context for the "neither provided" case), both giving clear messages.
  • Operational visibility: the wait_for_stable enhancement fills a real gap — previously the user saw nothing during a multi-minute rollout. Now transitions like DRAINING(1+1) → REPLACING(0→1) → RUNNING are visible in real time.
  • Documentation kept in sync with the code change.
Baseline Check
  • PR opened: 2026-07-04
  • Main already has: oabctl delete oabservice <name> (imperative form only), apply -f with file/directory support, load_manifests/parse_manifest_file in apply.rs (private)
  • Net-new value: file-based delete (-f), visibility change on manifest parser to enable reuse, and informative wait_for_stable status reporting

@thepagent thepagent merged commit 47129e0 into main Jul 5, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants