Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 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.
60 changes: 35 additions & 25 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 @@ -50,23 +49,15 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
return this.emitter.event;
}

// Create
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 @@ -78,8 +69,7 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
this.fireSoon({ type: FileChangeType.Created, uri });
}

// 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 +83,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 @@ -118,8 +108,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 +135,30 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
ext.state.notifyChildrenChanged(file.containerAppId);
}

// Delete
discardRevisionDraft<T extends ContainerAppsItem>(item: T): void {
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);
}

discardRevisionDraft(item: ContainerAppsItem): void {
const uri: Uri = this.buildUriFromItem(item);
if (!this.draftStore.has(uri.path)) {
return;
Expand All @@ -161,8 +172,7 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
this.fireSoon({ type: FileChangeType.Deleted, uri });
}

// 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;
}
}
54 changes: 26 additions & 28 deletions src/commands/scaling/addScaleRule/AddScaleRuleStep.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,55 +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 { ScaleRuleTypes } from "../../../constants";
import { ext } from "../../../extensionVariables";
import type { RevisionsItemModel } from "../../../tree/revisionManagement/RevisionItem";
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);
}

const template: Template = context.containerApp.template || {};
template.scale = context.scale || {};
template.scale.rules = context.scaleRules || [];
public async execute(context: IAddScaleRuleContext): Promise<void> {
this.revisionDraftTemplate.scale ||= {};
this.revisionDraftTemplate.scale.rules ||= [];

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

ext.outputChannel.appendLog(adding);
await updateContainerApp(context, context.subscription, context.containerApp, { template });
context.scaleRule = scaleRule;
void window.showInformationMessage(added);
ext.outputChannel.appendLog(added);
const resourceName = context.containerApp.revisionsMode === KnownActiveRevisionsMode.Single ? context.containerApp.name : this.baseItem.revision.name;
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 don't really have a preference, but if you wanted to keep it as "revision "revision.name"" or "container app "containerApp.name"", you could include that as part of the resourceName logic.

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.

I think I get what you mean, but let me know if I was off with the change I made

ext.outputChannel.appendLog(localize('addedScaleRule', 'Added {0} rule "{1}" to "{2}" (draft)', context.newRuleType, context.newRuleName, resourceName));
}

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

private buildRule(context: IAddScaleRuleContext): ScaleRule {
const scaleRule: ScaleRule = { name: context.ruleName };
switch (context.ruleType) {
const scaleRule: ScaleRule = { name: context.newRuleName };
switch (context.newRuleType) {
case ScaleRuleTypes.HTTP:
scaleRule.http = {
metadata: {
concurrentRequests: nonNullProp(context, 'concurrentRequests')
concurrentRequests: nonNullProp(context, 'newHttpConcurrentRequests')
}
};
break;
case ScaleRuleTypes.Queue:
scaleRule.azureQueue = {
queueName: context.queueName,
queueLength: context.queueLength,
auth: [{ secretRef: context.secretRef, triggerParameter: context.triggerParameter }]
queueName: context.newQueueName,
queueLength: context.newQueueLength,
auth: [{ secretRef: context.newQueueSecretRef, triggerParameter: context.newQueueTriggerParameter }]
}
break;
default:
Expand All @@ -60,12 +58,12 @@ export class AddScaleRuleStep extends AzureWizardExecuteStep<IAddScaleRuleContex
}

private integrateRule(context: IAddScaleRuleContext, scaleRules: ScaleRule[], scaleRule: ScaleRule): void {
switch (context.ruleType) {
switch (context.newRuleType) {
case ScaleRuleTypes.HTTP:
// 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
23 changes: 11 additions & 12 deletions src/commands/scaling/addScaleRule/IAddScaleRuleContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,28 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import type { Scale, ScaleRule } from "@azure/arm-appcontainers";
import { 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[];
parentResourceName: string;
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.

Kind of a weird name to me. I get that getParentResource is the command being invoked, but parentResourceName is always referring to the container app name, right? Since the parent of the scale rule is the scale rule group item this was especially confusing to me.

Can we just call it containerAppName? Or can we not just get the CA name from the container app model? I think whether or not we're using the template or revision, the containerApp.name property should be correct.

Copy link
Copy Markdown
Contributor Author

@MicroFish91 MicroFish91 Aug 23, 2023

Choose a reason for hiding this comment

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

No, so in general the parent resource name could be the container app name or the revision name, so we don't know which one it is until we actually build the context. It's following the same naming convention as used for the util which returns a container app model or revision model. We might still be able to come up with a better name though, since it's definitely not a direct parent and I see why that could be confusing

Copy link
Copy Markdown
Contributor Author

@MicroFish91 MicroFish91 Aug 23, 2023

Choose a reason for hiding this comment

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

Potentially I could add parentResource: ContainerAppItem | Revision and just access the name property later when I need it. Felt like overkill to tack the whole thing on at the time, but it would make it a lot less confusing for situations like this where the name parent is a bit ambiguous šŸ¤”

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.

Maybe just a comment about that then? I personally felt like it was pretty confusing.


// Base Rule Properties
ruleName?: string;
ruleType?: string;
newRuleName?: string;
newRuleType?: string;

// HTTP Rule Properties
concurrentRequests?: string;
newHttpConcurrentRequests?: string;

// Queue Rule Properties
queueName?: string;
queueLength?: number;
secretRef?: string;
triggerParameter?: string;
newQueueName?: string;
newQueueLength?: number;
newQueueSecretRef?: string;
newQueueTriggerParameter?: string;

scaleRule?: ScaleRule;
}
Loading
⚔