Skip to content

Allow equivalence checking of filer errors to fs errors #416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pietern merged 1 commit into from
May 31, 2023
Merged

Allow equivalence checking of filer errors to fs errors #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions bundle/deploy/terraform/state_pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ package terraform

import (
"context"
"errors"
"io"
"io/fs"
"os"
"path/filepath"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/libs/filer"
"github.com/databricks/cli/libs/log"
"github.com/databricks/databricks-sdk-go/apierr"
)

type statePull struct{}
Expand All @@ -34,7 +35,7 @@ func (l *statePull) Apply(ctx context.Context, b *bundle.Bundle) error {
remote, err := f.Read(ctx, TerraformStateFileName)
if err != nil {
// On first deploy this state file doesn't yet exist.
if apierr.IsMissing(err) {
if errors.Is(err, fs.ErrNotExist) {
Copy link
Copy Markdown
Contributor

@andrewnester andrewnester May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really an equivalent check? apierr checks the HTTP response and status code but we don't seem to be doing the same in Filer

Copy link
Copy Markdown
Contributor

@andrewnester andrewnester May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVM, found we're doing it here

cli/libs/filer/dbfs_client.go

Lines 156 to 160 in 42cd8da

if aerr.StatusCode == http.StatusNotFound {
if aerr.ErrorCode == "RESOURCE_DOES_NOT_EXIST" {
return nil, FileDoesNotExistError{absPath}
}
}

log.Infof(ctx, "Remote state file does not exist")
return nil
}
Expand Down
5 changes: 5 additions & 0 deletions internal/filer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,12 @@ func runFilerReadWriteTest(t *testing.T, ctx context.Context, f filer.Filer) {
// Write should fail because the root path doesn't yet exist.
err = f.Write(ctx, "/foo/bar", strings.NewReader(`hello world`))
assert.True(t, errors.As(err, &filer.NoSuchDirectoryError{}))
assert.True(t, errors.Is(err, fs.ErrNotExist))

// Read should fail because the root path doesn't yet exist.
_, err = f.Read(ctx, "/foo/bar")
assert.True(t, errors.As(err, &filer.FileDoesNotExistError{}))
assert.True(t, errors.Is(err, fs.ErrNotExist))

// Write with CreateParentDirectories flag should succeed.
err = f.Write(ctx, "/foo/bar", strings.NewReader(`hello world`), filer.CreateParentDirectories)
Expand All @@ -59,6 +61,7 @@ func runFilerReadWriteTest(t *testing.T, ctx context.Context, f filer.Filer) {
// Write should fail because there is an existing file at the specified path.
err = f.Write(ctx, "/foo/bar", strings.NewReader(`hello universe`))
assert.True(t, errors.As(err, &filer.FileAlreadyExistsError{}))
assert.True(t, errors.Is(err, fs.ErrExist))

// Write with OverwriteIfExists should succeed.
err = f.Write(ctx, "/foo/bar", strings.NewReader(`hello universe`), filer.OverwriteIfExists)
Expand All @@ -68,6 +71,7 @@ func runFilerReadWriteTest(t *testing.T, ctx context.Context, f filer.Filer) {
// Delete should fail if the file doesn't exist.
err = f.Delete(ctx, "/doesnt_exist")
assert.True(t, errors.As(err, &filer.FileDoesNotExistError{}))
assert.True(t, errors.Is(err, fs.ErrNotExist))

// Delete should succeed for file that does exist.
err = f.Delete(ctx, "/foo/bar")
Expand Down Expand Up @@ -102,6 +106,7 @@ func runFilerReadDirTest(t *testing.T, ctx context.Context, f filer.Filer) {
// Expect an error if the path doesn't exist.
_, err = f.ReadDir(ctx, "/dir/a/b/c/d/e")
assert.True(t, errors.As(err, &filer.NoSuchDirectoryError{}), err)
assert.True(t, errors.Is(err, fs.ErrNotExist))

// Expect two entries in the root.
entries, err = f.ReadDir(ctx, ".")
Expand Down
12 changes: 12 additions & 0 deletions libs/filer/filer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,18 @@ func (err FileAlreadyExistsError) Error() string {
return fmt.Sprintf("file already exists: %s", err.path)
}

func (err FileAlreadyExistsError) Is(other error) bool {
return other == fs.ErrExist
}

type FileDoesNotExistError struct {
path string
}

func (err FileDoesNotExistError) Is(other error) bool {
return other == fs.ErrNotExist
}

func (err FileDoesNotExistError) Error() string {
return fmt.Sprintf("file does not exist: %s", err.path)
}
Expand All @@ -38,6 +46,10 @@ func (err NoSuchDirectoryError) Error() string {
return fmt.Sprintf("no such directory: %s", err.path)
}

func (err NoSuchDirectoryError) Is(other error) bool {
return other == fs.ErrNotExist
}

// Filer is used to access files in a workspace.
// It has implementations for accessing files in WSFS and in DBFS.
type Filer interface {
Expand Down