Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

serviceconnector: Add CosmosDB and Key Vault support #1541

Merged
merged 4 commits into from
Aug 3, 2023

Conversation

motm32
Copy link
Contributor

@motm32 motm32 commented Jul 25, 2023

Adds support for creating CosmosCD and Key Vault service connectors.

@motm32 motm32 requested a review from a team as a code owner July 25, 2023 23:53
case TargetServiceTypeName.KeyVault:
return nonNullValue(context.keyVaultAccount?.id);
default:
throw new Error('No target type found');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this error message be localized?


export class CosmosDBAccountListStep extends AzureWizardPromptStep<ICreateLinkerContext>{
public async prompt(context: ICreateLinkerContext): Promise<void> {
const placeHolder: string = vscode.l10n.t('Select a Database Account');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Unless it's prefixed with "Azure", we shouldn't capitalize resource types.


export class KeyVaultListStep extends AzureWizardPromptStep<ICreateLinkerContext> {
public async prompt(context: ICreateLinkerContext): Promise<void> {
const placeHolder: string = vscode.l10n.t('Select a Key Vault');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Same here.

}

async function getKeyVaultAccounts(context: ICreateLinkerContext): Promise<KeyVaultJsonResponse> {
const url = `https://management.azure.com/subscriptions/${context.subscriptionId}/resources?$filter=resourceType eq 'Microsoft.KeyVault/vaults'&api-version=2015-11-01`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to use management.azure.com for these requests because then we don't support anything but the public cloud. I think if you check the context.subscription or maybe context.credentials you should see there's something called a resourceManagementEndpoint or something like that,

return !context.keyVaultAccount;
}

private async getPicks(accounts: KeyVaultAccountJsonResponse[]): Promise<IAzureQuickPickItem<KeyVaultAccountJsonResponse>[]> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be async since you're calling getKeyVaultAccounts in the prompt.

@@ -29,4 +31,17 @@ export class LinkerCreateStep extends AzureWizardExecuteStep<ICreateLinkerContex
public shouldExecute(context: ICreateLinkerContext): boolean {
return !context.linker;
}

public getSourceResourceId(context: ICreateLinkerContext): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't really matter, but this can be private.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for rhyming ✅

nturinski
nturinski previously approved these changes Aug 2, 2023
Copy link
Member

@nturinski nturinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All nits. Don't really care if you fix them or not :D

@motm32 motm32 merged commit a0a2ee4 into main Aug 3, 2023
4 checks passed
@motm32 motm32 deleted the meganmott/databaseConnector branch August 3, 2023 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants