From edff68c7637c3aae4c854b5cfd6858413beed90a Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 5 Nov 2024 10:30:11 +0100 Subject: [PATCH 1/5] Fix bundle run when run interactively (#1880) ## Changes The commit where resource lookup was factored out into a separate package (#1858) didn't take into account the use of `args` further down in the code. This change fixes that oversight by returning the tail arguments when determining which resource to run. The later call no longer has to index the `args` slice. ## Tests Manually confirmed that the command works when being prompted for the resource to run. --- cmd/bundle/run.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/cmd/bundle/run.go b/cmd/bundle/run.go index 96851d0c0..7a92766d9 100644 --- a/cmd/bundle/run.go +++ b/cmd/bundle/run.go @@ -35,17 +35,23 @@ func promptRunArgument(ctx context.Context, b *bundle.Bundle) (string, error) { return key, nil } -func resolveRunArgument(ctx context.Context, b *bundle.Bundle, args []string) (string, error) { +// resolveRunArgument resolves the resource key to run. +// It returns the remaining arguments to pass to the runner, if applicable. +func resolveRunArgument(ctx context.Context, b *bundle.Bundle, args []string) (string, []string, error) { // If no arguments are specified, prompt the user to select something to run. if len(args) == 0 && cmdio.IsPromptSupported(ctx) { - return promptRunArgument(ctx, b) + key, err := promptRunArgument(ctx, b) + if err != nil { + return "", nil, err + } + return key, args, nil } if len(args) < 1 { - return "", fmt.Errorf("expected a KEY of the resource to run") + return "", nil, fmt.Errorf("expected a KEY of the resource to run") } - return args[0], nil + return args[0], args[1:], nil } func keyToRunner(b *bundle.Bundle, arg string) (run.Runner, error) { @@ -109,7 +115,7 @@ task or a Python wheel task, the second example applies. return err } - arg, err := resolveRunArgument(ctx, b, args) + key, args, err := resolveRunArgument(ctx, b, args) if err != nil { return err } @@ -124,13 +130,13 @@ task or a Python wheel task, the second example applies. return err } - runner, err := keyToRunner(b, arg) + runner, err := keyToRunner(b, key) if err != nil { return err } // Parse additional positional arguments. - err = runner.ParseArgs(args[1:], &runOptions) + err = runner.ParseArgs(args, &runOptions) if err != nil { return err } From 26afab2ccb5e5c5a7bc3c9f520c917ec19f46045 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Tue, 5 Nov 2024 10:53:53 +0100 Subject: [PATCH 2/5] Fix relative path resolution for dashboards on Windows (#1881) ## Changes The file presence check for dashboard files was missing a `filepath.ToSlash`. This means it didn't work on Windows unless the dashboard was located at a path without slashes (i.e. the bundle root). Closes #1875. ## Tests * Added a unit test to cover this case (failed before the fix). * Manually ran a dashboard deployment on Windows. --- bundle/config/mutator/translate_paths.go | 2 +- .../translate_paths_dashboards_test.go | 54 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 bundle/config/mutator/translate_paths_dashboards_test.go diff --git a/bundle/config/mutator/translate_paths.go b/bundle/config/mutator/translate_paths.go index 82b0b3caa..321fa5b30 100644 --- a/bundle/config/mutator/translate_paths.go +++ b/bundle/config/mutator/translate_paths.go @@ -163,7 +163,7 @@ func (t *translateContext) translateNoOp(literal, localFullPath, localRelPath, r } func (t *translateContext) retainLocalAbsoluteFilePath(literal, localFullPath, localRelPath, remotePath string) (string, error) { - info, err := t.b.SyncRoot.Stat(localRelPath) + info, err := t.b.SyncRoot.Stat(filepath.ToSlash(localRelPath)) if errors.Is(err, fs.ErrNotExist) { return "", fmt.Errorf("file %s not found", literal) } diff --git a/bundle/config/mutator/translate_paths_dashboards_test.go b/bundle/config/mutator/translate_paths_dashboards_test.go new file mode 100644 index 000000000..c386f1bbe --- /dev/null +++ b/bundle/config/mutator/translate_paths_dashboards_test.go @@ -0,0 +1,54 @@ +package mutator_test + +import ( + "context" + "path/filepath" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/mutator" + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/bundle/internal/bundletest" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/vfs" + "github.com/databricks/databricks-sdk-go/service/dashboards" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTranslatePathsDashboards_FilePathRelativeSubDirectory(t *testing.T) { + dir := t.TempDir() + touchEmptyFile(t, filepath.Join(dir, "src", "my_dashboard.lvdash.json")) + + b := &bundle.Bundle{ + SyncRootPath: dir, + SyncRoot: vfs.MustNew(dir), + Config: config.Root{ + Resources: config.Resources{ + Dashboards: map[string]*resources.Dashboard{ + "dashboard": { + CreateDashboardRequest: &dashboards.CreateDashboardRequest{ + DisplayName: "My Dashboard", + }, + FilePath: "../src/my_dashboard.lvdash.json", + }, + }, + }, + }, + } + + bundletest.SetLocation(b, "resources.dashboards", []dyn.Location{{ + File: filepath.Join(dir, "resources/dashboard.yml"), + }}) + + diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths()) + require.NoError(t, diags.Error()) + + // Assert that the file path for the dashboard has been converted to its local absolute path. + assert.Equal( + t, + filepath.Join(dir, "src", "my_dashboard.lvdash.json"), + b.Config.Resources.Dashboards["dashboard"].FilePath, + ) +} From b81008e2f64d3ee9a29338f4e42032cb56630e86 Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Tue, 5 Nov 2024 20:59:27 +0530 Subject: [PATCH 3/5] Clean host URL in the `auth login` command (#1879) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Changes The host URL for databricks workspaces includes the workspaceId by default as a positional arg. Eg: https://e2-dogfood.staging.cloud.databricks.com/?o=1234 Thus a user can't simply copy paste the URL today to the auth login command. They'll see a runtime error: ``` ➜ cli git:(main) ✗ databricks auth login --host https://e2-dogfood.staging.cloud.databricks.com/\?o\=xxx --profile new-dg Error: oidc: fetch .well-known: failed to unmarshal response body: invalid character '<' looking for beginning of value. This is likely a bug in the Databricks SDK for Go or the underlying REST API. Please report this issue with the following debugging information to the SDK issue tracker at https://github.com/databricks/databricks-sdk-go/issues. Request log: GET /login.html ... ``` ## Tests Unit tests and manually. Now auth login works even when the workspace_id is included in the URL. --- libs/auth/oauth.go | 24 ++++++++++++++++++++++++ libs/auth/oauth_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/libs/auth/oauth.go b/libs/auth/oauth.go index 7c1cb9576..026c45468 100644 --- a/libs/auth/oauth.go +++ b/libs/auth/oauth.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "net" + "net/url" "strings" "time" @@ -143,6 +144,26 @@ func (a *PersistentAuth) Challenge(ctx context.Context) error { return nil } +// This function cleans up the host URL by only retaining the scheme and the host. +// This function thus removes any path, query arguments, or fragments from the URL. +func (a *PersistentAuth) cleanHost() { + parsedHost, err := url.Parse(a.Host) + if err != nil { + return + } + // when either host or scheme is empty, we don't want to clean it. This is because + // the Go url library parses a raw "abc" string as the path of a URL and cleaning + // it will return thus return an empty string. + if parsedHost.Host == "" || parsedHost.Scheme == "" { + return + } + host := url.URL{ + Scheme: parsedHost.Scheme, + Host: parsedHost.Host, + } + a.Host = host.String() +} + func (a *PersistentAuth) init(ctx context.Context) error { if a.Host == "" && a.AccountID == "" { return ErrFetchCredentials @@ -156,6 +177,9 @@ func (a *PersistentAuth) init(ctx context.Context) error { if a.browser == nil { a.browser = browser.OpenURL } + + a.cleanHost() + // try acquire listener, which we also use as a machine-local // exclusive lock to prevent token cache corruption in the scope // of developer machine, where this command runs. diff --git a/libs/auth/oauth_test.go b/libs/auth/oauth_test.go index ea6a8061e..fdf0d04bf 100644 --- a/libs/auth/oauth_test.go +++ b/libs/auth/oauth_test.go @@ -228,3 +228,37 @@ func TestChallengeFailed(t *testing.T) { assert.EqualError(t, err, "authorize: access_denied: Policy evaluation failed for this request") }) } + +func TestPersistentAuthCleanHost(t *testing.T) { + for _, tcases := range []struct { + in string + out string + }{ + {"https://example.com", "https://example.com"}, + {"https://example.com/", "https://example.com"}, + {"https://example.com/path", "https://example.com"}, + {"https://example.com/path/subpath", "https://example.com"}, + {"https://example.com/path?query=1", "https://example.com"}, + {"https://example.com/path?query=1&other=2", "https://example.com"}, + {"https://example.com/path#fragment", "https://example.com"}, + {"https://example.com/path?query=1#fragment", "https://example.com"}, + {"https://example.com/path?query=1&other=2#fragment", "https://example.com"}, + {"https://example.com/path/subpath?query=1", "https://example.com"}, + {"https://example.com/path/subpath?query=1&other=2", "https://example.com"}, + {"https://example.com/path/subpath#fragment", "https://example.com"}, + {"https://example.com/path/subpath?query=1#fragment", "https://example.com"}, + {"https://example.com/path/subpath?query=1&other=2#fragment", "https://example.com"}, + {"https://example.com/path?query=1%20value&other=2%20value", "https://example.com"}, + {"http://example.com/path/subpath?query=1%20value&other=2%20value", "http://example.com"}, + + // URLs without scheme should be left as is + {"abc", "abc"}, + {"abc.com/def", "abc.com/def"}, + } { + p := &PersistentAuth{ + Host: tcases.in, + } + p.cleanHost() + assert.Equal(t, tcases.out, p.Host) + } +} From b6a376bf8a917fa92c018870009b0296b026ca70 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 6 Nov 2024 15:03:54 +0100 Subject: [PATCH 4/5] [Release] Release v0.233.0 (#1886) CLI: * Clean host URL in the `auth login` command ([#1879](https://github.com/databricks/cli/pull/1879)). Bundles: * Fix bundle run when run interactively ([#1880](https://github.com/databricks/cli/pull/1880)). * Fix relative path resolution for dashboards on Windows ([#1881](https://github.com/databricks/cli/pull/1881)). Internal: * Address goreleaser deprecation warning ([#1872](https://github.com/databricks/cli/pull/1872)). * Update actions/github-script to v7 ([#1873](https://github.com/databricks/cli/pull/1873)). * Use Go 1.23 ([#1871](https://github.com/databricks/cli/pull/1871)). * [Internal] Always write message for manual integration test trigger ([#1874](https://github.com/databricks/cli/pull/1874)). * Add `cmd-exec-id` to user agent ([#1808](https://github.com/databricks/cli/pull/1808)). * Added E2E test to run Python wheels on interactive cluster created in bundle ([#1864](https://github.com/databricks/cli/pull/1864)). Dependency updates: * Bump github.com/hashicorp/terraform-json from 0.22.1 to 0.23.0 ([#1877](https://github.com/databricks/cli/pull/1877)). --- CHANGELOG.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 639270e32..9b08d7514 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,26 @@ # Version changelog +## [Release] Release v0.233.0 + +CLI: + * Clean host URL in the `auth login` command ([#1879](https://github.com/databricks/cli/pull/1879)). + +Bundles: + * Fix bundle run when run interactively ([#1880](https://github.com/databricks/cli/pull/1880)). + * Fix relative path resolution for dashboards on Windows ([#1881](https://github.com/databricks/cli/pull/1881)). + +Internal: + * Address goreleaser deprecation warning ([#1872](https://github.com/databricks/cli/pull/1872)). + * Update actions/github-script to v7 ([#1873](https://github.com/databricks/cli/pull/1873)). + * Use Go 1.23 ([#1871](https://github.com/databricks/cli/pull/1871)). + * [Internal] Always write message for manual integration test trigger ([#1874](https://github.com/databricks/cli/pull/1874)). + * Add `cmd-exec-id` to user agent ([#1808](https://github.com/databricks/cli/pull/1808)). + * Added E2E test to run Python wheels on interactive cluster created in bundle ([#1864](https://github.com/databricks/cli/pull/1864)). + + +Dependency updates: + * Bump github.com/hashicorp/terraform-json from 0.22.1 to 0.23.0 ([#1877](https://github.com/databricks/cli/pull/1877)). + ## [Release] Release v0.232.1 This patch release fixes the following error observed when deploying to /Shared root folder From 162aa212bc271c502adcbf9d6f80285838666a5d Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 7 Nov 2024 10:31:49 +0100 Subject: [PATCH 5/5] Do not execute build on bundle destroy (#1882) ## Changes There's no value in building artifacts on destroy because they are just removed from workspace as part of destroy. --- cmd/bundle/destroy.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cmd/bundle/destroy.go b/cmd/bundle/destroy.go index cd7e63062..711abbcd7 100644 --- a/cmd/bundle/destroy.go +++ b/cmd/bundle/destroy.go @@ -6,6 +6,7 @@ import ( "os" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" "github.com/databricks/cli/cmd/root" @@ -62,7 +63,12 @@ func newDestroyCommand() *cobra.Command { diags = bundle.Apply(ctx, b, bundle.Seq( phases.Initialize(), - phases.Build(), + // We need to resolve artifact variable (how we do it in build phase) + // because some of the to-be-destroyed resource might use this variable. + // Not resolving might lead to terraform "Reference to undeclared resource" error + mutator.ResolveVariableReferences( + "artifacts", + ), phases.Destroy(), )) if err := diags.Error(); err != nil {