Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/commands/revisionDraft/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
## Detecting Unsaved Changes

When comparing items for changes in single revision mode, prefer referencing the `containerApp` template. When comparing items in multiple revisions mode, prefer referencing the `revision` template.

Reason: Even though the `containerApp` template is essentially equivalent to the `latest` revision template... sometimes there are micro-differences present. Although they end up being functionally equivalent, they may not always be equivalent enough to consistently pass a deep copy test (which is how we are set up to detect unsaved changes).

## Data Sources for Tree Items

Until the addition of revision drafts, the view has always reflected only one source of truth - the latest deployed changes. With the addition of revision drafts, the view now prioritizes showing the latest draft `Unsaved changes` when they are present. Model properties `containerApp` and `revision` should be kept consistent with the latest deployed changes so that methods like `hasUnsavedChanges` always have a reliable data reference for deep copy comparison.
55 changes: 35 additions & 20 deletions src/commands/revisionDraft/RevisionDraftFileSystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import type { Template } from "@azure/arm-appcontainers";
import { nonNullValueAndProp } from "@microsoft/vscode-azext-utils";
import { KnownActiveRevisionsMode, type Template } from "@azure/arm-appcontainers";
import { nonNullValue, nonNullValueAndProp } from "@microsoft/vscode-azext-utils";
import { Disposable, Event, EventEmitter, FileChangeEvent, FileChangeType, FileStat, FileSystemProvider, FileType, TextDocument, Uri, window, workspace } from "vscode";
import { URI } from "vscode-uri";
import { ext } from "../../extensionVariables";
import { ContainerAppItem } from "../../tree/ContainerAppItem";
import type { ContainerAppsItem } from "../../tree/ContainerAppsBranchDataProvider";
import type { RevisionDraftItem } from "../../tree/revisionManagement/RevisionDraftItem";
import type { RevisionItem } from "../../tree/revisionManagement/RevisionItem";
import type { RevisionsItemModel } from "../../tree/revisionManagement/RevisionItem";
import { localize } from "../../utils/localize";

const notSupported: string = localize('notSupported', 'This operation is not currently supported.');
Expand Down Expand Up @@ -51,22 +50,15 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
}

// Create
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you want to leave all the //CRUD comments in here? I don't really mind but it's also kind of unnecessary.

