Skip to content

Commit cbb8893

Browse files
shreyas-goenkashreyas-goenka
authored
Use bricks sync in vscode extension (#105)
Tested manually that 1. The sync works 2. Switching profiles is seamless wrt syncing https://user-images.githubusercontent.com/88374338/191528171-b4ab1901-2d16-4c39-b16f-0dcfbf56666c.mov Question: how are we planning to bundle the vscode extension with the bricks cli ?
1 parent 471e2aa commit cbb8893

7 files changed

Lines changed: 40 additions & 88 deletions

File tree

‎packages/databricks-vscode/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -461,7 +461,7 @@
461461
"scripts": {
462462
"vscode:prepublish": "rm -rf out && yarn run package:compile && yarn run package:copy-webview-toolkit",
463463
"package": "vsce package",
464-
"package:cli:fetch": "BRICKS_VERSION=v0.0.2 && bash ./scripts/fetch-bricks-cli.sh ${BRICKS_VERSION} ${BRICKS_ARCH:-}",
464+
"package:cli:fetch": "BRICKS_VERSION=v0.0.3 && bash ./scripts/fetch-bricks-cli.sh ${BRICKS_VERSION} ${BRICKS_ARCH:-}",
465465
"package:cli:link": "rm -f ./bin/bricks && ln -s ../../../../bricks/bricks bin",
466466
"package:compile": "yarn run esbuild:base --minify",
467467
"package:copy-webview-toolkit": "cp ./node_modules/@vscode/webview-ui-toolkit/dist/toolkit.js ./out/toolkit.js",

‎packages/databricks-vscode/src/cli/BricksTasks.test.ts‎

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -48,16 +48,9 @@ describe(__filename, () => {
4848
);
4949

5050
let cliMock = mock(CliWrapper);
51-
when(
52-
cliMock.getSyncCommand(
53-
anything(),
54-
anything(),
55-
anything(),
56-
anything()
57-
)
58-
).thenReturn({
51+
when(cliMock.getSyncCommand(anything())).thenReturn({
5952
command: "bricks",
60-
args: ["sync", "incremental"],
53+
args: ["sync"],
6154
});
6255

6356
let task = new SyncTask(
@@ -69,17 +62,12 @@ describe(__filename, () => {
6962

7063
let execution = task.execution as ProcessExecution;
7164

72-
const syncCommandMock = cliMock.getSyncCommand(
73-
anything(),
74-
anything(),
75-
anything(),
76-
anything()
77-
);
65+
const syncCommandMock = cliMock.getSyncCommand(anything());
7866

7967
verify(syncCommandMock).never();
8068
assert.equal(execution.process, "bricks");
8169
verify(syncCommandMock).once();
82-
assert.deepEqual(execution.args, ["sync", "incremental"]);
70+
assert.deepEqual(execution.args, ["sync"]);
8371
verify(syncCommandMock).once();
8472
});
8573
});

‎packages/databricks-vscode/src/cli/BricksTasks.ts‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,10 @@ export class SyncTask extends Task {
3030
constructor(
3131
connection: ConnectionManager,
3232
cli: CliWrapper,
33+
// TODO: https://github.com/databricks/databricks-vscode/issues/111
34+
// use syncType to decide the sync type for bricks cli. Right now bricks cli
35+
// only supports full sync for multiple profiles.
36+
// see: https://github.com/databricks/bricks/issues/71
3337
syncType: "full" | "incremental"
3438
) {
3539
super(
@@ -88,8 +92,24 @@ class LazySyncProcessExecution extends ProcessExecution {
8892
throw new Error("!!!!!");
8993
}
9094

95+
const profile = this.connection.profile;
96+
if (!profile) {
97+
window.showErrorMessage(
98+
"Can't start sync: Databricks connection not configured!"
99+
);
100+
throw new Error(
101+
"Can't start sync: Databricks connection not configured!"
102+
);
103+
}
104+
91105
return {
92106
cwd: workspacePath,
107+
env: {
108+
/* eslint-disable @typescript-eslint/naming-convention */
109+
BRICKS_ROOT: workspacePath,
110+
DATABRICKS_CONFIG_PROFILE: profile,
111+
/* eslint-enable @typescript-eslint/naming-convention */
112+
},
93113
};
94114
},
95115
},
@@ -100,19 +120,8 @@ class LazySyncProcessExecution extends ProcessExecution {
100120
if (this.command) {
101121
return this.command;
102122
}
103-
104-
const me = this.connection.me;
105123
const syncDestination = this.connection.syncDestination;
106-
const profile = this.connection.profile;
107124

108-
if (!me || !profile) {
109-
window.showErrorMessage(
110-
"Can't start sync: Databricks connection not configured!"
111-
);
112-
throw new Error(
113-
"Can't start sync: Databricks connection not configured!"
114-
);
115-
}
116125
if (!syncDestination) {
117126
window.showErrorMessage(
118127
"Can't start sync: Databricks synchronization destination not configured!"
@@ -122,12 +131,7 @@ class LazySyncProcessExecution extends ProcessExecution {
122131
);
123132
}
124133

125-
this.command = this.cli.getSyncCommand(
126-
profile,
127-
me,
128-
syncDestination,
129-
this.syncType
130-
);
134+
this.command = this.cli.getSyncCommand(syncDestination);
131135

132136
return this.command;
133137
}

‎packages/databricks-vscode/src/cli/CliWrapper.test.ts‎

Lines changed: 2 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -29,43 +29,11 @@ describe(__filename, () => {
2929
Uri.file("/Users/fabian.jakobs/Desktop/notebook-best-practices")
3030
);
3131

32-
const {command, args} = cli.getSyncCommand(
33-
"DEFAULT",
34-
"fabian@databricks.com",
35-
mapper,
36-
"incremental"
37-
);
38-
39-
assert.equal(
40-
[command, ...args].join(" "),
41-
"dbx sync repo --profile DEFAULT --user fabian@databricks.com --dest-repo notebook-best-practices"
42-
);
43-
});
44-
45-
it("should create full sync command", () => {
46-
const cli = new CliWrapper({
47-
asAbsolutePath(path: string) {
48-
return path;
49-
},
50-
} as any);
51-
const mapper = new SyncDestination(
52-
Uri.from({
53-
scheme: "dbws",
54-
path: "/Workspace/Repos/fabian.jakobs@databricks.com/notebook-best-practices",
55-
}),
56-
Uri.file("/Users/fabian.jakobs/Desktop/notebook-best-practices")
57-
);
58-
59-
const {command, args} = cli.getSyncCommand(
60-
"DEFAULT",
61-
"fabian@databricks.com",
62-
mapper,
63-
"full"
64-
);
32+
const {command, args} = cli.getSyncCommand(mapper);
6533

6634
assert.equal(
6735
[command, ...args].join(" "),
68-
"dbx sync repo --profile DEFAULT --user fabian@databricks.com --dest-repo notebook-best-practices --full-sync"
36+
"./bin/bricks sync --remote-path /Repos/fabian.jakobs@databricks.com/notebook-best-practices --persist-snapshot=false"
6937
);
7038
});
7139

‎packages/databricks-vscode/src/cli/CliWrapper.ts‎

Lines changed: 6 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -24,30 +24,16 @@ export class CliWrapper {
2424
}
2525

2626
/**
27-
* Constructs the dbx sync command
27+
* Constructs the bricks sync command
2828
*/
29-
getSyncCommand(
30-
profile: string,
31-
me: string,
32-
syncDestination: SyncDestination,
33-
syncType: "full" | "incremental"
34-
): Command {
35-
const command = "dbx";
29+
getSyncCommand(syncDestination: SyncDestination): Command {
30+
const command = this.context.asAbsolutePath("./bin/bricks");
3631
const args = [
3732
"sync",
38-
"repo",
39-
"--profile",
40-
profile,
41-
"--user",
42-
me,
43-
"--dest-repo",
44-
syncDestination.name,
33+
"--remote-path",
34+
syncDestination.relativeRepoPath,
35+
"--persist-snapshot=false",
4536
];
46-
47-
if (syncType === "full") {
48-
args.push("--full-sync");
49-
}
50-
5137
return {command, args};
5238
}
5339

‎packages/databricks-vscode/src/cluster/ClusterManager.test.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ describe(__filename, async () => {
3333
"/api/2.0/clusters/get",
3434
"GET",
3535
deepEqual({
36+
// eslint-disable-next-line
3637
cluster_id: testClusterDetails.cluster_id,
3738
}),
3839
anything()
@@ -59,6 +60,7 @@ describe(__filename, async () => {
5960
"/api/2.0/clusters/get",
6061
"GET",
6162
deepEqual({
63+
// eslint-disable-next-line
6264
cluster_id: testClusterDetails.cluster_id,
6365
}),
6466
anything()

‎packages/databricks-vscode/src/configuration/SyncDestination.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ export class SyncDestination {
3636
return this.repoPath;
3737
}
3838

39+
get relativeRepoPath(): string {
40+
return this.repoPath.path.replace("/Workspace", "");
41+
}
42+
3943
/**
4044
* Strips the workspace path from an absolute path
4145
*/

0 commit comments

Comments
 (0)