Abstract over filesystem interaction with libs/vfs by pietern · Pull Request #1452 · databricks/cli

pietern · 2024-05-27T08:09:56Z

Changes

Introduce libs/vfs for an implementation of fs.FS and friends that includes the absolute path it is anchored to.

This is needed for:

Intercepting file operations to inject custom logic (e.g., logging, access control).
Traversing directories to find specific leaf directories (e.g., .git).
Converting virtual paths to OS-native paths.

Options 2 and 3 are not possible with the standard fs.FS interface. They are needed such that we can provide an instance to the sync package and still detect the containing .git directory and convert paths to native paths.

This change focuses on making the following packages use vfs.Path:

libs/fileset
libs/git
libs/sync

All entries returned by fileset.All are now slash-separated. This has 2 consequences:

The sync snapshot now always uses slash-separated paths
We don't need to call filepath.FromSlash as much as we did

Tests

All unit tests pass
All integration tests pass
Manually confirmed that a deployment made on Windows by a previous version of the CLI can be deployed by a new version of the CLI while retaining the validity of the local sync snapshot as well as the remote deployment state.

shreyas-goenka

Thanks, this is great. Some minor initial comments, There's some parts I still need to review.

andrewnester

Thanks, it makes it way easier to reason about! We have some usage filepath.FromSlash in local_client, shall we replaced it as well?

pietern · 2024-05-29T13:24:43Z

We have some usage filepath.FromSlash in local_client, shall we replaced it as well?

Good point! I'll file a separate PR, as removal is unrelated to this one.

shreyas-goenka

Thanks, looks good to me.

shreyas-goenka · 2024-05-29T14:50:47Z

@@ -0,0 +1 @@
+package vfs


Unnecessary file?

