Enable updating revision drafts through commands & leverage with `Scale Rules` by MicroFish91 · Pull Request #423 · microsoft/vscode-azurecontainerapps

MicroFish91 · 2023-08-03T22:26:44Z

Addresses 2/3 of #420
Closes #254

Summary:

Added a pathway for commands to leverage when updating the revision draft template
Hooked up Scale Rules to update the revision draft template rather than deploy a new revision
Refactor misc Add scale rule... improvements (this was my first big feature and going back I noticed a lot of things I wanted to change or fix 😮‍💨)
Turn all scale items into classes, and add logic so that unsaved changes are detected and shown in the tree view

Future:

Ended up removing + Create a secret related logic from this PR, will add it in the follow-up one
Will probably go back and refactor Add scale rule... a little bit more in the near future, mostly to clean up the validate input logic
Still need to add the rest of the CRUD ops for Scale Rules

Demo:

Scale Rules

Note: It works basically the same for multiple revisions mode. I was kind of lazy about recording that demo, but I at least tested the same ops in multiple revisions mode and it behaved the same way as in single revisions mode minus it being attached to a draft node.

MicroFish91 · 2023-08-04T18:53:31Z


    public shouldPrompt(context: IAddScaleRuleContext): boolean {
-        return context.queueName === undefined;
+        return !context.queueName;


Changed a lot of these to just a ! because I feel like there are other falsey values besides undefined that I would like to prevent as valid input.

nturinski · 2023-08-04T21:29:59Z

Just a sidenote: This would be a great place where we have a follow up action for the activity log. Underneath Add HTTP Scale Rule, it would awesome to have the Deploy/Update command underneath it to make it more obvious that they are unsaved changes.

nturinski · 2023-08-04T21:31:20Z

Also should change it from "creating revision" to "updating container app" or "deploying to container app"

MicroFish91 · 2023-08-04T21:57:01Z

Also should change it from "creating revision" to "updating container app" or "deploying to container app"

Yeah good call, I'll update this in a separate PR... there's another small change I want to make to the deploy command anyway

Added: #427

nturinski · 2023-08-16T22:01:14Z

@@ -51,22 +50,15 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
    }

    // Create


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

nturinski · 2023-08-17T17:00:38Z

+    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) {


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)

nturinski · 2023-08-17T17:07:28Z

+        this.updateRevisionDraftWithTemplate();
+
+        // Artificial delay to make the activity log look like it's performing an action
+        await delay(1000);


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.

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.

nturinski · 2023-08-17T17:11:48Z

+    const { subscription, containerApp, revision } = item;
+
+    // Branching path reasoning: https://github.com/microsoft/vscode-azurecontainerapps/blob/main/src/commands/revisionDraft/README.md
+    let scale: Scale | undefined;


We're going to have to duplicate this logic a lot (any time we add/update the draft). I think we should go ahead and extract this out as a util function.

The function should just return revision or containerApp depending on the revision type.

nturinski · 2023-08-17T17:23:00Z

