Add command `updateImage` for updating a container image via revision draft by MicroFish91 · Pull Request #477 · microsoft/vscode-azurecontainerapps

MicroFish91 · 2023-10-08T18:27:02Z

Part 2

Partially addresses #426

This is the new updateImage command implementation. It can be executed on the ContainerAppItem level or on the RevisionItem level to update the container image. Any changes will be made to the revision draft instead of automatically executing a deployment like before.

A few things to note:

I've also added a new thing where I append a short random hex value to the container name on draft update. This is because if you are re-deploying using the exact same image values, the extension will have no easy way to detect that there are changes to deploy. This was an easy way to fix that and get it to play nicely with our current setup.
One other noteworthy area is the new UpdateRegistryAndSecretsStep. I've added some deep copy comparison logic to try and see if we can skip credential updates since updating the container app's secrets before deployment can trigger a bit of a wait which feels bad to always do unless we absolutely need it.

Todo: In a follow-up PR I'm going to build out some sort of draft command information pop-up. Right now updating the image does get a little lost because we don't yet have the containers section of the tree implemented. Having a pop-up should help the results appear a bit more prominently and I think will be a nice addition.

Update: Here's the demo
This PR doesn't cover the pop-up part shown in the demo

…ode-azurecontainerapps into mwf/update-container-image

nturinski · 2023-10-10T22:31:20Z

 <b>Priority Range</b>: 400 - 490

-#### Build Image in Azure Steps
+#### General Steps


I kind of find this hard to read:

I feel like a use of a divider or something could really help readability. I won't block for that, just wanted to point it out.

Made an engineering issue

nturinski · 2023-10-10T22:37:37Z


- ContainerAppUpdateStep: 480 (Todo - investigate decoupling this command from imageSource when revision draft update support is added)
+- UpdateRegistryAndSecretsStep: 480
+- UpdateImageStep: 490 (revision draft)


Maybe we should just call this CreateRevisionDraft internally?

nturinski · 2023-10-10T22:51:44Z


-<!-- Todo: Add this back in when update image command is added in follow-up PR -->
-<!-- - `updateContainerImage` - An ACA exclusive command that updates the container app or revision's container image through updates to a revision draft.  The draft must be deployed for the changes to take effect and can be used to bundle together `Template` changes. -->
+- `updateImage` - An ACA exclusive command that updates the container app or revision's container image via revision draft.  The draft must be deployed for the changes to take effect and can be used to bundle together `Template` changes.


Had a similar comment in your other PR. #476 (comment)

