Add Service Connector #381
Conversation
| import { IServiceConnectorContext } from "./IServiceConnectorContext"; | ||
|
|
||
| export class ContainerPickStep extends AzureWizardPromptStep<IActionContext> { | ||
| public async prompt(context: IServiceConnectorContext): Promise<void> { |
There was a problem hiding this comment.
Just a note, you don't have to change it, adding public here is optional. TS assumes things are public by default.
I saw Phil H. doing this and I like it better because it shortens line widths and makes things a bit cleaner.
There was a problem hiding this comment.
I prefer keeping public to keep things explicit. Time for a cage match!
| import { ContainerAppModel } from "../../tree/ContainerAppItem"; | ||
|
|
||
| export interface IServiceConnectorContext extends ICreateLinkerContext { | ||
| containerApp: ContainerAppModel; |
There was a problem hiding this comment.
I think we could extend from IContainerAppContext (on main branch) which includes the containerApp and subscription.
There was a problem hiding this comment.
I extended from the ICreateLinkerContext since I needed the scope.
There was a problem hiding this comment.
Yeah, IStreamLogsContext and IAddScaleRuleWizardContext could as well though IContainerAppContext has containerApp as an optional property. Not sure if that gunks things up a bit.
There was a problem hiding this comment.
I extended from the ICreateLinkerContext since I needed the scope.
Is there a reason we shouldn't extend from both?
Yeah,
IStreamLogsContextandIAddScaleRuleWizardContextcould as well thoughIContainerAppContexthascontainerAppas an optional property. Not sure if that gunks things up a bit.
Yeah, there are a few contexts that probably still need to be migrated to extend from IContainerAppContext since it was only recently added. Scale rules is definitely one of those.
| import { IServiceConnectorContext } from "./IServiceConnectorContext"; | ||
|
|
||
| export class ContainerPickStep extends AzureWizardPromptStep<IActionContext> { | ||
| public async prompt(context: IServiceConnectorContext): Promise<void> { |
There was a problem hiding this comment.
I prefer keeping public to keep things explicit. Time for a cage match!
| import { localize } from "../../utils/localize"; | ||
| import { IServiceConnectorContext } from "./IServiceConnectorContext"; | ||
|
|
||
| export class ContainerPickStep extends AzureWizardPromptStep<IServiceConnectorContext> { |
There was a problem hiding this comment.
Generally this is called a ListStep not a PickStep.
Though I get that there is already a ContainerListStep, I think we should rename things to be more explicit. The ContainerListStep today is technically the ReplicaContainerListStep and has a hard requirement of having a replica defined on the context (which happens in the previous step). So maybe just rename that one and then call this ContainerListStep or TemplateContainerListStep?
There was a problem hiding this comment.
I will rename the other one!
| } | ||
| } | ||
|
|
||
| private async getPicks(context: IServiceConnectorContext): Promise<IAzureQuickPickItem<string>[]> { |
There was a problem hiding this comment.
This doesn't need to be async.
| import { ContainerAppModel } from "../../tree/ContainerAppItem"; | ||
|
|
||
| export interface IServiceConnectorContext extends ICreateLinkerContext { | ||
| containerApp: ContainerAppModel; |
There was a problem hiding this comment.
Yeah, IStreamLogsContext and IAddScaleRuleWizardContext could as well though IContainerAppContext has containerApp as an optional property. Not sure if that gunks things up a bit.
src/utils/azureClients.ts
Outdated
| // @azure/container-registry doesn't support ADAL tokens at all. If it sees `signRequest` is defined | ||
| // it errors, but we don't actually need `signRequest` because this is a T2 package |
There was a problem hiding this comment.
You can remove this comment now 😃
| import { ICreateLinkerContext } from "@microsoft/vscode-azext-serviceconnector"; | ||
| import { IContainerAppContext } from "../createContainerApp/IContainerAppContext"; | ||
|
|
||
| export interface IServiceConnectorContext extends ICreateLinkerContext, IContainerAppContext { |
There was a problem hiding this comment.
Nit: A possible cleaner looking alternative (if you don't end up adding any properties):
export type IServiceConnectorContext = ICreateLinkerContext & IContainerAppContext;
On a side note, I'm more and more agreeing with Alex on not liking the I prefix for context interfaces/types (that's not a change request, more of just a general musing 🙂).
Related to microsoft/vscode-azuretools#1419
A couple of notes:
Currently just for testing the create, delete, and validate service connector commands are accessible from right clicking on a container app.
These commands will be moved to the service connector node once that is added in tools. In order to view if the service connector was created/deleted correctly you will have to look at the portal.