Skip to content
Merged
Show file tree
Hide file tree
Changes from 14 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
30 changes: 10 additions & 20 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,6 @@
"title": "%containerApps.editContainerApp%",
"category": "Azure Container Apps"
},
{
"command": "containerApps.updateImage",
"title": "%containerApps.updateImage%",
"category": "Azure Container Apps"
},
{
"command": "containerApps.deployImageApi",
"title": "%containerApps.deployImageApi%",
Expand Down Expand Up @@ -203,6 +198,11 @@
"title": "%containerApps.openConsoleInPortal%",
"category": "Azure Container Apps"
},
{
"command": "containerApps.updateContainer",
"title": "%containerApps.updateContainer%",
"category": "Azure Container Apps"
},
Comment on lines +201 to +205
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.

Don't want to block this PR from merging, but I think we need to discuss the possibility of rewording the command to make it more clear to users what's happening.

We need users to understand that there are pretty much two main actions for container apps:

  1. Editing their Container App configuration (scale rules, etc.) - currently this is "Edit Container App..."
  2. Updating the image running in their containers (within the Container App) - currently this is "Update Container..."

Nathan and I think that "Update Container..." might be too close to "Edit Container App...". What about using Deploy instead of Update?

Whatever we decide to do, we can make that change after this is merged.

Copy link
Copy Markdown
Contributor Author

@MicroFish91 MicroFish91 Nov 26, 2024

Choose a reason for hiding this comment

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

I agree, I'll try to squeeze this discussion into one of our next group meetings (probably just the engineers for now). I think these sorts of improvements overlap a ton with my big goal for the next release, and I have some extra context to provide that would be helpful.

