Correctly identify local paths in libraries section #702

andrewnester · 2023-08-28T10:32:08Z

Changes

Fixes #699

Tests

Added unit test

pietern · 2023-08-28T15:16:17Z

bundle/libraries/libraries.go

@@ -92,13 +93,13 @@ func findArtifactsAndMarkForUpload(ctx context.Context, lib *compute.Library, b
 	}

 	if len(matches) == 0 && isLocalLibrary(lib) {
-		return fmt.Errorf("no library found for %s", libPath(lib))
+		return fmt.Errorf("file %s is referenced in libraries section but no such file found on local file system", libPath(lib))


Suggested change

return fmt.Errorf("file %s is referenced in libraries section but no such file found on local file system", libPath(lib))

return fmt.Errorf("file %s is referenced in libraries section but doesn't exist on the local file system", libPath(lib))

I think this only happens as part of the deploy, but I suspect we can check that this is all valid during validation if we don't require a build step, right?

libraries.MatchWithArtifacts must happen after artifact build and infer steps. Otherwise logic in MatchWithArtifacts becomes to complex and conditional, for example, is there no file locally because it was not yet build, or it really does not exists, or it wasn't infered.

The preferred flow is

Read the config and artifacts section

Infer anything that is missing and we can infer

Build artifacts based on the result configuration

Take the result artifacts configuration and state of the file system and see if libraries used matches the one on FS.

Thanks, understood. We should capture this somewhere.

pietern · 2023-08-28T15:20:43Z

bundle/libraries/libraries.go

 	}

 	for _, match := range matches {
 		af, err := findArtifactFileByLocalPath(match, b)
 		if err != nil {
-			cmdio.LogString(ctx, fmt.Sprintf("%s. Skipping %s. In order to use the library upload it manually", err.Error(), match))
+			cmdio.LogString(ctx, fmt.Sprintf("%s. Skipping uploading. In order to use the define 'artifacts' section", err.Error()))
 		} else {


If #703 is merged then will this condition still be valid?

@pietern it will be valid when an user defines and artifact section and then specifies libraries section which points to some different wheel. It might happen in case of monorepo where multiple wheel packages can be in different folder.

Yeah that's right -- it still feels weird to have to define this section when referring to a file.

Let's keep this in the back of our head when going over these flows again. Can keep as is now.

bundle/libraries/libraries.go

pietern

Does specifying a path with a file:// actually work?

I would expect it to not resolve the local path. There's also no test case for it.

andrewnester · 2023-08-29T08:27:36Z

@pietern I’ll verify the behaviour but indeed it’s likely not working as expected

CLI: * Fixed --environment flag ([#705](#705)). Bundles: * Support cluster overrides with cluster_key and compute_key ([#696](#696)). * Allow referencing local Python wheels without artifacts section defined ([#703](#703)). * Correctly identify local paths in libraries section ([#702](#702)). * Fixed path joining in FindFilesWithSuffixInPath ([#704](#704)). * Added transformation mutator for Python wheel task for them to work on DBR <13.1 ([#635](#635)). Internal: * Add a foundation for built-in templates ([#685](#685)).

Bundles: * Support cluster overrides with cluster_key and compute_key ([#696](#696)). * Allow referencing local Python wheels without artifacts section defined ([#703](#703)). * Fixed --environment flag ([#705](#705)). * Correctly identify local paths in libraries section ([#702](#702)). * Fixed path joining in FindFilesWithSuffixInPath ([#704](#704)). * Added transformation mutator for Python wheel task for them to work on DBR <13.1 ([#635](#635)). Internal: * Add a foundation for built-in templates ([#685](#685)). * Test transform when no Python wheel tasks defined ([#714](#714)). * Pin Terraform binary version to 1.5.5 ([#715](#715)). * Cleanup after "Add a foundation for built-in templates" ([#707](#707)). * Filter down to Python wheel tasks only for trampoline ([#712](#712)). * Update Terraform provider schema structs from 1.23.0 ([#713](#713)).

## Changes Fixes #699 ## Tests Added unit test

Bundles: * Support cluster overrides with cluster_key and compute_key ([#696](#696)). * Allow referencing local Python wheels without artifacts section defined ([#703](#703)). * Fixed --environment flag ([#705](#705)). * Correctly identify local paths in libraries section ([#702](#702)). * Fixed path joining in FindFilesWithSuffixInPath ([#704](#704)). * Added transformation mutator for Python wheel task for them to work on DBR <13.1 ([#635](#635)). Internal: * Add a foundation for built-in templates ([#685](#685)). * Test transform when no Python wheel tasks defined ([#714](#714)). * Pin Terraform binary version to 1.5.5 ([#715](#715)). * Cleanup after "Add a foundation for built-in templates" ([#707](#707)). * Filter down to Python wheel tasks only for trampoline ([#712](#712)). * Update Terraform provider schema structs from 1.23.0 ([#713](#713)).

## Changes Fixes databricks#699 ## Tests Added unit test Signed-off-by: Arpit Jasapara <arpit.jasapara@databricks.com>

Bundles: * Support cluster overrides with cluster_key and compute_key ([databricks#696](databricks#696)). * Allow referencing local Python wheels without artifacts section defined ([databricks#703](databricks#703)). * Fixed --environment flag ([databricks#705](databricks#705)). * Correctly identify local paths in libraries section ([databricks#702](databricks#702)). * Fixed path joining in FindFilesWithSuffixInPath ([databricks#704](databricks#704)). * Added transformation mutator for Python wheel task for them to work on DBR <13.1 ([databricks#635](databricks#635)). Internal: * Add a foundation for built-in templates ([databricks#685](databricks#685)). * Test transform when no Python wheel tasks defined ([databricks#714](databricks#714)). * Pin Terraform binary version to 1.5.5 ([databricks#715](databricks#715)). * Cleanup after "Add a foundation for built-in templates" ([databricks#707](databricks#707)). * Filter down to Python wheel tasks only for trampoline ([databricks#712](databricks#712)). * Update Terraform provider schema structs from 1.23.0 ([databricks#713](databricks#713)). Signed-off-by: Arpit Jasapara <arpit.jasapara@databricks.com>

Correctly identify local paths in libraries section

e23ca61

andrewnester requested review from pietern and shreyas-goenka August 28, 2023 10:32

pietern reviewed Aug 28, 2023

View reviewed changes

andrewnester added 2 commits August 28, 2023 18:14

fixes

2aa75ee

bring back missing test case

771c309

andrewnester requested a review from pietern August 28, 2023 16:20

pietern approved these changes Aug 29, 2023

View reviewed changes

andrewnester added this pull request to the merge queue Aug 29, 2023

Merged via the queue into main with commit 842cd8b Aug 29, 2023
4 checks passed

andrewnester deleted the whl-better-error-messages branch August 29, 2023 08:33

andrewnester mentioned this pull request Aug 30, 2023

Release v0.203.3 #711

Closed

andrewnester mentioned this pull request Aug 30, 2023

Release v0.203.3 #716

Merged

lennartkats-db pushed a commit that referenced this pull request Sep 1, 2023

Correctly identify local paths in libraries section (#702)

a7407f0

## Changes Fixes #699 ## Tests Added unit test

arpitjasa-db pushed a commit to arpitjasa-db/cli that referenced this pull request Sep 7, 2023

Correctly identify local paths in libraries section (databricks#702)

a0cecc8

## Changes Fixes databricks#699 ## Tests Added unit test Signed-off-by: Arpit Jasapara <arpit.jasapara@databricks.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly identify local paths in libraries section #702

Correctly identify local paths in libraries section #702

andrewnester commented Aug 28, 2023

pietern Aug 28, 2023

pietern Aug 28, 2023

andrewnester Aug 28, 2023

pietern Aug 29, 2023 •

edited

Loading

pietern Aug 28, 2023

andrewnester Aug 28, 2023

pietern Aug 29, 2023 •

edited

Loading

pietern left a comment

andrewnester commented Aug 29, 2023

	return fmt.Errorf("file %s is referenced in libraries section but no such file found on local file system", libPath(lib))
	return fmt.Errorf("file %s is referenced in libraries section but doesn't exist on the local file system", libPath(lib))

Correctly identify local paths in libraries section #702

Correctly identify local paths in libraries section #702

Conversation

andrewnester commented Aug 28, 2023

Changes

Tests

pietern Aug 28, 2023

Choose a reason for hiding this comment

pietern Aug 28, 2023

Choose a reason for hiding this comment

andrewnester Aug 28, 2023

Choose a reason for hiding this comment

pietern Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

pietern Aug 28, 2023

Choose a reason for hiding this comment

andrewnester Aug 28, 2023

Choose a reason for hiding this comment

pietern Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

pietern left a comment

Choose a reason for hiding this comment

andrewnester commented Aug 29, 2023

pietern Aug 29, 2023 •

edited

Loading

pietern Aug 29, 2023 •

edited

Loading