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 😄 )
| @@ -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 😄
| 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:
There was a problem hiding this comment.
yeah I agree, we already have the database resource there though, you might need to update it but it is there
There was a problem hiding this comment.
database is a different resource (it's lakebase v1), in the API postgres is the Lakebase v2 - you can see that in the Databricks CLI (databricks postgres list-projects, databricks database list-database-instances)
There was a problem hiding this comment.
we just need to sync with however the resource will look like in the UI and in DABs
| .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.
There was a problem hiding this comment.
Before merge please get rid of the hardcoded definitions 🙏
There was a problem hiding this comment.
on it, I thought I handled that 🤦🏻
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.