Refactor config loaders by kartikgupta-db · Pull Request #978 · databricks/databricks-vscode

kartikgupta-db · 2023-12-15T07:48:50Z

Changes

This changes the *ConfigReaderWriter classes into *ConfigModel classes. We introduce the following architecture:

Configs storage is handled by "Storage Medium" such as BundleFileSet and StateStorage. They emit event if there is a change. If we can't monitor the underlying medium for changes, we must emit event from the "set" method (see StateStorage).
Models are responsible for extracting their respective domain of configs from whatever storage medium they choose. They are also responsible for emitting and event if there is a change in any of the configs in their domain.
- Models might need some information such as target and auth params, to automatically refresh the configs in their domain. There are multiple setters which are exposed by each loader for taking this information. We want to have multiple setters and not a single one because multiple pieces of information can come at different times. Eg target -> load auth info and login -> pass auth params to loader
Models are responsible for correctly writing data to the storage medium.

Tests

packages/databricks-vscode/src/locking/CachedValue.ts

packages/databricks-vscode/src/configuration/ConnectionManager.ts

ilia-db · 2023-12-20T16:08:05Z

packages/databricks-vscode/src/configuration/ConfigModel.ts

+        private readonly overrideableConfigLoader: OverrideableConfigLoader,
+        private readonly overrideableConfigWriter: OverrideableConfigWriter,
+        public readonly bundleFileConfigLoader: BundleFileConfigLoader,
+        private readonly bundleFileConfigWriter: BundleFileConfigWriter,


I don't fully understand the reason behind splitting things into loaders and writers. They all have similar methods and operate on the same data. If the underlying data shape changes or we want to add some new methods, then both classes have to be changed at the same time, indicating that they might need to be the same thing after all. Fewer abstractions and moving parts is usually a good thing. But it feels like I'm missing some benefits here.

On top of that we have chains of CachedValues - the configCache here depends on the bundleFileConfigLoader (plus overrides cache), which depends on the bundleFileSet cache. It's hard to say if the current logic is correct and if all the caches are invalidated at the right times.

Makes sense to cache the IO responses, but I'm not sure about the intermediate cache from the FileConfigLoader for example. Can we try to simplify? Again, I might be missing some important edge cases, but in my view something like this can be a bit simpler and easier to maintain and evolve:

FSConfigModel - the class that reads and writes bundle configuration from to the file system. Uses the file watcher internally and exposes change events. Can cache the values. BundleFileSet, BundleFileConfigLoader, and BunleFileConfigWriter are removed.

OverridesConfigModel - the class that reads and writes bundle configuration from and to the vscode storage, doesn't really need a cache. OverrideableConfigLoader and OverrideConfigWriter are removed.

Those models will be used by the main ConfigModel and no one else

The current target name is saved only in the main ConfigModel, the internal models accept the current target name name as an argument to their methods, where necessary

The main ConfigModel also doesn't need a cache, but we can keep it if it makes the change events easier to implement

I wanted to split into the loaders because this way we can have multiple loaders for different lifecycle steps for bundles (before auth and post auth), but common writer. You are right that we do not really need this for override. I will revert that to a common one.

As for cache chains, this is essentially an abstraction for limiting change events.

DataIo classes like BundleFileset cache changes to files and react when there are actual changes.
Loader cache changes to only the relevant configs. And only react when these specific ones change.
ConfigModel is a consolidated view and that only changes when the specific configs change.

We have such caches hidden throughout the code. This just makes it more abstracted. We will even need it for override model in the future when we move to storing stuff in project.json again. The general feeling in the room was that people were not happy with vs code internal storage.

we can have multiple loaders for different lifecycle steps for bundles (before auth and post auth), but common writer

Hmm, I guess that's a question for the other PR, but why do we want to write to the bundle file something that was produced from the bundle validate command? We need to get the config with interpolated variables from the CLI, but that's not something we want to write back to the config file, right?

And another question is why do we need BundleFileConfigLoader (together with BundleFileSet) at all if we don't do variables interpolation ourselves? Seems like we can't use any of the values from it, as anything can be ${someVar} string instead of a real value.

…e into refactor-config-loaders

packages/databricks-vscode/src/configuration/writers/BundleFileConfigWriter.ts

