Add Scale Rules Feature #166

MicroFish91 · 2022-07-21T23:30:16Z

Feature support for adding scale rules. Currently only supporting HTTP and Azure Queue rules, no custom.

…inerapps into mwf/feat-add-scale-ruleset

nturinski · 2022-07-22T16:57:55Z

package.nls.json

@@ -26,5 +26,6 @@
    "containerApps.deleteConfirmation.EnterName": "Prompts with an input box where you enter the Container Apps environment name to delete.",
    "containerApps.deleteConfirmation.ClickButton": "Prompts with a warning dialog where you click a button to delete.",
    "containerApps.openConsoleInPortal": "Open Console in Portal",
-    "containerApps.editScalingRange": "Edit Scale Rule Setting..."
+    "containerApps.editScalingRange": "Edit Scale Range",


Usually any command that has follow up prompts ends with .... The idea is, something like Start App is one-click, and the action executes. Something like Edit Scale Range... has more prompts that the user must enter before the action occurs.

nturinski · 2022-07-22T16:58:54Z

package.json

@@ -167,6 +167,11 @@
                "command": "containerApps.editScalingRange",
                "title": "%containerApps.editScalingRange%",
                "category": "Azure Container Apps"
+            },
+            {
+                "command": "containerApps.addScaleRule",


Should also be added in "activationEvents" array in the package.json.

nturinski · 2022-07-22T17:09:21Z

src/commands/scaling/addScaleRule/AddNewScaleRule.ts

+import { updateContainerApp } from "../../updateContainerApp";
+import { IAddScaleRuleWizardContext } from "./IAddScaleRuleWizardContext";
+
+export class AddNewScaleRule extends AzureWizardExecuteStep<IAddScaleRuleWizardContext> {


nit: Usually we include Step in the class name. Also, I think Add and New are somewhat redundant, so maybe just name it AddScaleRuleStep?

nturinski · 2022-07-22T17:10:25Z

src/commands/scaling/addScaleRule/AddNewScaleRule.ts

+        const node: ScaleRuleGroupTreeItem = nonNullProp(context, "treeItem");
+        const containerApp: ContainerAppTreeItem = node.parent.parent instanceof RevisionTreeItem ? node.parent.parent.parent.parent : node.parent.parent;
+
+        const adding = localize('addingScaleRule', 'Adding scale rule setting to "{0}"...', containerApp.name);


May be helpful to include the type of scale rule being added.

nturinski · 2022-07-22T17:19:13Z

src/commands/scaling/addScaleRule/AddNewScaleRule.ts

+                if (idx !== -1) {
+                    scaleRules[idx] = scaleRule;
+                } else {
+                    scaleRules.push(scaleRule);


I think it makes sense to just have either a default case that always pushes the scaleRule in and instead of replacing the HTTP rule by overwriting scaleRules[ind], to just splice it out. That way you could do something more like

switch (context.ruleType) { case ScaleRuleTypes.HTTP: const idx: number = scaleRules.findIndex((rule) => rule.http); if (idx) { scaleRules.splice(idx, 1) } case ScaleRuleTypes.Queue: default: scaleRules.push(scaleRule); }

nturinski · 2022-07-22T17:38:00Z

src/commands/scaling/addScaleRule/GetScaleRuleTypeStep.ts

+import { GetQueueLengthStep } from './queue/GetQueueLengthStep';
+import { GetQueueNameStep } from './queue/GetQueueNameStep';
+
+export class GetScaleRuleTypeStep extends AzureWizardPromptStep<IAddScaleRuleWizardContext> {


I think we can just call it ScaleRuleTypeStep. I think a promptStep is kind of assumed to be a "get" of some sort.

nturinski · 2022-07-22T17:39:20Z

src/commands/scaling/addScaleRule/addScaleRule.ts

+        node = await ext.tree.showTreeItemPicker<ScaleRuleGroupTreeItem>(new RegExp(ScaleRuleGroupTreeItem.contextValue), context);
+    }
+
+    const title: string = localize('addScaleRuleTitle', 'Create Scale Rule');


Kind of weird that it's called "Add Scale Rule" everywhere, but the title is "Create Scale Rule".

Yeah, I actually consciously made that choice just because it felt like a more accurate description of those specific steps, but I totally see why the lack of consistency would feel weird so I'll change it back

nturinski · 2022-07-22T17:44:14Z

src/commands/scaling/addScaleRule/AddNewScaleRule.ts

+import { updateContainerApp } from "../../updateContainerApp";
+import { IAddScaleRuleWizardContext } from "./IAddScaleRuleWizardContext";
+
+export class AddNewScaleRule extends AzureWizardExecuteStep<IAddScaleRuleWizardContext> {


Some of this logic could be moved to ScaleRuleGroupTreeItem.createChildImpl. If you look at ManagedEnvironmentTreeItem, you'll see what I mean, but the benefit of doing this is that it'll automatically handle adding the child to the parent in the UI. You will also see the "Creating..." node under the parent as it's creating.

If you need more context/have questions, let's talk about it offline.

nturinski · 2022-07-22T17:47:31Z

src/commands/scaling/addScaleRule/queue/GetQueueLengthStep.ts

+        if (!/^[1-9]+[0-9]*$/.test(length)) {
+            return localize('invalidQueueLength', 'The number of requests must be a whole number greater than or equal to 1.');
+        }
+        if (Number(length) > thirtyTwoBitMaxSafeInteger) {


You can probably leave thirtyTwoBitMaxSafeInteger in this file since it's only being used in this one spot.

nturinski · 2022-07-22T17:50:04Z

src/commands/scaling/addScaleRule/IAddScaleRuleWizardContext.ts

+    ruleType?: string;
+    concurrentRequests?: string;
+    queueName?: string;
+    queueLength?: string;


It's a matter of preference, but I usually define the context values to match the "true" value is going to be at the time of the API call. What I mean is, queueLength will come in as a string because of user input, but when being sent in the request, it should be a number.

Therefore, I would do the conversion in the prompt step rather than at the time of the API call. The benefit being, if we ever use that step again for something else, we wouldn't have to remember to convert it to a number.

src/commands/scaling/addScaleRule/addScaleRule.ts

…inerapps into mwf/feat-add-scale-ruleset

src/commands/scaling/addScaleRule/AddScaleRuleStep.ts

nturinski · 2022-07-26T22:52:18Z

package.json

+                },
+                {
+                    "command": "containerApps.addScaleRule",
+                    "when": "view == containerApps && viewItem =~ /scaleRules(?![a-z])/i",


Somewhat of a nit, but this part of the regex (?![a-z]) is unnecessary. The reason the command above requires it because there are scale, scaleRule and scaleRules context values, so if just /scale/ is matched, it matches for all 3 of these. However, scaleRules will only match to scaleRules.

I don't necessarily feel strongly about keeping it this way because it could be kind of future proofing, but just thought you should understand why it was there in the first place.

nturinski · 2022-07-26T23:37:37Z

src/commands/scaling/addScaleRule/AddScaleRuleStep.ts

+    public priority: number = 100;
+
+    public async execute(context: IAddScaleRuleWizardContext, _progress: Progress<{ message?: string | undefined; increment?: number | undefined }>): Promise<void> {
+        const adding = localize('addingScaleRule', 'Adding "{0}" {1} type rule to "{2}"...', context.ruleName, context.ruleType, context.containerApp.name);


The wording of this seems kind of awkward to me:

"Adding "httpRule" HTTP scaling type rule to "app1"..."

Maybe?

Adding HTTP scaling rule "httpRule" to "app1"...

nturinski · 2022-07-26T23:41:03Z

src/commands/scaling/addScaleRule/AddScaleRuleStep.ts

+            void window.showInformationMessage(added);
+            ext.outputChannel.appendLog(added);
+            context.scaleRule = scaleRule;
+        } catch (error) {


Have you tried just throwing this error? Our callWithTelemetryAndErrorHandling should handle a thrown error by displaying an error window and output message. Plus, not including the error and just saying that it failed seems less useful to the user since the error usually contains information as to why it failed.

You actually shouldn't even need to wrap this in a try/catch at all and just let our error handling throw it.

Oh I see, I should have checked how createChildImpl was called, it looks like it just swallows the error without displaying it which covers the issue I was trying to avoid by doing this (having two errors pop up because of it not being percolated up properly)

nturinski · 2022-07-26T23:51:23Z

src/commands/scaling/addScaleRule/IAddScaleRuleWizardContext.ts

+    scaleRuleGroup: ScaleRuleGroupTreeItem;
+    ruleName?: string;
+    ruleType?: string;
+    concurrentRequests?: string;


It might be cleaner to group the properties that are specific to ruleTypes into their own objects. For example:

httpProperties: { concurrentRequests?: string; }, queueProperties?: { queueName?: string; queueLength?: string; secretRef?: string; triggerParameter?: string; }

src/commands/scaling/addScaleRule/ScaleRuleNameStep.ts

nturinski · 2022-07-27T00:08:03Z

src/commands/scaling/addScaleRule/queue/QueueLengthStep.ts

+        length = length ? length.trim() : '';
+
+        const thirtyTwoBitMaxSafeInteger = 2147483647;
+        if (!/^[1-9]+[0-9]*$/.test(length)) {


Could probably make this regex a constant.

Or if HttpConcurrentRequestStep actually follows thirtyTwoBitMaxSafeInteger as well, maybe you could make a base step that has this implementation of validateInput

nturinski · 2022-07-27T00:09:08Z

src/tree/ScaleRuleGroupTreeItem.ts

+        context.showCreatingTreeItem(nonNullProp(wizardContext, 'ruleName'));
+        await wizard.execute();
+
+        if (wizardContext.error !== undefined) {


You should be able to just throw the error from the wizard step instead of tracking it and then throwing it after the execution.

nturinski

Shoot, I think my suggestion may have introduced a bug in the back button. I can explain offline, but it's because you're using the httpProperties/queueProperties in the context now 😬

nturinski · 2022-07-29T19:52:26Z

src/commands/scaling/addScaleRule/ScaleRuleTypeStep.ts

+
+    public async prompt(context: IAddScaleRuleWizardContext): Promise<void> {
+        const qpItems: QuickPickItem[] = [];
+        for (const ruleType in ScaleRuleTypes) { qpItems.push({ label: ScaleRuleTypes[ruleType as keyof typeof ScaleRuleTypes] }); }


Suggested change

for (const ruleType in ScaleRuleTypes) { qpItems.push({ label: ScaleRuleTypes[ruleType as keyof typeof ScaleRuleTypes] }); }

const qpItems: QuickPickItem[] = Object.values(ScaleRuleTypes).map(type => { return { label: type } });

nturinski · 2022-07-29T19:55:00Z

src/commands/scaling/addScaleRule/queue/QueueAuthSecretStep.ts

+        const secrets: Secret[] | undefined = containerAppWithSecrets.configuration.secrets;
+        const qpItems: QuickPickItem[] = secrets?.map((secret) => {
+            return { label: nonNullProp(secret, "name") };
+        }) || [];


You might want to push a quickpick in here that says something like that they need a secretRef or maybe just throw an error if it's empty with an error saying to create a secretRef in the Portal?

nturinski · 2022-07-29T19:58:27Z

src/commands/scaling/addScaleRule/queue/QueueAuthSecretStep.ts

+        const qpItems: QuickPickItem[] = secrets?.map((secret) => {
+            return { label: nonNullProp(secret, "name") };
+        }) || [];
+        context.queueProps.secretRef = (await context.ui.showQuickPick(qpItems, {})).label;


No placeHolder text?

src/commands/scaling/addScaleRule/queue/QueueAuthSecretStep.ts

…inerapps into mwf/feat-add-scale-ruleset

nturinski · 2022-08-02T22:02:09Z

src/commands/scaling/addScaleRule/ScaleRuleNameStep.ts

+            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, this.containerApp?.name as string);


nit: I don't think you need the as string typing at the end since localize doesn't care if it's undefined or not.

nturinski · 2022-08-02T22:04:38Z

src/commands/scaling/addScaleRule/queue/QueueAuthSecretStep.ts

+
+export class QueueAuthSecretStep extends AzureWizardPromptStep<IAddScaleRuleWizardContext> {
+    public async prompt(context: IAddScaleRuleWizardContext): Promise<void> {
+        const noSecrets: string = localize('noSecretsFound', 'No secrets were found. Create a secret to proceed.');


nit: I personally prefer to initializing variables closer to where they are being used.

nturinski · 2022-08-02T22:09:44Z

src/commands/scaling/addScaleRule/AddScaleRuleStep.ts

+
+        ext.outputChannel.appendLog(adding);
+        await updateContainerApp(context, context.containerApp, { template });
+        context.scaleRule = scaleRule;


nit: I don't think that there's really any point in doing this. You're not really using it anywhere else, as far as I can tell.

Yeah good catch, this had a specific use in a previous iteration, but now has gone the way of the appendix 😂

Edit: Just kidding, I'm still using it here

nturinski

In general, anything I write with "nit:" means stylistically, it's something I felt like I could comment on, but it's not something that I would actually block you merging your PR in for (especially if you disagree)

nturinski · 2022-08-02T22:11:23Z

src/utils/treeUtils.ts

+            }
+            currentNode = currentNode.parent;
+        }
+        return foundParent ? currentNode as T : null;


I think you should throw some sort of error here if you don't get a parent. If it returns as null, you'll get some funky errors in other parts of code. That way you won't have to do any as T typings either.

Good call, this looks way cleaner now

MicroFish91 · 2022-08-03T17:34:23Z

Okay, I think I fixed all relevant change suggestions, please review again when ready :)

nturinski · 2022-08-03T18:17:53Z

src/commands/scaling/addScaleRule/addScaleRule.ts

+    }
+    const containerApp: ContainerAppTreeItem = treeUtils.findNearestParent(node, ContainerAppTreeItem.prototype);
+    await node.createChild(context);
+    await containerApp?.refresh(context);


nit: Probably don't need the ? anymore

nturinski · 2022-08-03T18:21:00Z

src/utils/treeUtils.ts

+        }
+        if (!foundParent) {
+            const notFound: string = localize('parentNotFound', 'Could not find nearest parent "{0}".', parentInstance);
+            throw Error(notFound);


You have this somewhere else, but in general, we do throw new Error(). I didn't want to make you change it without a good reason, but I found this https://www.geeksforgeeks.org/difference-between-throw-errormsg-and-throw-new-errormsg/ and think it makes sense to keep using new based off some of the benefits.

nturinski

🎉🎉🎉~~Congrats on your first big feature PR! 53 total comments isn't too bad ~~🎉🎉🎉

MicroFish91 added 8 commits July 15, 2022 17:49

temp

43239e0

Added placeholder validation logic for checking duplicate scaleRule name

01da6d0

Refactor validateInput logic for name step

4bcad2c

Setup with subwizard

2f75583

Http scale rule + queue prompting

9c3706d

queue auth secret step

3af523e

Merge branch 'main' of https://github.com/microsoft/vscode-azureconta…

4937401

…inerapps into mwf/feat-add-scale-ruleset

added queue scale rule logic

ea9a00d

MicroFish91 requested a review from a team as a code owner July 21, 2022 23:30

MicroFish91 added 2 commits July 21, 2022 18:11

Fixed a string typo

b33b46f

Updated validation requirements for naming

d2a546c

nturinski reviewed Jul 22, 2022

View reviewed changes

MicroFish91 added 8 commits July 22, 2022 11:55

renaming spree

b0b6d16

match context typing with azureQueue typing

ef94ff9

revise constants

1183834

restructure switch statements

fe84313

fix shouldPrompts

992abfa

move containerApp ref to context

34ebc45

unfinished createChildImpl migration

20323c1

update scaling verbiage

9f2e749

alexweininger reviewed Jul 25, 2022

View reviewed changes

src/commands/scaling/addScaleRule/addScaleRule.ts Outdated Show resolved Hide resolved

MicroFish91 added 6 commits July 25, 2022 13:08

Merge branch 'main' of https://github.com/microsoft/vscode-azureconta…

af37e5e

…inerapps into mwf/feat-add-scale-ruleset

createChildImpl

f8a56cd

'find' to 'some'

66cd4bb

add default

378c8dc

trigger containerApp refresh

30b2d6e

Added findNearestNeighbor treeUtil

f7f69a3

MicroFish91 force-pushed the mwf/feat-add-scale-ruleset branch from c0aa434 to f7f69a3 Compare July 26, 2022 01:46

renaming...

fe0f6b7

alexweininger reviewed Jul 26, 2022

View reviewed changes

src/commands/scaling/addScaleRule/AddScaleRuleStep.ts Show resolved Hide resolved

MicroFish91 added 2 commits July 26, 2022 14:32

update via scaleRules via context

7a1f814

Add HTTP rule comment

9c936bf

nturinski reviewed Jul 27, 2022

View reviewed changes

MicroFish91 added 4 commits July 28, 2022 11:21

misc fixes

7374e12

Create common step inheritance

5b5e51b

cleanup async validation

f7056a8

rename base step

bcaccee

nturinski reviewed Jul 29, 2022

View reviewed changes

add placeHolders + informational text

c9fc95e

nturinski reviewed Aug 1, 2022

View reviewed changes

src/commands/scaling/addScaleRule/queue/QueueAuthSecretStep.ts Show resolved Hide resolved

refactor IAddScaleRuleWizard

9323724

MicroFish91 force-pushed the mwf/feat-add-scale-ruleset branch from 22ff8f9 to 3c3818e Compare August 2, 2022 20:47

simplify no secrets error

a40cd1e

MicroFish91 force-pushed the mwf/feat-add-scale-ruleset branch from 3c3818e to a40cd1e Compare August 2, 2022 20:49

MicroFish91 added 2 commits August 2, 2022 13:50

remove space

7aa5712

Merge branch 'main' of https://github.com/microsoft/vscode-azureconta…

6434573

…inerapps into mwf/feat-add-scale-ruleset

nturinski reviewed Aug 2, 2022

View reviewed changes

remove nearestParent null return

dd5289a

nturinski reviewed Aug 3, 2022

View reviewed changes

throw new

31324a9

nturinski approved these changes Aug 3, 2022

View reviewed changes

MicroFish91 merged commit aeaad95 into main Aug 3, 2022

MicroFish91 deleted the mwf/feat-add-scale-ruleset branch August 3, 2022 20:30

alexweininger mentioned this pull request Oct 12, 2022

Create/edit scale rules #46

Closed

microsoft locked and limited conversation to collaborators Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Scale Rules Feature #166

Add Scale Rules Feature #166

MicroFish91 commented Jul 21, 2022

nturinski Jul 22, 2022

nturinski Jul 22, 2022

nturinski Jul 22, 2022

nturinski Jul 22, 2022

nturinski Jul 22, 2022

nturinski Jul 22, 2022

nturinski Jul 22, 2022

MicroFish91 Jul 22, 2022 •

edited

nturinski Jul 22, 2022

nturinski Jul 22, 2022

nturinski Jul 22, 2022

nturinski Jul 26, 2022

nturinski Jul 26, 2022

nturinski Jul 26, 2022

MicroFish91 Jul 28, 2022 •

edited

nturinski Jul 26, 2022

nturinski Jul 27, 2022

nturinski Jul 27, 2022

nturinski left a comment

nturinski Jul 29, 2022

nturinski Jul 29, 2022

nturinski Jul 29, 2022

nturinski Aug 2, 2022

nturinski Aug 2, 2022

nturinski Aug 2, 2022

MicroFish91 Aug 3, 2022 •

edited

nturinski Aug 3, 2022

nturinski left a comment

nturinski Aug 2, 2022

MicroFish91 Aug 3, 2022 •

edited

MicroFish91 commented Aug 3, 2022

nturinski Aug 3, 2022

nturinski Aug 3, 2022

nturinski left a comment

	for (const ruleType in ScaleRuleTypes) { qpItems.push({ label: ScaleRuleTypes[ruleType as keyof typeof ScaleRuleTypes] }); }
	const qpItems: QuickPickItem[] = Object.values(ScaleRuleTypes).map(type => { return { label: type } });

Add Scale Rules Feature #166

Add Scale Rules Feature #166

Conversation

MicroFish91 commented Jul 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicroFish91 Jul 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicroFish91 Jul 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nturinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicroFish91 Aug 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nturinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicroFish91 Aug 3, 2022 • edited

Choose a reason for hiding this comment

MicroFish91 commented Aug 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nturinski left a comment

Choose a reason for hiding this comment

MicroFish91 Jul 22, 2022 •

edited

MicroFish91 Jul 28, 2022 •

edited

MicroFish91 Aug 3, 2022 •

edited

MicroFish91 Aug 3, 2022 •

edited