apps: route deploy to API mode on request flags and reject incompatible flags - #6776
atilafassina wants to merge 2 commits into
Conversation
…le flags `apps deploy` previously routed purely on whether an APP_NAME argument was present: no arg meant bundle deploy, so an API request flag such as `--source-code-path` given without an app name was silently ignored while the bundle pipeline ran. Route to API deploy whenever an API request flag is set (inferring the app name from databricks.yml), and reject incompatible input early: - API deploy flags cannot be combined with bundle deploy flags - bundle deploy flags cannot be used together with an APP_NAME argument - API control flags (--no-wait, --timeout) do not select API mode on their own Add unit coverage for the rejected combinations and extend the bundle-with-appname acceptance test. Co-authored-by: Isaac <no-reply@databricks.com>
Git Bash on Windows converts leading-/ paths to Windows filesystem paths before the CLI binary sees them, so --source-code-path /Workspace/apps/inferred became C:/Program Files/Git/Workspace/apps/inferred. Set MSYS_NO_PATHCONV=1 to disable the conversion. Co-authored-by: Isaac <no-reply@databricks.com>
Integration test reportCommit: 7fa9510
Top 3 slowest tests (at least 2 minutes):
|
| apiFlagNames := flagNames(deployCmd.Flags()) | ||
| // Control flags must not bypass the bundle pipeline just because they are | ||
| // implemented by the generated API command. | ||
| requestFlagNames := map[string]struct{}{ |
There was a problem hiding this comment.
is there a way to avoid hardcoding those flags here? Can't we use some existing references?
If not, can we move this definition to the actual producer side, so that it's easier to keep the map up to date?
| return func(deployCmd *cobra.Command, deployReq *apps.CreateAppDeploymentRequest) { | ||
| return func(deployCmd *cobra.Command, _ *apps.CreateAppDeploymentRequest) { | ||
| var opts bundleDeployOptions | ||
| flagNames := func(flags *pflag.FlagSet) map[string]struct{} { |
There was a problem hiding this comment.
can we extract the full flag validation logic as a separate function, to keep the code easier to read?
all the logic: including collecting the flags and validating them against the known flags
| delete(bundleFlagNames, name) | ||
| } | ||
| // --var is inherited from the apps command after this override runs. | ||
| bundleFlagNames["var"] = struct{}{} |
There was a problem hiding this comment.
My agent reported that the --target is missing:
Verified facts:
- --target / -t (and deprecated --environment / -e) is registered as a persistent flag on the root command (cmd/root/bundle.go:260, via initTargetFlag at root.go:52). Persistent-on-root = inherited by every subcommand, including apps deploy.
- It's a bundle-only flag — it selects the bundle target and feeds TryConfigureBundle / configureBundle. Meaningless in the raw API path (no bundle there).
Why the PR misses it: the override builds apiFlagNames / bundleFlagNames by snapshotting deployCmd.Flags() at construction time. Inherited flags from root/parent aren't merged into that snapshot yet, so target is in neither set. changedFlagNames only loops over snapshot names, so --target is never checked. Result: apps deploy my-app --target prod → 0 api-flags, 0 bundle-flags → no "can't combine" error → routes to API deploy → --target silently ignored.
Anyway, that's similar issue as in the hardcoded flag map above - it's an error prone approach and it might get out of sync easily. Can we do something about it?
| originalRunE := deployCmd.RunE | ||
| deployCmd.RunE = func(cmd *cobra.Command, args []string) error { | ||
| if len(args) == 0 { | ||
| if err := validateFlags(cmd, args); err != nil { |
| } | ||
| // Generated API flags and API-specific overrides, including the Git source | ||
| // override, are registered before the bundle override. | ||
| apiFlagNames := flagNames(deployCmd.Flags()) |
There was a problem hiding this comment.
This only works because git_flags.go sorts before overrides.go, so its init() registers the git flags before this snapshot runs.
Can we avoid that? The easier way would be to use PreRunE/RunE where all the flags would be present.
There was a problem hiding this comment.
High-level comment: Can we make the logic a bit simpler?
- Route by app name only, error instead of infer. No app name → bundle. App name → API. API-only flag with no app name → error "provide APP_NAME". This deletes requestFlagNames entirely, deletes the request-vs-control distinction, and deletes the databricks.yml name-inference path (getAppNameFromArgs in the API branch). It also makes deploy match start/stop/delete.
- Detect bundle flags without a name list. API set = the local-flag snapshot (already automatic). Bundle set = the override's own flags (automatic) plus the inherited ones. For the inherited three, either reference the producer's definition or annotate them at registration.
What that removes: the requestFlagNames map, the control-flag special-casing, the double validateFlags call, and the app-name inference. What it keeps: the two genuinely useful errors ("can't combine bundle + API flags", "provide APP_NAME") — which are the part that satisfies the repo's "never silently ignore a flag" rule. Result: one mental model, no hardcoded flag lists, --target and the init-order trap fixed as a side effect.
This branch has not been deployed
3 participants
apps deployrouting was purely positional:len(args) == 0meant bundle deploy, anything else meant API deploy. This silently ignored API request flags like--source-code-pathwhen no app name was given inside a bundle directory — running the full bundle pipeline instead.This PR:
databricks.ymlwhen noAPP_NAMEis given)APP_NAME+ bundle flags → error--no-wait,--timeout) alone → error (they tune a call but don't select a mode)Notes
requestFlagNamesis an explicit allowlist distinguishing API request flags (which select API mode) from API control flags (which don't). New request fields added to the generated command must be added here.