ilia-db · 2024-01-08T15:11:40Z

packages/databricks-vscode/src/configuration/loaders/OverrideableConfigLoaderWriter.ts

+import {Disposable} from "vscode";
+import {Mutex} from "../../locking";
+
+export class OverrideableConfigLoaderWriter implements Disposable {


loaders/OverrideableConfigLoaderWriter.ts - nitpick, but should the "writer" be in the loader directory? Can we move things from here to one level higher and just name things as *Model (or without "model", pretty much the same as we had before)?

I agree. I refactored this. Also refactored a bunch of other code to be more streamlined. Ig we can make the next PR a bit more streamlined as well! Thanks for the patience here :)

## packages/databricks-vscode ## (2024-03-07) * Refactor configuration UI to better support override and bundle configuration model (#940) ([b5a80a7](b5a80a7)), closes [#940](#940) * Add a model for remote state (`BundleRemoteStateModel`) (#1006) ([125a2ec](125a2ec)), closes [#1006](#1006) * Add a note to our quick start (#1126) ([93cbbe5](93cbbe5)), closes [#1126](#1126) * Add a view for DABs resource viewer (#1002) ([b853df6](b853df6)), closes [#1002](#1002) * Add bundle project manager (#1011) ([acfa61a](acfa61a)), closes [#1011](#1011) * Add config level change events in `ConfigModel` (#937) ([84e2198](84e2198)), closes [#937](#937) * Add DATABRICKS_CLI_UPSTREAM env var to CLI calls (#1112) ([8c60fa5](8c60fa5)), closes [#1112](#1112) * Add decoration provider for decorating treeView items with modified status (#1035) ([863162f](863162f)), closes [#1035](#1035) * Add e2e tests for bundle init flow (#1073) ([efa3c6b](efa3c6b)), closes [#1073](#1073) * Add e2e tests for standalone running flows (#1085) ([168d69f](168d69f)), closes [#1085](#1085) * Add full UI support for supported auth types loaded from profiles (#1061) ([af29a83](af29a83)), closes [#1061](#1061) * Add icons for more task types (#1030) ([3682c76](3682c76)), closes [#1030](#1030) * Add telemetry for bundle init flow (#1117) ([ee955d4](ee955d4)), closes [#1117](#1117) * Add telemetry for the v2 (#1105) ([12f306a](12f306a)), closes [#1105](#1105) * Add UI to configure PAT auth (#1033) ([2533aec](2533aec)), closes [#1033](#1033) * Added functionality to deploy the current bundle (#1010) ([936fc49](936fc49)), closes [#1010](#1010) * All `AuthProviders` must implement a non silent `check`. (#939) ([41f076c](41f076c)), closes [#939](#939) * Allow configuring current project with minimal config (#1039) ([385d1fc](385d1fc)), closes [#1039](#1039) * Allow right click -> copy for all tree items (#1101) ([ab214b4](ab214b4)), closes [#1101](#1101) * Append CLI path to make sure it is picked up last (#1121) ([53debe9](53debe9)), closes [#1121](#1121) * Append profile to databrickscfg instead of writing the full file (#1060) ([2af50c6](2af50c6)), closes [#1060](#1060) * Automatically read auth info from override, bundle and .databrickscfg, in order (#995) ([956a9e6](956a9e6)), closes [#995](#995) * Backend for reading and writing overrides for bundle config (#901) ([157fb6f](157fb6f)), closes [#901](#901) * Bring missing changes from the main branch (#1093) ([e2adc6d](e2adc6d)), closes [#1093](#1093) * Bump CLI to 0.215.0 (#1119) ([c121b60](c121b60)), closes [#1119](#1119) * Bump databricks-cli to v0.212.3 (#1037) ([661b66f](661b66f)), closes [#1037](#1037) * Bump to 2.0.0 (#1049) ([2dcf080](2dcf080)), closes [#1049](#1049) * Bundle integ merge main (#1009) ([d8852bd](d8852bd)), closes [#1009](#1009) * Bundle watcher (#892) ([559911c](559911c)), closes [#892](#892) * Cancel run in the UI when CLI fails (#1102) ([71303b8](71303b8)), closes [#1102](#1102) * Check for idle deployment state and not running (#1064) ([f2f0126](f2f0126)), closes [#1064](#1064) * Create profile wizard for Azure-Cli and OAuth (U2M) auth types (#990) ([263f731](263f731)), closes [#990](#990) * Deeplink to the PAT creation page in PAT profile creation wizard (#1047) ([fc9d3e7](fc9d3e7)), closes [#1047](#1047) * Delete yaml from files watched by bundle autocomplete, when the yaml is deleted (#931) ([76d7d37](76d7d37)), closes [#931](#931) [/github.com//pull/922#discussion_r1382076227](https://github.com//github.com/databricks/databricks-vscode/pull/922/issues/discussion_r1382076227) * Deploy and run resource (#1012) ([9dcbd87](9dcbd87)), closes [#1012](#1012) * Deploy bundle before running files using remote run modes (#1019) ([a55eb15](a55eb15)), closes [#1019](#1019) * Dissallow all overrides except cluster ID for dev mode and Auth Params for all modes (#972) ([837d3f2](837d3f2)), closes [#972](#972) * Don't allow per file run modes in prod/staging target (#1032) ([5e44b1f](5e44b1f)), closes [#1032](#1032) * Don't show deeplinks for undeployed resources (#1043) ([62a47e3](62a47e3)), closes [#1043](#1043) * Expose bundle-init wizard in empty workspace (#1034) ([4f8ece7](4f8ece7)), closes [#1034](#1034) * Fix "Cluster not found" error when attaching cluster on target change (#1003) ([ab4e603](ab4e603)), closes [#1003](#1003) * Fix bundle-init on windows (#1058) ([626e134](626e134)), closes [#1058](#1058) * Fix bundle-run, use logging env vars (#1048) ([26e03ad](26e03ad)), closes [#1048](#1048) * Fix deeplinks format to work across all clouds (#1044) ([0f85446](0f85446)), closes [#1044](#1044) * Fix regression where bundle logs are not displayed during deploy (#1086) ([9e05703](9e05703)), closes [#1086](#1086) * Fix regression where switching targets, with the same host, does not trigger a model refresh (#1088) ([d5cfc5e](d5cfc5e)), closes [#1088](#1088) * Hide authentication quickpicks when checking auth provider (#1040) ([76c33fd](76c33fd)), closes [#1040](#1040) * Hide context buttons during deployment (#1062) ([927ea42](927ea42)), closes [#1062](#1062) * Improve bundle init (#1041) ([3bb953e](3bb953e)), closes [#1041](#1041) * Improve in-progress auth UI (#1038) ([6f6fd6f](6f6fd6f)), closes [#1038](#1038) * Improve login flow, properly init cluster manager ([772d112](772d112)) * Improve login wizard (#1045) ([25f964f](25f964f)), closes [#1045](#1045) * Improve UX for bundle init folder selection (#1026) ([0035539](0035539)), closes [#1026](#1026) * Initialise -> Initialize (#1051) ([4351936](4351936)), closes [#1051](#1051) * Inject env vars into python unit test debugging sessions (#1120) ([264cb54](264cb54)), closes [#1120](#1120) * Limit onError usages to high level functions (#999) ([cac9604](cac9604)), closes [#999](#999) * Listen to changes to "host" in databricks.yml to retrigger login (#1057) ([8471909](8471909)), closes [#1057](#1057) * Make all models inherit from a common base class (#994) ([7acb093](7acb093)), closes [#994](#994) * Make DABs autocomplete detect new bundle files (#922) ([f356219](f356219)), closes [#922](#922) * Merge branch 'main' of github.com:databricks/databricks-vscode into bundle-integ (#991) ([3c578aa](3c578aa)), closes [#991](#991) * Migrate from project.json to databricks.yml (#1013) ([ba2327d](ba2327d)), closes [#1013](#1013) * Minor fixes for `configurationView` (#1031) ([9f16b58](9f16b58)), closes [#1031](#1031) * Move all dbconnect run options to Databricks Run Icon (#1066) ([c28620a](c28620a)), closes [#1066](#1066) * Move existing run options to new Databricks Run icon (#968) ([717c94d](717c94d)), closes [#968](#968) * Move openDatabricksConfigFile to utils (#1067) ([e5f0780](e5f0780)), closes [#1067](#1067) * Pass CLI path to all auth providers and login only once during bundle init (#1114) ([c6e9b0e](c6e9b0e)), closes [#1114](#1114) * Prevent running resources when cli already launched but no run status (#1072) ([7412d95](7412d95)), closes [#1072](#1072) * Read data from global workspace block in databaricks.yml (#1059) ([fd8ae92](fd8ae92)), closes [#1059](#1059) * Redirect summarise and validate errors to bundle output panel (#1076) ([65f03ee](65f03ee)), closes [#1076](#1076) * Refactor `StateStore` to make keys more explicit at the point of use. (#913) ([5b8fb23](5b8fb23)), closes [#913](#913) * Refactor config loaders (#978) ([8ce4c9a](8ce4c9a)), closes [#978](#978) * Remove bundle source (#1005) ([ea3ed45](ea3ed45)), closes [#1005](#1005) * Remove sync destination logic (#1096) ([bb63006](bb63006)), closes [#1096](#1096) * Remove unused sync code (#1087) ([83da2b5](83da2b5)), closes [#1087](#1087) * Replace `JSON.stringify` based object comparisions with `lodash.isEqual` (#943) ([3d7abe3](3d7abe3)), closes [#943](#943) * Return config source from `ConfigModel` (#941) ([9a32556](9a32556)), closes [#941](#941) * Revert "Trigger target change on prevalidate changes, so that auth resolution happens again" (#1056) ([c4f25a2](c4f25a2)), closes [#1056](#1056) [#1055](#1055) * Send environment type (tests, prod) in telemetry events (#944) ([09a59a3](09a59a3)), closes [#944](#944) * Set authprovider when logging out (#1042) ([5b1eb9b](5b1eb9b)), closes [#1042](#1042) * Setup pre-release CI for v2 (#1109) ([5b3fbd3](5b3fbd3)), closes [#1109](#1109) * Show a more actionable error message when deployment fails (#1050) ([f012ee5](f012ee5)), closes [#1050](#1050) * Show deploy and run commands in terminal (#1020) ([2cb93aa](2cb93aa)), closes [#1020](#1020) * Show resources in DABS Resource Explorer (#1008) ([f6dcb48](f6dcb48)), closes [#1008](#1008) * Show run status for jobs and pipeline (#1017) ([ad72331](ad72331)), closes [#1017](#1017) * Show t&c popup for private preview (#1128) ([d5b69e4](d5b69e4)), closes [#1128](#1128) * State message tooltips (#1100) ([f0986e1](f0986e1)), closes [#1100](#1100) * Take a hard dependency on redhat.vscode-yaml extension (#1127) ([f3354a7](f3354a7)), closes [#1127](#1127) * Trigger target change on prevalidate changes, so that auth resolution happens again (#1055) ([6c00c17](6c00c17)), closes [#1055](#1055) * Update CLI to v0.214.1 (#1099) ([ea7b079](ea7b079)), closes [#1099](#1099) * Update dependencies (#974) ([1835fc2](1835fc2)), closes [#974](#974) * Update JS SDK to 0.6.1 (#1053) ([847ce72](847ce72)), closes [#1053](#1053) * Update login wizard and show cancel button for running resources (#1071) ([fa4013c](fa4013c)), closes [#1071](#1071) * Update NOTICE.md (#1115) ([8d60383](8d60383)), closes [#1115](#1115) * Use `bundle validate` to load interpolated view of configs after login (#979) ([6b2caca](6b2caca)), closes [#979](#979) * Use a tree representation to structure resource explorer UI components (#1029) ([a3c5f14](a3c5f14)), closes [#1029](#1029) * Use bundle for configuration and not project.json (#924) ([29474b3](29474b3)), closes [#924](#924) * Use raw app insights keys for the telemetry reporter (#1116) ([18bb45b](18bb45b)), closes [#1116](#1116) * Use sync icon for sync destination and make it green (#1054) ([5da4b76](5da4b76)), closes [#1054](#1054) ## packages/databricks-vscode-types ## (2024-03-07) * Bump to 2.0.0 (#1049) ([2dcf080](2dcf080)), closes [#1049](#1049) * Bundle integ merge main (#1009) ([d8852bd](d8852bd)), closes [#1009](#1009) * Merge branch 'main' of github.com:databricks/databricks-vscode into bundle-integ (#991) ([3c578aa](3c578aa)), closes [#991](#991) * Update dependencies (#974) ([1835fc2](1835fc2)), closes [#974](#974) * Use bundle for configuration and not project.json (#924) ([29474b3](29474b3)), closes [#924](#924) --------- Co-authored-by: releasebot <[email protected]> Co-authored-by: kartikgupta-db <[email protected]>

