Added deployment state for bundles by andrewnester · Pull Request #1267 · databricks/cli

andrewnester · 2024-03-08T11:02:32Z

Changes

This PR introduces new structure (and a file) being used locally and synced remotely to Databricks workspace to track bundle deployment related metadata.

The state is pulled from remote, updated and pushed back remotely as part of bundle deploy command.

This state can be used for deployment sequencing as it's Version field is monotonically increasing on each deployment.

Currently, it only tracks files being synced as part of the deployment.

This helps fix the issue with files not being removed during deployments on CI/CD as sync snapshot was never present there.

Fixes#943

Tests

Added E2E (regression) test for files removal on CI/CD

codecov-commenter · 2024-03-08T11:05:22Z

Codecov Report

Attention: Patch coverage is 64.51613% with 121 lines in your changes are missing coverage. Please review.

Project coverage is 52.50%. Comparing base (29ab96f) to head (e5538a1).
Report is 19 commits behind head on main.

Files	Patch %	Lines
bundle/deploy/state_pull.go	64.94%	24 Missing and 10 partials ⚠️
bundle/deploy/state_update.go	56.00%	23 Missing and 10 partials ⚠️
bundle/deploy/state.go	74.69%	14 Missing and 7 partials ⚠️
bundle/deploy/state_push.go	44.00%	10 Missing and 4 partials ⚠️
internal/bundle/helpers.go	0.00%	6 Missing ⚠️
cmd/bundle/sync.go	0.00%	4 Missing ⚠️
libs/fileset/file.go	91.42%	2 Missing and 1 partial ⚠️
bundle/deploy/filer.go	0.00%	2 Missing ⚠️
bundle/deploy/terraform/state_pull.go	50.00%	1 Missing ⚠️
bundle/deploy/terraform/state_push.go	50.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@ Coverage Diff @@## main #1267 +/- ##
==========================================
- Coverage 52.54% 52.50% -0.05% 
==========================================
Files 308 321 +13 Lines 17614 18300 +686 ==========================================
+ Hits 9256 9609 +353 - Misses 7664 7967 +303 - Partials 694 724 +30

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrewnester · 2024-03-13T16:18:22Z

Integration tests passed

pietern · 2024-03-14T11:58:02Z

+
+	if b.Config.Workspace.CurrentUser != nil {
+ opts.CurrentUser = b.Config.Workspace.CurrentUser.User
+	}


Curious: when is this nil?

Good point, I don't think realistically it can, just did the check based on type and it can be potentially nil.

shreyas-goenka

Thanks! This one is slightly tricky to get right.

shreyas-goenka · 2024-03-14T17:13:58Z

+
+	// Create a new snapshot based on the deployment state file.
+	log.Infof(ctx, "Creating new snapshot")
+	snapshotState, err := sync.NewSnapshotState(state.Files.ToSlice(b.Config.Path))


We can instead merge the deployment state with the existing snapshot state here, to ensure that any files deleted locally (file is present in the deployment state but not the local sync snapshot state) to make incremental sync work (like alluded to above)

We can't really keep the local snapshot as @pietern pointed earlier for cases when someone else have made a deployment with new files. If we keep local snapshot we will just lose track of these files because we don't sync them from workspace to local system. So if someone does deployment in this case remote files will stay lingering there. But if we take deployment state as source of truth, they will be removed which is in line with local state of files when deployment was made.
Instead we better do just full sync, which makes reconsolidating a bit more straight forward.

pietern · 2024-03-18T09:54:37Z

 remoteName := filepath.ToSlash(f.Relative)
- isNotebook, _, err := notebook.Detect(f.Absolute)
+ isNotebook, err := f.IsNotebook()
+


If the underlying file does not exist on the local file system, and we're restoring a remote snapshot, then the file should still be included in the snapshot such that a new sync will issue a delete for the notebook.

Yes, indeed, that's what's happening right now (confirmed with test coverage)

Ah, I understand now; this works because whether the file is a notebook or not is cached in the underlying object. The lines are blurred w.r.t. responsibilities, between what happens in the bundle code, what happens in fileset, and what happens here. I don't have a concrete proposal for a different approach, but this feels a bit scattered. Thoughts?