CLI: * Update OpenAPI spec ([#1466](#1466)). Bundles: * Upgrade TF provider to 1.46.0 ([#1460](#1460)). * Add support for Lakehouse monitoring ([#1307](#1307)). * Make dbt-sql and default-sql templates public ([#1463](#1463)). Internal: * Abstract over filesystem interaction with libs/vfs ([#1452](#1452)). * Add `filer.Filer` to read notebooks from WSFS without omitting their extension ([#1457](#1457)). * Fix listing notebooks in a subdirectory ([#1468](#1468)). API Changes: * Changed `databricks account storage-credentials list` command to return . * Added `databricks consumer-listings batch-get` command. * Added `databricks consumer-providers batch-get` command. * Removed `databricks apps create-deployment` command. * Added `databricks apps deploy` command. OpenAPI commit 37b925eba37dfb3d7e05b6ba2d458454ce62d3a0 (2024-06-03) Dependency updates: * Bump github.com/hashicorp/go-version from 1.6.0 to 1.7.0 ([#1454](#1454)). * Bump github.com/hashicorp/hc-install from 0.6.4 to 0.7.0 ([#1453](#1453)).

## Changes Note: this doesn't cover _all_ filesystem interaction. To intercept calls where read or stat files to determine their type, we need a layer between our code and the `os` package calls that interact with the local file system. Interception is necessary to accommodate differences between a regular local file system and the FUSE-mounted Workspace File System when running the CLI on DBR. This change makes use of #1452 in the bundle struct. It uses #1525 to access the bundle variable in path rewriting. ## Tests * Unit tests pass. * Integration tests pass.

## Changes Introduce `libs/vfs` for an implementation of `fs.FS` and friends that _includes_ the absolute path it is anchored to. This is needed for: 1. Intercepting file operations to inject custom logic (e.g., logging, access control). 2. Traversing directories to find specific leaf directories (e.g., `.git`). 3. Converting virtual paths to OS-native paths. Options 2 and 3 are not possible with the standard `fs.FS` interface. They are needed such that we can provide an instance to the sync package and still detect the containing `.git` directory and convert paths to native paths. This change focuses on making the following packages use `vfs.Path`: * libs/fileset * libs/git * libs/sync All entries returned by `fileset.All` are now slash-separated. This has 2 consequences: * The sync snapshot now always uses slash-separated paths * We don't need to call `filepath.FromSlash` as much as we did ## Tests * All unit tests pass * All integration tests pass * Manually confirmed that a deployment made on Windows by a previous version of the CLI can be deployed by a new version of the CLI while retaining the validity of the local sync snapshot as well as the remote deployment state.

CLI: * Update OpenAPI spec ([#1466](#1466)). Bundles: * Upgrade TF provider to 1.46.0 ([#1460](#1460)). * Add support for Lakehouse monitoring ([#1307](#1307)). * Make dbt-sql and default-sql templates public ([#1463](#1463)). Internal: * Abstract over filesystem interaction with libs/vfs ([#1452](#1452)). * Add `filer.Filer` to read notebooks from WSFS without omitting their extension ([#1457](#1457)). * Fix listing notebooks in a subdirectory ([#1468](#1468)). API Changes: * Changed `databricks account storage-credentials list` command to return . * Added `databricks consumer-listings batch-get` command. * Added `databricks consumer-providers batch-get` command. * Removed `databricks apps create-deployment` command. * Added `databricks apps deploy` command. OpenAPI commit 37b925eba37dfb3d7e05b6ba2d458454ce62d3a0 (2024-06-03) Dependency updates: * Bump github.com/hashicorp/go-version from 1.6.0 to 1.7.0 ([#1454](#1454)). * Bump github.com/hashicorp/hc-install from 0.6.4 to 0.7.0 ([#1453](#1453)).

## Changes Note: this doesn't cover _all_ filesystem interaction. To intercept calls where read or stat files to determine their type, we need a layer between our code and the `os` package calls that interact with the local file system. Interception is necessary to accommodate differences between a regular local file system and the FUSE-mounted Workspace File System when running the CLI on DBR. This change makes use of #1452 in the bundle struct. It uses #1525 to access the bundle variable in path rewriting. ## Tests * Unit tests pass. * Integration tests pass.

pietern added 18 commits May 23, 2024 17:08

Make the notebook package use fs.FS
1246f58

Remove unused function
adbbf44

Add libs/vfs to abstract over filesystem interaction
07eb3a3

Ensure that vfs.Path is always absolute
c5bd637

Fix Windows test
7f9e2fb

Use vfs.Path in libs/fileset and its callers
ea459be

Fix test on Windows
1194b89

Use vfs.Path in libs/git and its callers
e033f91

Fix test on Windows
fce00a1

Initialize vfs.Path only once
a18ba6c

Fix test on Windows
1fcd599

Use vfs.Path in libs/sync
483c953

Fix tests
e02be32

Include comment for ToSlash
c132793

Fix sync command test
2aac1d1

Remove need for path/filepath in libs/fileset
ccb0f48

Use vfs.Path for ignore file in libs/git
5821d58

Remove ToSlash where we know we have slash-separated paths
f33b47f

pietern commented May 27, 2024
View reviewed changes

Comment threadlibs/git/repository.go
Comment threadlibs/git/repository.go
Comment threadlibs/git/repository.go
Comment threadlibs/git/view.go
Comment threadlibs/git/view.go
Comment threadlibs/git/view.go
Comment threadlibs/sync/diff.go

pietern added 3 commits May 27, 2024 21:06

filepath -> path
9337c92

Call into fs.ReadFile
2c46e56

Convert sync snapshot to use slashes on load
25abe18

pietern requested review from andrewnester and shreyas-goenka May 27, 2024 20:20

shreyas-goenka reviewed May 28, 2024
View reviewed changes

Comment threadlibs/vfs/path.go
Comment threadlibs/vfs/path.go
Comment threadcmd/sync/sync.go

Fix integration tests on Windows
16ab5b5

pietern marked this pull request as ready for review May 29, 2024 08:17

andrewnester approved these changes May 29, 2024
View reviewed changes

Comment threadlibs/git/ignore.go
Comment threadlibs/vfs/path.go

Mention vfs.Path is read-only
2aec9ac

pietern mentioned this pull request May 29, 2024
Remove unnecessary filepath.FromSlash calls #1458
Merged

shreyas-goenka approved these changes May 29, 2024
View reviewed changes

Comment threadlibs/vfs/path_test.go
@@ -0,0 +1 @@
package vfs
shreyas-goenkaMay 29, 2024
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary file?

pietern added this pull request to the merge queue May 30, 2024

Merged via the queue into main with commit 424499eMay 30, 2024

pietern deleted the fsys-notebook branch May 30, 2024 07:49

pietern mentioned this pull request May 31, 2024
Databricks Bundle Deploy fails from within Databricks for databricks notebooks. #1263
Closed

pietern mentioned this pull request Jun 4, 2024
Release v0.221.0 #1470
Merged

pietern mentioned this pull request Jul 3, 2024
Use vfs.Path for filesystem interaction #1554
Merged

andrewnester mentioned this pull request Jul 30, 2024
Bundle deploy sync whl takes .gitignore into account on Windows #1572
Closed

Navigation Menu

Uh oh!

Abstract over filesystem interaction with libs/vfs - #1452

Abstract over filesystem interaction with libs/vfs#1452
pietern merged 23 commits into
mainfrom
fsys-notebook

pietern commented May 27, 2024•
edited
Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shreyas-goenka left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewnester left a comment

Uh oh!

Uh oh!

Uh oh!

pietern commented May 29, 2024

Uh oh!

shreyas-goenka left a comment

Uh oh!

shreyas-goenkaMay 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

pietern commented May 27, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shreyas-goenka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewnester left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pietern commented May 29, 2024

Uh oh!

shreyas-goenka left a comment

Choose a reason for hiding this comment

Uh oh!

shreyas-goenkaMay 29, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pietern commented May 27, 2024•
edited
Loading