Add confirmation webview to "Deploy to Container App..." command with copilot support#897
Add confirmation webview to "Deploy to Container App..." command with copilot support#897
Conversation
* initial commit * small changes * requested changes to commonChannel * small changes and revert previous commit * remove line * prod changes * Rename confirmationViewController.ts to ConfirmationViewController.ts
* initial commit * small changes * add deploy changes * requested changes to commonChannel * cancel bug fix * small changes and revert previous commit * remove line * prod changes * Rename confirmationViewController.ts to ConfirmationViewController.ts * Implement utils changes (#890) * change copilot button location * small changes after merge * pass in view description * Add functionality to copilot button (#896) * initial * small changes * requested changes * small change --------- Co-authored-by: Alex Weininger <alex.weininger@live.com>
| "@microsoft/vscode-azext-github": "^1.0.2", | ||
| "@microsoft/vscode-azext-utils": "^3.1.4", | ||
| "@microsoft/vscode-azureresources-api": "^2.0.2", | ||
| "@swc/core-linux-x64-gnu": "^1.12.9", |
There was a problem hiding this comment.
Probably need to move this into the azure-pipelines since we can't run npm install with this
| } | ||
| wizardContext.telemetry.properties.revisionMode = item.containerApp.revisionsMode; | ||
|
|
||
| const confirmationViewTitle: string = localize('confirmAndDeploy', 'Confirm + Deploy') |
There was a problem hiding this comment.
"Deployment Summary" or "Deployment Overview" maybe?
There was a problem hiding this comment.
"Deployment Summary" is alright but I don't like "Deployment Overview". Any opinions @alexweininger?
There was a problem hiding this comment.
Either of those is good. I like "Deployment Summary"
There was a problem hiding this comment.
After trying it out Nathan and I thought Summary looked better. Any thoughts @alexweininger?
| Object.defineProperty(exports, "__esModule", { value: true }); | ||
|
|
||
| const extension = require('./out/src/extension'); | ||
| const extension = require('./dist/extension.bundle'); |
There was a problem hiding this comment.
You're gonna wanna change this back.
| @@ -0,0 +1,24 @@ | |||
| .confirmationView { | |||
| } | ||
|
|
||
| async function installSwcCore(): Promise<void> { | ||
| if (process.platform === 'linux') { |
There was a problem hiding this comment.
I think a comment explaining why we need to have this would be helpful. I have a feeling we'll only have a vague recollection in the future.
| "@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", |
There was a problem hiding this comment.
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.
Talked offline but the size doesn't change too much with the dependencies.
|
|
||
| const wizard: AzureWizard<ContainerAppDeployContext> = new AzureWizard(wizardContext, { | ||
| title: localize('deployContainerAppTitle', 'Deploy image to container app'), | ||
| title: title, |
There was a problem hiding this comment.
nit: Is it possible to have the title and the editor tab title to be different?
While I prefer summary for the header title, I think it just saying "Summary" in the VS Code tab doesn't look as good.
There was a problem hiding this comment.
Hmm, could we have the text here (and maybe the header) based on the command? Ex: "Deploy image to container app - Summary"
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 Summary would be my preference.
| return { | ||
| name: localize('environmentVariables', 'Environment Variables'), | ||
| value: context.envPath ?? localize('useExisting', 'Use existing configuration'), | ||
| contextPropertyName: 'envPath' |
There was a problem hiding this comment.
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 nonNullProp
3 participants
Most of this code has already been reviewed in previous PR's see: #896, #887, #886
Here is how the view looks:
Here is how the copilot button looks like:
Todo: