From ddce5b1544ac62e650878cb514ecfbd176068e74 Mon Sep 17 00:00:00 2001 From: Matt Seddon <37993418+mattseddon@users.noreply.github.com> Date: Fri, 19 May 2023 16:05:43 +1000 Subject: [PATCH] Add command to add remote (#3929) * add add remote command * add tests * refactor * apply review feedback --- .eslintrc.js | 4 +- demo | 2 +- extension/package.json | 10 ++ extension/src/cli/dvc/constants.ts | 5 +- extension/src/cli/dvc/executor.ts | 7 +- extension/src/commands/external.ts | 2 + extension/src/extension.ts | 2 +- extension/src/setup/commands/index.ts | 86 ++++++++++++++++ .../src/setup/{ => commands}/register.ts | 21 ++-- extension/src/setup/webview/messages.ts | 2 + extension/src/telemetry/constants.ts | 2 + extension/src/test/suite/setup/index.test.ts | 98 +++++++++++++++++++ extension/src/test/suite/setup/util.ts | 3 +- extension/src/vscode/title.ts | 5 +- extension/src/webview/contract.ts | 2 + extension/src/workspace/index.ts | 14 +-- extension/src/workspace/util.ts | 14 +++ scripts/virtualenv-install.ts | 1 + webview/src/setup/components/App.test.tsx | 28 +++++- webview/src/setup/components/messages.ts | 3 + .../src/setup/components/remote/Connect.tsx | 3 + webview/src/stories/Setup.stories.tsx | 13 +++ 22 files changed, 297 insertions(+), 30 deletions(-) create mode 100644 extension/src/setup/commands/index.ts rename extension/src/setup/{ => commands}/register.ts (82%) create mode 100644 extension/src/workspace/util.ts diff --git a/.eslintrc.js b/.eslintrc.js index 1a67e932df..ca685b45e1 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -3,6 +3,7 @@ module.exports = { env: { 'jest/globals': true }, + extends: [ 'prettier-standard/prettier-file', 'plugin:@typescript-eslint/eslint-recommended', @@ -22,7 +23,8 @@ module.exports = { '**/*.js', '*.d.ts', 'tsconfig.json', - 'webpack.config.ts' + 'webpack.config.ts', + 'scripts/virtualenv-install.ts' ], overrides: [ { diff --git a/demo b/demo index a20953baeb..5f06c3734d 160000 --- a/demo +++ b/demo @@ -1 +1 @@ -Subproject commit a20953baeb985bdfa41490d220da32942345864f +Subproject commit 5f06c3734d76cb7ca894895e89d6d06dd878f8c4 diff --git a/extension/package.json b/extension/package.json index 9b611999b5..187fd9afa3 100644 --- a/extension/package.json +++ b/extension/package.json @@ -100,6 +100,12 @@ "category": "DVC", "icon": "$(add)" }, + { + "title": "Add Remote", + "command": "dvc.addRemote", + "category": "DVC", + "icon": "$(add)" + }, { "title": "Filter Experiments Table to Starred", "command": "dvc.addStarredExperimentsTableFilter", @@ -654,6 +660,10 @@ "command": "dvc.addExperimentsTableSort", "when": "dvc.commands.available && dvc.project.available" }, + { + "command": "dvc.addRemote", + "when": "dvc.commands.available && dvc.project.available && !dvc.experiment.running.workspace" + }, { "command": "dvc.addStarredExperimentsTableFilter", "when": "dvc.commands.available && dvc.project.available" diff --git a/extension/src/cli/dvc/constants.ts b/extension/src/cli/dvc/constants.ts index 2759f0ff5a..f2c03f7716 100644 --- a/extension/src/cli/dvc/constants.ts +++ b/extension/src/cli/dvc/constants.ts @@ -38,6 +38,7 @@ export enum Command { } export enum SubCommand { + ADD = 'add', DIFF = 'diff', LIST = 'list', STATUS = 'status', @@ -47,13 +48,15 @@ export enum SubCommand { export enum Flag { ALL_COMMITS = '-A', FOLLOW = '-f', + DEFAULT = '-d', FORCE = '-f', GLOBAL = '--global', GRANULAR = '--granular', - LOCAL = '--local', JOBS = '-j', JSON = '--json', KILL = '--kill', + LOCAL = '--local', + PROJECT = '--project', NUM_COMMIT = '-n', OUTPUT_PATH = '-o', SUBDIRECTORY = '--subdir', diff --git a/extension/src/cli/dvc/executor.ts b/extension/src/cli/dvc/executor.ts index 8136837ab5..c0a9a2ef50 100644 --- a/extension/src/cli/dvc/executor.ts +++ b/extension/src/cli/dvc/executor.ts @@ -7,8 +7,7 @@ import { ExperimentSubCommand, Flag, GcPreserveFlag, - QueueSubCommand, - SubCommand + QueueSubCommand } from './constants' import { addStudioAccessToken } from './options' import { CliResult, CliStarted, typeCheckCommands } from '..' @@ -198,8 +197,8 @@ export class DvcExecutor extends DvcCli { return this.blockAndExecuteProcess(cwd, Command.REMOVE, ...args) } - public remote(cwd: string, arg: typeof SubCommand.LIST) { - return this.executeDvcProcess(cwd, Command.REMOTE, arg) + public remote(cwd: string, ...args: Args) { + return this.executeDvcProcess(cwd, Command.REMOTE, ...args) } private executeExperimentProcess(cwd: string, ...args: Args) { diff --git a/extension/src/commands/external.ts b/extension/src/commands/external.ts index 151f106098..8e9ea99f03 100644 --- a/extension/src/commands/external.ts +++ b/extension/src/commands/external.ts @@ -41,6 +41,8 @@ export enum RegisteredCliCommands { REMOVE_TARGET = 'dvc.removeTarget', RENAME_TARGET = 'dvc.renameTarget', + REMOTE_ADD = 'dvc.addRemote', + GIT_STAGE_ALL = 'dvc.gitStageAll', GIT_UNSTAGE_ALL = 'dvc.gitUnstageAll' } diff --git a/extension/src/extension.ts b/extension/src/extension.ts index 66bc19f6c8..4a55541d39 100644 --- a/extension/src/extension.ts +++ b/extension/src/extension.ts @@ -47,7 +47,7 @@ import { Flag } from './cli/dvc/constants' import { LanguageClient } from './languageClient' import { collectRunningExperimentPids } from './experiments/processExecution/collect' import { DvcViewer } from './cli/dvc/viewer' -import { registerSetupCommands } from './setup/register' +import { registerSetupCommands } from './setup/commands/register' import { Status } from './status' import { registerPersistenceCommands } from './persistence/register' diff --git a/extension/src/setup/commands/index.ts b/extension/src/setup/commands/index.ts new file mode 100644 index 0000000000..b4245c89d3 --- /dev/null +++ b/extension/src/setup/commands/index.ts @@ -0,0 +1,86 @@ +import { Setup } from '..' +import { Flag, SubCommand } from '../../cli/dvc/constants' +import { AvailableCommands, InternalCommands } from '../../commands/internal' +import { definedAndNonEmpty } from '../../util/array' +import { getInput } from '../../vscode/inputBox' +import { quickPickYesOrNo } from '../../vscode/quickPick' +import { Title } from '../../vscode/title' +import { Toast } from '../../vscode/toast' +import { getOnlyOrPickProject } from '../../workspace/util' + +const noExistingOrUserConfirms = async ( + internalCommands: InternalCommands, + dvcRoot: string +): Promise => { + const remoteList = await internalCommands.executeCommand( + AvailableCommands.REMOTE, + dvcRoot, + SubCommand.LIST + ) + + if (!remoteList) { + return true + } + + return await quickPickYesOrNo( + 'make this new remote the default', + 'keep the current default', + { + placeHolder: 'Would you like to set this new remote as the default?', + title: Title.SET_REMOTE_AS_DEFAULT + } + ) +} + +const addRemoteToProject = async ( + internalCommands: InternalCommands, + dvcRoot: string +): Promise => { + const name = await getInput(Title.ENTER_REMOTE_NAME) + if (!name) { + return + } + + const url = await getInput(Title.ENTER_REMOTE_URL) + if (!url) { + return + } + + const args = [Flag.PROJECT, name, url] + + const shouldSetAsDefault = await noExistingOrUserConfirms( + internalCommands, + dvcRoot + ) + if (shouldSetAsDefault === undefined) { + return + } + + if (shouldSetAsDefault) { + args.unshift(Flag.DEFAULT) + } + + return await Toast.showOutput( + internalCommands.executeCommand( + AvailableCommands.REMOTE, + dvcRoot, + SubCommand.ADD, + ...args + ) + ) +} + +export const getAddRemoteCommand = + (setup: Setup, internalCommands: InternalCommands) => + async (): Promise => { + const dvcRoots = setup.getRoots() + if (!definedAndNonEmpty(dvcRoots)) { + return Toast.showError('Cannot add a remote without a DVC project') + } + const dvcRoot = await getOnlyOrPickProject(dvcRoots) + + if (!dvcRoot) { + return + } + return addRemoteToProject(internalCommands, dvcRoot) + } diff --git a/extension/src/setup/register.ts b/extension/src/setup/commands/register.ts similarity index 82% rename from extension/src/setup/register.ts rename to extension/src/setup/commands/register.ts index f0e8468abc..9efd2c852e 100644 --- a/extension/src/setup/register.ts +++ b/extension/src/setup/commands/register.ts @@ -1,10 +1,14 @@ import { commands } from 'vscode' -import { Setup } from '.' -import { run } from './runner' -import { SetupSection } from './webview/contract' -import { AvailableCommands, InternalCommands } from '../commands/internal' -import { RegisteredCliCommands, RegisteredCommands } from '../commands/external' -import { getFirstWorkspaceFolder } from '../vscode/workspaceFolders' +import { getAddRemoteCommand } from '.' +import { Setup } from '..' +import { run } from '../runner' +import { SetupSection } from '../webview/contract' +import { AvailableCommands, InternalCommands } from '../../commands/internal' +import { + RegisteredCliCommands, + RegisteredCommands +} from '../../commands/external' +import { getFirstWorkspaceFolder } from '../../vscode/workspaceFolders' const registerSetupConfigCommands = ( setup: Setup, @@ -100,6 +104,11 @@ export const registerSetupCommands = ( } ) + internalCommands.registerExternalCliCommand( + RegisteredCliCommands.REMOTE_ADD, + getAddRemoteCommand(setup, internalCommands) + ) + registerSetupConfigCommands(setup, internalCommands) registerSetupShowCommands(setup, internalCommands) registerSetupStudioCommands(setup, internalCommands) diff --git a/extension/src/setup/webview/messages.ts b/extension/src/setup/webview/messages.ts index 2c6f767478..0a91f48b92 100644 --- a/extension/src/setup/webview/messages.ts +++ b/extension/src/setup/webview/messages.ts @@ -104,6 +104,8 @@ export class WebviewMessages { ) case MessageFromWebviewType.OPEN_EXPERIMENTS_WEBVIEW: return commands.executeCommand(RegisteredCommands.EXPERIMENT_SHOW) + case MessageFromWebviewType.REMOTE_ADD: + return commands.executeCommand(RegisteredCliCommands.REMOTE_ADD) default: Logger.error(`Unexpected message: ${JSON.stringify(message)}`) diff --git a/extension/src/telemetry/constants.ts b/extension/src/telemetry/constants.ts index da8ef8a87b..7fc37368c9 100644 --- a/extension/src/telemetry/constants.ts +++ b/extension/src/telemetry/constants.ts @@ -187,6 +187,8 @@ export interface IEventNamePropertyMapping { [EventName.REMOVE_TARGET]: undefined [EventName.RENAME_TARGET]: undefined + [EventName.REMOTE_ADD]: undefined + [EventName.GIT_STAGE_ALL]: undefined [EventName.GIT_UNSTAGE_ALL]: undefined diff --git a/extension/src/test/suite/setup/index.test.ts b/extension/src/test/suite/setup/index.test.ts index 9ef5143756..e823b30192 100644 --- a/extension/src/test/suite/setup/index.test.ts +++ b/extension/src/test/suite/setup/index.test.ts @@ -69,6 +69,7 @@ suite('Setup Test Suite', () => { ]) }) + // eslint-disable-next-line sonarjs/cognitive-complexity describe('Setup', () => { it('should handle an initialize git message from the webview', async () => { const { messageSpy, setup, mockInitializeGit } = buildSetup(disposable) @@ -843,6 +844,103 @@ suite('Setup Test Suite', () => { expect(mockShowWebview).to.be.calledOnce }).timeout(WEBVIEW_TEST_TIMEOUT) + it('should handle a message to add a remote', async () => { + const { messageSpy, setup, mockExecuteCommand } = buildSetup(disposable) + + const webview = await setup.showWebview() + await webview.isReady() + mockExecuteCommand.restore() + + const mockMessageReceived = getMessageReceivedEmitter(webview) + + const mockRemote = stub(DvcExecutor.prototype, 'remote') + + const remoteAdded = new Promise(resolve => + mockRemote.callsFake((cwd, ...args) => { + if (args.includes('add')) { + resolve(undefined) + } + return Promise.resolve('') + }) + ) + + const mockShowInputBox = stub(window, 'showInputBox') + .onFirstCall() + .resolves('storage') + .onSecondCall() + .resolves('s3://my-bucket') + + messageSpy.resetHistory() + mockMessageReceived.fire({ + type: MessageFromWebviewType.REMOTE_ADD + }) + + await remoteAdded + + expect(mockShowInputBox).to.be.calledTwice + expect( + mockRemote, + 'new remote is set as the default' + ).to.be.calledWithExactly( + dvcDemoPath, + 'add', + '-d', + '--project', + 'storage', + 's3://my-bucket' + ) + }).timeout(WEBVIEW_TEST_TIMEOUT) + + it('should be able to add a remote', async () => { + const mockRemote = stub(DvcExecutor.prototype, 'remote') + + const remoteAdded = new Promise(resolve => + mockRemote.callsFake((cwd, ...args) => { + if (args.includes('list')) { + return Promise.resolve('storage s3://my-bucket') + } + + if (args.includes('add')) { + resolve(undefined) + } + return Promise.resolve('') + }) + ) + + const mockShowInputBox = stub(window, 'showInputBox') + .onFirstCall() + .resolves('backup') + .onSecondCall() + .resolves('s3://my-backup-bucket') + + const mockShowQuickPick = ( + stub(window, 'showQuickPick') as SinonStub< + [items: readonly QuickPickItem[], options: QuickPickOptionsWithTitle], + Thenable | undefined> + > + ).resolves({ + label: 'No', + value: false + }) + + await commands.executeCommand(RegisteredCliCommands.REMOTE_ADD) + + await remoteAdded + + expect(mockShowInputBox).to.be.calledTwice + expect(mockShowQuickPick).to.be.calledOnce + expect( + mockRemote, + 'should not set a remote as the default unless the user explicitly chooses to' + ).to.be.calledWithExactly( + dvcDemoPath, + 'add', + '--project', + 'backup', + 's3://my-backup-bucket' + ) + }).timeout(WEBVIEW_TEST_TIMEOUT) + it('should send the appropriate messages to the webview to focus different sections', async () => { const { setup, messageSpy } = buildSetup(disposable) messageSpy.restore() diff --git a/extension/src/test/suite/setup/util.ts b/extension/src/test/suite/setup/util.ts index fd0d8a2405..f113dfde7c 100644 --- a/extension/src/test/suite/setup/util.ts +++ b/extension/src/test/suite/setup/util.ts @@ -44,7 +44,7 @@ export const buildSetup = ( const mockEmitter = disposer.track(new EventEmitter()) stub(dvcReader, 'root').resolves(mockDvcRoot) - stub(dvcExecutor, 'remote').resolves('') + const mockRemote = stub(dvcExecutor, 'remote').resolves('') const mockVersion = stub(dvcReader, 'version').resolves(MIN_CLI_VERSION) const mockGlobalVersion = stub(dvcReader, 'globalVersion').resolves( MIN_CLI_VERSION @@ -112,6 +112,7 @@ export const buildSetup = ( mockGlobalVersion, mockInitializeGit, mockOpenExternal, + mockRemote, mockRunSetup, mockShowWebview, mockVersion, diff --git a/extension/src/vscode/title.ts b/extension/src/vscode/title.ts index f5eb35800a..694ceea7ab 100644 --- a/extension/src/vscode/title.ts +++ b/extension/src/vscode/title.ts @@ -6,6 +6,8 @@ export enum Title { ENTER_EXPERIMENT_WORKER_COUNT = 'Enter the Number of Queue Workers', ENTER_FILTER_VALUE = 'Enter a Filter Value', ENTER_RELATIVE_DESTINATION = 'Enter a Destination Relative to the Root', + ENTER_REMOTE_NAME = 'Enter a Name for the Remote', + ENTER_REMOTE_URL = 'Enter the URL for the Remote', ENTER_PATH_OR_CHOOSE_FILE = 'Enter the path to your training script or select it', ENTER_STUDIO_USERNAME = 'Enter your Studio username', ENTER_STUDIO_TOKEN = 'Enter your Studio access token', @@ -36,7 +38,8 @@ export enum Title { SELECT_SORTS_TO_REMOVE = 'Select Sort(s) to Remove', SELECT_TRAINING_SCRIPT = 'Select your training script', SETUP_WORKSPACE = 'Setup the Workspace', - SET_EXPERIMENTS_HEADER_HEIGHT = 'Set Maximum Experiment Table Header Height' + SET_EXPERIMENTS_HEADER_HEIGHT = 'Set Maximum Experiment Table Header Height', + SET_REMOTE_AS_DEFAULT = 'Set Default Remote' } export const getEnterValueTitle = (path: string): Title => diff --git a/extension/src/webview/contract.ts b/extension/src/webview/contract.ts index fbf109194b..1f98ea74cf 100644 --- a/extension/src/webview/contract.ts +++ b/extension/src/webview/contract.ts @@ -51,6 +51,7 @@ export enum MessageFromWebviewType { SET_EXPERIMENTS_AND_OPEN_PLOTS = 'set-experiments-and-open-plots', SET_STUDIO_SHARE_EXPERIMENTS_LIVE = 'set-studio-share-experiments-live', TOGGLE_PLOTS_SECTION = 'toggle-plots-section', + REMOTE_ADD = 'remote-add', REMOVE_CUSTOM_PLOTS = 'remove-custom-plots', REMOVE_STUDIO_TOKEN = 'remove-studio-token', MODIFY_EXPERIMENT_PARAMS_AND_QUEUE = 'modify-experiment-params-and-queue', @@ -160,6 +161,7 @@ export type MessageFromWebview = type: MessageFromWebviewType.REMOVE_COLUMN_SORT payload: string } + | { type: MessageFromWebviewType.REMOTE_ADD } | { type: MessageFromWebviewType.REMOVE_CUSTOM_PLOTS } diff --git a/extension/src/workspace/index.ts b/extension/src/workspace/index.ts index f04f607355..207cc6e827 100644 --- a/extension/src/workspace/index.ts +++ b/extension/src/workspace/index.ts @@ -1,6 +1,6 @@ +import { getOnlyOrPickProject } from './util' import { InternalCommands } from '../commands/internal' import { Disposables, reset } from '../util/disposable' -import { quickPickOne } from '../vscode/quickPick' import { DeferredDisposable } from '../class/deferred' export abstract class BaseWorkspace< @@ -39,17 +39,9 @@ export abstract class BaseWorkspace< this.resetDeferred() } - public async getOnlyOrPickProject() { + public getOnlyOrPickProject() { const dvcRoots = this.getDvcRoots() - - if (dvcRoots.length === 1) { - return dvcRoots[0] - } - - return await quickPickOne( - dvcRoots, - 'Select which project to run command against' - ) + return getOnlyOrPickProject(dvcRoots) } public getRepository(dvcRoot: string) { diff --git a/extension/src/workspace/util.ts b/extension/src/workspace/util.ts new file mode 100644 index 0000000000..1ca5036298 --- /dev/null +++ b/extension/src/workspace/util.ts @@ -0,0 +1,14 @@ +import { quickPickOne } from '../vscode/quickPick' + +export const getOnlyOrPickProject = async ( + dvcRoots: string[] +): Promise => { + if (dvcRoots.length === 1) { + return dvcRoots[0] + } + + return await quickPickOne( + dvcRoots, + 'Select which project to run command against' + ) +} diff --git a/scripts/virtualenv-install.ts b/scripts/virtualenv-install.ts index e756cd683f..f2b64af35a 100644 --- a/scripts/virtualenv-install.ts +++ b/scripts/virtualenv-install.ts @@ -1,3 +1,4 @@ +/* eslint-disable */ import { join, resolve } from 'path' require('dvc/src/vscode/mockModule') diff --git a/webview/src/setup/components/App.test.tsx b/webview/src/setup/components/App.test.tsx index 672b161cc4..03beb10a26 100644 --- a/webview/src/setup/components/App.test.tsx +++ b/webview/src/setup/components/App.test.tsx @@ -792,10 +792,32 @@ describe('App', () => { } }) - const setupDVCButton = screen.getByText('Connect to Remote Storage') + const title = screen.getByText('Connect to Remote Storage') - expect(setupDVCButton).toBeInTheDocument() - expect(setupDVCButton).toBeVisible() + expect(title).toBeInTheDocument() + expect(title).toBeVisible() + }) + + it('should allow the user to connect a remote if they do not already have one', () => { + renderApp({ + remoteList: { demo: undefined, 'example-get-started': undefined }, + sectionCollapsed: { + [SetupSection.DVC]: true, + [SetupSection.EXPERIMENTS]: true, + [SetupSection.REMOTES]: false, + [SetupSection.STUDIO]: true + } + }) + mockPostMessage.mockReset() + const startButton = screen.getByText('Add Remote') + + expect(startButton).toBeInTheDocument() + expect(startButton).toBeVisible() + fireEvent.click(startButton) + expect(mockPostMessage).toHaveBeenCalledTimes(1) + expect(mockPostMessage).toHaveBeenCalledWith({ + type: MessageFromWebviewType.REMOTE_ADD + }) }) it('should show the list of remotes if there is only one project in the workspace', () => { diff --git a/webview/src/setup/components/messages.ts b/webview/src/setup/components/messages.ts index 6f58caf0cc..698ac972f1 100644 --- a/webview/src/setup/components/messages.ts +++ b/webview/src/setup/components/messages.ts @@ -52,3 +52,6 @@ export const saveStudioToken = () => export const removeStudioToken = () => sendMessage({ type: MessageFromWebviewType.REMOVE_STUDIO_TOKEN }) + +export const addRemote = () => + sendMessage({ type: MessageFromWebviewType.REMOTE_ADD }) diff --git a/webview/src/setup/components/remote/Connect.tsx b/webview/src/setup/components/remote/Connect.tsx index eea4acf0a1..9f5b68ca5c 100644 --- a/webview/src/setup/components/remote/Connect.tsx +++ b/webview/src/setup/components/remote/Connect.tsx @@ -1,5 +1,7 @@ import React from 'react' import { EmptyState } from '../../../shared/components/emptyState/EmptyState' +import { StartButton } from '../../../shared/components/button/StartButton' +import { addRemote } from '../messages' export const Connect: React.FC = () => ( @@ -18,5 +20,6 @@ export const Connect: React.FC = () => ( {' '} for details on how to connect to a remote

+ addRemote()} text="Add Remote" /> ) diff --git a/webview/src/stories/Setup.stories.tsx b/webview/src/stories/Setup.stories.tsx index 0e15735914..5be17782fc 100644 --- a/webview/src/stories/Setup.stories.tsx +++ b/webview/src/stories/Setup.stories.tsx @@ -154,6 +154,19 @@ CliAboveLatestTested.args = getUpdatedArgs({ isAboveLatestTestedVersion: true }) +export const NoRemoteSetup = Template.bind({}) +NoRemoteSetup.args = getUpdatedArgs({ + remoteList: { + '/Users/thatguy/projects/vscode-dvc/rootB': undefined + }, + sectionCollapsed: { + [SetupSection.DVC]: true, + [SetupSection.EXPERIMENTS]: true, + [SetupSection.REMOTES]: false, + [SetupSection.STUDIO]: true + } +}) + export const ProjectRemoteSetup = Template.bind({}) ProjectRemoteSetup.args = getUpdatedArgs({ remoteList: {