Conversation
d4fef20 to
416ca6a
Compare
Integration test reportCommit: 3a91fa6
Top 3 slowest tests (at least 2 minutes):
|
416ca6a to
319effe
Compare
Approval status: pending
|
319effe to
62668a9
Compare
pietern
left a comment
There was a problem hiding this comment.
The tests in this PR mock pretty much everything and then go through e2e flows.
I recommend testing the units independently and reserving e2e for acc tests.
62668a9 to
e07b06f
Compare
fc154fa to
2e32231
Compare
I restructured the unit tests to exercise each component directly, covering host derivation, helper status, output formats, errors, and config immutability. I couldn’t add a successful host acceptance test without a much larger change because the logic depends on the profile’s workspace domain. I believe the existing configure acceptance test still provides solid end-to-end coverage of the Docker helper flow. |
Co-authored-by: Isaac <no-reply@databricks.com>
2e32231 to
3a91fa6
Compare
🥞 Stacked PR (generated by git ns)
Use this link to review incremental changes.
Changes
Docker setup now needs only a workspace profile. The CLI resolves the workspace ID, obtains the workspace region from its metastore summary, derives the full Artifact Registry hostname, and configures Docker for that host:
Users no longer need to look up and supply the workspace home region. The existing
--regionflag remains available for compatibility, emits a deprecation warning when used, and is scheduled for removal in the next release.The new read-only
hostcommand returns both the derived registry hostname and whether Docker is already configured to use the Databricks credential helper:Structured output makes the result available to scripts without reconstructing the hostname:
Both commands now turn an invalid OAuth refresh token into a direct recovery step instead of exposing the underlying token request:
Profiles whose host is not a Databricks workspace domain fail before any workspace request or Docker configuration change:
To run these flows end to end against the acceptance test server, a workspace served from
http://127.0.0.1maps to a.localhostregistry DNS zone (e.g.900800700600.container.us-west-2.localhost). This is the same local-testing exception OAuth login already makes forhttp://127.0.0.1hosts. The credential helper returns tokens for.localhostregistries only to profiles onhttp://127.0.0.1, so a real workspace's token never reaches a local registry. The test server also gains a defaultmetastore_summaryhandler.Why
Workspace profiles do not expose the home region directly, and asking users to discover it separately made Docker setup unnecessarily error-prone. The workspace metastore already provides the matching region, so the CLI can derive the correct registry host from the selected profile and expose that same result for later commands and automation.
The error handling keeps authentication and profile-selection failures actionable, especially when a saved OAuth session has expired or an account profile is selected accidentally.
Tests
Acceptance tests in
acceptance/cmd/auth/docker/now run the real flow against the test server, without--regionand without rewriting the profile mid-test:configure:hostreports the helper as not configured;configureresolves the workspace ID (SCIM) and region (metastore summary), savesworkspace_idto the profile, and writes the exact Docker config;host --output jsonreports it as configured;docker-credential-databricks getreturns a token. Recorded requests show which API calls each step makes. A rerun leaves the Docker config unchanged, and the deprecated--regionflag makes no API calls.configure-errors: unsupported flags, conflicting profile selectors, a non-Databricks host, and a duplicateworkspace_idall fail without creating the Docker config or changing profiles.host: text and JSON output,credsStoredetection, and the profile is never modified.token: cached vs. refreshed token, unknown workspace, non-registry host,--profilerejected, a test-server registry never matching a remote profile, duplicate profiles.invalid-refresh-token:host,configure, andtokenall print the re-login command, and no Docker config is created.Unit tests that walked these flows through mocks were removed. The remaining ones cover what the test server can't reach: shim install ordering, the PATH warning, metastore failures, and the
workspace_id = nonesentinel.go test ./acceptance -run 'TestAccept/cmd/auth' -count=1(129 passed)go test ./cmd/auth/... ./libs/dockercredentials/... ./libs/testserver/..../task lint-q,./task fmt-q,./task wsThis pull request and its description were written by Isaac.