feat(0.27.0): data-app command group -- first-class lifecycle for Keboola data apps - #237
Conversation
e257b73 to
738eff5
Compare
|
Iteration 3 (post-PR review loop) -- amended. Two independent reviewers walked the playbook with fresh context: Code review: 0 BLOCKING / 6 NON-BLOCKING / 5 NIT. Material non-blockers fixed in the amended commit:
3 new service-level tests cover the new validators ( |
738eff5 to
f39d9e1
Compare
|
Iteration 4 (post-PR convergence review) -- amended. A fresh-context reviewer caught one BLOCKING and several non-blockers I missed. Material findings fixed:
|
f39d9e1 to
ad355a8
Compare
|
Iteration 5 (post-CI convergence review) -- amended. Iteration-5 reviewer caught a sibling defect to the iteration-4 BLOCKING: the Fixed:
|
|
Convergence reached — review loop closed. Six independent review iterations on this branch (2 pre-PR + 4 post-PR), each with a fresh-context reviewer, each given the full diff and the security checklist. Final iteration: zero material findings in either CODE or SECURITY. Loop summary:
The two iter-4/5 BLOCKINGs were exactly the kind of bugs review loops are for: real, user-visible defects that all 5 prior reviewers missed because the test suite only covered the happy path. The fix is now defended by a parametrized compile-check that runs Final state:
Ready for human review. |
padak
left a comment
There was a problem hiding this comment.
Verdict
Mechanically clean — full CONTRIBUTING.md plugin-sync map walked correctly. The 4-footgun encapsulation is the headline value of this PR and it is implemented, tested, AND documented end-to-end. Ready to merge from a code-review standpoint.
Two extras worth landing before / shortly after merge:
Asks
docs/TUTORIAL.md extension — new §9 "Data apps lifecycle"
The TUTORIAL is the second onboarding doc after the README. §8 already covers "Advanced storage: native column types"; §9 should mirror the same shape for data apps so a first-time user can go from kbagent project add to a running data app without leaving the TUTORIAL:
- §9.0 — one-paragraph framing of the 4 footguns the CLI encapsulates (§9 redeploy contract, per-project KMS, cleanup-in-finally, transient-
stoppedpoll). Concise: this is the headline of the feature; users skim TUTORIAL for the "why". - §9.1 — Public-repo golden path:
data-app create --git-public --no-deploy→data-app deploy --wait. Explain the auto-injectedparameters.idquirk if relevant. - §9.2 — Private-repo golden path:
--git-pat-env VARwith the per-project-KMS round-trip mention (no plaintext on argv, no cross-project ciphertext). - §9.3 — The "deploy-after-
config update" gotcha: Storage bumps the version, but the deployment record'sconfigVersionis a pinned pointer.kbagent data-app deployis the rollout step;data-app startonly wakes a parked container. - §9.4 —
stop(reversible, zero-cost wake) vsdelete(cascades to Storage config; URL retired). - §9.5 — (optional) Demo GIF in the same style as §8's
demo-storage-types.gif.
README.md — small but visible
Neither README nor TUTORIAL is CI-checked, but they are the first impressions for GitHub visitors:
- "What it does" table — new Data apps row between Workspaces and Sharing. One sentence highlighting the lifecycle + 4 footguns.
- "All commands" code block — add the line:
kbagent data-app list | detail | create | deploy | start | stop | delete | password
NIT (defer if you like)
--hint client data-app deploywithout--config-version— rendered Python emitsconfig_version=None, which the deploy endpoint rejects with HTTP 422. The service-hint resolvesNoneat runtime correctly; only the client-hint paste-and-run path hits the wall. Suggest emitting<resolved-from-storage-config>placeholder text + a comment in the rendered snippet so a user knows to fill it in.data_app_service.pyis 1243 lines — three logical sections (lifecycle methods / redaction helpers / validation) split into separate files would help future maintainers. Not a 0.27.0 blocker; raise the discussion if the file grows further (e.g. when--reload-on-configor sync-pull/push for data apps lands).- If TUTORIAL §9 lands, mirror the link from
docs/use-cases.mdfor docs-index consistency.
What I verified
- Full diff against CONTRIBUTING.md plugin synchronization map (every silent-drift surface touched: AGENT_CONTEXT, CLAUDE.md, keboola-expert.md Rule 6 + matrix + inline gotchas, SKILL.md description triggers + workflow link, commands-reference.md, gotchas.md with 3×
(since v0.27.0)tags, newdata-app-workflow.md). - 4-footgun encapsulation:
- §9 trio sent together at
data_app_service.py:506-510(create) and:617-621(deploy). - KMS round-trip prefix-check at
:1001-1009(3 ciphertext prefixes whitelisted, refuses plaintext). - Cleanup-in-finally at
:430-558with--keep-on-failureopt-out + dedicated test. - Transient-
stoppedpoll-loop at:1140-1189withtest_stopped_is_not_terminal_during_initial_deploycovering the create→stopped→starting→running sequence.
- §9 trio sent together at
- 70 unit tests (39 service + 10 CLI + 16 parametrized hint compile-checks via
ast.parse) + 2 E2E gated onE2E_DATA_APP_GIT_REPO_PUBLIC/_PRIVATEwith autouse cleanup fixture. - Security: Manage token never persists on
DataScienceClient(per-callheaders=kwarg only);_redact_storage_configcovers all echo sites; length + control-char + URL-scheme defence-in-depth at service boundary;--git-pat-encryptedprefix tightened to 3 specific KMS prefixes (KBC::ProjectSecure::,KBC::ProjectSecureGKMS::,KBC::ProjectSecureKMS::).
ad355a8 to
0b96ccd
Compare
|
Docs amendment per the maintainer review — TUTORIAL §9 + README rows.
Demo GIF deferred — Padak's review marked it as (optional), and recording it cleanly requires picking a small public Node repo for §9.1; will land as a polish follow-up. Padak's nit on
|
…oola data apps
Adds a `kbagent data-app` group covering create, list, detail, deploy,
start, stop, delete, and password retrieval for `keboola.data-apps` +
the Data Science API `/apps`. The CLI encapsulates the documented
footguns so callers cannot hit them:
- §9 redeploy contract: `data-app deploy` always sends the
`{desiredState=running, configVersion, restartIfRunning=true}` trio
together. Sending bare `desiredState=running` (the obvious shape)
silently pins to the empty-shell v2 from `POST /apps`; the runner
then errors `dataApp.git.repository is required in /data/config.json`
with no top-level error surfaced.
- Per-project KMS encryption: `data-app create` always re-encrypts the
plaintext PAT via the target project's Encryption API. Refuses to
write plaintext if the round-trip does not return a `KBC::Project*`
ciphertext. Ciphertext does not cross projects.
- Cleanup-in-finally: if the Storage PUT or initial deploy fails after
`POST /apps`, the orphan shell is deleted unless `--keep-on-failure`.
- Poll loop: `state == stopped` is NOT terminal while
`desiredState == running` -- the platform transitions
`created -> stopped -> starting -> running` during initial deploy.
Layering follows the project convention (commands -> services ->
clients) and mirrors workspace.py end-to-end:
- Client: new `DataScienceClient` (`data_science_client.py`) inheriting
`BaseHttpClient`. URL derived as `data-science.<stack-suffix>` from
the connection URL. `get_app_password` accepts the Manage token
per-call so it never lives on the persistent client.
- Service: `DataAppService` (`services/data_app_service.py`) accepts
both a Storage-client factory and a Data-Science-client factory plus
the existing `EncryptService`. One method per CLI subcommand.
- Commands: `commands/data_app.py` -- thin Typer subcommands; mutual
exclusion validation for git auth modes; dual JSON / human output;
confirmation prompt on delete.
- Permissions: 8 entries added to OPERATION_REGISTRY (read for list /
detail / password; write for create / deploy / start / stop;
destructive for delete). `data-app.password` is `read` for parity
with the existing `workspace.password`.
- Errors: 3 new `ErrorCode` members (`DATA_APP_BUILD_FAILED`,
`DATA_APP_DEPLOY_TIMEOUT`, `DATA_APP_INVALID_GIT`); existing codes
cover the rest.
- Hints: new `data_science` `client_type` in the renderer. Hint mode
generates `DataScienceClient` instantiation + the §9 trio inline.
Tests: 30 service-level tests (validation, dry-run, happy path,
cleanup-in-finally, encryption-failure-aborts-loud, poll-loop semantics
including the transient-stopped invariant), 10 CLI tests (mutual
exclusion, dual output, manage-token forwarding without leaking the
token to stdout), 2 E2E tests gated on
`E2E_DATA_APP_GIT_REPO_PUBLIC` / `E2E_DATA_APP_GIT_REPO_PRIVATE`.
Sync map: pyproject 0.27.0; changelog entry; AGENT_CONTEXT block;
CLAUDE.md `## All CLI Commands`; keboola-expert.md matrix (5 rows) +
inline gotcha + version-gate bump; SKILL.md description triggers +
workflow link + auto-table regen; commands-reference; gotchas.md (two
`(since v0.27.0)` entries -- the redeploy contract and the
cross-project KMS); new data-app-workflow.md; plugin.json /
marketplace.json synced via `make version-sync`.
Bump 0.26.0 -> 0.27.0 (minor): new top-level command group + new
underlying API surface (Data Science).
0b96ccd to
bf20800
Compare
|
Post-amend security review caught a BLOCKING issue + cleanup pass — fixed. Two independent fresh-context reviewers (one CODE, one SECURITY) walked the docs amendment. Findings: SECURITY BLOCKING (fixed):
SECURITY NON-BLOCKING (fixed):
CODE NON-BLOCKING (fixed):
CODE NIT (fixed):
|
|
Convergence reached on the docs amendment. Iteration 2 (fresh-context reviewer over the sanitized diff Verifications:
Note: the reviewer also flagged that other unrelated real-looking IDs exist in PR ready for human review. |
Summary
Adds a new
kbagent data-appcommand group covering the full lifecycle of Keboola data apps (keboola.data-appsStorage component + Data Science API/apps). Eight subcommands:list,detail,create,deploy,start,stop,delete,password.The CLI encapsulates four documented footguns so callers cannot hit them:
data-app deployalways sends the{desiredState=running, configVersion, restartIfRunning=true}trio together. Sending baredesiredState=running(the obvious shape) silently pins to the empty-shell v2 fromPOST /apps; the runner then errorsdataApp.git.repository is required in /data/config.jsonwith no top-level error surfaced -- only visible in the UI's Terminal Logs.data-app createre-encrypts the plaintext PAT under the target project's KMS via the Encryption API. Refuses to write plaintext if the round-trip does not return a project-scoped (KBC::ProjectSecure*) ciphertext. Ciphertext does not cross projects.POST /apps, the orphan shell is deleted automatically (override with--keep-on-failurefor forensics).state == stoppedis NOT terminal whiledesiredState == running. The platform transitionscreated → stopped → starting → runningduring initial deploy; a naive poll exits prematurely.Bumps
0.26.0→0.27.0(minor — new top-level command group + new API surface).Architecture
Three-layer split mirrors
workspace.pyend-to-end:src/keboola_agent_cli/data_science_client.py(new; inheritsBaseHttpClient; URL derived asdata-science.<stack-suffix>;get_app_passwordaccepts the Manage token per-call so it never lives on the persistent client)src/keboola_agent_cli/services/data_app_service.py(new)src/keboola_agent_cli/commands/data_app.py(new)Other surfaces:
OPERATION_REGISTRY(read for list / detail / password; write for create / deploy / start / stop; destructive for delete).data-app.passwordisreadfor parity withworkspace.password.ErrorCodemembers:DATA_APP_BUILD_FAILED,DATA_APP_DEPLOY_TIMEOUT,DATA_APP_INVALID_GIT.data_scienceclient_typeplumbed through the hint renderer;--hint client data-app deploy ...generatesDataScienceClientinstantiation + the §9 trio inline.name,description,git_repo,git_branch,git_usernameso external callers using the service directly cannot smuggle giant payloads or NULs into the audit log.Live-validation receipts
Replayed the recipe end-to-end against
https://connection.us-east4.gcp.keboola.com(project 5726, admin):data-app list --project cuesta-training-admin[APP01](id 43661270, configVersion=3);errors: []data-app detail --app-id 43661270<encrypted>;config_version_storage=3 == config_version_deployed=3data-app create --git-public --auth public --no-deployparameters.id) → 3 (full PUT)data-app deploy --app-id 43661325https://kbagent-livetest-pub-43661325.hub.us-east4.gcp.keboola.com;desiredState=running, configVersion=3, restartIfRunning=true(the §9 trio)data-app stop --app-id 43661325state=stopped, desiredState=stoppeddata-app delete --app-id 43661325 --yesFinal cleanup verified:
data-app listshows only the original[APP01]. Zero leaked apps, zero leaked Storage configs.Tests
tests/test_data_app_service.py(validation, dry-run, happy-path orchestration, cleanup-in-finally, encryption-failure-aborts-loud, poll-loop semantics including the transient-stopped invariant, length / control-char bounds, redaction helpers).tests/test_data_app_cli.py(mutual-exclusion validation, dual JSON+human output,--yesfor delete, manage-token forwarding without leaking the token to stdout/stderr).tests/test_e2e.py::TestE2EDataAppLifecycle(public-repo lifecycle, private-repo + redeploy contract). Gated onE2E_DATA_APP_GIT_REPO_PUBLIC/E2E_DATA_APP_GIT_REPO_PRIVATEenv vars.make check: 2,411 passed, 5 skipped. Lint, format, skill-gen, version-sync, changelog-check, error-code lint all clean.Plugin synchronization map walk
All silent-drift surfaces from
CONTRIBUTING.md:251-272updated:pyproject.toml0.26.0 → 0.27.0src/keboola_agent_cli/changelog.py— new0.27.0entrysrc/keboola_agent_cli/commands/context.py—AGENT_CONTEXTblock addedCLAUDE.md ## All CLI Commandsplugins/kbagent/.claude-plugin/plugin.json— auto-synced viamake version-sync.claude-plugin/marketplace.json— auto-syncedplugins/kbagent/agents/keboola-expert.md— 5 matrix rows, version-gate bump, inline gotcha for the §9 contract + cross-project KMSplugins/kbagent/skills/kbagent/SKILL.md— description triggers, workflow link, auto-table regenplugins/kbagent/skills/kbagent/references/commands-reference.mdplugins/kbagent/skills/kbagent/references/gotchas.md— two(since v0.27.0)entries (redeploy contract, cross-project KMS)plugins/kbagent/skills/kbagent/references/data-app-workflow.md(new)Self-review
Two independent reviewer iterations (general-purpose agents with full diff + security checklist) walked the playbook. Iteration 1: 0 BLOCKING / 2 NON-BLOCKING / 5 NIT in CODE; 0 BLOCKING / 1 NON-BLOCKING / 3 NIT in SECURITY. All material findings fixed:
get_app_passwordrefactored to inherit retry/backoff via_do_requestwith per-callheaders=name/description/git_repo/git_branch/git_username--git-pat-encryptedprefix tightened from anyKBC::Project*to the known list (KBC::ProjectSecure::,KBC::ProjectSecureGKMS::,KBC::ProjectSecureKMS::)_redact_storage_confighelper redacts encrypted PATs inraw.storage_configechoes tooIteration 2 verdict: CONVERGED — zero material findings in both code and security categories.
Test plan
make checkgreen locally (2,432 passed; 16 new parametrized hint compile-checks added in iter-5)connection.us-east4.gcp.keboola.com(project 5726): all 6 lifecycle scenarios verified, zero leaked artifactsTestE2EDataAppLifecycleran against the live admin project:test_data_app_lifecycle_public(id 43661328, configVersion=3, cleanup verified) andtest_data_app_lifecycle_private_and_redeploy(real KMS encryption of git PAT, §9 deploy contract executed, id 43661331, cleanup verified)ad355a8, 1m17s)What this PR deliberately does NOT cover
kbagent config update --component-id keboola.data-apps --config-id ID --set 'runtime.backend.size="medium"' --mergethendata-app deploy.PATCH /apps {config:{...}}is silently dropped by the API.app/{component_id}/{config_name}— manifest stub already exists; full sync support is a follow-up.