createRevisionDraft(item: ContainerAppItem | RevisionItem | RevisionDraftItem): void {
createRevisionDraft(item: ContainerAppItem | RevisionsItemModel): void {
const uri: Uri = this.buildUriFromItem(item);
if (this.draftStore.has(uri.path)) {
return;
}

// Branching path reasoning: https://github.com/microsoft/vscode-azurecontainerapps/blob/main/src/commands/revisionDraft/README.md
let file: RevisionDraftFile | undefined;
if (item instanceof ContainerAppItem) {
/**
* Sometimes there are micro-differences present between the latest revision template and the container app template.
* They end up being essentially equivalent, but not so equivalent as to always pass a deep copy test (which is how we detect unsaved changes).
*
* Since only container app template data is shown in single revision mode, and since revision data is not directly present on
* the container app tree item without further changes, it is easier to just use the container app template
* as the primary source of truth when in single revision mode.
*/
if (item instanceof ContainerAppItem || item.containerApp.revisionsMode === KnownActiveRevisionsMode.Single) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We like to avoid using instanceof if we can. Here it can be replaced with an isContainerAppItem function.

https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates

const revisionContent: Uint8Array = Buffer.from(JSON.stringify(nonNullValueAndProp(item.containerApp, 'template'), undefined, 4));
file = new RevisionDraftFile(revisionContent, item.containerApp.id, nonNullValueAndProp(item.containerApp, 'latestRevisionName'));
} else {
Expand All @@ -79,7 +71,7 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
}

// Read
parseRevisionDraft<T extends ContainerAppsItem>(item: T): Template | undefined {
parseRevisionDraft(item: ContainerAppsItem): Template | undefined {
const uri: URI = this.buildUriFromItem(item);
if (!this.draftStore.has(uri.path)) {
return undefined;
Expand All @@ -93,12 +85,12 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
return contents ? Buffer.from(contents) : Buffer.from('');
}

doesContainerAppsItemHaveRevisionDraft<T extends ContainerAppsItem>(item: T): boolean {
doesContainerAppsItemHaveRevisionDraft(item: ContainerAppsItem): boolean {
const uri: Uri = this.buildUriFromItem(item);
return this.draftStore.has(uri.path);
}

getRevisionDraftFile<T extends ContainerAppsItem>(item: T): RevisionDraftFile | undefined {
getRevisionDraftFile(item: ContainerAppsItem): RevisionDraftFile | undefined {
const uri: Uri = this.buildUriFromItem(item);
return this.draftStore.get(uri.path);
}
Expand All @@ -119,7 +111,7 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
}

// Update
async editRevisionDraft(item: ContainerAppItem | RevisionItem | RevisionDraftItem): Promise<void> {
async editRevisionDraft(item: ContainerAppItem | RevisionsItemModel): Promise<void> {
const uri: Uri = this.buildUriFromItem(item);
if (!this.draftStore.has(uri.path)) {
this.createRevisionDraft(item);
Expand All @@ -146,8 +138,31 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
ext.state.notifyChildrenChanged(file.containerAppId);
}

updateRevisionDraftWithTemplate(item: RevisionsItemModel, template: Template): void {
const uri: Uri = this.buildUriFromItem(item);
if (!this.draftStore.has(uri.path)) {
this.createRevisionDraft(item);
}

const newContent: Uint8Array = Buffer.from(JSON.stringify(template, undefined, 4));
const file: RevisionDraftFile = nonNullValue(this.draftStore.get(uri.path));
if (file.contents === newContent) {
return;
}

file.contents = newContent;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be worth extracting this out to an updateFile function or something similar. You do it above as well, and I could potentially see what we set here being updated in the future.

Could also include

   this.draftStore.set(uri.path, file);
   this.fireSoon({ type: FileChangeType.Changed, uri });

   // Any new changes to the draft file can cause the states of a container app's children to change (e.g. displaying "Unsaved changes")
   ext.state.notifyChildrenChanged(file.containerAppId);

to be included in the function. I can't really think of an instance where we update and don't fire an event.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I think that separate function is already written actually. I think I can just call this.writeFile() with the new contents

file.size = newContent.byteLength;
file.mtime = Date.now();

this.draftStore.set(uri.path, file);
this.fireSoon({ type: FileChangeType.Changed, uri });

// Any new changes to the draft file can cause the states of a container app's children to change (e.g. displaying "Unsaved changes")
ext.state.notifyChildrenChanged(file.containerAppId);
}

// Delete
discardRevisionDraft<T extends ContainerAppsItem>(item: T): void {
discardRevisionDraft(item: ContainerAppsItem): void {
const uri: Uri = this.buildUriFromItem(item);
if (!this.draftStore.has(uri.path)) {
return;
Expand All @@ -162,7 +177,7 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
}

// Helper
private buildUriFromItem<T extends ContainerAppsItem>(item: T): Uri {
private buildUriFromItem(item: ContainerAppsItem): Uri {
return URI.parse(`${RevisionDraftFileSystem.scheme}:/${item.containerApp.name}.json`);
}

Expand Down
41 changes: 41 additions & 0 deletions src/commands/revisionDraft/RevisionDraftUpdateBaseStep.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import type { Template } from "@azure/arm-appcontainers";
import { AzureWizardExecuteStep, nonNullValueAndProp } from "@microsoft/vscode-azext-utils";
import type { Progress } from "vscode";
import { ext } from "../../extensionVariables";
import type { RevisionsItemModel } from "../../tree/revisionManagement/RevisionItem";
import type { IContainerAppContext } from "../IContainerAppContext";

export abstract class RevisionDraftUpdateBaseStep<T extends IContainerAppContext> extends AzureWizardExecuteStep<T> {
/**
* This property holds the active template revisions used for updating the revision draft
*/
protected revisionDraftTemplate: Template;

constructor(readonly baseItem: RevisionsItemModel) {
super();
this.revisionDraftTemplate = this.initRevisionDraftTemplate();
}

abstract execute(context: T, progress: Progress<{ message?: string | undefined; increment?: number | undefined }>): Promise<void>;
abstract shouldExecute(context: T): boolean;

/**
* Call this method to upload `revisionDraftTemplate` changes to the container app revision draft
*/
protected updateRevisionDraftWithTemplate(): void {
ext.revisionDraftFileSystem.updateRevisionDraftWithTemplate(this.baseItem, this.revisionDraftTemplate);
}

private initRevisionDraftTemplate(): Template {
let template: Template | undefined = ext.revisionDraftFileSystem.parseRevisionDraft(this.baseItem);
if (!template) {
template = nonNullValueAndProp(this.baseItem.revision, 'template');
}
return template;
}
}
53 changes: 34 additions & 19 deletions src/commands/scaling/addScaleRule/AddScaleRuleStep.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,53 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import type { ScaleRule, Template } from "@azure/arm-appcontainers";
import { AzureWizardExecuteStep, nonNullProp } from "@microsoft/vscode-azext-utils";
import { Progress, window } from "vscode";
import { KnownActiveRevisionsMode, type ScaleRule } from "@azure/arm-appcontainers";
import { nonNullProp } from "@microsoft/vscode-azext-utils";
import type { Progress } from "vscode";
import { ScaleRuleTypes } from "../../../constants";
import { ext } from "../../../extensionVariables";
import type { RevisionsItemModel } from "../../../tree/revisionManagement/RevisionItem";
import { delay } from "../../../utils/delay";
import { localize } from "../../../utils/localize";
import { updateContainerApp } from "../../../utils/updateContainerApp";
import { RevisionDraftUpdateBaseStep } from "../../revisionDraft/RevisionDraftUpdateBaseStep";
import type { IAddScaleRuleContext } from "./IAddScaleRuleContext";

export class AddScaleRuleStep extends AzureWizardExecuteStep<IAddScaleRuleContext> {
public priority: number = 100;
export class AddScaleRuleStep<T extends IAddScaleRuleContext> extends RevisionDraftUpdateBaseStep<T> {
public priority: number = 200;

public async execute(context: IAddScaleRuleContext, _progress: Progress<{ message?: string | undefined; increment?: number | undefined }>): Promise<void> {
const adding = localize('addingScaleRule', 'Adding {0} rule "{1}" to "{2}"...', context.ruleType, context.ruleName, context.containerApp.name);
const added = localize('addedScaleRule', 'Successfully added {0} rule "{1}" to "{2}".', context.ruleType, context.ruleName, context.containerApp.name);
constructor(baseItem: RevisionsItemModel) {
super(baseItem);
}

public async execute(context: IAddScaleRuleContext, progress: Progress<{ message?: string | undefined; increment?: number | undefined }>): Promise<void> {
let adding: string | undefined;
let added: string | undefined;
if (context.containerApp.revisionsMode === KnownActiveRevisionsMode.Single) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think you could have a "resourceName" variable here that is either "container app" or "revision" and replace that in the adding and added strings. It's just a little messy to see these long sentences written out twice (at least in GitHub)

adding = localize('addingScaleRuleSingle', 'Add {0} rule "{1}" to container app "{2}" (draft)', context.ruleType, context.ruleName, context.containerApp.name);
added = localize('addedScaleRuleSingle', 'Added {0} rule "{1}" to container app "{2}" (draft).', context.ruleType, context.ruleName, context.containerApp.name);
} else {
adding = localize('addingScaleRuleMultiple', 'Add {0} rule "{1}" to revision "{2}" (draft)', context.ruleType, context.ruleName, this.baseItem.revision.name);
added = localize('addedScaleRuleMultiple', 'Added {0} rule "{1}" to revision "{2}" (draft)', context.ruleType, context.ruleName, this.baseItem.revision.name);
}

const template: Template = context.containerApp.template || {};
template.scale = context.scale || {};
template.scale.rules = context.scaleRules || [];
context.activityTitle = adding;
progress.report({ message: localize('addingRule', 'Adding scale rule...') });

this.revisionDraftTemplate.scale ||= {};
this.revisionDraftTemplate.scale.rules ||= [];

const scaleRule: ScaleRule = this.buildRule(context);
this.integrateRule(context, template.scale.rules, scaleRule);
this.integrateRule(context, this.revisionDraftTemplate.scale.rules, scaleRule);
this.updateRevisionDraftWithTemplate();

// Artificial delay to make the activity log look like it's performing an action
await delay(1000);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like a second is a little bit long to block a user for something that's artificial.

My understanding is that it's so that you can see Adding scale rule... appear, but even if they don't see that progress message, they can still see that the "Added scale rule to container app (draft)` completed successfully, right?

I don't know if we need that progress message since it's instantaneous to begin with.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do want to keep the delay though, I think we should put the delay in updateRevisionDraftWithTemplate so that all of the actions behave similarily.


ext.outputChannel.appendLog(adding);
await updateContainerApp(context, context.subscription, context.containerApp, { template });
context.scaleRule = scaleRule;
void window.showInformationMessage(added);
ext.outputChannel.appendLog(added);
}

public shouldExecute(context: IAddScaleRuleContext): boolean {
return context.ruleName !== undefined && context.ruleType !== undefined;
return !!context.ruleName && !!context.ruleType;
}

private buildRule(context: IAddScaleRuleContext): ScaleRule {
Expand Down Expand Up @@ -65,7 +80,7 @@ export class AddScaleRuleStep extends AzureWizardExecuteStep<IAddScaleRuleContex
// Portal only allows one HTTP rule per revision
const idx: number = scaleRules.findIndex((rule) => rule.http);
if (idx !== -1) {
scaleRules.splice(idx, 0);
scaleRules.splice(idx, 1);
}
break;
case ScaleRuleTypes.Queue:
Expand Down
8 changes: 3 additions & 5 deletions src/commands/scaling/addScaleRule/IAddScaleRuleContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import type { Scale, ScaleRule } from "@azure/arm-appcontainers";
import type { ScaleRule } from "@azure/arm-appcontainers";
import type { ExecuteActivityContext } from "@microsoft/vscode-azext-utils";
import type { ContainerAppModel } from "../../../tree/ContainerAppItem";
import type { IContainerAppContext } from "../../IContainerAppContext";

export interface IAddScaleRuleContext extends IContainerAppContext {
export interface IAddScaleRuleContext extends IContainerAppContext, ExecuteActivityContext {
// Make containerApp _required_
containerApp: ContainerAppModel;

scale: Scale;
scaleRules: ScaleRule[];

// Base Rule Properties
Expand All @@ -26,6 +26,4 @@ export interface IAddScaleRuleContext extends IContainerAppContext {
queueLength?: number;
secretRef?: string;
triggerParameter?: string;

scaleRule?: ScaleRule;
}
33 changes: 16 additions & 17 deletions src/commands/scaling/addScaleRule/ScaleRuleNameStep.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import type { ScaleRule } from '@azure/arm-appcontainers';
import { AzureWizardPromptStep } from '@microsoft/vscode-azext-utils';
import { localize } from '../../../utils/localize';
import type { IAddScaleRuleContext } from './IAddScaleRuleContext';
Expand All @@ -14,29 +13,29 @@ export class ScaleRuleNameStep extends AzureWizardPromptStep<IAddScaleRuleContex
public async prompt(context: IAddScaleRuleContext): Promise<void> {
context.ruleName = (await context.ui.showInputBox({
prompt: localize('scaleRuleNamePrompt', 'Enter a name for the new scale rule.'),
validateInput: (name: string | undefined): string | undefined => {
return validateScaleRuleInput(name, context.containerApp?.name, context.scaleRules);
}
validateInput: (name: string | undefined) => this.validateInput(context, name)
})).trim();
}

public shouldPrompt(context: IAddScaleRuleContext): boolean {
return context.ruleName === undefined;
return !context.ruleName;
}
}

function validateScaleRuleInput(name: string | undefined, containerAppName: string, scaleRules: ScaleRule[]): string | undefined {
name = name ? name.trim() : '';
private validateInput(context: IAddScaleRuleContext, name: string | undefined): string | undefined {
name = name ? name.trim() : '';

if (!/^[a-z0-9]([-a-z0-9]*[a-z0-9])?$/.test(name)) {
return localize('invalidChar', `A name must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character.`);
}
if (!/^[a-z0-9]([-a-z0-9]*[a-z0-9])?$/.test(name)) {
return localize('invalidChar', `A name must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character.`);
}

const scaleRuleExists: boolean = !!context.scaleRules?.some((rule) => {
return rule.name?.length && rule.name === name;
});

if (scaleRuleExists) {
return localize('scaleRuleExists', 'The scale rule "{0}" already exists in container app "{1}". Please enter a unique name.', name, context.containerApp.name);
}

const scaleRuleExists: boolean = !!scaleRules?.some((rule) => {
return rule?.name?.length && rule?.name === name;
});
if (scaleRuleExists) {
return localize('scaleRuleExists', 'The scale rule "{0}" already exists in container app "{1}". Please enter a unique name.', name, containerAppName);
return undefined;
}
return undefined;
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,21 @@ import { QueueAuthTriggerStep } from './queue/QueueAuthTriggerStep';
import { QueueLengthStep } from './queue/QueueLengthStep';
import { QueueNameStep } from './queue/QueueNameStep';

export class ScaleRuleTypeStep extends AzureWizardPromptStep<IAddScaleRuleContext> {
export class ScaleRuleTypeListStep extends AzureWizardPromptStep<IAddScaleRuleContext> {
public hideStepCount: boolean = true;

public async prompt(context: IAddScaleRuleContext): Promise<void> {
const placeHolder: string = localize('chooseScaleType', 'Choose scale type');
const qpItems: QuickPickItem[] = Object.values(ScaleRuleTypes).map(type => { return { label: type } });
context.ruleType = (await context.ui.showQuickPick(qpItems, { placeHolder })).label;
const qpItems: QuickPickItem[] = Object.values(ScaleRuleTypes).map(type => {
return { label: type };
});

context.ruleType = (await context.ui.showQuickPick(qpItems, {
placeHolder: localize('chooseScaleType', 'Choose scale type')
})).label;
}

public shouldPrompt(context: IAddScaleRuleContext): boolean {
return context.ruleType === undefined;
return !context.ruleType;
}

public async getSubWizard(context: IAddScaleRuleContext): Promise<IWizardOptions<IAddScaleRuleContext>> {
Expand Down
Loading