Create from Deploy feature and some local UX changes #4130

nturinski · 2024-05-14T01:09:48Z

This is basically this PR #4042 but I removed some of the UX changes regarding "Create function locally...".

I really didn't like having the prompt asking for what type of project does the user want to create. It doesn't really make sense to have it in our main entry point, so until we can think of a good solution, the command list looks like this:

Old:

New:

Removed Create Function App... option in the local workspace view
- This was replaced with just Deploy function to Azure... which includes a + Create new function app option as a quick pick when we prompt for the app

Other changes I probably should have made separate PRs for 😅:

Added Create New Project... to workspace view if there's no project in the workspace. It actually does call azureFunctions.createNewProject because we know that we need to start from scratch. createFunction has to do some checks before it ultimately cancels and calls createNewProject so performance is slightly better to just call it directly.
Old:

New:
Added two entry points for "Create Function..." which live on the local project node and context menu

…ions into nat/createFromDeploy

nturinski · 2024-05-14T01:17:14Z

src/commands/createFunctionApp/createCreateFunctionAppComponents.ts

@@ -0,0 +1,164 @@
+/*---------------------------------------------------------------------------------------------


This is basically moving all the code from SubscriptionTreeItem. Please pay careful attention that I actually got everything-- small things have been changing with create here and there (flex, containerized, etc.)

alexweininger · 2024-05-14T02:02:12Z

package.nls.json

@@ -24,6 +24,7 @@
    "azureFunctions.deleteFunction": "Delete Function...",
    "azureFunctions.deleteFunctionApp": "Delete Function App...",
    "azureFunctions.deleteSlot": "Delete Slot...",
+    "azureFunctions.deployProject": "Deploy to Azure...",


Removed Create Function App... option in the local workspace view
This was replaced with just Deploy function to Azure... which includes a + Create new function app option as a quick pick when we prompt for the app

Is it supposed to be "Deploy to Azure..." or "Deploy function to Azure..."?

Ah, I think we originally went with "Deploy function to Azure..." but after more discussion, just went with a more generic "Deploy to Azure..."

We don't think we need to give more context to users on what they are deploying?

Yeah, I mean you're in the workspace part of the extension so I think it's kind of clear. That being said, I think it still prompts you for a workspace folder.

I'm thinking of adding a right-click context item on the local project node though that also has "Deploy to Azure..." on it.

Well I was thinking that they might need to know that the command is for deploying Functions to Azure and not App Service or anything else. But maybe they should already know?

Well, you click the Functions symbol to open the menu for the commands. I thought that was kind of the reason we had those buttons there to give them that context.

Ok, that's fine

alexweininger · 2024-05-15T00:55:17Z

src/commands/createFunctionApp/createCreateFunctionAppComponents.ts

+    return version;
+}
+
+async function createFunctionAppWizard(wizardContext: IFunctionAppWizardContext): Promise<{ promptSteps: AzureWizardPromptStep<IAppServiceWizardContext>[], executeSteps: AzureWizardExecuteStep<IAppServiceWizardContext>[] }> {


I don't love that this function is returning a whole wizard object, but then only the prompt and execute steps on that object are used. I think the return type should be changed to reflect what is actually used.

Also it's a bit weird that this function also sets some wizard context, but I can't provide a better place to set it so 🤷

I don't disagree with you, but this is logic that was pre-existing to this PR. I'm open to changing it, but I'd rather do it in a separate PR since I wanted to keep the core logic fundamentally the same.

https://github.com/microsoft/vscode-azurefunctions/pull/4130/files#diff-3b26e2c6749863538fbe756b9eb0e37aaa43b559e370bd6daf7aaa84a871a158L233

alexweininger · 2024-05-15T23:13:23Z

src/commands/SubscriptionListStep.ts

+        }
+    }
+
+    private async getPicks(_: IFuncDeployContext): Promise<IAzureQuickPickItem<AzureSubscription>[]> {


Any reason we didn't just remove the parameter if it's not being used?

alexweininger · 2024-05-15T23:25:49Z

src/commands/deploy/getOrCreateFunctionApp.ts

+    const promptSteps = [new SubscriptionListStep(), new FunctionAppListStep()];
+    const title: string = l10n.t('Select Function App');
+    const wizard: AzureWizard<IAppServiceWizardContext> = new AzureWizard(context, {
+        promptSteps,
+        title
+    });


I prefer if you just put the promptSteps and title values inline here since they aren't used anywhere else.

alexweininger · 2024-05-15T23:26:51Z

src/commands/deploy/getOrCreateFunctionApp.ts

+
+    await wizard.prompt();
+
+    node = context.site ? await ext.rgApi.tree.findTreeItem(nonNullProp(context.site, 'id'), context) : undefined;


Find tree item is sort of sketchy to use. Have you tested this when the tree hasn't been loaded yet?

Yeah, it works though it's pretty unbearably slow. I may try to refactor this to just use the data model instead because of the performance.

It's pretty tough without some significant refactoring. We need to pass the tree ndoe to deploy because it does the whole temporary description thing on it. If the node isn't there or if the parent is undefined, it'll throw an error.

alexweininger · 2024-05-15T23:29:48Z

src/commands/deploy/FunctionAppListStep.ts

+            }
+        });
+
+        qp.unshift({ label: '$(plus) Create new function app', description: '', data: undefined });


Why the empty description?

You see the description as half empty, but I see it as half full.

alexweininger · 2024-05-15T23:31:34Z

src/commands/deploy/FunctionAppListStep.ts

+    }
+
+    public async getSubWizard(context: IFuncDeployContext): Promise<IWizardOptions<IFuncDeployContext> | undefined> {
+        if (!context.site) {


Maybe add a comment here. It seems like this the pivotal place where the code decides if we're creating a FA or not for deploy.

Nit: I would also like to suggest using a guard clause here to clean this up a bit more :)

Could you clarify what you mean by using a guard clause here? I'm not really sure what I would be type guarding in this case.

Basically invert it and add this guard at the beginning instead:

if (context.site) { return undefined; } // Do the rest of your normal logic but now without the nesting

Hey guy, I already did that.

alexweininger · 2024-05-15T23:48:35Z

I expected there to be a way to deploy from the local project. Was that ever discussed?

MicroFish91 · 2024-05-16T18:36:23Z

src/agent/agentIntegration.ts

-            commandId: "azureFunctions.createNewProject",
-            displayName: "Create Function Project",
+            commandId: "azureFunctions.createFunction",
+            displayName: "Create Function Locally",


Do you remember if there was a reason we couldn't do something like Create Function (Local)? I vaguely recall a reason but can't really remember anymore

I don't know if there was a reason or not, but personally if I see (Local), it makes me think that there is another version of Create Function. Probably Create Function (Remote) or something like that.

…ions into nat/createFromDeploy2

nturinski · 2024-05-20T18:06:36Z

Whoo boy 🥵

nturinski added 10 commits April 14, 2024 23:31

merge from main

138f6c7

Add the unique step that got removed

a03c9ed

ignore eslint any errors for now

77a1d7a

Fix up some typings to fix the build

30370b0

Merge branch 'main' of https://github.com/microsoft/vscode-azurefunct…

e41ed65

…ions into nat/createFromDeploy

Some fixes for the tests

0cd040f

merge from main

402bcc1

Remove local project UX changes

f7507c7

More reverting

6362c06

Save files

2daa8f2

nturinski requested a review from a team as a code owner May 14, 2024 01:09

Move create function button

cbac861

nturinski commented May 14, 2024

View reviewed changes

Some clean up

ac66289

alexweininger reviewed May 14, 2024

View reviewed changes

nturinski added 2 commits May 14, 2024 14:47

Change label for new project

ec6a50d

Merge branch 'main' into nat/createFromDeploy2

9c29a96

alexweininger reviewed May 15, 2024

View reviewed changes

nturinski added 3 commits May 16, 2024 00:07

Merge main, deploy to Azure button

01d9864

Fetch

77d3490

Add deploy button and context menu action

13782f8

MicroFish91 reviewed May 16, 2024

View reviewed changes

nturinski added 3 commits May 16, 2024 15:21

Remove extra create step

e324d96

Merge branch 'main' of https://github.com/microsoft/vscode-azurefunct…

a357727

…ions into nat/createFromDeploy2

PR feedback

1c260b4

alexweininger approved these changes May 20, 2024

View reviewed changes

nturinski merged commit 6f40f43 into main May 20, 2024
2 checks passed

nturinski deleted the nat/createFromDeploy2 branch May 20, 2024 18:06

nturinski added this to the 1.15.0 milestone May 21, 2024

microsoft locked and limited conversation to collaborators Jul 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create from Deploy feature and some local UX changes #4130

Create from Deploy feature and some local UX changes #4130

nturinski commented May 14, 2024 •

edited

Loading

nturinski May 14, 2024

alexweininger May 14, 2024

nturinski May 14, 2024

alexweininger May 14, 2024

nturinski May 14, 2024

alexweininger May 14, 2024

nturinski May 15, 2024

alexweininger May 15, 2024

alexweininger May 15, 2024

nturinski May 16, 2024

alexweininger May 15, 2024

alexweininger May 15, 2024

alexweininger May 15, 2024

nturinski May 16, 2024

nturinski May 16, 2024

alexweininger May 15, 2024

nturinski May 16, 2024

alexweininger May 15, 2024

MicroFish91 May 16, 2024 •

edited

Loading

nturinski May 16, 2024

MicroFish91 May 17, 2024 •

edited

Loading

nturinski May 17, 2024

alexweininger commented May 15, 2024

MicroFish91 May 16, 2024

nturinski May 16, 2024

nturinski commented May 20, 2024

		@@ -0,0 +1,164 @@
		/*---------------------------------------------------------------------------------------------


		await wizard.prompt();

		node = context.site ? await ext.rgApi.tree.findTreeItem(nonNullProp(context.site, 'id'), context) : undefined;

Create from Deploy feature and some local UX changes #4130

Create from Deploy feature and some local UX changes #4130

Conversation

nturinski commented May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicroFish91 May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicroFish91 May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexweininger commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nturinski commented May 20, 2024

nturinski commented May 14, 2024 •

edited

Loading

MicroFish91 May 16, 2024 •

edited

Loading

MicroFish91 May 17, 2024 •

edited

Loading