{
"command": "containerApps.editScaleRange",
"title": "%containerApps.editScaleRange%",
Expand Down Expand Up @@ -373,11 +373,6 @@
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /containerAppItem(.*)revisionMode:single(.*)unsavedChanges:true/i",
"group": "3@2"
},
{
"command": "containerApps.updateImage",
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /containerAppItem(.*)revisionMode:single/i",
"group": "4@1"
},
{
"command": "containerApps.editContainerApp",
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /containerAppItem(.*)revisionMode:single/i",
Expand Down Expand Up @@ -438,11 +433,6 @@
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /revisionItem(.*)revisionState:active/i",
"group": "2@2"
},
{
"command": "containerApps.updateImage",
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /revisionDraft:false(.*)revisionItem/i",
"group": "3@1"
},
{
"command": "containerApps.deployRevisionDraft",
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /revisionDraftItem(.*)unsavedChanges:true/i",
Expand All @@ -463,16 +453,16 @@
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /revisionDraftItem/i",
"group": "1@2"
},
{
"command": "containerApps.updateImage",
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /revisionDraftItem/i",
"group": "2@1"
},
{
"command": "containerApps.editRevisionDraft",
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /revisionDraftItem/i",
"group": "3@1"
},
{
"command": "containerApps.updateContainer",
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /containerItem/i",
"group": "1@1"
},
{
"command": "containerApps.editScaleRange",
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /scaleItem/i",
Expand Down
2 changes: 1 addition & 1 deletion package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"containerApps.createContainerApp": "Create Container App...",
"containerApps.createContainerAppFromWorkspace": "Create Container App from Workspace...",
"containerApps.editContainerApp": "Edit Container App (Advanced)...",
"containerApps.updateImage": "Update Container Image...",
"containerApps.updateContainer": "Update Container...",
"containerApps.deployImageApi": "Deploy Image to Container App (API)...",
"containerApps.deployWorkspaceProject": "Deploy Project from Workspace...",
"containerApps.deployWorkspaceProjectApi": "Deploy Project from Workspace (API)...",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,28 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { AzureWizardExecuteStep } from "@microsoft/vscode-azext-utils";
import { parseImageName } from "../../../../utils/imageNameUtils";
import { localize } from "../../../../utils/localize";
import { AzureWizardActivityOutputExecuteStep } from "../../../AzureWizardActivityOutputExecuteStep";
import { type ContainerRegistryImageSourceContext } from "./ContainerRegistryImageSourceContext";
import { getLoginServer } from "./getLoginServer";

export class ContainerRegistryImageConfigureStep extends AzureWizardExecuteStep<ContainerRegistryImageSourceContext> {
export class ContainerRegistryImageConfigureStep<T extends ContainerRegistryImageSourceContext> extends AzureWizardActivityOutputExecuteStep<T> {
public priority: number = 570;
public stepName: string = 'containerRegistryImageConfigureStep';
protected getSuccessString = (context: T) => localize('successOutput', 'Successfully set container app image to "{0}".', context.image);
protected getFailString = (context: T) => localize('failOutput', 'Failed to set container app image to "{0}".', context.image);
protected getTreeItemLabel = (context: T) => localize('treeItemLabel', 'Set container app image to "{0}"', context.image);

public async execute(context: ContainerRegistryImageSourceContext): Promise<void> {
public async execute(context: T): Promise<void> {
context.image = `${getLoginServer(context)}/${context.repositoryName}:${context.tag}`;

const { registryName, registryDomain } = parseImageName(context.image);
context.telemetry.properties.registryName = registryName;
context.telemetry.properties.registryDomain = registryDomain ?? 'other';
}

public shouldExecute(context: ContainerRegistryImageSourceContext): boolean {
public shouldExecute(context: T): boolean {
return !context.image;
}
}
44 changes: 0 additions & 44 deletions src/commands/image/updateImage/UpdateImageDraftStep.ts

This file was deleted.

80 changes: 0 additions & 80 deletions src/commands/image/updateImage/updateImage.ts

This file was deleted.

6 changes: 4 additions & 2 deletions src/commands/registerCommands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { openGitHubRepo } from './gitHub/openGitHubRepo';
import { deployImageApi } from './image/deployImageApi/deployImageApi';
import { createAcr } from './image/imageSource/containerRegistry/acr/createAcr/createAcr';
import { openAcrBuildLogs } from './image/openAcrBuildLogs';
import { updateImage } from './image/updateImage/updateImage';
import { disableIngress } from './ingress/disableIngress/disableIngress';
import { editTargetPort } from './ingress/editTargetPort/editTargetPort';
import { enableIngress } from './ingress/enableIngress/enableIngress';
Expand All @@ -40,6 +39,7 @@ import { deleteScaleRule } from './scaling/scaleRule/deleteScaleRule/deleteScale
import { addSecret } from './secret/addSecret/addSecret';
import { deleteSecret } from './secret/deleteSecret/deleteSecret';
import { editSecretValue } from './secret/editSecret/editSecretValue';
import { updateContainer } from './updateContainer/updateContainer';
import { addWorkspaceProjectWalkthrough } from './walkthrough/addWorkspaceProject';
import { azureSignInWalkthrough } from './walkthrough/azureSignIn';
import { cleanUpResourcesWalkthrough } from './walkthrough/cleanUpResources';
Expand All @@ -58,12 +58,14 @@ export function registerCommands(): void {
registerCommandWithTreeNodeUnwrapping('containerApps.deleteContainerApp', deleteContainerApp);
registerCommandWithTreeNodeUnwrapping('containerApps.editContainerApp', editContainerApp);
registerCommandWithTreeNodeUnwrapping('containerApps.openConsoleInPortal', openConsoleInPortal);
registerCommandWithTreeNodeUnwrapping('containerApps.updateImage', updateImage);
registerCommandWithTreeNodeUnwrapping('containerApps.toggleEnvironmentVariableVisibility',
async (context: IActionContext, item: EnvironmentVariableItem) => {
await item.toggleValueVisibility(context);
});

// containers
registerCommandWithTreeNodeUnwrapping('containerApps.updateContainer', updateContainer);

// deploy
registerCommandWithTreeNodeUnwrapping('containerApps.deployImageApi', deployImageApi);
registerCommandWithTreeNodeUnwrapping('containerApps.deployRevisionDraft', deployRevisionDraft);
Expand Down
16 changes: 16 additions & 0 deletions src/commands/updateContainer/ContainerUpdateContext.ts
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.

nit: the command is named "update container" while the context is the reverse, "container update context". If there's not a good reason for this, then I'd prefer if they matched.

Copy link
Copy Markdown
Contributor Author

@MicroFish91 MicroFish91 Nov 26, 2024

Choose a reason for hiding this comment

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

I think I'm bouncing this one to @nturinski 🤭

I think he asked in a previous meeting people's preferences and was trying to improve naming conventions... but I can't remember who all was there to chime in

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.

Ah, yeah. I believe the conversation was basically anything that is a class/context should be noun (in this case, Container) prefixed so that in an alphabetical list, it's easy to see all of the Container related files grouped together. However, since commands have been following the "verbNoun" format, I was fine with the reverse order specifically for commands.

Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { type ExecuteActivityContext } from "@microsoft/vscode-azext-utils";
import { type SetTelemetryProps } from "../../telemetry/SetTelemetryProps";
import { type ContainerUpdateTelemetryProps as TelemetryProps } from "../../telemetry/commandTelemetryProps";
import { type IContainerAppContext } from "../IContainerAppContext";
import { type ImageSourceBaseContext } from "../image/imageSource/ImageSourceContext";

export interface ContainerUpdateBaseContext extends IContainerAppContext, ImageSourceBaseContext, ExecuteActivityContext {
containersIdx: number;
}

export type ContainerUpdateContext = ContainerUpdateBaseContext & SetTelemetryProps<TelemetryProps>;
Loading