I'd argue that the way things now is quite reasonable:
within snapshot_state scope if IsNotebook fails with whatever reason - we can't proceed further
within file scope we keep an internal value of isNotebook but provide public functions to create files of specific types like NewNotebookFile
within development state scope, we know what is the type of the file we work with and thus can instantiate either NewNotebookFile or NewSourceFile
I guess the confusion comes from knowing that previously we have to always do calls to FS and now that's not the case anymore

Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>

pietern

How are the integration tests looking?

pietern · 2024-03-18T13:59:00Z

 terraform.Load(),
 metadata.Compute(),
 metadata.Upload(),
+ deploy.StatePush(),


Can move right after the TF state push. We could also parallelize them.

Yeah, I think as a general improvement it would be nice to introduce bundle.Parallel and use it in various phases

pietern · 2024-03-18T14:06:44Z

 remoteName := filepath.ToSlash(f.Relative)
- isNotebook, _, err := notebook.Detect(f.Absolute)
+ isNotebook, err := f.IsNotebook()
+


Ah, I understand now; this works because whether the file is a notebook or not is cached in the underlying object. The lines are blurred w.r.t. responsibilities, between what happens in the bundle code, what happens in fileset, and what happens here. I don't have a concrete proposal for a different approach, but this feels a bit scattered. Thoughts?

andrewnester · 2024-03-18T14:41:55Z

How are the integration tests looking?

All green ✅