kartikgupta-db added 3 commits December 14, 2023 12:22

Refactor onChange to return event in configModel

b38d159

lint

041a996

Refactor into separate loaders and writers

353761a

kartikgupta-db temporarily deployed to azure-prod-usr December 15, 2023 07:48 — with GitHub Actions Inactive

fix build and run

de9cad0

kartikgupta-db temporarily deployed to azure-prod-usr December 15, 2023 10:27 — with GitHub Actions Inactive

kartikgupta-db requested a review from ilia-db December 18, 2023 12:26

ilia-db reviewed Dec 20, 2023

View reviewed changes

packages/databricks-vscode/src/locking/CachedValue.ts Outdated Show resolved Hide resolved

packages/databricks-vscode/src/configuration/ConnectionManager.ts Show resolved Hide resolved

ilia-db reviewed Dec 20, 2023

View reviewed changes

kartikgupta-db added 2 commits January 2, 2024 11:00

Merge branch 'bundle-integ' of github.com:databricks/databricks-vscod…

62627d1

…e into refactor-config-loaders

merge overrideable config loader and writer; use mutex.sync

fc45c6b

kartikgupta-db force-pushed the refactor-config-loaders branch from a1b5261 to fc45c6b Compare January 2, 2024 10:01

kartikgupta-db temporarily deployed to azure-prod-usr January 2, 2024 10:02 — with GitHub Actions Inactive

