From 8677ffc68c3f018e40dc88feb0100d3eaeb823cc Mon Sep 17 00:00:00 2001 From: Charles Gagnon Date: Thu, 28 Jan 2021 09:00:46 -0800 Subject: [PATCH] Fix deployment wizard to not close when cancelling out of password prompt (#14083) --- extensions/arc/src/common/cacheManager.ts | 83 ------------------- extensions/arc/src/localizedConstants.ts | 2 +- .../arcControllersOptionsSourceProvider.ts | 15 +--- .../resource-deployment/src/common/utils.ts | 5 ++ .../src/ui/modelViewUtils.ts | 18 ++-- .../ui/notebookWizard/notebookWizardModel.ts | 12 +-- 6 files changed, 27 insertions(+), 108 deletions(-) delete mode 100644 extensions/arc/src/common/cacheManager.ts diff --git a/extensions/arc/src/common/cacheManager.ts b/extensions/arc/src/common/cacheManager.ts deleted file mode 100644 index bd9624065f..0000000000 --- a/extensions/arc/src/common/cacheManager.ts +++ /dev/null @@ -1,83 +0,0 @@ -/*--------------------------------------------------------------------------------------------- - * Copyright (c) Microsoft Corporation. All rights reserved. - * Licensed under the Source EULA. See License.txt in the project root for license information. - *--------------------------------------------------------------------------------------------*/ - -import { Deferred } from './promise'; - -const enum Status { - notStarted, - inProgress, - done -} - -interface State { - entry?: T, - error?: Error, - status: Status, - id: number, - pendingOperation: Deferred -} - -/** - * An implementation of Cache Manager which ensures that only one call to populate cache miss is pending at a given time. - * All remaining calls for retrieval are awaited until the one in progress finishes and then all awaited calls are resolved with the value - * from the cache. - */ -export class CacheManager { - private _cache = new Map>(); - private _id = 0; - - public async getCacheEntry(key: K, retrieveEntry: (key: K) => Promise): Promise { - const cacheHit: State | undefined = this._cache.get(key); - // each branch either throws or returns the password. - if (cacheHit === undefined) { - // populate a new state entry and add it to the cache - const state: State = { - status: Status.notStarted, - id: this._id++, - pendingOperation: new Deferred() - }; - this._cache.set(key, state); - // now that we have the state entry initialized, retry to fetch the cacheEntry - let returnValue: T = await this.getCacheEntry(key, retrieveEntry); - await state.pendingOperation; - return returnValue!; - } else { - switch (cacheHit.status) { - case Status.notStarted: { - cacheHit.status = Status.inProgress; - // retrieve and populate the missed cache hit. - try { - cacheHit.entry = await retrieveEntry(key); - } catch (error) { - cacheHit.error = error; - } finally { - cacheHit.status = Status.done; - // we do not reject here even in error case because we do not want our awaits on pendingOperation to throw - // We track our own error state and when all done we throw if an error had happened. This results - // in the rejection of the promised returned by this method. - cacheHit.pendingOperation.resolve(); - } - return await this.getCacheEntry(key, retrieveEntry); - } - - case Status.inProgress: { - await cacheHit.pendingOperation; - return await this.getCacheEntry(key, retrieveEntry); - } - - case Status.done: { - if (cacheHit.error !== undefined) { - await cacheHit.pendingOperation; - throw cacheHit.error; - } - else { - await cacheHit.pendingOperation; - return cacheHit.entry!; - } - } - } - } - } -} diff --git a/extensions/arc/src/localizedConstants.ts b/extensions/arc/src/localizedConstants.ts index 53f85b3d48..147532d3f2 100644 --- a/extensions/arc/src/localizedConstants.ts +++ b/extensions/arc/src/localizedConstants.ts @@ -218,7 +218,7 @@ export function couldNotFindAzureResource(name: string): string { return localiz export function passwordResetFailed(error: any): string { return localize('arc.passwordResetFailed', "Failed to reset password. {0}", getErrorMessage(error)); } export function errorConnectingToController(error: any): string { return localize('arc.errorConnectingToController', "Error connecting to controller. {0}", getErrorMessage(error, true)); } export function passwordAcquisitionFailed(error: any): string { return localize('arc.passwordAcquisitionFailed', "Failed to acquire password. {0}", getErrorMessage(error)); } -export const loginFailed = localize('arc.loginFailed', "Error logging into controller, try again."); +export const loginFailed = localize('arc.loginFailed', "Error logging into controller - wrong username or password"); export function errorVerifyingPassword(error: any): string { return localize('arc.errorVerifyingPassword', "Error encountered while verifying password. {0}", getErrorMessage(error)); } export const noControllersConnected = localize('noControllersConnected', "No Azure Arc controllers are currently connected. Please run the command: 'Connect to Existing Azure Arc Controller' and then try again"); export const variableValueFetchForUnsupportedVariable = (variableName: string) => localize('getVariableValue.unknownVariableName', "Attempt to get variable value for unknown variable:{0}", variableName); diff --git a/extensions/arc/src/providers/arcControllersOptionsSourceProvider.ts b/extensions/arc/src/providers/arcControllersOptionsSourceProvider.ts index 2a50e7cc55..611f3b98f5 100644 --- a/extensions/arc/src/providers/arcControllersOptionsSourceProvider.ts +++ b/extensions/arc/src/providers/arcControllersOptionsSourceProvider.ts @@ -7,7 +7,6 @@ import * as arc from 'arc'; import * as azdata from 'azdata'; import * as rd from 'resource-deployment'; import { getControllerPassword, getRegisteredDataControllers, reacquireControllerPassword } from '../common/api'; -import { CacheManager } from '../common/cacheManager'; import { throwUnless } from '../common/utils'; import * as loc from '../localizedConstants'; import { AzureArcTreeDataProvider } from '../ui/tree/azureArcTreeDataProvider'; @@ -16,11 +15,10 @@ import { AzureArcTreeDataProvider } from '../ui/tree/azureArcTreeDataProvider'; * Class that provides options sources for an Arc Data Controller */ export class ArcControllersOptionsSourceProvider implements rd.IOptionsSourceProvider { - private _cacheManager = new CacheManager(); readonly id = 'arc.controllers'; constructor(private _treeProvider: AzureArcTreeDataProvider) { } - async getOptions(): Promise { + public async getOptions(): Promise { const controllers = await getRegisteredDataControllers(this._treeProvider); throwUnless(controllers !== undefined && controllers.length !== 0, loc.noControllersConnected); return controllers.map(ci => { @@ -28,8 +26,7 @@ export class ArcControllersOptionsSourceProvider implements rd.IOptionsSourcePro }); } - private async retrieveVariable(key: string): Promise { - const [variableName, controllerLabel] = JSON.parse(key); + public async getVariableValue(variableName: string, controllerLabel: string): Promise { const controller = (await getRegisteredDataControllers(this._treeProvider)).find(ci => ci.label === controllerLabel); throwUnless(controller !== undefined, loc.noControllerInfoFound(controllerLabel)); switch (variableName) { @@ -42,12 +39,6 @@ export class ArcControllersOptionsSourceProvider implements rd.IOptionsSourcePro } } - getVariableValue(variableName: string, controllerLabel: string): Promise { - // capture 'this' in an arrow function object - const retrieveVariable = (key: string) => this.retrieveVariable(key); - return this._cacheManager.getCacheEntry(JSON.stringify([variableName, controllerLabel]), retrieveVariable); - } - private async getPassword(controller: arc.DataController): Promise { let password = await getControllerPassword(this._treeProvider, controller.info); if (!password) { @@ -57,7 +48,7 @@ export class ArcControllersOptionsSourceProvider implements rd.IOptionsSourcePro return password; } - getIsPassword(variableName: string): boolean { + public getIsPassword(variableName: string): boolean { switch (variableName) { case 'endpoint': return false; case 'username': return false; diff --git a/extensions/resource-deployment/src/common/utils.ts b/extensions/resource-deployment/src/common/utils.ts index 2dd8a56385..505a732a0e 100644 --- a/extensions/resource-deployment/src/common/utils.ts +++ b/extensions/resource-deployment/src/common/utils.ts @@ -3,6 +3,7 @@ * Licensed under the Source EULA. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ import * as path from 'path'; +import { ErrorType, ErrorWithType } from 'resource-deployment'; import { ToolsInstallPath } from '../constants'; import { ITool, NoteBookEnvironmentVariablePrefix } from '../interfaces'; @@ -12,6 +13,10 @@ export function getErrorMessage(error: any): string { : typeof error === 'string' ? error : `${JSON.stringify(error, undefined, '\t')}`; } +export function isUserCancelledError(err: any): boolean { + return err instanceof Error && 'type' in err && (err).type === ErrorType.userCancelled; +} + export function getDateTimeString(): string { return new Date().toISOString().slice(0, 19).replace(/[^0-9]/g, ''); // Take the date time information and only leaving the numbers } diff --git a/extensions/resource-deployment/src/ui/modelViewUtils.ts b/extensions/resource-deployment/src/ui/modelViewUtils.ts index 6b46169db8..c540205802 100644 --- a/extensions/resource-deployment/src/ui/modelViewUtils.ts +++ b/extensions/resource-deployment/src/ui/modelViewUtils.ts @@ -10,7 +10,7 @@ import * as path from 'path'; import { IOptionsSourceProvider } from 'resource-deployment'; import * as vscode from 'vscode'; import * as nls from 'vscode-nls'; -import { getDateTimeString, getErrorMessage, throwUnless } from '../common/utils'; +import { getDateTimeString, getErrorMessage, isUserCancelledError, throwUnless } from '../common/utils'; import { AzureAccountFieldInfo, AzureLocationsFieldInfo, ComponentCSSStyles, DialogInfoBase, FieldInfo, FieldType, FilePickerFieldInfo, instanceOfDynamicEnablementInfo, IOptionsSource, KubeClusterContextFieldInfo, LabelPosition, NoteBookEnvironmentVariablePrefix, OptionsInfo, OptionsType, PageInfoBase, RowInfo, SectionInfo, TextCSSStyles } from '../interfaces'; import * as loc from '../localizedConstants'; import { apiService } from '../services/apiService'; @@ -667,12 +667,16 @@ async function configureOptionsSourceSubfields(context: FieldContext, optionsSou try { return await optionsSourceProvider.getVariableValue!(variableKey, value); } catch (e) { - disableControlButtons(context.container); - context.container.message = { - text: getErrorMessage(e), - description: '', - level: azdata.window.MessageLevel.Error - }; + if (!isUserCancelledError(e)) { + // User cancelled is a normal scenario so we shouldn't disable anything in that case + // so that the user can retry if they want to + disableControlButtons(context.container); + context.container.message = { + text: getErrorMessage(e), + description: '', + level: azdata.window.MessageLevel.Error + }; + } throw e; } }, diff --git a/extensions/resource-deployment/src/ui/notebookWizard/notebookWizardModel.ts b/extensions/resource-deployment/src/ui/notebookWizard/notebookWizardModel.ts index 37476de135..37361d9067 100644 --- a/extensions/resource-deployment/src/ui/notebookWizard/notebookWizardModel.ts +++ b/extensions/resource-deployment/src/ui/notebookWizard/notebookWizardModel.ts @@ -12,7 +12,7 @@ import { DeploymentType, NotebookWizardDeploymentProvider, NotebookWizardInfo } import { IPlatformService } from '../../services/platformService'; import { NotebookWizardAutoSummaryPage } from './notebookWizardAutoSummaryPage'; import { NotebookWizardPage } from './notebookWizardPage'; -import { ErrorType, ErrorWithType } from 'resource-deployment'; +import { isUserCancelledError } from '../../common/utils'; export class NotebookWizardModel extends ResourceTypeModel { private _inputComponents: InputComponents = {}; @@ -68,11 +68,13 @@ export class NotebookWizardModel extends ResourceTypeModel { try { notebook = await this.prepareNotebookAndEnvironment(); } catch (e) { - const isUserCancelled = e instanceof Error && 'type' in e && (e).type === ErrorType.userCancelled; - // user cancellation is a normal scenario, we just bail out of the wizard without actually opening the notebook, so rethrow for any other case - if (!isUserCancelled) { - throw e; + // If there was a user prompt while preparing the Notebook environment (such as prompting for password) and the user + // cancelled out of that then we shouldn't display an error since that's a normal case but should still keep the Wizard + // open so they can make any changes they want and try again without needing to re-enter the information again. + if (isUserCancelledError(e)) { + return false; } + throw e; } if (notebook) { // open the notebook if it was successfully prepared await this.openNotebook(notebook);