Open
Conversation
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: 14ba838
16 interesting tests: 7 SKIP, 6 RECOVERED, 3 flaky
Top 20 slowest tests (at least 2 minutes):
|
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
Adds `databricks experimental open RESOURCE_TYPE ID_OR_PATH` to open workspace resources (jobs, notebooks, clusters, pipelines, dashboards, warehouses, queries, apps) in the default browser. Handles both path-based and fragment-based URL patterns correctly. Includes shell completion for resource types. Co-authored-by: Isaac
- Fix fragment URLs to include `/` before `#` (host/#notebook vs host#notebook) - Update cluster pattern to `compute/clusters/%s` (matching bundle code) - Update dashboard pattern to `dashboardsv3/%s/published` (matching bundle code) - Add leading `/` to all path-based URL patterns - Add `?o=<workspace-id>` via CurrentWorkspaceID for shared-domain workspaces - Add tests for notebook path-style IDs, workspace ID parameter - Add doc comment for openURLSuppressingBrowserStderr Co-authored-by: Isaac
Move shared workspace UI routes and base URL synthesis into libs/workspaceurls so experimental open and bundle resource URLs stay aligned, including vanity hosts that still require the ?o workspace suffix.
simonfaltum
force-pushed
the
simonfaltum/workspace-open
branch
from
cb07ae7 to
59d2959
Compare
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
The shared workspaceurls lib uses a stricter adb- hostname check for workspace ID detection. This is correct but changes bundle summary URL output on non-Azure workspaces (adds ?o= where it was previously omitted due to a loose strings.Contains match). That behavior change should be a separate PR with its own integration test updates. experimental open still uses the shared lib with the strict check.
simonfaltum
force-pushed
the
simonfaltum/workspace-open
branch
from
c672536 to
c5ae950
Compare
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
reviewed
Mar 13, 2026
cmd/experimental/workspace_open.go
Outdated
|
|
||
| // buildWorkspaceURL constructs a full workspace URL for a given resource type and ID. | ||
| func buildWorkspaceURL(host, resourceType, id string, workspaceID int64) (string, error) { | ||
| if !slices.Contains(supportedOpenResourceTypes, resourceType) { |
Contributor
There was a problem hiding this comment.
Why have this check, should not LookupPattern below handle the case or unsupported resource type?
denik
reviewed
Mar 13, 2026
| workspaceurls.ResourcePipelines, | ||
| workspaceurls.ResourceQueries, | ||
| workspaceurls.ResourceWarehouses, | ||
| } |
Contributor
There was a problem hiding this comment.
question - Why is there less resource types than supported in libs/workspaceurls?
|
|
||
| cmd.Flags().BoolVar(&printURL, "url", false, "Print the workspace URL instead of opening the browser") | ||
|
|
||
| return cmd |
Contributor
There was a problem hiding this comment.
We have acceptance test for bundle open + completion (as it was otherwise easy to break in the past). Would it make sense to add such test here as well? See acceptance/bundle/open
Contributor
shreyas-goenka
left a comment
shreyas-goenka
left a comment
There was a problem hiding this comment.
Note: This review was posted by Claude (AI assistant). Shreyas will do a separate, more thorough review pass.
Priority: MEDIUM — Redundant checks and missing tests
MEDIUM: Redundant double-check pattern
The code checks for the workspace client / configuration validity in multiple places redundantly. One check should be sufficient.
MEDIUM: Missing test coverage
The workspace open command lacks tests for error paths (invalid URLs, missing workspace client, browser launch failures).
LOW: Partial bundle refactor revert
Some changes appear to partially revert or duplicate work from a related bundle refactoring effort. Worth confirming this is intentional.
What looks good
- The command itself is a useful addition for the experimental feature set
- Clean integration with the existing command structure
- Proper use of the
experimentalcommand group
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
…cceptance test Remove the redundant slices.Contains check in buildWorkspaceURL since LookupPattern already handles unknown resource types. Add missing resource types (alerts, models, model_serving_endpoints, registered_models) so the command supports all types from libs/workspaceurls. Add acceptance test for the experimental open command covering URL generation, error handling, and tab completion.
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Fix three issues in workspace open and browser opener: 1. Convert dot-separated registered_models names (catalog.schema.model) to slash-separated URL paths automatically. Add help text and example. 2. Handle quoted BROWSER env values (e.g. open -a "Google Chrome") by delegating to sh -c instead of naive whitespace splitting. 3. Make the BROWSER=none fallback message configurable via WithDisabledMessage option. Auth callers keep the auth-specific message, workspace open uses a generic resource message.
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Opening workspace resources in the browser requires manually constructing URLs. A quick command to open jobs, notebooks, clusters, etc. by type and ID saves time during development.
Changes
Before: Users had to manually construct workspace URLs to open resources in the browser. URL patterns were duplicated between
bundle open(via per-resourceInitializeURLmethods) and would need to be hardcoded again here.Now:
databricks experimental open RESOURCE_TYPE ID_OR_PATHopens any supported resource directly.Resource types use plural names matching CLI command groups:
jobs,clusters,notebooks,pipelines,dashboards,warehouses,queries,apps,experiments.URL pattern construction is shared with
bundle openvia a newlibs/workspaceurlspackage. This package provides:jobs->jobs/%s,notebooks->#notebook/%s)?o=<workspace-id>handlingBoth
experimental openandbundle/config/mutator/initialize_urls.gonow use this shared helper instead of duplicating patterns. The stricteradb-<id>hostname check for workspace ID detection is also shared, fixing a loosestrings.Containsmatch that existed in the bundle code.A
--urlflag prints the URL to stdout without opening the browser, useful for scripting and SSH sessions. Browser opening respects theBROWSERenv var (includingBROWSER=none) via a sharedlibs/browserhelper.Test plan
make lintfullpassesmake checkspasses