kartikgupta-db requested a review from ilia-db January 8, 2024 13:54

Merge branch 'bundle-integ' of github.com:databricks/databricks-vscod…

37eec44

…e into refactor-config-loaders

kartikgupta-db temporarily deployed to azure-prod-usr January 8, 2024 14:15 — with GitHub Actions Inactive

merge

3fcc902

kartikgupta-db force-pushed the refactor-config-loaders branch from f1c4a32 to 3fcc902 Compare January 8, 2024 14:16

kartikgupta-db temporarily deployed to azure-prod-usr January 8, 2024 14:17 — with GitHub Actions Inactive

lint

736031a

kartikgupta-db temporarily deployed to azure-prod-usr January 8, 2024 14:19 — with GitHub Actions Inactive

ilia-db reviewed Jan 8, 2024

View reviewed changes

Refactor back into consolidated *configModels

f83c021

kartikgupta-db temporarily deployed to azure-prod-usr January 8, 2024 17:00 — with GitHub Actions Inactive

kartikgupta-db requested a review from ilia-db January 8, 2024 17:39

ilia-db approved these changes Jan 9, 2024

View reviewed changes

kartikgupta-db merged commit 8ce4c9a into bundle-integ Jan 9, 2024

kartikgupta-db deleted the refactor-config-loaders branch January 9, 2024 11:51

This was referenced Mar 5, 2024

Release: v2.0.0-preview #1110

Closed

Release: v2.0.1-preview #1130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor config loaders#978

Refactor config loaders#978
kartikgupta-db merged 10 commits intobundle-integfrom
refactor-config-loaders

kartikgupta-db commented Dec 15, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

ilia-db Dec 20, 2023

Uh oh!

kartikgupta-db Dec 20, 2023

Uh oh!

kartikgupta-db Dec 20, 2023

Uh oh!

ilia-db Dec 20, 2023 •

edited

Loading

Uh oh!

Uh oh!

ilia-db Jan 8, 2024

Uh oh!

kartikgupta-db Jan 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kartikgupta-db commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

Uh oh!

Uh oh!

ilia-db Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

kartikgupta-db Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

kartikgupta-db Dec 20, 2023

Choose a reason for hiding this comment

Uh oh!

ilia-db Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ilia-db Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

kartikgupta-db Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kartikgupta-db commented Dec 15, 2023 •

edited

Loading

ilia-db Dec 20, 2023 •

edited

Loading