From 3461018e8a5a921463f9e80a80111642ee90f5fb Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 2 Dec 2024 14:01:10 +0100 Subject: [PATCH] better error message --- bundle/libraries/filer_volume.go | 8 +++++- bundle/libraries/filer_volume_test.go | 41 +++++++++++++++++++++------ internal/bundle/artifacts_test.go | 4 +-- 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/bundle/libraries/filer_volume.go b/bundle/libraries/filer_volume.go index 1ef8d401f..962a6f622 100644 --- a/bundle/libraries/filer_volume.go +++ b/bundle/libraries/filer_volume.go @@ -81,12 +81,18 @@ func filerForVolume(ctx context.Context, b *bundle.Bundle) (filer.Filer, string, baseErr := diag.Diagnostic{ Severity: diag.Error, - Summary: fmt.Sprintf("failed to fetch metadata for %s: %s", volumePath, err), + Summary: fmt.Sprintf("unable to determine if volume at %s exists: %s", volumePath, err), Locations: b.Config.GetLocations("workspace.artifact_path"), Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, } if errors.Is(err, apierr.ErrNotFound) { + // Since the API returned a 404, the volume does not exist in the workspace. + // Modify the error message to provide more context. + baseErr.Summary = fmt.Sprintf("volume %s does not exist: %s", volumePath, err) + + // If the volume is defined in the bundle, provide a more helpful error diagnostic, + // with more details and location information. path, locations, ok := findVolumeInBundle(b, catalogName, schemaName, volumeName) if !ok { return nil, "", diag.Diagnostics{baseErr} diff --git a/bundle/libraries/filer_volume_test.go b/bundle/libraries/filer_volume_test.go index bbd8f20f3..d084d936a 100644 --- a/bundle/libraries/filer_volume_test.go +++ b/bundle/libraries/filer_volume_test.go @@ -87,7 +87,33 @@ func TestFindVolumeInBundle(t *testing.T) { assert.Equal(t, dyn.MustPathFromString("resources.volumes.foo"), path) } -func TestFilerForVolumeNotInBundle(t *testing.T) { +func TestFilerForVolumeForErrorFromAPI(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + ArtifactPath: "/Volumes/main/my_schema/my_volume", + }, + }, + } + + bundletest.SetLocation(b, "workspace.artifact_path", []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}) + + m := mocks.NewMockWorkspaceClient(t) + m.WorkspaceClient.Config = &sdkconfig.Config{} + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(fmt.Errorf("error from API")) + b.SetWorkpaceClient(m.WorkspaceClient) + + _, _, diags := filerForVolume(context.Background(), b) + assert.Equal(t, diag.Diagnostics{ + { + Severity: diag.Error, + Summary: "unable to determine if volume at /Volumes/main/my_schema/my_volume exists: error from API", + Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, + Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, + }}, diags) +} + +func TestFilerForVolumeWithVolumeNotFound(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -100,20 +126,20 @@ func TestFilerForVolumeNotInBundle(t *testing.T) { m := mocks.NewMockWorkspaceClient(t) m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(fmt.Errorf("error from API")) + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/doesnotexist").Return(apierr.NotFound("some error message")) b.SetWorkpaceClient(m.WorkspaceClient) _, _, diags := filerForVolume(context.Background(), b) assert.Equal(t, diag.Diagnostics{ { Severity: diag.Error, - Summary: "failed to fetch metadata for /Volumes/main/my_schema/doesnotexist: error from API", + Summary: "volume /Volumes/main/my_schema/doesnotexist does not exist: some error message", Locations: []dyn.Location{{File: "config.yml", Line: 1, Column: 2}}, Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path")}, }}, diags) } -func TestFilerForVolumeInBundle(t *testing.T) { +func TestFilerForVolumeNotFoundAndInBundle(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Workspace: config.Workspace{ @@ -139,17 +165,14 @@ func TestFilerForVolumeInBundle(t *testing.T) { m := mocks.NewMockWorkspaceClient(t) m.WorkspaceClient.Config = &sdkconfig.Config{} - m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(&apierr.APIError{ - StatusCode: 404, - Message: "error from API", - }) + m.GetMockFilesAPI().EXPECT().GetDirectoryMetadataByDirectoryPath(mock.Anything, "/Volumes/main/my_schema/my_volume").Return(apierr.NotFound("error from API")) b.SetWorkpaceClient(m.WorkspaceClient) _, _, diags := GetFilerForLibraries(context.Background(), b) assert.Equal(t, diag.Diagnostics{ { Severity: diag.Error, - Summary: "failed to fetch metadata for /Volumes/main/my_schema/my_volume: error from API", + Summary: "volume /Volumes/main/my_schema/my_volume does not exist: error from API", Locations: []dyn.Location{{"config.yml", 1, 2}, {"volume.yml", 1, 2}}, Paths: []dyn.Path{dyn.MustPathFromString("workspace.artifact_path"), dyn.MustPathFromString("resources.volumes.foo")}, Detail: `You are using a volume in your artifact_path that is managed by diff --git a/internal/bundle/artifacts_test.go b/internal/bundle/artifacts_test.go index 2d3c151fb..6a3992fb5 100644 --- a/internal/bundle/artifacts_test.go +++ b/internal/bundle/artifacts_test.go @@ -259,7 +259,7 @@ func TestAccUploadArtifactFileToVolumeThatDoesNotExist(t *testing.T) { stdout, stderr, err := internal.RequireErrorRun(t, "bundle", "deploy") assert.Error(t, err) - assert.Equal(t, fmt.Sprintf(`Error: failed to fetch metadata for /Volumes/main/%s/doesnotexist: Not Found + assert.Equal(t, fmt.Sprintf(`Error: volume /Volumes/main/%s/doesnotexist does not exist: Not Found at workspace.artifact_path in databricks.yml:6:18 @@ -296,7 +296,7 @@ func TestAccUploadArtifactToVolumeNotYetDeployed(t *testing.T) { stdout, stderr, err := internal.RequireErrorRun(t, "bundle", "deploy") assert.Error(t, err) - assert.Equal(t, fmt.Sprintf(`Error: failed to fetch metadata for /Volumes/main/%s/my_volume: Not Found + assert.Equal(t, fmt.Sprintf(`Error: volume /Volumes/main/%s/my_volume does not exist: Not Found at workspace.artifact_path resources.volumes.foo in databricks.yml:6:18