Fix "bundle init" when run from Databricks #1744
Conversation
There was a problem hiding this comment.
Thanks! This also needs some unit tests. You could use a mocked workspace client and set it in the context for the unit test. You can use t.Setenv to set the DATABRICKS_RUNTIME_VERSION env var for the tests.
See TestResolveClusterReference for a reference of a mocked workplace client.
fjakobs
force-pushed
the
init_on_databricks
branch
2 times, most recently
from
a46942f
to
20ee23e
Compare
|
|
||
| const envDatabricksRuntimeVersion = "DATABRICKS_RUNTIME_VERSION" | ||
|
|
||
| func RunsOnDatabricks(ctx context.Context) bool { |
There was a problem hiding this comment.
+1 for a common lib function. I'd expect this to also check for the existence of /Workspace though to avoid false positives. There may be all kinds of reasons why customers set the env var locally.
There was a problem hiding this comment.
I'd prefer not to test for /Workspace as it makes the code using it harder to test
libs/template/renderer.go
Outdated
| @@ -313,8 +319,7 @@ func (r *renderer) persistToDisk() error { | |||
| _, err := os.Stat(path) | |||
| if err == nil { | |||
| return fmt.Errorf("failed to initialize template, one or more files already exist: %s", path) | |||
| } | |||
| if err != nil && !errors.Is(err, fs.ErrNotExist) { | |||
| } else if !errors.Is(err, fs.ErrNotExist) { | |||
There was a problem hiding this comment.
Nit: isn't this easier to read without the else if?
libs/template/file.go
Outdated
| if strings.HasPrefix(path, "/Workspace/") && runtime.RunsOnDatabricks(ctx) { | ||
| isNotebook, _, _ := notebook.DetectWithContent(path, content) | ||
| return isNotebook | ||
| } else { |
There was a problem hiding this comment.
style nit: remove else block
fjakobs
force-pushed
the
init_on_databricks
branch
from
c409696
to
84d1bbf
Compare
There was a problem hiding this comment.
Couple small comments remaining.
FWIW, in hindsight it would have been neater to use a filer for writing everything. Then we could have swapped out the writing filer for the extension-aware workspace filer and things would have worked transparently (and all writes would use the API as opposed to only notebooks).
| assert.False(t, shouldUseImportNotebook(ctx, "/Workspace/foo/bar", data)) | ||
| assert.False(t, shouldUseImportNotebook(ctx, "/Workspace/foo/bar.ipynb", data)) | ||
|
|
||
| t.Setenv("DATABRICKS_RUNTIME_VERSION", "14.3") |
There was a problem hiding this comment.
What's the difference? I see t.Setenv used in tests. If I change it to env.Set then my tests break.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Changes
When started from a Databricks cluster use import API when creating notebooks from
bundle init.Tests
Manually tested by executing
databricks bundle initfrom a web terminal on a Databricks cluster.