Add revision draft support (single revision mode) by MicroFish91 · Pull Request #405 · microsoft/vscode-azurecontainerapps

MicroFish91 · 2023-07-07T17:54:41Z

Background:

Based on team feedback surrounding the initial revision draft PR (v1), here is the new and improved v2! Major changes include:

Removal of the "Latest" node in single revisions mode. We now try to hide the concept of revisions a little more in single revisions mode. The children of the "Latest" node have now come up one level (right now that's really mainly the scaleItem ("Scaling")
Instead of creating a draft node in single revisions mode, we now show "unsaved changes" indicators
The edit container app json no longer opens up by default and is less easily found, now labeled as an advanced editing feature
No longer persist revision drafts in Memento storage (in-memory only)

Purpose:

See Enable revision updates through editing container app envelope JSON #311

What is covered in this PR:

Revision draft support for single revision mode

What is coming down the line:

Deploy revision draft command
Revision draft support for multiple revisions mode

Demo of the full feature:

Demo (Not a super polished demo, but good enough to give some context)

I had to include a little bit of the multiple revisions mode code (for example, RevisionDraftItem) because of some loose coupling, but overall I tried to split it up so it's a little bit less code to review overall.

If you see missing command spots in the package.json, you can see here for what the full thing will eventually look like: a58ee13

MicroFish91 · 2023-07-10T19:18:01Z

+        if (this.containerApp.revisionsMode === KnownActiveRevisionsMode.Single) {
+            return !!this.containerApp.template?.scale && !isDeepEqual(this.containerApp.template.scale, scaleDraftTemplate);
+        } else {
+            // We only care about showing changes to descendants of the revision draft item when in multiple revisions mode


This else code segment will be introduced in the multiple revisions PR, commented out for now with a return placeholder

nturinski · 2023-07-11T22:45:41Z

+        };
+    }
+
+    getChildren(): TreeElementBase[] {


This being the same as the ContainerAppItem kind of makes me want this class to be extended from or based off of a an abstract ContainerAppItemBase. I think it'll be kind of annoying when we change/add children or the container app item itself.

Yeah, technically you'd probably have the ContainerAppItem, the RevisionItem, and the RevisionDraftItem all extend from this then. I'm open to some base class that has maybe a private method that you would call within getChildren to obtain the shared children.

Actually, instead of enforcing inheritance for this, how do you feel about providing the children through a static method on the RevisionItem. That feels simpler to me and we can still manage the children in one place. It also could make sense to keep it on that item because the template changes are analogous to the concept of revisions anyway.

nturinski · 2023-07-11T22:47:54Z

+    getChildren(): TreeElementBase[] {
+        let scale: Scale | undefined;
+
+        if (this.hasUnsavedChanges()) {


Don't need it now, but maybe just a side-note. Anytime we have an item that is part of the "batch changes", we could have a base model for it that has the same implementation for how to get the value/children and check for unsavedChanges, etc.

Yeah, I totally get what you mean, I was thinking about doing something like this eventually as well further down the line when we have a better vision for how all the batch revision work will tie together.

I'm not sure if I prefer extending from a base here or just implementing an interface (leaning towards the latter). If we ever decide we want to implement discard changes on the item level, that would also be a good candidate to include as well.

MicroFish91 · 2023-07-14T00:49:17Z

Just a heads up, I think this is ready for review again!

nturinski · 2023-07-18T17:37:57Z

    "containerApps.createContainerApp": "Create Container App...",
-    "containerApps.deployImage": "Update Container App Image......",
+    "containerApps.editContainerApp": "Edit Container App (Advanced)",
+    "containerApps.deployImage": "Update Container App Image...",


I'm wondering if we should call this: "Update Container..." or "Edit Container..." after our conversation offline. The image is just a part of the overall container, after all.

It's annoying and confusing, but Container =/= Container App.

Yeah it's 😵‍💫

…inerapps into mwf/revision-draft-alt-2

MicroFish91 added 2 commits July 7, 2023 10:50

Impl migration

848165c

Single revision only (reduce amount of code to review)

24e27fa

MicroFish91 changed the title ~~Introduce revision draft FS and revision draft CRUD operations (v2)~~ Add revision draft support (Single Revision Mode) Jul 7, 2023

MicroFish91 changed the title ~~Add revision draft support (Single Revision Mode)~~ Add revision draft support (single revision mode) Jul 7, 2023

MicroFish91 added 8 commits July 7, 2023 11:20

Remove unused commands

a58ee13

delay util

a62d704

Fix editContainerApp view context

0ef0044

Fix copy paste

3ae11b1

Remove more multiple revisions mode changes

c133338

Update method scope

9bf039f

Nit

f79c419

Import type

02dc1a4

MicroFish91 marked this pull request as ready for review July 7, 2023 19:58

MicroFish91 requested a review from a team as a code owner July 7, 2023 19:58

MicroFish91 added 2 commits July 7, 2023 13:52

Comment out multiple revisions specific impl

6e5ff2f

readonly

73a82b1

MicroFish91 commented Jul 10, 2023

View reviewed changes

nturinski reviewed Jul 11, 2023

View reviewed changes

alexweininger reviewed Jul 11, 2023

View reviewed changes

Comment thread src/commands/editContainerApp.ts Outdated

Comment thread src/tree/ContainerAppItem.ts Outdated

bwateratmsft reviewed Jul 12, 2023

View reviewed changes

Comment thread src/utils/isDeepEqual.ts Outdated

Comment thread src/utils/isDeepEqual.ts Outdated

Comment thread src/commands/editContainerApp.ts Outdated

MicroFish91 added 5 commits July 12, 2023 11:25

First pass review changes

60737de

baseRevisionName getter

a66b3ff

Non important edge case

67a5079

Comment

ea677ed

deep-eql

0b07f91

MicroFish91 force-pushed the mwf/revision-draft-alt-2 branch from bff2eff to 2fed691 Compare July 14, 2023 00:34

MicroFish91 force-pushed the mwf/revision-draft-alt-2 branch from 116d22f to 0b07f91 Compare July 14, 2023 19:02

MicroFish91 added 2 commits July 14, 2023 12:05

.has()

6621678

Merge with main

0256b14

Fix merge issues

242b6ad

MicroFish91 force-pushed the mwf/revision-draft-alt-2 branch from babd790 to 242b6ad Compare July 14, 2023 22:30

nturinski reviewed Jul 18, 2023

View reviewed changes

MicroFish91 added 2 commits July 18, 2023 11:43

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

ebe802e

…inerapps into mwf/revision-draft-alt-2

More feedback

91a7cdb

nturinski approved these changes Jul 18, 2023

View reviewed changes

MicroFish91 merged commit 48644c2 into main Jul 18, 2023

MicroFish91 deleted the mwf/revision-draft-alt-2 branch July 18, 2023 19:03

MicroFish91 mentioned this pull request Jul 19, 2023

There is an extra "..." for the action "Update Container App Image......" in the context menu #409

Closed

microsoft locked and limited conversation to collaborators Mar 8, 2024

Conversation

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

Uh oh!

Uh oh!

MicroFish91 Jul 10, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nturinski Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

MicroFish91 Jul 12, 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 Jul 12, 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!

nturinski Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

MicroFish91 Jul 12, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nturinski Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

MicroFish91 Jul 18, 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MicroFish91 commented Jul 7, 2023 •

edited

Loading

MicroFish91 Jul 10, 2023 •

edited

Loading

MicroFish91 Jul 12, 2023 •

edited

Loading

MicroFish91 Jul 12, 2023 •

edited

Loading

MicroFish91 Jul 12, 2023 •

edited

Loading

MicroFish91 commented Jul 14, 2023 •

edited

Loading

MicroFish91 Jul 18, 2023 •

edited

Loading