+        readonly containerApp: ContainerAppModel,
+        readonly revision: Revision
+    ) {
+        if (this.hasUnsavedChanges()) {


It may be easier to discuss this offline, but I'll write a short blurb and we can discuss further.

But I think RevisionsDraftModel should have a function to set the properties based on if it has unsaved changes or not. That way, the ScaleRuleGroupItem (and any other item that needs this logic) doesn't have to have the branching logic in its constructor.

RevisionDraftModel should also have two abstract methods: setProperties and setPropertiesDraft (or something like that-- maybe setTreeItem?) that does the assignments.

So I think that would look something like this:

// For tree items that depend on the container app's revision draft template export abstract class RevisionsDraftModel { hasUnsavedChanges: () => boolean | Promise<boolean>; abstract function setProperties(): void; abstract function setPropertiesDraft(): void; constructor(...) { this.hasUnsavedChanges() ? this.setPropertiesDraft() : this.setProperties(); } }

After some testing, ran into some limitations with doing it this way, will discuss more offline

nturinski · 2023-08-17T17:25:05Z

+    }
+
+    private get parentResource(): ContainerAppModel | Revision {
+        return this.containerApp.revisionsMode === KnownActiveRevisionsMode.Single ? this.containerApp : this.revision;


Another spot we can use the util to choose between app or revision.

…inerapps into mwf/wire-scale-rule

MicroFish91 · 2023-08-21T17:38:02Z

-    });
-    if (scaleRuleExists) {
-        return localize('scaleRuleExists', 'The scale rule "{0}" already exists in container app "{1}". Please enter a unique name.', name, containerAppName);
+    private async validateNameAvailable(context: IAddScaleRuleContext, name: string): Promise<string | undefined> {


Heads up, I also added validation that always does a network call to ensure we aren't working with stale data when we run our verification logic.

alexweininger · 2023-08-21T18:37:49Z

-             * 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) {


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

nturinski · 2023-08-23T18:36:25Z

+            return;
+        }
+
+        file.contents = newContent;


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.

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

nturinski · 2023-08-23T19:05:12Z

-        await delay(1000);
-
-        ext.outputChannel.appendLog(added);
+        const resourceName = context.containerApp.revisionsMode === KnownActiveRevisionsMode.Single ? context.containerApp.name : this.baseItem.revision.name;


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.

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

nturinski · 2023-08-23T19:13:35Z

    containerApp: ContainerAppModel;
-
-    scaleRules: ScaleRule[];
+    parentResourceName: string;


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.

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

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 🤔

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

nturinski · 2023-08-23T19:55:33Z

+/**
+ * Can be implemented by any tree item that has the potential to show up as a RevisionDraftItem's descendant
+ */
+export abstract class RevisionDraftDescendantBase implements RevisionsDraftModel {


I feel like that this should implement RevisionsItemModel as well. viewProperties isn't declared here, but all of the RevisionDraftDescendants have it. It should define these types a bit more since I could see that being an easy property to forget for future items.

The interface RevisionsDraftModel could also just implement RevisionsItemModel as well to achieve this effect.

Yeah good catch, I should have included that as well

Unfortunately ContainerAppItem uses RevisionsDraftModel without RevisionsItemModel which is why I left them uncoupled, but I agree that the base item should include both

nturinski

(seal of approval)

MicroFish91 added 3 commits August 3, 2023 14:50

Impl

9ef3efa

Misc

15f370f

Import type

fc5b808

MicroFish91 force-pushed the mwf/wire-scale-rule branch from 9da09cc to fc5b808 Compare August 3, 2023 23:46

MicroFish91 changed the title ~~Allow updating the revision draft through commands & leverage with Scale Rules~~ Enable updating revision drafts through commands & leverage with Scale Rules Aug 4, 2023

MicroFish91 added 3 commits August 4, 2023 10:53

Misc

26da94a

Remove secret changes for separate PR

f8cd7e0

Remove extra refresh

10fa6d6

MicroFish91 marked this pull request as ready for review August 4, 2023 18:36

MicroFish91 requested a review from a team as a code owner August 4, 2023 18:37

MicroFish91 commented Aug 4, 2023

View reviewed changes

MicroFish91 marked this pull request as draft August 4, 2023 19:10

Fix pick item logic in multiple revisions mode

7da6384

MicroFish91 marked this pull request as ready for review August 4, 2023 19:27

noPicksMessage

8f53127

MicroFish91 added 2 commits August 4, 2023 15:05

Revert change for future PR

32b9861

Merge with main

95b66b3

nturinski reviewed Aug 17, 2023

View reviewed changes

MicroFish91 added 3 commits August 19, 2023 13:28

First pass of fixes

d95d633

Remove abstract class

8fe805b

Rename context vars

fe93023

MicroFish91 force-pushed the mwf/wire-scale-rule branch from ab5bfb2 to fe93023 Compare August 21, 2023 17:36

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

837d356

…inerapps into mwf/wire-scale-rule

MicroFish91 commented Aug 21, 2023

View reviewed changes

RevisionDraftDescendantBase

1abc75d

alexweininger reviewed Aug 21, 2023

View reviewed changes

remove instanceof

a40fc12

MicroFish91 added 3 commits August 21, 2023 12:51

import type

c13608a

Update regexp ref

fb901f2

Remove optional

bb2484b

nturinski reviewed Aug 23, 2023

View reviewed changes

More changes

0fe631d

MicroFish91 force-pushed the mwf/wire-scale-rule branch from 96c2fbf to 0fe631d Compare August 23, 2023 22:56

MicroFish91 added 2 commits August 23, 2023 16:22

writeFile

1847804

Changes

24da2bf

nturinski approved these changes Aug 31, 2023

View reviewed changes

MicroFish91 merged commit ca50bc6 into main Aug 31, 2023

MicroFish91 deleted the mwf/wire-scale-rule branch August 31, 2023 02:11

microsoft locked and limited conversation to collaborators Mar 8, 2024

		@@ -51,22 +50,15 @@ export class RevisionDraftFileSystem implements FileSystemProvider {
		}

		// Create

Conversation

MicroFish91 commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nturinski commented Aug 4, 2023

Uh oh!

nturinski commented Aug 4, 2023

Uh oh!

MicroFish91 commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nturinski Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MicroFish91 Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MicroFish91 Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MicroFish91 Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nturinski left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MicroFish91 commented Aug 3, 2023 •

edited

Loading

MicroFish91 commented Aug 4, 2023 •

edited

Loading

nturinski Aug 17, 2023 •

edited

Loading

MicroFish91 Aug 21, 2023 •

edited

Loading

MicroFish91 Aug 23, 2023 •

edited

Loading

MicroFish91 Aug 23, 2023 •

edited

Loading