Conversation
be88a0d to
c4c2fc9
Compare
chore: fixup
767f200 to
34f8509
Compare
34f8509 to
f593d8f
Compare
a3a17fd to
9f60455
Compare
pkosiec
left a comment
There was a problem hiding this comment.
Nice job!
(There was a lot of code to review so I focused mostly on the end-user experience, and just checked it briefly 😄 )
|
|
||
| ## Plugin CLI | ||
|
|
||
| AppKit includes a CLI for managing plugins. All commands are available under `npx appkit plugin`. |
There was a problem hiding this comment.
We should use npx @databricks/appkit everywhere, to be consistent with the other docs (like llms.txt) and to ensure it always works, whenever it's ran outside scope of a project or inside of it
| AppKit includes a CLI for managing plugins. All commands are available under `npx appkit plugin`. | |
| AppKit includes a CLI for managing plugins. All commands are available under `npx @databricks/appkit plugin`. |
|
|
||
| For complete configuration options, see [`createApp`](api/appkit/Function.createApp.md). | ||
|
|
||
| ## Plugin CLI |
There was a problem hiding this comment.
The CLI is not only about plugins, right? So I'd suggest
| ## Plugin CLI | |
| ## Plugin management |
| @@ -219,13 +201,13 @@ | |||
| }, | |||
| "alias": { | |||
There was a problem hiding this comment.
BTW IMO much clearer label would be displayName - but not sure if that's the best moment to change it 😄
| }, | ||
| "additionalProperties": false | ||
| "additionalProperties": false, | ||
| "allOf": [ |
There was a problem hiding this comment.
Oh man, this is getting super complicated 🤔
We could use
"additionalProperties": true,
"oneOf": [
{
"properties": {
"type": { "const": "secret" },
"permission": { "$ref": "#/$defs/secretPermission" }
}
},
{
"properties": {
"type": { "const": "job" },
"permission": { "$ref": "#/$defs/jobPermission" }
}
}
// ... etc
]WDYT? is much simpler without ifs - but requires allowed additional properties (IMO that's not an issue as we validate the fields we need anyway)
| export const RESOURCE_TYPE_OPTIONS: ResourceTypeOption[] = [ | ||
| { value: "secret", label: "Secret" }, | ||
| { value: "job", label: "Job" }, | ||
| { value: "sql_warehouse", label: "SQL Warehouse" }, | ||
| { value: "serving_endpoint", label: "Serving Endpoint" }, | ||
| { value: "volume", label: "Volume" }, | ||
| { value: "vector_search_index", label: "Vector Search Index" }, | ||
| { value: "uc_function", label: "UC Function" }, | ||
| { value: "uc_connection", label: "UC Connection" }, | ||
| { value: "database", label: "Database" }, | ||
| { value: "genie_space", label: "Genie Space" }, | ||
| { value: "experiment", label: "Experiment" }, | ||
| { value: "app", label: "App" }, | ||
| ]; | ||
|
|
||
| /** All valid permissions per resource type, aligned with the schema if/then rules. */ | ||
| export const PERMISSIONS_BY_TYPE: Record<string, string[]> = { | ||
| secret: ["READ", "WRITE", "MANAGE"], | ||
| job: ["CAN_VIEW", "CAN_MANAGE_RUN", "CAN_MANAGE"], | ||
| sql_warehouse: ["CAN_USE", "CAN_MANAGE"], | ||
| serving_endpoint: ["CAN_QUERY", "CAN_VIEW", "CAN_MANAGE"], | ||
| volume: ["READ_VOLUME", "WRITE_VOLUME"], | ||
| vector_search_index: ["SELECT"], | ||
| uc_function: ["EXECUTE"], | ||
| uc_connection: ["USE_CONNECTION"], | ||
| database: ["CAN_CONNECT_AND_CREATE"], | ||
| genie_space: ["CAN_VIEW", "CAN_RUN", "CAN_EDIT", "CAN_MANAGE"], | ||
| experiment: ["CAN_READ", "CAN_EDIT", "CAN_MANAGE"], | ||
| app: ["CAN_USE"], | ||
| }; |
There was a problem hiding this comment.
I don't like the fact that those resources (and sometimes permissions) are defined on:
- CLI: https://github.com/databricks/cli/blob/main/libs/apps/generator/generator.go#L233 + https://github.com/databricks/cli/blob/main/libs/apps/prompt/resource_registry.go#L10
- AppKit plugin-manifest schema: https://github.com/databricks/appkit/blob/main/packages/shared/src/schemas/plugin-manifest.schema.json#L92
- now here, in App CLI
This becomes super difficult to maintain. When I'll be adding the postgres resource support, I'll need to update so many places (and wait for CLI release) 😶
I think we should get that from the schema: https://github.com/databricks/appkit/blob/main/packages/shared/src/schemas/plugin-manifest.schema.json - especially that we host it on docs:
| const NAME_PATTERN = /^[a-z][a-z0-9-]*$/; | ||
| const DEFAULT_VERSION = "1.0.0"; | ||
|
|
||
| async function runPluginCreate(): Promise<void> { |
There was a problem hiding this comment.
To be honest there are so many steps that I'd simplify it:
I'd suggest removing the following steps:
- version (always start with 0.1.0)
- maybe the confirmation at the end (why?) because on mistake everyone can do CTRL+C and exit
- not sure about the author and license 🤔 maybe we can get rid of license at least?
Even reducing the flow by 2 steps would be super beneficial 👍
There was a problem hiding this comment.
I got
npx appkit plugin validate ../../template
Warning: Could not load JSON schema at /Users/pawel.kosiec/repositories/databricks-os/appkit/apps/dev-playground/node_modules/@databricks/appkit/dist/schemas/plugin-manifest.schema.json: ENOENT: no such file or directory, open '/Users/pawel.kosiec/repositories/databricks-os/appkit/apps/dev-playground/node_modules/@databricks/appkit/dist/schemas/plugin-manifest.schema.json'. Falling back to basic validation.
✓ ../../template/appkit.plugins.json
Even thought I did build the appkit and force installed it in the dev playground catalog (like here #64). Are you sure the schema is embedded properly in the package?
| .option( | ||
| "-d, --dir <path>", | ||
| "Scan directory for plugin folders (each with manifest.json)", | ||
| ) |
There was a problem hiding this comment.
I expected the same syntax as validate (npx appkit plugin list ../../template). By default it would use . as before.
WDYT? IMO it would be great to keep the consistency across commands.
Introduces a full suite of CLI commands under
npx appkit pluginfor managing plugin manifests — creating, validating, listing, syncing, and adding resources. Restructures the previous singleplugins synccommand into a modular command tree and strengthens the JSON schema with type-specific permission validation.New CLI commands
plugin create— Interactive wizard (via@clack/prompts) to scaffold a new plugin with manifest, TypeScript class, and barrel exports. Supports in-repo or standalone placement, resource selection, and metadata fields.plugin validate— Validatesmanifest.jsonorappkit.plugins.jsonagainst the JSON schema. Auto-detects schema type, reports humanized errors with actual values and expected enums.plugin list— Lists plugins fromappkit.plugins.jsonor scans a directory. Supports--jsonoutput for scripting.plugin add-resource— Interactively adds a resource requirement to an existing plugin manifest.plugin sync— Refactored from the previousplugins sync; discovers plugin manifests from packages and local imports, writes consolidatedappkit.plugins.json.Schema improvements
allOf/if-thenrules (e.g.sql_warehouseonly allowsCAN_MANAGE | CAN_USE).resourceTypeenum is the single source of truth — unknown types are rejected at the schema level.template-plugins.schema.jsonnow$refs definitions fromplugin-manifest.schema.jsoninstead of duplicating them.