Detect mismatch between host defined in config and env variable #2549
Conversation
This file contains 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
ilyakuz-db
requested review from
pietern,
andrewnester,
shreyas-goenka,
denik and
anton-107
as code owners
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
commented
Mar 22, 2025
| @@ -7,19 +7,14 @@ Warning: Variable interpolation is not supported for fields that configure authe | |||
| Interpolation is not supported for the field workspace.host. Please set | |||
| the DATABRICKS_HOST environment variable if you wish to configure this field at runtime. | |||
|
|
|||
| Error: failed during request visitor: parse "https://${var.host}": invalid character "{" in host name | |||
| Error: cannot resolve bundle auth configuration: config host mismatch: DATABRICKS_HOST is defined as [DATABRICKS_URL], but CLI configured to use ${var.host} | |||
There was a problem hiding this comment.
Just in case - we have a warning displayed above that explains incorrect usage of variables in auth-related fields
ilyakuz-db
commented
Mar 22, 2025
| @@ -1,26 +0,0 @@ | |||
| package databrickscfg | |||
There was a problem hiding this comment.
Moved to shared package
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
commented
Mar 24, 2025
| return u1.Host == u2.Host | ||
| } | ||
|
|
||
| func fixUrlIfNeeded(s string) string { |
There was a problem hiding this comment.
Apparently u := url.Parse('foo.com') will put foo.com in u.Path instead of u.Host so we're handling such cases explicitly
ilyakuz-db
commented
Mar 25, 2025
| hostFromProfile := normalizeHost(match.Key("host").Value()) | ||
| if hostFromProfile != "" && host != "" && hostFromProfile != host { | ||
| hostFromProfile := workspace.NormalizeHost(match.Key("host").Value()) | ||
| if hostFromProfile != "" && host != "" && !workspace.MatchHost(hostFromProfile, host) { |
There was a problem hiding this comment.
Added same ignore-scheme-behavior for config-defined hosts
cmd/root/bundle.go
Outdated
| @@ -94,6 +95,8 @@ func configureBundle(cmd *cobra.Command, b *bundle.Bundle) (*bundle.Bundle, diag | |||
| return b, diags | |||
| } | |||
|
|
|||
| diags = diags.Extend(bundle.Apply(ctx, b, validate.NoInterpolationInAuthConfig())) | |||
There was a problem hiding this comment.
You can actually convert the whole NoInterpolationInAuthConfig mutator in a usual Go function and don't use bundle.Apply here at all because this mutator does not actually mutate anything
bundle/config/workspace_test.go
Outdated
| @@ -149,3 +149,34 @@ func TestWorkspaceVerifyProfileForHost(t *testing.T) { | |||
| assert.ErrorContains(t, err, "config host mismatch") | |||
| }) | |||
| } | |||
|
|
|||
| func TestWorkspaceValidateConfigAndEnvHost(t *testing.T) { | |||
There was a problem hiding this comment.
Can you instead convert this to acceptance test?
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
andrewnester
approved these changes
Mar 25, 2025
deco-sdk-tagging bot
added a commit
that referenced
this pull request
Mar 26, 2025
## Release v0.245.0 ### Bundles * Processing 'artifacts' section is now done in "bundle validate" (adding defaults, inferring "build", asserting required fields) ([#2526])(#2526)) * When uploading artifacts, include relative path in log message ([#2539])(#2539)) * Added support for clusters in deployment bind/unbind commands ([#2536](#2536)) * Added support for volumes in deployment bind/unbind commands ([#2527](#2527)) * Added support for dashboards in deployment bind/unbind commands ([#2516](#2516)) * Added support for registered models in deployment bind/unbind commands ([#2556](#2556)) * Added a mismatch check when host is defined in config and as an env variable ([#2549](#2549)) * New attribute on artifacts entries: `dynamic_version`. When set to true, it patches the wheel with dynamic version suffix so it is always used by Databricks environments, even if original wheel version is the same. Intended for development loop on interactive clusters. ([#2520](#2520))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes
https://prefix is ignored during validation on host mismatch, both in env and profilesWhy
If I defined both DATABRICKS_TOKEN and DATABRICKS_HOST I expect this token to be used only for requests to DATABRICKS_HOST, while now we have it added to
w.CurrentUser.Merequests to the hosts defined in the configTests
Added acceptance tests