-
Notifications
You must be signed in to change notification settings - Fork 142
Miscellaneous improvements related to durable task hub #4641
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
base: main
Are you sure you want to change the base?
Changes from all commits
b55c245
df78db4
36ef45c
532564e
bbfc738
0dbbf41
b3e9c40
b605d92
001ac6b
9dffe69
a8b3df3
7e5c8a2
845a829
d33d06c
825ff7f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,11 +3,13 @@ | |
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
|
||
import { parseAzureResourceId } from '@microsoft/vscode-azext-azureutils'; | ||
import { AzureWizardPromptStep, nonNullProp, type IAzureQuickPickItem, type IWizardOptions } from '@microsoft/vscode-azext-utils'; | ||
import { CommonRoleDefinitions, createAuthorizationManagementClient, createRoleId, parseAzureResourceId, RoleAssignmentExecuteStep, uiUtils, type Role } from '@microsoft/vscode-azext-azureutils'; | ||
import { ActivityChildItem, ActivityChildType, activitySuccessContext, activitySuccessIcon, AzureWizardPromptStep, createContextValue, nonNullProp, type AzureWizardExecuteStep, type IAzureQuickPickItem, type IWizardOptions } from '@microsoft/vscode-azext-utils'; | ||
import { localSettingsDescription } from '../../../../../constants-nls'; | ||
import { ext } from '../../../../../extensionVariables'; | ||
import { localize } from '../../../../../localize'; | ||
import { HttpDurableTaskSchedulerClient, type DurableTaskHubResource, type DurableTaskSchedulerClient } from '../../../../../tree/durableTaskScheduler/DurableTaskSchedulerClient'; | ||
import { FunctionAppUserAssignedIdentitiesListStep } from '../../../../identity/FunctionAppUserAssignedIdentitiesListStep'; | ||
import { type IDTSAzureConnectionWizardContext } from '../IDTSConnectionWizardContext'; | ||
import { DurableTaskHubCreateStep } from './DurableTaskHubCreateStep'; | ||
import { DurableTaskHubNameStep } from './DurableTaskHubNameStep'; | ||
|
@@ -22,7 +24,7 @@ export class DurableTaskHubListStep<T extends IDTSAzureConnectionWizardContext> | |
|
||
public async prompt(context: T): Promise<void> { | ||
context.dtsHub = (await context.ui.showQuickPick(await this.getPicks(context), { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. UX nit: placeholder casing. The placeholder now reads "Select a durable task hub" (line 26). Other pickers often capitalize proper nouns. Consider "Select a Durable Task Hub" for consistency. Review generated with Copilot There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I often make these lower case if not prefixed with Azure. If enough people want me to make this upper case, I can do that |
||
placeHolder: localize('selectTaskScheduler', 'Select a Durable Task Scheduler'), | ||
placeHolder: localize('selectTaskScheduler', 'Select a durable task hub'), | ||
})).data; | ||
|
||
if (context.dtsHub) { | ||
|
@@ -35,17 +37,6 @@ export class DurableTaskHubListStep<T extends IDTSAzureConnectionWizardContext> | |
return !context.dtsHub; | ||
} | ||
|
||
public async getSubWizard(context: T): Promise<IWizardOptions<T> | undefined> { | ||
if (context.dtsHub) { | ||
return undefined; | ||
} | ||
|
||
return { | ||
promptSteps: [new DurableTaskHubNameStep(this.schedulerClient)], | ||
executeSteps: [new DurableTaskHubCreateStep(this.schedulerClient)], | ||
}; | ||
} | ||
|
||
private async getPicks(context: T): Promise<IAzureQuickPickItem<DurableTaskHubResource | undefined>[]> { | ||
const taskHubs: DurableTaskHubResource[] = context.dts ? | ||
await this.schedulerClient.getSchedulerTaskHubs(nonNullProp(context, 'subscription'), parseAzureResourceId(context.dts.id).resourceGroup, context.dts.name) : []; | ||
|
@@ -66,4 +57,80 @@ export class DurableTaskHubListStep<T extends IDTSAzureConnectionWizardContext> | |
}), | ||
]; | ||
} | ||
|
||
public async getSubWizard(context: T): Promise<IWizardOptions<T> | undefined> { | ||
const promptSteps: AzureWizardPromptStep<T>[] = []; | ||
const executeSteps: AzureWizardExecuteStep<T>[] = []; | ||
|
||
if (!context.dtsHub) { | ||
promptSteps.push(new DurableTaskHubNameStep(this.schedulerClient)); | ||
executeSteps.push(new DurableTaskHubCreateStep(this.schedulerClient)); | ||
} | ||
|
||
const dtsContributorRole: Role = { | ||
scopeId: context.dtsHub?.id, | ||
roleDefinitionId: createRoleId(context.subscriptionId, CommonRoleDefinitions.durableTaskDataContributor), | ||
roleDefinitionName: CommonRoleDefinitions.durableTaskDataContributor.roleName, | ||
}; | ||
|
||
promptSteps.push(new FunctionAppUserAssignedIdentitiesListStep(dtsContributorRole /** targetRole */, { identityAssignStepPriority: 180 })); | ||
executeSteps.push(new RoleAssignmentExecuteStep(this.getDTSRoleAssignmentCallback(context, dtsContributorRole), { priority: 190 })); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Pre-check may skip when creating a new hub. Here Review generated with Copilot There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This scenario is fine and not a bug. If it's |
||
return { promptSteps, executeSteps }; | ||
} | ||
|
||
private getDTSRoleAssignmentCallback(context: T, role: Role): () => Promise<Role[]> { | ||
return async () => { | ||
const roleAssignment: Role = { | ||
...role, | ||
// This id may be missing when the role is initially passed in, | ||
// but by the time we run the step, we should have the populated id ready. | ||
scopeId: context.dtsHub?.id, | ||
}; | ||
|
||
if (!roleAssignment.scopeId) { | ||
return []; | ||
} | ||
|
||
const amClient = await createAuthorizationManagementClient(context); | ||
|
||
let hasRoleAssignment: boolean = false; | ||
if (context.dtsHub) { | ||
const taskHubRoleAssignments = await uiUtils.listAllIterator(amClient.roleAssignments.listForScope( | ||
context.dtsHub.id, | ||
{ | ||
// $filter=principalId eq {id} | ||
filter: `principalId eq '{${context.managedIdentity?.principalId}}'`, | ||
} | ||
)); | ||
hasRoleAssignment = taskHubRoleAssignments.some(r => !!r.roleDefinitionId?.endsWith(role.roleDefinitionId)); | ||
} | ||
|
||
if (!hasRoleAssignment && context.dts) { | ||
const taskSchedulerRoleAssignments = await uiUtils.listAllIterator(amClient.roleAssignments.listForScope( | ||
context.dts.id, | ||
{ | ||
// $filter=principalId eq {id} | ||
filter: `principalId eq '{${context.managedIdentity?.principalId}}'`, | ||
} | ||
)); | ||
hasRoleAssignment = taskSchedulerRoleAssignments.some(r => !!r.roleDefinitionId?.endsWith(role.roleDefinitionId)); | ||
} | ||
|
||
if (hasRoleAssignment) { | ||
context.activityChildren?.push( | ||
new ActivityChildItem({ | ||
label: localize('verifyIdentityWithRoleLabel', 'Verify identity "{0}" has role "{1}"', context.managedIdentity?.name, role.roleDefinitionName), | ||
description: '0s', | ||
contextValue: createContextValue(['roleAssignmentExecuteStepItem', activitySuccessContext]), | ||
activityType: ActivityChildType.Success, | ||
iconPath: activitySuccessIcon, | ||
}), | ||
); | ||
ext.outputChannel.appendLog(localize('verifyIdentity', 'Successfully verified identity "{0}" has role "{1}".', context.managedIdentity?.name, role.roleDefinitionName)); | ||
} | ||
|
||
return hasRoleAssignment ? [] : [roleAssignment]; | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,14 +4,19 @@ | |
*--------------------------------------------------------------------------------------------*/ | ||
|
||
import { AzureWizardExecuteStep } from '@microsoft/vscode-azext-utils'; | ||
import { getSchedulerConnectionString, SchedulerAuthenticationType } from '../../../../durableTaskScheduler/copySchedulerConnectionString'; | ||
import { clientIdKey, getSchedulerConnectionString, SchedulerAuthenticationType } from '../../../../durableTaskScheduler/copySchedulerConnectionString'; | ||
import { type IDTSAzureConnectionWizardContext } from '../IDTSConnectionWizardContext'; | ||
|
||
export class DurableTaskSchedulerGetConnectionStep<T extends IDTSAzureConnectionWizardContext> extends AzureWizardExecuteStep<T> { | ||
public priority: number = 200; | ||
|
||
public async execute(context: T): Promise<void> { | ||
context.newDTSConnectionSettingValue = getSchedulerConnectionString(context.dts?.properties.endpoint ?? '', SchedulerAuthenticationType.UserAssignedIdentity); | ||
|
||
if (context.managedIdentity) { | ||
context.newDTSConnectionSettingValue = context.newDTSConnectionSettingValue.replace(clientIdKey, context.managedIdentity?.clientId ?? clientIdKey); | ||
} | ||
|
||
context.valuesToMask.push(context.newDTSConnectionSettingValue); | ||
} | ||
|
||
Comment on lines
+16
to
22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ClientId placeholder may persist. If Review generated with Copilot There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not super worried because the if statement already verifies the managed identity exists. I could throw a nonNull error here if you'd prefer that. |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential duplicates after removing guard. Without
hasLogged
,configureBeforePrompt
can prepend items multiple times if the step re-runs (back/forward). DoesprependOrInsertAfterLastInfoChild
dedupe usingstepId
? If not, please re-add a guard or enforce deduplication.Review generated with Copilot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but prompt steps will automatically remove activity children by step id when you go back.