Skip to content

fix(services): derive commandArgv when an editor sends only the text command - #574

Open
AbdullahM07 wants to merge 1 commit into
oblien:mainfrom
AbdullahM07:fix/service-command-argv
Open

fix(services): derive commandArgv when an editor sends only the text command#574
AbdullahM07 wants to merge 1 commit into
oblien:mainfrom
AbdullahM07:fix/service-command-argv

Conversation

@AbdullahM07

Copy link
Copy Markdown
Member

Fixes #573.

What was broken

#332 made commandArgv the argv Openship hands Docker, keeping ["sh","-c",<command>] only as the fallback for legacy rows that have no argv at all. Neither service editor derives that argv, and the dashboard's settings form sends the one-line command field alone — so the two columns drift apart:

  • updateService writes the new command and leaves the row's previous argv in place (patch = { ...data }, command gets trimOrNull, and repos.service.update is a raw set). toComposeSpec backfills only a null argv, so the stale one survives the deploy-time syncFromCompose and reconcileFromCompose keeps "ours" for an edited row. The container goes on running the old command while the UI shows the new one, clearing the field never restores the image CMD, and composeSpecDiff reports phantom commandArgv drift on a service the operator edited to match its own compose file.
  • createService stores a text command with commandArgv: null — the app installer forwards a template's string command through here (13 catalog services declare one). That null is covered by the backfill, so it is not a live sh -c regression today; I traced build-pipeline.tssyncFromCompose → the pipeline's fresh listByProject read and confirmed the first deploy repairs it. Included anyway so the row is written consistently instead of depending on a repair step.

The compose-file path was already correct — shellSplitWords, commandToArgv and resolveComposeCmd all behave as documented. This is only the API write path.

The change

Derive the argv from the text command in both editors when the caller didn't send one — in updateService alongside the existing trim, so the two columns can't be written out of sync, and in createService at the insert. An explicitly-sent commandArgv stays authoritative, [] (clear the image CMD) included, so argv-aware callers — the compose parser, the CLI, a snapshot replay — are never second-guessed and a quoted sh -c "…" argv is never re-split.

Two derivation lines plus a regression test; no schema, endpoint or runtime change.

How I verified it

apps/api/test/modules/services/service-command-argv.test.ts drives the real createService / updateService against mocked repos and asserts the patch that reaches the DB.

On main, 3 of the 4 assertions fail:

× re-derives the argv when an edit sends only the text command
  → AssertionError: expected undefined to deeply equal [ 'ak', 'worker' ]
× clears the argv when an edit clears the command
  → AssertionError: expected undefined to be null
× derives the argv on create instead of storing none
  → AssertionError: expected null to deeply equal [ 'ak', 'worker' ]
✓ keeps an explicit argv over the text command

With the change, all 4 pass. The fourth passes both ways on purpose — it pins the boundary, since the tempting wrong implementation (always re-split the text) would mangle an explicit ["sh","-c","echo hi && ak server"].

Also run:

  • bun run --cwd apps/api test — 328/333 files pass on the branch. The 5 that fail are pre-existing on this machine and unrelated: the same suite on clean main fails 6 files (a superset), always the whole-app route/permission builders (app.routes, collection-project-routes, env-reveal-gating, pending-actions-tools, service-exec-isolation) plus container-events, all on 10–20s timeouts, and one surfacing INTERNAL_TOKEN is required when DEPLOY_MODE="docker" from my local .env. Their membership shifts run to run while the diff stays constant. test/modules/services/ — the module this change touches, service-exec-isolation included — passes 5/5 files, 42 tests, plus the 4 new ones.
  • bun run --cwd apps/api linttsc --noEmit clean.
  • bunx prettier --check on both touched files. The new test file is clean. service.service.ts was already Prettier-dirty on main (54 hunks — long single-line imports), so per CONTRIBUTING I left that drift alone rather than reformatting lines this change doesn't need; the added lines match the surrounding style.

Not in this PR

entrypoint: is still listed in UNSUPPORTED_SERVICE_KEYS — a compose file that overrides the entrypoint gets a warning and is then ignored. Noted in #573 as a separate gap.

…command

oblien#332 made `commandArgv` the argv Openship hands Docker, keeping
`["sh","-c",<command>]` only as the fallback for legacy rows that have no argv.
Neither service editor derives it, and the dashboard's settings form sends the
one-line `command` field alone, so the two columns drift apart.

updateService writes the new `command` and leaves the row's PREVIOUS argv in
place. `toComposeSpec` backfills only a null argv, so the stale one survives the
deploy-time syncFromCompose and reconcileFromCompose keeps "ours" for an edited
row: the container goes on running the old command while the UI shows the new
one, clearing the field never restores the image CMD, and composeSpecDiff
reports phantom `commandArgv` drift on a service the operator edited to match
its own compose file.

createService stores a text command with a null argv — the app installer
forwards a template's string command through here. That null IS covered by the
deploy-time backfill, so it is not a live regression; derived at the insert
anyway so the row is written consistently rather than depending on a repair
step.

Derive the argv alongside the existing trim in both editors. An explicitly-sent
argv stays authoritative, `[]` included, so the compose parser, the CLI and a
snapshot replay are never second-guessed and a quoted `sh -c "…"` argv is never
re-split. Routing-only callers are untouched — the update path keys off
`"command" in patch`.

Three of the four new assertions fail on main (stale argv, uncleared argv, null
argv on create) and pass with this change. `tsc --noEmit` clean for apps/api.
The file's pre-existing Prettier drift is left alone.

Closes oblien#573
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.

Editing a compose service's command in the dashboard leaves command_argv stale, so the container keeps running the old command

1 participant