Move artifacts validation to initialize phase; simplify by denik · Pull Request #2526 · databricks/cli

denik · 2025-03-19T15:39:30Z

Changes

Moved processing & validating of artifacts section into initialize phase so that it runs as part of 'bundle validate' and results can be inspected in 'bundle validate -o json'.
The following things where moved from build phase to initialize:

Creating python_artifact when artifacts is nil.
Adding default 'build' field for whl artifacts.
Expanding globs for cases where 'build' is absent.
Cleaning up 'dist' and 'build' folders.

I've also removed a bunch of abstractions and put everything into two (small) mutators

artifacts.Validate() to do all the validations and processing
artifacts.Build() to do the build and related post-processing.

Why

Having this things in validate allows users to learn early if something is wrong with the configuration.
Putting everything into 2 mutators allows to understand the whole flow around artifacts. I need that for expanding it with dynamic_version feature Implement dynamic_version attribute in artifacts section #2520.

Tests

Existing tests.
Converted expand_globs_test.go into acceptance test that runs "bundle validate".

andrewnester · 2025-03-19T15:52:48Z

+	"github.com/databricks/cli/libs/python"
+)
+
+func Validate() bundle.Mutator {


It does more than just validation, shall we call it SetupAndValidate?

Makes sense not to use "Validate". Prefer something shorter than SetupAndValidate. "Initialize" or "Prepare"?

andrewnester · 2025-03-19T16:04:48Z


 ### Bundles

+* Processing 'artifacts' section is now done in "bundle validate" (adding defaults, inferring "build", asserting required fields).


Can you please add a link to the PR as in the other changelog entries?

added, thanks

denik · 2025-03-20T08:11:38Z

The error in merge queue is unrelated to the PR (cc @shreyas-goenka):

=== Failed
=== FAIL: libs/daemon TestDaemon (0.32s)
daemon_test.go:38: Error Trace:	/home/runner/work/cli/cli/libs/daemon/daemon_test.go:38
Error: Received unexpected error:
strconv.Atoi: parsing "": invalid syntax
Test: TestDaemon

…2530) This test highlights inconsistency between type:whl and non-whl artifacts. The test for non-whl artifacts exists [here](https://github.com/databricks/cli/tree/main/acceptance/bundle/artifacts/build_and_files) added in #2526

Bug introduced in #2526.

## Changes Bug introduced in #2526. When calling expandGlobs as a function without mutator entry/exit copy, we did not copy typed structure into dynamic structure. Thus expandGlobs saw an old version of config, without absolute paths introduced by code before it. Since it calls filepath.Glob which depends on CWD, it stopped doing the matching. ## Tests New regression test to catch this issues.

## Release v0.245.0 ### Bundles * Processing 'artifacts' section is now done in "bundle validate" (adding defaults, inferring "build", asserting required fields) ([#2526])(#2526)) * When uploading artifacts, include relative path in log message ([#2539])(#2539)) * Added support for clusters in deployment bind/unbind commands ([#2536](#2536)) * Added support for volumes in deployment bind/unbind commands ([#2527](#2527)) * Added support for dashboards in deployment bind/unbind commands ([#2516](#2516)) * Added support for registered models in deployment bind/unbind commands ([#2556](#2556)) * Added a mismatch check when host is defined in config and as an env variable ([#2549](#2549)) * New attribute on artifacts entries: `dynamic_version`. When set to true, it patches the wheel with dynamic version suffix so it is always used by Databricks environments, even if original wheel version is the same. Intended for development loop on interactive clusters. ([#2520](#2520))

## Changes - Move artifacts clean up procedure to "bundle deploy". This reverts change in behaviour introduced in 0.245.0 #2526 ## Why We don't expect major changes on the file system during "validate", it should be as much as possible about reporting issues with the bundle without taking action. ## Tests Existing tests.

## Changes Moved processing & validating of artifacts section into initialize phase so that it runs as part of 'bundle validate' and results can be inspected in 'bundle validate -o json'. The following things where moved from build phase to initialize: - Creating python_artifact when artifacts is nil. - Adding default 'build' field for whl artifacts. - Expanding globs for cases where 'build' is absent. - Cleaning up 'dist' and 'build' folders. I've also removed a bunch of abstractions and put everything into two (small) mutators - artifacts.Validate() to do all the validations and processing - artifacts.Build() to do the build and related post-processing. ## Why - Having this things in validate allows users to learn early if something is wrong with the configuration. - Putting everything into 2 mutators allows to understand the whole flow around artifacts. I need that for expanding it with dynamic_version feature #2520. ## Tests Existing tests. Converted expand_globs_test.go into acceptance test that runs "bundle validate".

…2530) This test highlights inconsistency between type:whl and non-whl artifacts. The test for non-whl artifacts exists [here](https://github.com/databricks/cli/tree/main/acceptance/bundle/artifacts/build_and_files) added in #2526

## Changes Bug introduced in #2526. When calling expandGlobs as a function without mutator entry/exit copy, we did not copy typed structure into dynamic structure. Thus expandGlobs saw an old version of config, without absolute paths introduced by code before it. Since it calls filepath.Glob which depends on CWD, it stopped doing the matching. ## Tests New regression test to catch this issues.

## Release v0.245.0 ### Bundles * Processing 'artifacts' section is now done in "bundle validate" (adding defaults, inferring "build", asserting required fields) ([#2526])(#2526)) * When uploading artifacts, include relative path in log message ([#2539])(#2539)) * Added support for clusters in deployment bind/unbind commands ([#2536](#2536)) * Added support for volumes in deployment bind/unbind commands ([#2527](#2527)) * Added support for dashboards in deployment bind/unbind commands ([#2516](#2516)) * Added support for registered models in deployment bind/unbind commands ([#2556](#2556)) * Added a mismatch check when host is defined in config and as an env variable ([#2549](#2549)) * New attribute on artifacts entries: `dynamic_version`. When set to true, it patches the wheel with dynamic version suffix so it is always used by Databricks environments, even if original wheel version is the same. Intended for development loop on interactive clusters. ([#2520](#2520))

## Changes - Move artifacts clean up procedure to "bundle deploy". This reverts change in behaviour introduced in 0.245.0 #2526 ## Why We don't expect major changes on the file system during "validate", it should be as much as possible about reporting issues with the bundle without taking action. ## Tests Existing tests.

denik added 11 commits March 19, 2025 16:28

WIP
8eeb6dc

add acc test for expand_globs
602118e

add output.txt
596a0b3

fix
d7b0bc0

fixes
fa97c0c

add globs_invalid
67d746c

globs_in_files
f9b9d34

clean up
6c45063

update acc tests
f38d9f2

clean up
11e5f11

add missing validate.go
de893fb

denik requested review from andrewnester, pietern and shreyas-goenka as code owners March 19, 2025 15:39

denik temporarily deployed to test-trigger-is March 19, 2025 15:39 — with GitHub Actions Inactive

clean up
c509947

denik temporarily deployed to test-trigger-is March 19, 2025 15:43 — with GitHub Actions Inactive

denik added 2 commits March 19, 2025 16:45

remove ()
c7c7fbc

update acc tests
762b1d9

denik temporarily deployed to test-trigger-is March 19, 2025 15:47 — with GitHub Actions Inactive

update NEXT_CHANGELOG.md
388b881

denik temporarily deployed to test-trigger-is March 19, 2025 15:50 — with GitHub Actions Inactive

add replacement for windows
8421379

denik temporarily deployed to test-trigger-is March 19, 2025 16:00 — with GitHub Actions Inactive

denik changed the title ~~Move artifacts processing to validate; simplify~~Move artifacts validation to initialize phase; simplifyMar 19, 2025

andrewnester approved these changes Mar 19, 2025
View reviewed changes

add link to PR in NEXT_CHANGELOG.md
4e961c5

denik temporarily deployed to test-trigger-is March 19, 2025 16:10 — with GitHub Actions Inactive

rename Validate() to Prepare()
46c9ce8

denik temporarily deployed to test-trigger-is March 19, 2025 19:53 — with GitHub Actions Inactive

denik added 2 commits March 19, 2025 22:33

windows slash fix
e35b6c3

ignore build/path (for windows)
844f003

denik force-pushed the denik/simplify-artifacts branch from e9d2d72 to 844f003Compare March 19, 2025 21:33

denik temporarily deployed to test-trigger-is March 19, 2025 21:33 — with GitHub Actions Inactive

denik enabled auto-merge March 19, 2025 21:35

denik added this pull request to the merge queue Mar 19, 2025

github-merge-queueBot removed this pull request from the merge queue due to failed status checks Mar 19, 2025

denik added this pull request to the merge queue Mar 20, 2025

Merged via the queue into main with commit 3985c43Mar 20, 2025

denik deleted the denik/simplify-artifacts branch March 20, 2025 08:19

denik mentioned this pull request Mar 20, 2025
Add acc test for case where both 'build' and 'files' are specified #2530
Merged

denik added a commit that referenced this pull request Mar 20, 2025
Add a test that show the wheel being uploaded twice
f5d4b5f
Bug introduced in #2526.

denik mentioned this pull request Mar 20, 2025
Fix a bug in artifacts where glob was applied on non-abs path #2540
Merged

denik added a commit that referenced this pull request Mar 20, 2025
Add a test that show the wheel being uploaded twice
bcc702b
Bug introduced in #2526.

denik added a commit that referenced this pull request Mar 20, 2025
Add a test that show the wheel being uploaded twice
1a8dc5d
Bug introduced in #2526.

denik added a commit that referenced this pull request Mar 20, 2025
Add a test that show the wheel being uploaded twice
e1e973f
Bug introduced in #2526.

cptshrk108 mentioned this pull request Apr 7, 2025
Artifacts clean up bug fix causing issues #2671
Closed

denik mentioned this pull request Apr 15, 2025
Move artifacts clean up back to deploy phase #2722
Merged

magnusasperud mentioned this pull request Jun 2, 2025
DABs cleanup of dist and .internal causing locked wheel versions to fail #2969
Closed

Navigation Menu

Uh oh!

Move artifacts validation to initialize phase; simplify - #2526

Move artifacts validation to initialize phase; simplify#2526
denik merged 24 commits into
mainfrom
denik/simplify-artifacts

denik commented Mar 19, 2025•
edited
Loading

Uh oh!

andrewnesterMar 19, 2025

Uh oh!

denikMar 19, 2025

Uh oh!

andrewnesterMar 19, 2025

Uh oh!

denikMar 19, 2025

Uh oh!

Uh oh!

denik commented Mar 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		### Bundles

		* Processing 'artifacts' section is now done in "bundle validate" (adding defaults, inferring "build", asserting required fields).

Uh oh!

Conversation

denik commented Mar 19, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Why

Tests

Uh oh!

andrewnesterMar 19, 2025

Choose a reason for hiding this comment

Uh oh!

denikMar 19, 2025

Choose a reason for hiding this comment

Uh oh!

andrewnesterMar 19, 2025

Choose a reason for hiding this comment

Uh oh!

denikMar 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

denik commented Mar 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

denik commented Mar 19, 2025•
edited
Loading