Activity children and output log support for deployWorkspaceProject#464
Activity children and output log support for deployWorkspaceProject#464MicroFish91 merged 56 commits intomainfrom
deployWorkspaceProject#464Conversation
…de-azurecontainerapps into mwf/deployWorkspaceProject-V-I
…inerapps into mwf/deployWorkspaceProject-V-I
…inerapps into mwf/deployWorkspaceProject-VI
…icrosoft/vscode-azurecontainerapps into mwf/deployWorkspaceProject-VI
…icrosoft/vscode-azurecontainerapps into mwf/deployWorkspaceProject-VIII
MicroFish91
changed the title
deployWorkspaceProject
…crosoft/vscode-azurecontainerapps into mwf/deployWorkspaceProject-VII
…icrosoft/vscode-azurecontainerapps into mwf/deployWorkspaceProject-VIII
MicroFish91
force-pushed
the
mwf/deployWorkspaceProject-VIII
branch
from
c8f4bb8 to
e75a78a
Compare
| context.activityChildren?.push( | ||
| new GenericTreeItem(undefined, { | ||
| contextValue: createActivityChildContext(['environmentVariablesListStep', setEnvironmentVariableOption, activitySuccessContext]), | ||
| label: localize('saveEnvVarsLabel', 'Save environment variable configuration for the container app'), |
There was a problem hiding this comment.
Just wondering, but what do you think about wording it as "Save container app's environment variable configuration"? Ending it with for the container app sounds clunky to me.
Maybe just remove "for the container app" entirely as well since it's kind of implied (being under it as a child after all)
| } | ||
|
|
||
| // Need to place this outside of 'initSuccessOutput' so we can use the image after it has had a chance to become defined | ||
| (this.success.output as string[])?.push(localize('useImage', 'Using image "{0}".', context.image)); |
There was a problem hiding this comment.
I'm not entirely sure why this is part of the successOutput. Wouldn't you want this to be displayed even if it fails? I'm also not sure what's the difference between context.image and context.imageName?
There was a problem hiding this comment.
context.image is a more full version of the image name that gets assigned after it finishes building. It would include the full <acrName>.azurecr.io/<imageName>:<tag>...
We wouldn't want to show that as part of the fail output because if this fails, the whole command fails and we wouldn't be able to proceed any further to use the image (I assume because the remote image would not exist)
|
|
||
| protected initFailOutput(context: IBuildImageInAzureContext): ExecuteActivityOutput { | ||
| return { | ||
| item: new GenericTreeItem(undefined, { |
There was a problem hiding this comment.
The failure won't be output by the build image step also?
There was a problem hiding this comment.
This one is kind of confusing and I wasn't sure what kind of comment to leave for this. My understanding is basically the building starts in this RunStep... but the actual meaningful output gets detected in the following step BuildImageStep. Thus, what we want to actually display on success doesn't exist yet, so I defer showing a success output until we can confirm it in the following step.
For the fail, we can still trigger a fail-point within this step still, so we should still define the potential fail output here.
| return { | ||
| item: new GenericTreeItem(undefined, { | ||
| contextValue: createActivityChildContext(['registryCreateStep', activitySuccessContext]), | ||
| label: localize('createRegistryLabel', 'Create Azure Container Registry "{0}"', context.newRegistryName), |
There was a problem hiding this comment.
I'd just say "Create registry". Bit weird that you have all of the other resources be generic, and this one is branded.
| label: saveSettingsLabel, | ||
| iconPath: activitySuccessIcon | ||
| }), | ||
| output: localize('savedSettingsSuccess', 'Saved deployment settings to workspace: "{0}".', relativeSettingsFilePath) |
There was a problem hiding this comment.
Weird that this one has a colon, but the other resources output messages do not.
| new GenericTreeItem(undefined, { | ||
| contextValue: createActivityChildContext(['useExistingResourceGroup', activitySuccessContext]), | ||
| label: localize('useResourceGroup', 'Use resource group "{0}"', resourceGroupName), | ||
| iconPath: activitySuccessIcon |
There was a problem hiding this comment.
This is bit of an aside, but should we have another icon for children that are just relaying information? It's kind of weird to me that we'd have green checkmarks for all of these.
Not something we need to decide on now, just curious.
There was a problem hiding this comment.
Yeah, I agree, it might look cool to experiment with the icons for these
There was a problem hiding this comment.
Just playing around with the icon... and these are out of order, but thoughts? We can add another time though if we find something cool.
There was a problem hiding this comment.
Yeah, I do prefer this. It's still a bit busy in my opinion, but better.
Maybe even just a dash or a bullet point would work?
| new GenericTreeItem(undefined, { | ||
| contextValue: createActivityChildContext(['ingressPromptStep', activitySuccessContext]), | ||
| label: context.enableIngress ? | ||
| localize('ingressEnableLabel', 'Enable ingress on port {0} (found Dockerfile configuration)', context.targetPort) : |
There was a problem hiding this comment.
I'd prefer we said (from Dockerfile configuration) rather than found.
nturinski
left a comment
nturinski
left a comment
There was a problem hiding this comment.
Approving but with expectation some refactors happen in another PR
MicroFish91
force-pushed
the
mwf/deployWorkspaceProject-VIII
branch
from
0e23da5 to
07cc7a1
Compare
2 participants
Closes #425
Closes #457
Additions:
deployWorkspaceProjectflowdemo