Add Local and Remote Settings conversion by motm32 · Pull Request #4387 · microsoft/vscode-azurefunctions

motm32 · 2025-02-28T18:19:27Z

For both local and remote there are multiple entry points:
Local
On the top level Local Project node in the workspace view and on each connection under the local settings node:

I have also added a warning node on the specific connections that need to be converted.

Remote
On the top level App Setting Item and on the specific connection:

After converting connections and assigning roles to the chosen managed identity the activity log will look like this:

Notes:

We are not deleting local or remote settings as this can break users apps as changes to the trigger code needs to be made in order for it to work. We currently are just adding the new settings and setting up the roles for the remote
We cannot add roles for Cosmos DB triggers as they have their own RBAC roles see here for more information
When locally debugging role assignments aren't necessary as your local machine cannot access the managed identity.

Todo:

Release appsettings and azure utils package and update the package json
Test each connection type
When right clicking on a remote app setting the command should only show if there are connections to be converted

MicroFish91 · 2025-03-04T01:23:19Z

+                        context.activityChildren?.push(
+                            new GenericTreeItem(undefined, {
+                                contextValue: createUniversallyUniqueContextValue(['useExistingResourceGroupInfoItem', activitySuccessContext]),
+                                label: localize('deletedSetting', `Deleted app setting "${connection.originalValue}"`),


Should all the activity child labels be present tense instead of past tense?

Left a similar comment on this PR as well with more context:
#4382 (comment)

nturinski

🥲

nturinski · 2025-03-10T22:40:27Z

    "azureFunctions.createNewProjectWithDockerfile": "Create New Containerized Project...",
    "azureFunctions.createPythonVenv": "Create a virtual environment when creating a new Python project.",
    "azureFunctions.createSlot": "Create Slot...",
+    "azureFunctions.convertLocalConnections": "Convert Local Project Connections to Identity-Based Connections...",


We may want to change the verbiage from "convert" to "add" considering we aren't deleting the old settings anymore.

nturinski · 2025-03-10T23:05:55Z

+import { type IConvertConnectionsContext } from "./IConvertConnectionsContext";
+import { type Connection } from "./SelectConnectionsStep";
+
+export class ConvertSettingsStep extends AzureWizardExecuteStep<IConvertConnectionsContext> {


nit: I prefer having the step names following a <noun><verb>Step convention. The reason being, if you have multiple steps that are for Settings, they would all be listed together in the folder, like SettingsConvertStep, SettingsListStep, etc.

nturinski · 2025-03-10T23:08:12Z

+    public priority: number = 100;
+
+    public async execute(context: IConvertConnectionsContext): Promise<void> {
+        if (context.connections) {


I'd prefer to take this out of the if statement and just do nonNullProp(context, connections).

The reason being that this should only execute if !!context.connections evaluates to true, and if there is some reason that this is running when it is not, I'd rather the step throw an internal error than just silently skipping it, making it a harder to uncover error.

nturinski · 2025-03-10T23:31:39Z

+                        ...getClientIdAndCredentialProperties(context, storageAccountName)
+                    );
+                    context.roles?.push({
+                        scopeId: await getScopeHelper(context, storageAccountName, `resourceType eq 'Microsoft.Storage/storageAccounts'`),


Is the filter supposed to be resourceType eq on all of these? Can you explain this a little bit?

Within the scope helper function I only want to look at resources that are the same resource type as the settings i am looking at. This is because resources can have the same names and that is the only information I have to find which resource the scope should be assigned to.

nturinski · 2025-03-10T23:33:02Z

+                        roleDefinitionId: createRoleId(context.subscriptionId, CommonRoleDefinitions.storageBlobDataOwner),
+                        roleDefinitionName: CommonRoleDefinitions.storageBlobDataOwner.roleName
+                    });
+                } else if (connection.name.includes('STORAGE')) {


I think it's safe to combine this and connection.name.includes('AzureWebJobsStorage') as one conditional?

if (connection.name.includes('AzureWebJobsStorage') || connection.name.includes('STORAGE') to save on a lot of space/copied logic.

Yeah I thought about doing this. Originally I had AzureWebJobsStorage set to the __accountName property and only recently found out we shouldn't be using that. I'll combine the two.

nturinski · 2025-03-10T23:47:21Z

+        return picks;
+    }
+
+    private async getRemoteQuickPics(context: IConvertConnectionsContext): Promise<IAzureQuickPickItem<Connection>[]> {


nit:

Suggested change

private async getRemoteQuickPics(context: IConvertConnectionsContext): Promise<IAzureQuickPickItem<Connection>[]> {

private async getRemoteQuickPicks(context: IConvertConnectionsContext): Promise<IAzureQuickPickItem<Connection>[]> {

nturinski · 2025-03-10T23:48:26Z

+    private async getRemoteQuickPics(context: IConvertConnectionsContext): Promise<IAzureQuickPickItem<Connection>[]> {
+        const picks: IAzureQuickPickItem<Connection>[] = [];
+
+        if (context.functionapp) {


Similar thing to what I had said in another step, but I'd prefer you use nonNullValue here because otherwise, it'll just return an empty array and we won't know if that's because there is no function app or if it's because the function app isn't listing app settings correctly.

nturinski · 2025-03-10T23:49:01Z

+export async function convertLocalConnections(context: IActionContext, node?: AppSettingTreeItem): Promise<void> {
+    const connections: Connection[] = [];
+    if (node instanceof AppSettingTreeItem) {
+        connections.push({ name: node.id, value: node.value })


Suggested change

connections.push({ name: node.id, value: node.value })

connections.push({ name: node.id, value: node.value });

nturinski · 2025-03-10T23:50:13Z

+
+    const continueOn: MessageItem = { title: localize('continueOn', 'Continue') };
+    const message: string = localize('setConnectionsProperty', 'Successfully converted local connections in order to use identity based connections you may need to set the connection property within your trigger.');
+    await context.ui.showWarningMessage(message, { learnMoreLink: "https://aka.ms/AAuroke", modal: true }, continueOn);


I believe that we should show this message before they start converting. We can show it again after in the activity log, but if they're worried about messing up their project, it's a little bit late by this point.

nturinski · 2025-03-10T23:51:43Z

+    }
+}
+
+export async function convertRemoteConnectionsInternal(context: IActionContext, connections?: Connection[], node?: AppSettingTreeItem): Promise<void> {


Are these internal functions identical besides setting local to false?

There are some slight differences beyond that but once I split up the ConvertSettingsStep into remote and local they will also use different execute steps.

nturinski · 2025-03-19T20:27:33Z

+            context.localSettingsPath = await getLocalSettingsFile(context, message);
+        }
+
+        const localSettings = await AzExtFsExtra.readJSON<ILocalSettingsJson>(nonNullProp(context, 'localSettingsPath'));


I feel like we should just make getting the settings from the local.settings.json a helper function.

nturinski · 2025-03-19T20:29:34Z

+import { type SlotTreeItem } from "../../tree/SlotTreeItem";
+import { type Connection } from "./ConnectionsListStep";
+
+export interface IAddMIConnectionsContext extends ExecuteActivityContext, IResourceGroupWizardContext, ISubscriptionActionContext {


nit: No "👁️" needed here.................................

nturinski · 2025-03-19T20:34:07Z

+    public priority: number = 110;
+
+    public async execute(context: IAddMIConnectionsContext): Promise<void> {
+        if (!context.functionapp) {


nit: Why not just use nonNullProp?

nturinski · 2025-03-19T21:18:38Z

+    const promptSteps: AzureWizardPromptStep<IAddMIConnectionsContext>[] = [];
+    const executeSteps: AzureWizardExecuteStep<IAddMIConnectionsContext>[] = [];
+
+    promptSteps.push(new ConnectionsListStep())


nit: You're not pushing steps in dynamically anymore so might as well just instantiate promptSteps and executeSteps with the steps that they need instead of creating empty arrays first,

nturinski · 2025-03-19T21:19:26Z

+        await node.runWithTemporaryDescription(context, localize('adding', 'Adding...'), async () => {
+            await addRemoteMIConnectionsInternal(context, undefined, node);
+        });
+    } else {


Why do you need this else? What's the third case here?

nturinski · 2025-03-19T21:24:28Z

+    await context.ui.showWarningMessage(localize('rolesWillBeAssignedMessage', 'This command will assign a managed identity and roles would you like to continue?'), { modal: true }, continueOn);
+    await wizard.execute();
+
+    const message: string = localize('setConnectionsProperty', 'Successfully added remote connections in order to use identity connections your application may require additional permisssions based on your code. You also may need to modify the connection property within your trigger.');


nturinski · 2025-03-19T21:24:55Z

 import { ext } from '../extensionVariables';
 import { installOrUpdateFuncCoreTools } from '../funcCoreTools/installOrUpdateFuncCoreTools';
 import { uninstallFuncCoreTools } from '../funcCoreTools/uninstallFuncCoreTools';
+import { type DurableTaskSchedulerClient } from '../tree/durableTaskScheduler/DurableTaskSchedulerClient';


You really just went and reorganized this whole file huh?

nturinski · 2025-03-21T21:16:11Z

+    }
+
+    private async getPicks(context: AddMIConnectionsContext): Promise<IAzureQuickPickItem<Connection>[]> {
+        if (context.functionapp) {


nit: return context.functionapp ? this.getRemoteQuickPicks(context) : this.getLocalQuickPicks(context)

nturinski · 2025-03-21T21:20:31Z

+        const localSettingsPath: string = await getLocalSettingsFile(context, message, workspaceFolder);
+        context.localSettingsPath = localSettingsPath;
+
+        if (await AzExtFsExtra.pathExists(localSettingsPath)) {


Now that you're using this function, I think you can get rid of the if statement above.

nturinski · 2025-03-21T21:28:31Z

+    await wizard.execute();
+
+    const continueOn: MessageItem = { title: localize('continueOn', 'Continue') };
+    const message: string = localize('setConnectionsProperty', 'Successfully added local connections. In order to use identity based connections, you may need to set the connection properties within your trigger.');


Successfully added local connections. To use identity-based connections, you may need to configure the connection properties within your trigger.

nturinski · 2025-03-21T21:30:47Z

+            if (localSettingsPath) {
+                if (await AzExtFsExtra.pathExists(localSettingsPath)) {
+                    const localSettingsUri: vscode.Uri = vscode.Uri.file(localSettingsPath);
+                    let localSettings: ILocalSettingsJson = <ILocalSettingsJson>await AzExtFsExtra.readJSON(localSettingsPath);


Use your helper function here.

nturinski · 2025-03-24T17:11:16Z

+
+        const localSettings = await getLocalSettingsJson(context, localSettingsPath);
+        if (localSettings.Values) {
+            for (const [key, value] of Object.entries(localSettings.Values)) {


nit: If you want to be even cleaner, this can be a helper function since it's used by getRemoteQuickPicks too (don't hate me)

nturinski · 2025-03-24T17:14:48Z

+            const localSettingsPath: string | undefined = await getLocalSettingsFileNoPrompt(context, this._workspaceFolder);
            if (localSettingsPath === undefined) {
                return { properties: {} };
            } else {


You can use your helper function again in here (a couple lines below this).

It also may make more sense to export getLocalSettingsJson from here since it's more related to the LocalSettingsClient than anything else. You could consider adding it to a util file too.

nturinski · 2025-03-24T17:15:55Z

+import { SettingsAddBaseStep } from "./SettingsAddBaseStep";
+
+export async function addLocalMIConnections(context: IActionContext, node?: AppSettingTreeItem): Promise<void> {
+    const connections: Connection[] = [];


I think it's possible to have multiple local project nodes, so if you enter through the command palette, you'll need something like:

if (!node) { node = await pickLocalProject(); // not real code }

nturinski · 2025-03-24T17:20:54Z

+                    }
+                }
+            }
+


nit: Extra line breaks.

motm32 added 6 commits January 29, 2025 09:36

initial commit

b8f19e5

Add changes

6979c6e

merge main

f8f10e3

add ability to right click from nodes

562fad5

more changes

b75809a

small changes

5e423da

motm32 requested a review from a team as a code owner February 28, 2025 18:19

microsoft deleted a comment Mar 3, 2025

MicroFish91 reviewed Mar 4, 2025

View reviewed changes

small changes

df7712c

Cxz324 mentioned this pull request Mar 5, 2025

Should all the activity child labels be present tense instead of past tense? microsoft/vscode-remote-release#10755

Closed

motm32 added 2 commits March 7, 2025 10:06

add in clientId and credential

453f0af

don't delete settings

4387565

nturinski reviewed Mar 10, 2025

View reviewed changes

motm32 added 8 commits March 12, 2025 11:01

requested changes

2d13d56

update appsettings and small changes

4373847

change warning message

64cee3d

remove managed identity step from local

3c126a7

fix

2e432f7

changed to appSettings package

1d9d860

add app setting changes

7164e58

fix right click on node

af3704b

nturinski reviewed Mar 19, 2025

View reviewed changes

motm32 added 3 commits March 19, 2025 21:34

requested changes

842b462

merge main

099a0f4

more changes

b5250a9

nturinski reviewed Mar 21, 2025

View reviewed changes

motm32 added 3 commits March 24, 2025 09:10

small changes

a7520fd

merge main

098b932

update

b6433f8

nturinski reviewed Mar 24, 2025

View reviewed changes

motm32 and others added 13 commits March 24, 2025 20:05

add changes

28e99cf

change

87ed16d

more changes

cdfc374

more changes

e82e6f6

merge main

5a969bc

add changed for regex

87d2988

small changes so nathan hopefully doesn't yell at me

8c63d4f

merge main

b70e19f

small change

f43f490

change package lock

0dd20d5

merge main

e1dafec

Minor fixes

e4819a6

Merge branch 'main' into meganmott/convertSettings

4e63165

nturinski approved these changes Mar 28, 2025

View reviewed changes

nturinski enabled auto-merge (squash) March 28, 2025 22:18

nturinski merged commit d62c7bb into main Mar 28, 2025
2 checks passed

nturinski deleted the meganmott/convertSettings branch March 28, 2025 22:19

	private async getRemoteQuickPics(context: IConvertConnectionsContext): Promise<IAzureQuickPickItem<Connection>[]> {
	private async getRemoteQuickPicks(context: IConvertConnectionsContext): Promise<IAzureQuickPickItem<Connection>[]> {

	connections.push({ name: node.id, value: node.value })
	connections.push({ name: node.id, value: node.value });

Conversation

motm32 commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MicroFish91 Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nturinski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

motm32 commented Feb 28, 2025 •

edited

Loading

MicroFish91 Mar 4, 2025 •

edited

Loading