-
Notifications
You must be signed in to change notification settings - Fork 17
Enable new workspace re-deploy settings for deployWorkspaceProject
#454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
e531cea
e0b6080
387bde6
e6e327e
d351562
99701c9
ef76b24
2db14e8
ad4f149
c2de90b
052fc86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import type { WorkspaceFolder } from "vscode"; | ||
| import { settingUtils } from "../../utils/settingUtils"; | ||
|
|
||
| export interface DeployWorkspaceProjectSettings { | ||
| // Container app names are unique to a resource group | ||
| containerAppResourceGroupName?: string; | ||
| containerAppName?: string; | ||
|
|
||
| containerRegistryName?: string; | ||
| } | ||
|
|
||
| const deployWorkspaceProjectPrefix: string = 'deployWorkspaceProject'; | ||
|
|
||
| export async function getDeployWorkspaceProjectSettings(rootFolder: WorkspaceFolder): Promise<DeployWorkspaceProjectSettings | undefined> { | ||
| const settingsPath: string = settingUtils.getDefaultRootWorkspaceSettingsPath(rootFolder); | ||
|
|
||
| try { | ||
| const containerAppName: string | undefined = settingUtils.getWorkspaceSetting(`${deployWorkspaceProjectPrefix}.containerAppName`, settingsPath); | ||
| const containerAppResourceGroupName: string | undefined = settingUtils.getWorkspaceSetting(`${deployWorkspaceProjectPrefix}.containerAppResourceGroupName`, settingsPath); | ||
| const containerRegistryName: string | undefined = settingUtils.getWorkspaceSetting(`${deployWorkspaceProjectPrefix}.containerRegistryName`, settingsPath); | ||
|
|
||
| if (containerAppName || containerAppResourceGroupName || containerRegistryName) { | ||
| return { | ||
| containerAppName, | ||
| containerAppResourceGroupName, | ||
| containerRegistryName | ||
| }; | ||
| } | ||
| } catch { /** Do nothing */ } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be useful to add a comment here that explains what errors are being ignored here and why
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok now that I see this again, you should probably log something inside that catch block too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a good suggestion, especially because it actually pushed me to test the get setting method a little bit more 😆 I realized that it doesn't actually throw an error even with bad settings path and property... so I removed the catch entirely. I also made a small change to the setting utils so that it reads a missing value as undefined rather than as an empty string
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok that's great |
||
|
|
||
| return undefined; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.md in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import type { Registry } from "@azure/arm-containerregistry"; | ||
| import type { ISubscriptionActionContext } from "@microsoft/vscode-azext-utils"; | ||
| import { ext } from "../../../extensionVariables"; | ||
| import { localize } from "../../../utils/localize"; | ||
| import { AcrListStep } from "../../deployImage/imageSource/containerRegistry/acr/AcrListStep"; | ||
| import { DeployWorkspaceProjectSettings } from "../DeployWorkspaceProjectSettings"; | ||
|
|
||
| interface DefaultAcrResources { | ||
| registry?: Registry; | ||
| imageName?: string; | ||
| } | ||
|
|
||
| export async function getDefaultAcrResources(context: ISubscriptionActionContext, settings: DeployWorkspaceProjectSettings | undefined): Promise<DefaultAcrResources> { | ||
| const noMatchingResource = { registry: undefined }; | ||
|
|
||
| if (!settings || !settings.containerRegistryName) { | ||
| return noMatchingResource; | ||
| } | ||
|
|
||
| const registries: Registry[] = await AcrListStep.getRegistries(context); | ||
| const savedRegistry: Registry | undefined = registries.find(r => r.name === settings.containerRegistryName); | ||
|
|
||
| if (savedRegistry) { | ||
| ext.outputChannel.appendLog(localize('foundResourceMatch', 'Used saved workspace settings and found an existing container registry.')); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Big fan of the logs in this function. You could include the found resource names in the log if you wanted to be even more verbose. But just an idea.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This suggestion will be included in the following PR! |
||
| return { | ||
| registry: savedRegistry, | ||
| imageName: `${settings.containerAppName || savedRegistry.name}:latest` | ||
| }; | ||
| } else { | ||
| ext.outputChannel.appendLog(localize('noResourceMatch', 'Used saved workspace settings to search for Azure Container Registry "{0}" but found no match.', settings.containerRegistryName)); | ||
| return noMatchingResource; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.md in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import { ContainerApp, ContainerAppsAPIClient, ManagedEnvironment } from "@azure/arm-appcontainers"; | ||
| import type { ResourceGroup } from "@azure/arm-resources"; | ||
| import { ResourceGroupListStep, uiUtils } from "@microsoft/vscode-azext-azureutils"; | ||
| import type { ISubscriptionActionContext } from "@microsoft/vscode-azext-utils"; | ||
| import { ext } from "../../../extensionVariables"; | ||
| import { ContainerAppItem, ContainerAppModel } from "../../../tree/ContainerAppItem"; | ||
| import { createContainerAppsAPIClient } from "../../../utils/azureClients"; | ||
| import { localize } from "../../../utils/localize"; | ||
| import { DeployWorkspaceProjectSettings } from "../DeployWorkspaceProjectSettings"; | ||
|
|
||
| interface DefaultContainerAppsResources { | ||
| resourceGroup?: ResourceGroup; | ||
| managedEnvironment?: ManagedEnvironment; | ||
| containerApp?: ContainerAppModel; | ||
| } | ||
|
|
||
| export async function getDefaultContainerAppsResources(context: ISubscriptionActionContext, settings: DeployWorkspaceProjectSettings | undefined): Promise<DefaultContainerAppsResources> { | ||
| const noMatchingResources = { | ||
| resourceGroup: undefined, | ||
| managedEnvironment: undefined, | ||
| containerApp: undefined | ||
| }; | ||
|
|
||
| if (!settings || !settings.containerAppResourceGroupName || !settings.containerAppName) { | ||
| return noMatchingResources; | ||
| } | ||
|
|
||
| const resourceGroupName: string = settings.containerAppResourceGroupName; | ||
| const containerAppName: string = settings.containerAppName; | ||
|
|
||
| try { | ||
| const client: ContainerAppsAPIClient = await createContainerAppsAPIClient(context) | ||
| const containerApp: ContainerApp = await client.containerApps.get(resourceGroupName, containerAppName); | ||
| const containerAppModel: ContainerAppModel = ContainerAppItem.CreateContainerAppModel(containerApp); | ||
|
|
||
| const managedEnvironments: ManagedEnvironment[] = await uiUtils.listAllIterator(client.managedEnvironments.listBySubscription()); | ||
| const managedEnvironment = managedEnvironments.find(env => env.id === containerAppModel.managedEnvironmentId); | ||
|
|
||
| const resourceGroups: ResourceGroup[] = await ResourceGroupListStep.getResourceGroups(context); | ||
| const resourceGroup = resourceGroups.find(rg => rg.name === containerAppModel.resourceGroup); | ||
|
|
||
| ext.outputChannel.appendLog(localize('foundResourceMatch', 'Used saved workspace settings and found existing container app resources.')); | ||
|
|
||
| return { | ||
| resourceGroup, | ||
| managedEnvironment, | ||
| containerApp: containerAppModel | ||
| }; | ||
| } catch { | ||
| ext.outputChannel.appendLog(localize('noResourceMatch', 'Used saved workspace settings to search for container app "{0}" in resource group "{1}" but found no match.', settings.containerAppName, settings.containerAppResourceGroupName)); | ||
| return noMatchingResources; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a blocker, but I think it's awkward to only return an object if one of the properties is defined. And otherwise we return undefined.
In the future, if you add more settings, then you need to add them to this if expression.
I think I'd prefer if you just always return an object, even if the object just contains undefined values.