nturinski · 2023-10-10T22:51:59Z

    const registries: RegistryCredentials[] = containerAppSettings?.registries?.filter(r => r.server !== registry.loginServer) ?? [];
    registries?.push(
        {
+            identity: '',


What's the dealio with this?

The server populates an undefined identity as '', so when I deep copy compare in the later steps in UpdateRegistryAndSecretsStep, I need to add this here to keep them being detected as equivalent. I forgot but I should probably add a comment explaining this

nturinski · 2023-10-10T22:56:47Z

+import { getContainerNameForImage } from "../imageSource/containerRegistry/getContainerNameForImage";
+import type { UpdateImageContext } from "./updateImage";
+
+export class UpdateImageStep<T extends UpdateImageContext> extends RevisionDraftUpdateBaseStep<T> {


I mentioned it in the execute priority markdown, but I think it makes more sense to call this CreateRevisionDraft or CreateImageDraft. Update feels like the wrong word here since it can mean remotely or locally. Update and upload also look super similar, so I feel like the brain just fills in the gaps.

Maybe we should use the word save? Save tends to mean locally, or at least in place?

Yeah let me look through some of the other steps and I'll get back to you. Maybe we can brainstorm a naming convention that works, I was thinking something similar before but didn't have any ideas that stuck.

nturinski · 2023-10-10T22:58:28Z

+        containerAppEnvelope.configuration.secrets = context.secrets;
+        containerAppEnvelope.configuration.registries = context.registries;
+
+        await updateContainerApp(context, context.subscription, containerAppEnvelope);


The whole update/save thing would apply to all these strings as well if we decide to make a change.

nturinski · 2023-10-10T23:06:01Z

-            template = nonNullValueAndProp(this.baseItem.revision, 'template');
+            // Branching path reasoning: https://github.com/microsoft/vscode-azurecontainerapps/blob/main/src/commands/revisionDraft/README.md
+            // Make deep copies so we don't accidentally modify the cached values
+            if (ContainerAppItem.isContainerAppItem(this.baseItem) || this.baseItem.containerApp.revisionsMode === KnownActiveRevisionsMode.Single) {


Can you not use the getParentResourceFromItem util function here to pick between containerApp or revision?

Oh yeah good catch, I think you should be able to

nturinski · 2023-10-11T22:06:38Z

Just approving ahead of time to not block you

MicroFish91 added 4 commits October 7, 2023 23:17

Impl

6060f1a

Misc

d80b108

More

b3bb7ad

Impl

f1c4700

MicroFish91 changed the title ~~Add support for updating a container image via revision draft~~ Add command updateImage for updating a container image via revision draft Oct 8, 2023

Updates

1880dd7

MicroFish91 force-pushed the mwf/update-container-image branch from 84dd53d to 1880dd7 Compare October 8, 2023 22:14

Update execute priority

2b8aa8b

MicroFish91 force-pushed the mwf/update-container-image branch from e9f8729 to 2b8aa8b Compare October 8, 2023 22:19

MicroFish91 added 3 commits October 8, 2023 15:21

Import type

c26a876

Merge branch 'mwf/image-organize' of https://github.com/microsoft/vsc…

b8a1d5b

…ode-azurecontainerapps into mwf/update-container-image

Import type

53de3bf

MicroFish91 marked this pull request as ready for review October 8, 2023 22:45

MicroFish91 requested a review from a team as a code owner October 8, 2023 22:45

Update

ce16017

MicroFish91 force-pushed the mwf/update-container-image branch from e8db514 to ce16017 Compare October 10, 2023 17:51

nturinski reviewed Oct 10, 2023

View reviewed changes

Base automatically changed from mwf/image-organize to main October 11, 2023 21:16

Merge with main

822d394

nturinski approved these changes Oct 11, 2023

View reviewed changes

MicroFish91 added 3 commits October 11, 2023 15:19

Feedback

8c71fbb

Update

d309d97

Update

ccea660

MicroFish91 merged commit 657473c into main Oct 11, 2023

MicroFish91 deleted the mwf/update-container-image branch October 11, 2023 23:38

microsoft locked and limited conversation to collaborators Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command `updateImage` for updating a container image via revision draft#477

Add command `updateImage` for updating a container image via revision draft#477
MicroFish91 merged 14 commits intomainfrom
mwf/update-container-image

MicroFish91 commented Oct 8, 2023 •

edited

Loading

Uh oh!

nturinski Oct 10, 2023

Uh oh!

MicroFish91 Oct 11, 2023

Uh oh!

nturinski Oct 10, 2023

Uh oh!

nturinski Oct 10, 2023

Uh oh!

nturinski Oct 10, 2023

Uh oh!

MicroFish91 Oct 11, 2023 •

edited

Loading

Uh oh!

nturinski Oct 10, 2023

Uh oh!

MicroFish91 Oct 11, 2023 •

edited

Loading

Uh oh!

nturinski Oct 10, 2023

Uh oh!

nturinski Oct 10, 2023

Uh oh!

MicroFish91 Oct 11, 2023 •

edited

Loading

Uh oh!

nturinski commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

MicroFish91 commented Oct 8, 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!

Choose a reason for hiding this comment

Uh oh!

MicroFish91 Oct 11, 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!

MicroFish91 Oct 11, 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!

MicroFish91 Oct 11, 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 commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MicroFish91 commented Oct 8, 2023 •

edited

Loading

MicroFish91 Oct 11, 2023 •

edited

Loading

MicroFish91 Oct 11, 2023 •

edited

Loading

MicroFish91 Oct 11, 2023 •

edited

Loading