CLI: * Propagate correct `User-Agent` for CLI during OAuth flow ([#1264](#1264)). * Add usage string when command fails with incorrect arguments ([#1276](#1276)). Bundles: * Include `dyn.Path` as argument to the visit callback function ([#1260](#1260)). * Inline logic to set a value in `dyn.SetByPath` ([#1261](#1261)). * Add assertions for the `dyn.Path` argument to the visit callback ([#1265](#1265)). * Add `dyn.MapByPattern` to map a function to values with matching paths ([#1266](#1266)). * Filter current user from resource permissions ([#1262](#1262)). * Retain location annotation when expanding globs for pipeline libraries ([#1274](#1274)). * Added deployment state for bundles ([#1267](#1267)). * Do CheckRunningResource only after terraform.Write ([#1292](#1292)). * Rewrite relative paths using `dyn.Location` of the underlying value ([#1273](#1273)). * Push deployment state right after files upload ([#1293](#1293)). * Make `Append` function to `dyn.Path` return independent slice ([#1295](#1295)). * Move bundle tests into bundle/tests ([#1299](#1299)). * Upgrade Terraform provider to 1.38.0 ([#1308](#1308)). Internal: * Add integration test for mlops-stacks initialization ([#1155](#1155)). * Update actions/setup-python to v5 ([#1290](#1290)). * Update codecov/codecov-action to v4 ([#1291](#1291)). API Changes: * Changed `databricks catalogs list` command. * Changed `databricks online-tables create` command. * Changed `databricks lakeview publish` command. * Added `databricks lakeview create` command. * Added `databricks lakeview get` command. * Added `databricks lakeview get-published` command. * Added `databricks lakeview trash` command. * Added `databricks lakeview update` command. * Moved settings related commands to `databricks settings` and `databricks account settings`. OpenAPI commit 93763b0d7ae908520c229c786fff28b8fd623261 (2024-03-20) Dependency updates: * Bump golang.org/x/oauth2 from 0.17.0 to 0.18.0 ([#1270](#1270)). * Bump golang.org/x/mod from 0.15.0 to 0.16.0 ([#1271](#1271)). * Update Go SDK to v0.35.0 ([#1300](#1300)). * Update Go SDK to v0.36.0 ([#1304](#1304)).

## Changes This PR introduces new structure (and a file) being used locally and synced remotely to Databricks workspace to track bundle deployment related metadata. The state is pulled from remote, updated and pushed back remotely as part of `bundle deploy` command. This state can be used for deployment sequencing as it's `Version` field is monotonically increasing on each deployment. Currently, it only tracks files being synced as part of the deployment. This helps fix the issue with files not being removed during deployments on CI/CD as sync snapshot was never present there. Fixes#943 ## Tests Added E2E (regression) test for files removal on CI/CD --------- Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>

CLI: * Propagate correct `User-Agent` for CLI during OAuth flow ([#1264](#1264)). * Add usage string when command fails with incorrect arguments ([#1276](#1276)). Bundles: * Include `dyn.Path` as argument to the visit callback function ([#1260](#1260)). * Inline logic to set a value in `dyn.SetByPath` ([#1261](#1261)). * Add assertions for the `dyn.Path` argument to the visit callback ([#1265](#1265)). * Add `dyn.MapByPattern` to map a function to values with matching paths ([#1266](#1266)). * Filter current user from resource permissions ([#1262](#1262)). * Retain location annotation when expanding globs for pipeline libraries ([#1274](#1274)). * Added deployment state for bundles ([#1267](#1267)). * Do CheckRunningResource only after terraform.Write ([#1292](#1292)). * Rewrite relative paths using `dyn.Location` of the underlying value ([#1273](#1273)). * Push deployment state right after files upload ([#1293](#1293)). * Make `Append` function to `dyn.Path` return independent slice ([#1295](#1295)). * Move bundle tests into bundle/tests ([#1299](#1299)). * Upgrade Terraform provider to 1.38.0 ([#1308](#1308)). Internal: * Add integration test for mlops-stacks initialization ([#1155](#1155)). * Update actions/setup-python to v5 ([#1290](#1290)). * Update codecov/codecov-action to v4 ([#1291](#1291)). API Changes: * Changed `databricks catalogs list` command. * Changed `databricks online-tables create` command. * Changed `databricks lakeview publish` command. * Added `databricks lakeview create` command. * Added `databricks lakeview get` command. * Added `databricks lakeview get-published` command. * Added `databricks lakeview trash` command. * Added `databricks lakeview update` command. * Moved settings related commands to `databricks settings` and `databricks account settings`. OpenAPI commit 93763b0d7ae908520c229c786fff28b8fd623261 (2024-03-20) Dependency updates: * Bump golang.org/x/oauth2 from 0.17.0 to 0.18.0 ([#1270](#1270)). * Bump golang.org/x/mod from 0.15.0 to 0.16.0 ([#1271](#1271)). * Update Go SDK to v0.35.0 ([#1300](#1300)). * Update Go SDK to v0.36.0 ([#1304](#1304)).

Added deployment state for bundles
dd15c9d

andrewnester requested review from pietern and shreyas-goenka March 8, 2024 11:02

added unit tests
7a762b0

pietern reviewed Mar 13, 2024
View reviewed changes

Comment threadlibs/sync/snapshot_state.go Outdated

pietern reviewed Mar 13, 2024
View reviewed changes

fixes
680388b

andrewnester requested a review from pietern March 13, 2024 13:12

fix test
b6c1459

pietern reviewed Mar 14, 2024
View reviewed changes

shreyas-goenka reviewed Mar 14, 2024
View reviewed changes

fixes
3119ae7

andrewnester requested review from pietern and shreyas-goenka March 14, 2024 19:03

added deployment state version validation
e5538a1

pietern reviewed Mar 18, 2024
View reviewed changes

andrewnesterand others added 2 commits March 18, 2024 11:10

Update bundle/deploy/state.go
ed44963
Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>

fixes
f5f105b

andrewnester requested a review from pietern March 18, 2024 10:55

pietern approved these changes Mar 18, 2024
View reviewed changes

fixes
b977e17

andrewnester added this pull request to the merge queue Mar 18, 2024

Merged via the queue into main with commit 1b0ac61Mar 18, 2024

andrewnester deleted the deloyment-state branch March 18, 2024 14:48

pietern mentioned this pull request Mar 25, 2024
Release v0.216.0 #1309
Merged

Uh oh!

Conversation

andrewnester commented Mar 8, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

codecov-commenter commented Mar 8, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewnester commented Mar 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pietern left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewnester commented Mar 18, 2024

Uh oh!

Reviewers

Assignees

Labels

andrewnester commented Mar 8, 2024•
edited
Loading

codecov-commenter commented Mar 8, 2024•
edited
Loading