-
Notifications
You must be signed in to change notification settings - Fork 17
Add confirmation webview to "Deploy to Container App..." command with copilot support #897
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 6 commits
b1feee0
50c71c8
1e96abf
0ae2b50
67b4de6
eb30214
48a4956
8465297
f3bb214
ffaf4c2
27dbe78
870bbed
7943127
c8c7da3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -805,27 +805,34 @@ | |
| "lint-fix": "eslint --ext .ts . --fix", | ||
| "pretest": "npm run webpack-prod", | ||
| "test": "node ./dist/test/runTest.js", | ||
| "watch:views": "webpack serve --mode development --config ./webpack.config.views.js", | ||
| "webpack": "npm run build && gulp webpack-dev", | ||
| "webpack-prod": "npm run build && gulp webpack-prod", | ||
| "webpack-prod": "npm run build && gulp webpack-prod && npm run webpack-prod-wv ", | ||
| "webpack-prod-wv": "webpack --mode production --config ./webpack.config.views.js", | ||
| "webpack-profile": "webpack --profile --json --mode production > webpack-stats.json && echo Use http://webpack.github.io/analyse to analyze the stats", | ||
| "prepare": "husky install" | ||
| }, | ||
| "devDependencies": { | ||
| "@azure/ms-rest-azure-env": "^2.0.0", | ||
| "@microsoft/eslint-config-azuretools": "^0.2.2", | ||
| "@microsoft/vscode-azext-dev": "^2.1.0", | ||
| "@swc/core": "^1.7.36", | ||
| "@types/deep-eql": "^4.0.0", | ||
| "@types/fs-extra": "^8.1.1", | ||
| "@types/gulp": "^4.0.6", | ||
| "@types/mocha": "^8.2.2", | ||
| "@types/node": "^16.18.36", | ||
| "@types/semver": "^7.3.8", | ||
| "@types/tar": "^6.1.11", | ||
| "@types/uuid": "^10.0.0", | ||
| "@types/vscode": "^1.95.0", | ||
| "@types/vscode-webview": "^1.57.5", | ||
| "@typescript-eslint/eslint-plugin": "^5.59.11", | ||
| "@vscode/test-electron": "^2.3.8", | ||
| "@vscode/vsce": "^2.19.0", | ||
| "assert": "^2.0.0", | ||
| "copy-webpack-plugin": "^12.0.2", | ||
| "css-loader": "^7.1.2", | ||
| "eslint": "^8.42.0", | ||
| "eslint-plugin-import": "^2.27.5", | ||
| "glob": "^7.1.6", | ||
|
|
@@ -834,10 +841,13 @@ | |
| "mocha": "^10.1.0", | ||
| "mocha-junit-reporter": "^2.0.0", | ||
| "mocha-multi-reporters": "^1.1.7", | ||
| "style-loader": "^4.0.0", | ||
| "swc-loader": "^0.2.6", | ||
| "ts-node": "^10.9.1", | ||
| "typescript": "^5.1.3", | ||
| "webpack": "^5.94.0", | ||
| "webpack-cli": "^4.6.0" | ||
| "webpack-cli": "^5.1.4", | ||
| "webpack-dev-server": "^5.1.0" | ||
| }, | ||
| "dependencies": { | ||
| "@azure/arm-appcontainers": "^2.1.0-beta.1", | ||
|
|
@@ -848,18 +858,27 @@ | |
| "@azure/container-registry": "1.0.0-beta.5", | ||
| "@azure/core-rest-pipeline": "1.10.3", | ||
| "@azure/storage-blob": "^12.4.1", | ||
| "@fluentui/react-components": "^9.56.2", | ||
| "@fluentui/react-icons": "^2.0.265", | ||
| "@microsoft/vscode-azext-azureutils": "^3.3.3", | ||
| "@microsoft/vscode-azext-github": "^1.0.2", | ||
| "@microsoft/vscode-azext-utils": "^3.1.1", | ||
| "@microsoft/vscode-azext-utils": "^3.1.4", | ||
| "@microsoft/vscode-azureresources-api": "^2.0.2", | ||
| "@swc/core-linux-x64-gnu": "^1.12.9", | ||
|
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. Probably need to move this into the azure-pipelines since we can't run npm install with this |
||
| "buffer": "^6.0.3", | ||
| "dayjs": "^1.11.3", | ||
| "deep-eql": "^4.1.3", | ||
| "dotenv": "^16.0.0", | ||
| "fs-extra": "^8.1.0", | ||
| "monaco-editor": "~0.51.0", | ||
| "p-retry": "^4.6.2", | ||
| "react": "^18.3.1", | ||
| "react-dom": "^18.3.1", | ||
| "sass": "~1.79.6", | ||
| "sass-loader": "^16.0.2", | ||
| "semver": "^7.5.2", | ||
| "tar": "^6.2.1", | ||
| "uuid": "^10.0.0", | ||
| "vscode-nls": "^4.1.1", | ||
| "vscode-uri": "^3.0.2" | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import { getManagedEnvironmentFromContainerApp } from "../../utils/getResourceUt | |
| import { getVerifyProvidersStep } from "../../utils/getVerifyProvidersStep"; | ||
| import { localize } from "../../utils/localize"; | ||
| import { pickContainerApp } from "../../utils/pickItem/pickContainerApp"; | ||
| import { OpenConfirmationViewStep } from "../../webviews/OpenConfirmationViewStep"; | ||
| import { ContainerAppOverwriteConfirmStep } from "../ContainerAppOverwriteConfirmStep"; | ||
| import { deployWorkspaceProject } from "../deployWorkspaceProject/deployWorkspaceProject"; | ||
| import { editContainerCommandName } from "../editContainer/editContainer"; | ||
|
|
@@ -56,18 +57,22 @@ export async function deployContainerApp(context: IActionContext, node?: Contain | |
| } | ||
| wizardContext.telemetry.properties.revisionMode = item.containerApp.revisionsMode; | ||
|
|
||
| const confirmationViewTitle: string = localize('confirmAndDeploy', 'Confirm + Deploy') | ||
|
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. "Deployment Summary" or "Deployment Overview" maybe?
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. "Deployment Summary" is alright but I don't like "Deployment Overview". Any opinions @alexweininger?
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. Either of those is good. I like "Deployment Summary"
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. After trying it out Nathan and I thought Summary looked better. Any thoughts @alexweininger? |
||
| const confirmationViewDescription: string = localize('viewDescription', 'Please select an input you would like to change. Otherwise click "Confirm" to deploy.'); | ||
| const title: string = localize('deployContainerAppTitle', 'Deploy image to container app') | ||
|
|
||
| const wizard: AzureWizard<ContainerAppDeployContext> = new AzureWizard(wizardContext, { | ||
| title: localize('deployContainerAppTitle', 'Deploy image to container app'), | ||
| title: title, | ||
|
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.
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. Hmm, could we have the text here (and maybe the header) based on the command? Ex: "Deploy image to container app - Summary"
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. Yeah I can make them different. Any ideas what we want there @nturinski . Or does alex's suggestion "summary" in the tab and the header being longer make sense?
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. Yeah I think Alex's suggestion makes sense for the tab title, but I think it'll look very cluttered in the header. I'd mess around with it, but I think leaving that as |
||
| promptSteps: [ | ||
| new ContainerAppDeployStartingResourcesLogStep(), | ||
| new ImageSourceListStep(), | ||
| new ContainerAppOverwriteConfirmStep(), | ||
| new OpenConfirmationViewStep(confirmationViewTitle, confirmationViewDescription, title, () => wizard.confirmationViewProperties) | ||
| ], | ||
| executeSteps: [ | ||
| getVerifyProvidersStep<ContainerAppDeployContext>(), | ||
| new ContainerAppUpdateStep(), | ||
| ], | ||
| showLoadingPrompt: true | ||
| }); | ||
|
|
||
| await wizard.prompt(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import { type EnvironmentVar } from "@azure/arm-appcontainers"; | ||
| import { ActivityChildItem, ActivityChildType, AzExtFsExtra, AzureWizardPromptStep, activityInfoContext, activityInfoIcon, activitySuccessContext, activitySuccessIcon, createContextValue } from "@microsoft/vscode-azext-utils"; | ||
| import { ActivityChildItem, ActivityChildType, AzExtFsExtra, AzureWizardPromptStep, activityInfoContext, activityInfoIcon, activitySuccessContext, activitySuccessIcon, createContextValue, type ConfirmationViewProperty } from "@microsoft/vscode-azext-utils"; | ||
| import { parse, type DotenvParseOutput } from "dotenv"; | ||
| import { RelativePattern, workspace, type Uri, type WorkspaceFolder } from "vscode"; | ||
| import { ImageSource, envFileGlobPattern } from "../../../constants"; | ||
|
|
@@ -73,6 +73,14 @@ export class EnvFileListStep<T extends EnvFileListContext> extends AzureWizardPr | |
| return context.imageSource !== ImageSource.QuickstartImage && context.environmentVariables === undefined; | ||
| } | ||
|
|
||
| public confirmationViewProperty(context: T): ConfirmationViewProperty { | ||
| return { | ||
| name: localize('environmentVariables', 'Environment Variables'), | ||
| value: context.envPath ?? localize('useExisting', 'Use existing configuration'), | ||
| contextPropertyName: 'envPath' | ||
|
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. This can be a future thing, but I was thinking that we should make this type a key of the context. That way the only accepted values would be properties that actually exist on the context and prevent any typos, would provide auto-complete, etc. Similar to how we do that with |
||
| }; | ||
| } | ||
|
|
||
| private async promptForEnvPath(context: T, showHasExistingData?: boolean): Promise<string | undefined> { | ||
| const placeHolder: string = localize('setEnvVar', 'Select a {0} file to set the environment variables for the container instance', '.env'); | ||
| const skipLabel: string | undefined = showHasExistingData ? localize('useExisting', 'Use existing configuration') : undefined; | ||
|
|
||


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.
Just out of curiosity, do you know how much bigger the vsix is after adding all of these dependencies?
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.
Talked offline but the size doesn't change too much with the dependencies.