Repository navigation
docs(design-proposals): focus CLI proposal on cozyctl users - #51
myasnikovdaniil wants to merge 3 commits into
Conversation
Define separate operator and tenant command-line interfaces backed by shared Kubernetes client, discovery, schema, output, wait, and plugin modules. Document how cozypkg and check-readiness migrate into cozystackctl while remaining compatibility entry points, and how cozyctl discovers managed applications from the aggregated API and OpenAPI. Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
|
| Layer / File(s) | Summary |
|---|---|
CLI architecture and contracts design-proposals/cozystack-cli-interfaces/README.md |
Defines operator and tenant command trees, shared client and discovery modules, generic application workflows, capability extensions, configuration and output contracts, plugin execution rules, compatibility behavior, security constraints, failure handling, testing, rollout phases, and alternatives. |
Estimated code review effort: 2 (Simple) | ~10 minutes
🚥 Pre-merge checks | ✅ 5
-
✅ Passed checks (5 passed)
-
Check name Status Explanation Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request. Title check ✅ Passed The title clearly identifies the main documentation change: narrowing the CLI proposal to focus on cozyctl users. It is concise and specific. Description check ✅ Passed The description explains the scope, rationale, testing, downstream impact, and release-note status. It does not include the template's Before review section or explicitly state whether a decision reco…
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
proposal/cozystack-cli-interfaces
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
CodeRabbit Verify
- Run CodeRabbit Verify to test the changes in this pull request.
Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Record wrapping or reimplementing virtctl as a rejected alternative: it ties cozyctl to virtctl flags and KubeVirt API versions, locks the command tree to one virtualization backend, and conflicts with the rule against executing external binaries. Add an open question on how a user obtains the workload name to pass to type-specific tools, since ApplicationDefinition has no workload selector. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Reviewed as a cozystack platform-architecture and CLI-design pass. Platform claims below are checked against the code at commit 340562503.
The direction is right. One user-facing CLI, dynamic discovery over the aggregated API and ApplicationDefinition, server stays authoritative. Withdrawing the two-CLI approach is the correct call: installation and upgrade need durable in-cluster state, not a client binary standing in for the control plane. The catalog/app split, with the application type as an argument under stable commands, is the strongest part of the design and keeps help, completion, and scripts stable across clusters with different catalogs.
Four items block acceptance, then a set of gaps worth closing in the same revision.
Blocking
1. Open question #4 is under-researched, and "No downstream changes" is not accurate for any real answer to it.
The question frames the "workload behind an application" problem as a choice between a new selector field and the <prefix><app name> chart convention. The platform already has a label contract for this. The apiserver stamps every HelmRelease with apps.cozystack.io/application.{group,kind,name} (rest.go#L235-L237), the lineage webhook propagates the same labels onto pods, secrets, services and PVCs (types.go#L14), and the apiserver resolves an application's resources by those labels, not by the name prefix (rest.go#L1151-L1159). The <prefix><name> form only computes the HelmRelease name; it is not the authoritative link.
The catch is that this label does not reach the Workload CR. Workloads copy only workloads.cozystack.io/* labels (workloadmonitor_controller.go#L39), so a client cannot filter cozystack.io/workloads by application.name. Resolving an app to its workloads today is a two-hop traversal (application labels to WorkloadMonitor, then workloads.cozystack.io/monitor to Workloads) that does not disambiguate instances in a shared namespace on its own. A clean answer to #4 therefore needs a server-side addition, most naturally a workloads selector on ApplicationDefinition, which also matches the workloads value already present in the DashboardTab enum (CRD#L104-L111). This is also where community#106 is heading: its fix for the dashboard knowing kinds is "a catalog declares the views it needs as data: typed tabs and actions and the icon in the AD". That same server-declared data is the right home for the workload link. Please either commit the proposal to that selector and correct "No downstream changes" in the description, or specify the traversal contract explicitly. Dropping <prefix><name> as the candidate also avoids baking a client-side heuristic the dashboard cannot share (point 5).
2. The tenant-selection model is an open question, but Rollout phase 2 already ships "tenant selection".
Open question #1 leaves unresolved whether a selected tenant is an XDG preference or a kubeconfig context and namespace. That is the center of the tool, not a detail. A parallel XDG store plus kubeconfig's own current-context is two sources of truth that drift: a user who switches context with kubectl keeps a stale cozyctl tenant. kubectl keeps a single current-context for exactly this reason. Resolve the model before phase 2, and prefer subordinating any stored tenant to kubeconfig with per-call validation over a free-standing store.
3. cozyctl app wait has no defined readiness contract, and the condition it will consume has a trap.
The aggregated Application surfaces Ready, Released, and WorkloadsReady, but Ready deliberately reflects HelmRelease state only and is not overridden by WorkloadsReady (rest.go#L1437, whose in-code rationale cites kubectl wait compatibility and startup-window false negatives). So wait has to state which condition it blocks on. Ready alone can return before workloads are up; WorkloadsReady can false-negative during startup. Name the exact done condition, and the behavior on WorkloadsReady=Unknown, in Design or the output contract. "readiness evaluation" in Testing is not a contract.
4. Scope omits the neighbours that define this CLI's foundation, and the CLI landscape around it.
"Scope and related proposals" cites #64 and #16 but not the proposals that shape the surface cozyctl consumes:
- community#106 (platform and catalogs split) is the ground this stands on. Its contracts K1-K6 are the seam between platform and catalogs; K1 "Core API for applications" is exactly the
ApplicationDefinitioncontractcozyctldiscovers through, and it movesopenAPISchemato admission-validated JSON Schema and validates ADs against platform N-1/N/N+1. After the split, applications move into separately versioned catalogs delivered throughPackageSourceandcozypkg, socozyctldiscovery must survive apps living out-of-tree. The upgrade/compatibility story here should align with K1's N-1/N/N+1 model rather than inventing its own. - community#39 (fold extra into apps) adds declarative
visibility,cardinality, andprotectioncapabilities toApplicationDefinition.catalog listandapp createmust honour them (hidden apps, one-per-tenant), and today the design does not mention them. - community#45 (application endpoints / EndpointAttachment) redesigns how endpoints are modeled, which the "find tenant-visible endpoints" workflow and
app resourcesdepend on.
Separately, situate cozyctl in the CLI landscape. cozypkg already exists as "A CLI for managing Cozystack packages" and is the admin/dev and marketplace-delivery tool (PackageSource, catalogs). Keeping it admin-facing and giving users a separate cozyctl is a reasonable split, but it is an unstated decision: add "extend cozypkg into a user CLI" as a rejected alternative and name the cozyctl (users) / cozypkg (packages, admin) / future cozystackctl (operator lifecycle) boundary, so the roles are clear to users and reviewers.
Should address (non-blocking)
5. State a "no client-side heuristics" invariant. The dashboard is cozystack-ui (TypeScript) and the CLI is Go, so they cannot share discovery or readiness logic in code. Their only shared contract is the aggregated API plus ApplicationDefinition plus OpenAPI. Anything both clients need (the app-to-workload link above, readiness semantics) has to live in server metadata, or the two will diverge. This is the same direction community#106 takes with "views as data", and making it an explicit invariant here settles #1, #4, and the UI-divergence risk together.
6. The Security section is thin on its own surface. The icon in ApplicationDefinition is inline SVG or base64 and is untrusted like the rest of the discovery metadata, but it is not in the list. The "retrieve an already-authorized tenant-visible Secret" path is a real exfiltration surface and deserves its own paragraph (what counts as tenant-visible, who authorizes), not one clause in the output contract. And client-side validation driven by server-published OpenAPI should be stated as a UX aid only, never authoritative, since the schema is untrusted input.
7. The output contract has two gaps. Exit codes are promised ("predictable exit codes", "exit-code mapping" in tests) but never enumerated; for a scripting-facing tool the code table is part of the contract (success, partial, forbidden, not-found, timeout). And "Mutating commands support confirmation, dry-run, waiting, and timeout flags where the API supports them" makes the flag surface vary by cluster, which contradicts the goal of scripts stable across clusters. Pick one: flags always present and degrading predictably, or an acknowledged cluster-dependent surface.
8. Two design areas are missing. There is no discovery cache: paying an API-discovery plus ApplicationDefinition plus OpenAPI round-trip on every invocation is slow, and the "kind disappears between discovery and execution" edge case already implies a cache without designing its invalidation. And there is no distribution-and-versioning section: how the binary ships (krew, brew, container, release artifact), how its version relates to the cluster version, and who maintains the compatibility matrix. Since the apps group is served at v1alpha1 only, "older API surfaces" in the compatibility tests means catalog composition and optional fields, not API versions, which is worth saying plainly.
9. The dynamic-discovery tests need a harness that does not exist yet. "Add and remove application kinds dynamically" and "newest client against older API surfaces" both need a live aggregated apiserver with a varying catalog. The current apps-aggregation tests are unit-level. Say how the fixture is built (envtest with CRDs, a fake aggregated API, or recorded discovery), because that harness is the costliest and least-specified part of the plan.
Minor
- The template gained a
## Decisionssection after this proposal was first written, so its absence is not a conformance violation. A refresh that adds the placeholder would still be good hygiene, given the proposal already went through one direction change. - "tenants and managed-service users" are named as one audience but never differentiated. A couple of lines on the two personas would clarify who phase 2 serves.
- Rollout is ordered by build-readiness, but
waitandresourcesare most of why a user reaches for this over kubectl. Consider ordering by user value.
None of this is a reason to drop the approach. It needs the tenant model and the workload-resolution path nailed down, the neighbouring proposals wired into scope, and the description's "No downstream changes" corrected to match.
What this PR does
This proposal now covers one user CLI,
cozyctl, for tenants and managed-service users. Previous operator CLI scope moved out, installation and upgrades need durable in-cluster API and controller, socozystackctlwill be designed separately as client of that API.cozyctlkeeps catalog discovery, generic application workflows, tenant selection and machine-readable output based on aggregated API,ApplicationDefinition, and OpenAPI.Testing
git diff --checkDownstream repositories
No downstream changes, this PR changes design only.