Add ability to choose an existing managed environment for deployWorkspaceProject
#481
Conversation
MicroFish91
force-pushed
the
mwf/click2run-man-env
branch
from
5cf4184 to
5b259b4
Compare
There was a problem hiding this comment.
I don't remember exactly how this implementation works, but I'm guessing that getDefaultContainerAppsResources is called somewhere and then we do some prompts or something based off of that?
Could you show me how it's used? There's been so many PRs, I'm not even sure if that part has been merged in yet.
But the way I envision it working is we call getDefaultContainerAppsResources and it will set the context values appropriately. After that, we just call the wizards and the ListSteps will handle the logic of whether or not we should skip the steps.
| @@ -56,3 +67,38 @@ export async function getDefaultContainerAppsResources(context: ISubscriptionAct | |||
| return noMatchingResources; | |||
| } | |||
| } | |||
|
|
|||
| async function promptForContainerAppsEnvironmentResources(context: ISubscriptionActionContext): Promise<DefaultContainerAppsResources> { | |||
There was a problem hiding this comment.
Why not call it promptForManagedEnvironmentResources?
| const managedEnvironments: ManagedEnvironment[] = await uiUtils.listAllIterator(client.managedEnvironments.listBySubscription()); | ||
|
|
||
| if (!managedEnvironments.length) { | ||
| return noMatchingResources; |
There was a problem hiding this comment.
If there are no managed environments, shouldn't we still prompt with only $(plus) Create new container apps environment as the option?
There was a problem hiding this comment.
I think we should be able to skip it without prompting. An undefined resource returned here means that we were unable to find an existing resource to use/default to.
An undefined default resource therefore indicates to the command that we need to create the missing resource later, thus in this scenario, returning undefined is equivalent to the user selecting the create button. Since there's only one option to show in the above scenario, we can just pick it for them by returning undefined and skip displaying the prompt altogether.
There was a problem hiding this comment.
I think I get how this is supposed to work now. I honestly really don't like this design pattern-- I find it's super confusing and not very congruent with how we would normally do it with wizard steps.
That being said, I understand that we've been kicking this can on the release for quite a while, so let's just get this in. We can always refactor into wizard steps later.
There was a problem hiding this comment.
Yeah, given the time restraints and the potential for side effects, I think it's just easier for now to go with the implementation like this. After release we can add an engineering task to refactor this into a list step and fit it into the normal wizard flow.
| const placeHolder: string = localize('selectManagedEnvironment', 'Select a container apps environment'); | ||
| const managedEnvironment: ManagedEnvironment | undefined = (await context.ui.showQuickPick(picks, { placeHolder })).data; | ||
|
|
||
| if (!managedEnvironment) { |
There was a problem hiding this comment.
Wouldn't managedEnvironment be undefined here if the user selected create new container app environment? I'm thoroughly confused on what is going on here.
Like you said in your PR, I feel like this logic should be in a ManagedEnvironmentListStep rather than having it be a part of the getDefaultContainerAppsResources.
My opinion is that this command should only try to get the default values. If it can't, it exits out and we rely on the wizard prompts to get input from the user.
There was a problem hiding this comment.
Wouldn't managedEnvironment be undefined here if the user selected create new container app environment?
Yes, exactly this, if the user selects create, by setting as undefined, we are indicating that no default resource exists and that one needs to be created later.
Like you said in your PR, I feel like this logic should be in a ManagedEnvironmentListStep rather than having it be a part of the getDefaultContainerAppsResources.
I thought about making it all as a list step, but there's so much other stuff I would then have to build out and test/think about when all I really need is the getPicks portion of the list step essentially... so I decided to defer that for now.
Yeah, come to think of it, I think some of the related sections were merged while you were out, so you may not have the full context with what you already reviewed. Let me message you offline to discuss more. |
Some of this logic can probably be eventually re-purposed into a
ManagedEnvironmentListStep