Add changes for re running "Deploy to Container App" command #999
Add changes for re running "Deploy to Container App" command #999
Conversation
src/commands/createManagedEnvironment/ManagedEnvironmentListStep.ts
Outdated
Show resolved
Hide resolved
| import { type ManagedEnvironmentContext } from "../../ManagedEnvironmentContext"; | ||
|
|
||
| export interface ManagedIdentityRegistryCredentialsContext extends CreateAcrContext, ManagedEnvironmentRequiredContext, IContainerAppContext { | ||
| export interface ManagedIdentityRegistryCredentialsContext extends CreateAcrContext, ManagedEnvironmentContext, IContainerAppContext { |
There was a problem hiding this comment.
Context loosened from ManagedEnvironmentRequiredContext → ManagedEnvironmentContext.
This is fine only if all consumers tolerate managedEnvironment being undefined. Worth a quick scan of downstream usage.
| wizardContext.telemetry.properties.revisionMode = item.containerApp.revisionsMode; | ||
|
|
||
| const confirmationViewTitle: string = localize('summary', 'Summary'); | ||
| let confirmationViewDescription: string = localize('viewDescription', 'Please select an input you would like to change. Note: Any input proceeding the changed input will need to change as well'); |
There was a problem hiding this comment.
| let confirmationViewDescription: string = localize('viewDescription', 'Please select an input you would like to change. Note: Any input proceeding the changed input will need to change as well'); | |
| let confirmationViewDescription: string = localize('viewDescription', 'Please select an input you would like to change. Note: Any input preceding the changed input will need to change as well'); |
There was a problem hiding this comment.
Wait proceeding is correct here right? It's what comes after that might need to change?
| if (wizardContext.ui instanceof CopilotUserInput) { | ||
| promptSteps.push(new OpenLoadingViewStep()); | ||
| if (isCopilotUserInput(wizardContext)) { | ||
| confirmationViewDescription = localize('viewDescription', 'Please review AI generated inputs and select any you would like to modify. Note: Any input proceeding the modified input will need to change as well'); |
There was a problem hiding this comment.
| confirmationViewDescription = localize('viewDescription', 'Please review AI generated inputs and select any you would like to modify. Note: Any input proceeding the modified input will need to change as well'); | |
| confirmationViewDescription = localize('viewDescription', 'Please review AI generated inputs and select any you would like to modify. Note: Any input preceding the modified input will need to change as well'); |
|
|
||
| let wizardContext: ContainerAppDeployContext = {} as ContainerAppDeployContext; | ||
|
|
||
| if (node && imageSource) { |
There was a problem hiding this comment.
Can I get some context about this if else if here?
If you have a node and an image source, what does that imply? And if you have a subscription?
There was a problem hiding this comment.
node and image source implies you are coming from the regular flow. Just having a subscription and not those other two implies you are coming from a copilot version since we don't pass in a node in that case. I can add comments or something if that is super confusing.
There was a problem hiding this comment.
Personally, I think it'd be more readable if you had a flag that says something like: runFromCopilot that's set to true if the subscription is defined or something like that. It just makes it clearer as to why we're checking for it. Maybe add it as a flag up at L68.
There was a problem hiding this comment.
+1 I'd love to see Copilot specific logic consolidated as much as possible to improve maintainability of the normal command flow vs. copilot command flow.
|
|
||
| let wizardContext: ContainerAppDeployContext = {} as ContainerAppDeployContext; | ||
|
|
||
| if (node && imageSource) { |
There was a problem hiding this comment.
+1 I'd love to see Copilot specific logic consolidated as much as possible to improve maintainability of the normal command flow vs. copilot command flow.
|
|
||
| const subscription = (context as { azureSubscription?: AzureSubscription }).azureSubscription; | ||
|
|
||
| if (!subscription && !node) { |
There was a problem hiding this comment.
If I'm understanding this correctly, could this check be consolidated to a section dedicated to Copilot flow specific changes? A brief comment noting that Copilot may sometimes fail to provide a subscription would help make the intent clearer.
| await openLoadingViewPanel(context); | ||
| } else { | ||
| // Prompt for image source before initializing the wizard in case we need to redirect the call to 'deployWorkspaceProject' instead | ||
| imageSource = await promptImageSource(context); |
There was a problem hiding this comment.
What was the reason for moving the image source prompt into internal instead of leaving it in the parent function deployContainerApp?
This PR adds a couple things:
Some things to note
Things to do: