-
Notifications
You must be signed in to change notification settings - Fork 133
feat: Appkit cli commands #4247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
04f4c88 to
6e23dec
Compare
|
Commit: 81e7752
26 interesting tests: 22 KNOWN, 2 flaky, 1 SKIP, 1 RECOVERED
Top 50 slowest tests (at least 2 minutes):
|
experimental/apps-mcp/templates/appkit/generic/client/index.html
Outdated
Show resolved
Hide resolved
experimental/apps-mcp/templates/appkit/generic/databricks.yml.tmpl
Outdated
Show resolved
Hide resolved
experimental/dev/cmd/app/prompt.go
Outdated
| @@ -0,0 +1,419 @@ | |||
| package app | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check with @pietern. He wants to unify CLI UX.
d552cd0 to
f0b22d5
Compare
9cbff3d to
38cc5a4
Compare
748d4c1 to
bc617fb
Compare
bc617fb to
26f3f28
Compare
26f3f28 to
037121a
Compare
037121a to
9ca1219
Compare
pietern
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking merging. Need to discuss the deps first.
arsenyinfo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those changes duplicate existing entities like template, validation etc and are not in sync with agentic prompts atm. We can't merge it unless there is a unification plan.
|
This PR largely duplicates cli aitools already implemented http://github.com/databricks/cli/tree/main/experimental/aitools Also https://github.com/databricks/appkit/pull/57/changes template - another duplication https://github.com/databricks/cli/tree/main/experimental/aitools/templates/appkit |
the idea is to unify yes, but this comes with the approach of having this as humans first, through the apps scope, but of course, many of this should be used by the ai tools |
It has some similar things yes, I already mentioned this, we were doing similar things and we needed to converge into one. Let's talk about it in the sync |
|
@arsenyinfo @keugenek this indeed duplicates functionality from We keep the existing tools under |
7eb0a86 to
f277573
Compare
chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: fixup chore: validate command chore: fixup chore: remove import command chore: fixup
f277573 to
ef5a6c6
Compare
|
It would be nice to make |
| return fmt.Errorf("failed to run app: %w. Run `databricks apps logs %s` to view logs", err, appName) | ||
| } | ||
|
|
||
| cmdio.LogString(ctx, "✔ Deployment complete!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewnester We'll need to update this after adding a "started" state to apps.
| } else { | ||
| log.Debugf(ctx, "No validator found for project type, skipping validation") | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of validation happens here that doesn't happen in the regular "bundle deploy"?
We need to be careful not to make this a hard prereq for deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is actually to make things better, it runs validation of the app by running linting, typechecking, and building, so it verifies that everything works and the app won't fail to deploy once its running the building process inside the apps process.
Its basically to avoid the annoying behaviour of having to go into databricks or check the logs command to see why the app failed to deploy.
Right now we just have 1 validator for node apps, at some point we will probably have one for python apps too.
But if it doesn't find a validator, then it just keeps going (to avoid introducing breaking changes)
libs/apps/prompt/prompt.go
Outdated
|
|
||
| // printAnswered is an alias for internal use. | ||
| func printAnswered(title, value string) { | ||
| PrintAnswered(title, value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Print where? This should use cobra's stdout or stderr from the context or directly.
libs/apps/validation/nodejs.go
Outdated
|
|
||
| // runValidationCommand executes a shell command in the specified directory. | ||
| func runValidationCommand(ctx context.Context, workDir, command string) *ValidationDetail { | ||
| cmd := exec.CommandContext(ctx, "sh", "-c", command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't work on Windows. Check out libs/exec for something that'll use cmd.exe if needed.
go.mod
Outdated
| require ( | ||
| github.com/charmbracelet/huh v0.8.0 | ||
| github.com/charmbracelet/lipgloss v1.1.0 | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fold into the main list above and include the license as a suffix comment.
Also please include in the NOTICE file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the resulting binary size with/without these deps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need these direct & indirect dependencies though? We need to move toward covergence between the DABs apps tempalte, the aitools template, and the template here. And convergence in the terminal UX. I'd be okay with this only if we have a plan: we will move everything to charmbracelet in the future / we will remove charmbracelet in a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the plan is the former: we move everything to the charmbracelet libs in the near future.
| // substituteVars replaces template variables in a string. | ||
| func substituteVars(s string, vars templateVars) string { | ||
| s = strings.ReplaceAll(s, "{{.project_name}}", vars.ProjectName) | ||
| s = strings.ReplaceAll(s, "{{.sql_warehouse_id}}", vars.SQLWarehouseID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this low-level string mangling stuff seems incredibly unfortunate. Is the aitools template not already usable here, minus the AGENTS.md file? cc @fjakobs
go.mod
Outdated
| require ( | ||
| github.com/charmbracelet/huh v0.8.0 | ||
| github.com/charmbracelet/lipgloss v1.1.0 | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need these direct & indirect dependencies though? We need to move toward covergence between the DABs apps tempalte, the aitools template, and the template here. And convergence in the terminal UX. I'd be okay with this only if we have a plan: we will move everything to charmbracelet in the future / we will remove charmbracelet in a followup.
| github.com/Masterminds/semver/v3 v3.4.0 // MIT | ||
| github.com/briandowns/spinner v1.23.1 // Apache 2.0 | ||
| github.com/charmbracelet/huh v0.8.0 | ||
| github.com/charmbracelet/lipgloss v1.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also include the license like the other deps.
| // detectAppNameFromBundle tries to extract the app name from a databricks.yml bundle config. | ||
| // Returns the app name if found, or empty string if no bundle or no apps found. | ||
| func detectAppNameFromBundle() string { | ||
| const bundleFile = "databricks.yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, this doesn't work. The app name may have a prefix, variable reference, etc.
You can look at bundle run for the right way to load/initialize a bundle.
| } | ||
|
|
||
| return tempDir, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use git.Clone from libs/git.
| profile := "" | ||
| if w := cmdctx.WorkspaceClient(ctx); w != nil && w.Config != nil { | ||
| workspaceHost = w.Config.Host | ||
| profile = w.Config.Profile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Profile may be empty.
| // runPostCreateDeploy runs the deploy command in the current directory. | ||
| func runPostCreateDeploy(ctx context.Context) error { | ||
| // Use os.Args[0] to get the path to the current executable | ||
| executable := os.Args[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incompatible with os.Chdir, unless you expect the CLI to be in $PATH.
For example, if you invoke as .databricks/databricks (snapshot build), and chdir, it won't work.
| srcProjectDir = src | ||
| } | ||
|
|
||
| log.Debugf(context.Background(), "Copying template from: %s", srcProjectDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect context.
|
|
||
| // Skip certain directories | ||
| if info.IsDir() && skipDirs[baseName] { | ||
| log.Debugf(context.Background(), "Skipping directory: %s", baseName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More incorrect context's.
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestParseGitHubURL(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Candidate to move to libs/git.
Changes
Overview
This PR adds new AppKit development commands to
databricks apps, making them first-class citizens alongside the auto-generated Apps API commands. The implementation follows the same pattern established by thepipelinescommands.New Command Structure
databricks appsnow has three command groups:deploy,dev-remote,init,logs,run-local,validatecreate,delete,get,list,start,stop, etc.get-permissions,set-permissions, etc.Key Features
databricks apps init- Initialize new AppKit projects from templates with interactive prompts, but also allowing full prompt override via flags.databricks apps dev-remote- Run local Vite dev server with WebSocket bridge to remote app (this command already exists, but now has some improvements like deriving the project from the folder and reconnecting).databricks apps validate- Run validation of the app running type checking, linting and building.databricks apps deploy- Dual-mode deployment:Directory Structure
Why
Tests