Show quickpicks if there are multiple runtime matches for `targetFramework` #4165

motm32 · 2024-06-03T17:14:52Z

Related to microsoft/vscode-azurestaticwebapps#899

Currently the runtime is chosen using .find so the first match will be chosen. We should allow users to choose which runtime they want.

alexweininger · 2024-06-03T18:33:52Z

src/commands/createNewProject/dotnetSteps/DotnetRuntimeStep.ts

@@ -16,7 +16,7 @@ export class DotnetRuntimeStep extends AzureWizardPromptStep<IProjectWizardConte
    public static async createStep(context: IProjectWizardContext): Promise<DotnetRuntimeStep> {
        if (context.targetFramework) {
            context.targetFramework = typeof context.targetFramework === 'string' ? [context.targetFramework] : context.targetFramework;
-            const runtimes = await getRuntimes(context);
+            const runtimes = (await getRuntimes(context)).reverse(); // reverse so that the latest version is first


Did you verify that getRuntimes always returns them in the same order?

Mine always does. I will look into it some more to see how we are exactly getting it.

Yes please 👍

I think if we are going the prompt route as mentioned below it won't matter either way.

nturinski · 2024-06-03T20:59:06Z

I'm a little perplexed why it would have that behavior. Shouldn't it being an exact match on the .NET framework? So looking for .NET 8.0, and .NET 6.0 shouldn't match?

Edit: Oh, I think I understand what you mean. Since the targetFramework can be an array as well, it's matching the very first element of the array.

I think we should probably either a.) prompt the user if there are multiple matches or b.) add a property to the elements that indicates which is default

There are some concerns such as what Alex brought up that I have with the current logic.

motm32 · 2024-06-03T21:08:33Z

I think we should probably either a.) prompt the user if there are multiple matches or b.) add a property to the elements that indicates which is default

I think originally the SWA team wanted to prompt so I could see "a" being the best option. And I do think the list is always returned the same way but either way makes more sense to me to prompt than to automatically choose.

alexweininger

We should also add tests that cover this scenario in this PR

alexweininger · 2024-06-11T16:30:17Z

test/api.test.ts


-suite(`AzureFunctionsExtensionApi`, () => {
+suite.only(`AzureFunctionsExtensionApi`, () => {


Gotta remove dis

test/api.test.ts

alexweininger · 2024-06-14T18:41:51Z

test/api.test.ts

+        await validateProject(folderPath, validateOptions);
+    });
+
+    // When passing in a language version that is not in the target framework the last two inputs should not be prompted if they are this test will fail


This test is ensuring that target framework property is properly used to filter the picks from all available framework versions.

From this comment, it's not clear to me:

Why api.createFunction doesn't throw here, since selecting version 8 shouldn't be possible

What last two inputs do you mean, do you mean the next steps in the createFunction wizard?

How do you know this test will fail if 8 is a valid pick?

I think a better comment would start like:

// Intentionally pass a version (8) that hasn't been specified in targetFramework (6 & 7) to verify it isn't a possible pick. In the correct case (when 8 isn't a pick)... And in the incorrect case (when 8 is available)...

alexweininger · 2024-06-26T19:01:17Z

test/api.test.ts

+    // Intentionally pass a version (8) that hasn't been specified in targetFramework (6 & 7) to verify it isn't a possible pick. In the correct case (when 8 isn't a pick) this will cause an error to be thrown so no more picks are needed.
+    // In the incorrect case (when 8 is a pick) this will cause the test to fail as there are no further picks being passed in.


Suggested change

// Intentionally pass a version (8) that hasn't been specified in targetFramework (6 & 7) to verify it isn't a possible pick. In the correct case (when 8 isn't a pick) this will cause an error to be thrown so no more picks are needed.

// In the incorrect case (when 8 is a pick) this will cause the test to fail as there are no further picks being passed in.

// Intentionally pass a version (8) that hasn't been specified in targetFramework (6 & 7) to verify it isn't a possible pick. In the correct case (when 8 isn't a pick) we throw an error. api.createFunction swallows the error and returns undefined.

// In the incorrect case (when 8 is a pick) the test fails since the 2 provided test inputs have already been used, but there are more prompts.

alexweininger · 2024-06-26T23:47:27Z

Make sure these tests are passing in the Actions before merging. Also is the plan that this should not change any existing Functions UX right? We might want to inform CTI of this change and have them watch out for it.

motm32 · 2024-06-28T17:00:58Z

Make sure these tests are passing in the Actions before merging. Also is the plan that this should not change any existing Functions UX right? We might want to inform CTI of this change and have them watch out for it.

I am investigating right now since one of the tests I added is failing. And you are correct this should only change when a targetFramework exists which at the moment should only be in SWA. I will inform CTI of the change.

nturinski · 2024-07-03T21:13:51Z

test/MockIProjectWizardContext.ts

@@ -0,0 +1,45 @@
+/* eslint-disable no-restricted-imports */


Do you need this still?

src/commands/createNewProject/dotnetSteps/DotnetRuntimeStep.ts

Reverse list

c163011

motm32 requested a review from a team as a code owner June 3, 2024 17:14

alexweininger reviewed Jun 3, 2024

View reviewed changes

change to quickpicks

6a34002

motm32 changed the title ~~Fix targetFramework so that te latest runtime is chosen~~ Show quickpicks if there are multiple runtime matches for targetFramework Jun 4, 2024

alexweininger reviewed Jun 5, 2024

View reviewed changes

motm32 added 2 commits June 7, 2024 11:00

add test

284e066

remove

9257f30

alexweininger reviewed Jun 11, 2024

View reviewed changes

test/api.test.ts Show resolved Hide resolved

motm32 added 2 commits June 13, 2024 13:45

Add another test

260e01d

remove

e34414b

alexweininger reviewed Jun 14, 2024

View reviewed changes

update comment

463244c

alexweininger reviewed Jun 26, 2024

View reviewed changes

update again

0c7d002

alexweininger previously approved these changes Jun 26, 2024

View reviewed changes

see if tests work

192b95a

motm32 dismissed alexweininger’s stale review via 192b95a July 3, 2024 19:04

nturinski previously approved these changes Jul 3, 2024

View reviewed changes

test/MockIProjectWizardContext.ts Outdated

@@ -0,0 +1,45 @@

/* eslint-disable no-restricted-imports */

Copy link

Member

nturinski Jul 3, 2024

Choose a reason for hiding this comment

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

Do you need this still?

delete

c0cd710

motm32 dismissed nturinski’s stale review via c0cd710 July 8, 2024 15:47

alexweininger reviewed Jul 8, 2024

View reviewed changes

src/commands/createNewProject/dotnetSteps/DotnetRuntimeStep.ts Outdated Show resolved Hide resolved

add

9758da7

alexweininger approved these changes Jul 8, 2024

View reviewed changes

motm32 merged commit 6867559 into main Jul 8, 2024
2 checks passed

motm32 deleted the meganmott/targetFrameworkFix branch July 8, 2024 17:41

motm32 mentioned this pull request Aug 20, 2024

Let users specify .NET version when adding an HTTP Function microsoft/vscode-azurestaticwebapps#761

Closed

microsoft locked and limited conversation to collaborators Aug 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show quickpicks if there are multiple runtime matches for `targetFramework` #4165

Show quickpicks if there are multiple runtime matches for `targetFramework` #4165

motm32 commented Jun 3, 2024 •

edited

Loading

alexweininger Jun 3, 2024

motm32 Jun 3, 2024

alexweininger Jun 3, 2024

motm32 Jun 3, 2024

nturinski commented Jun 3, 2024 •

edited

Loading

motm32 commented Jun 3, 2024

alexweininger left a comment

alexweininger Jun 11, 2024

alexweininger Jun 14, 2024 •

edited

Loading

alexweininger Jun 26, 2024

alexweininger commented Jun 26, 2024 •

edited

Loading

motm32 commented Jun 28, 2024

nturinski Jul 3, 2024


		suite(`AzureFunctionsExtensionApi`, () => {
		suite.only(`AzureFunctionsExtensionApi`, () => {

		// Intentionally pass a version (8) that hasn't been specified in targetFramework (6 & 7) to verify it isn't a possible pick. In the correct case (when 8 isn't a pick) this will cause an error to be thrown so no more picks are needed.
		// In the incorrect case (when 8 is a pick) this will cause the test to fail as there are no further picks being passed in.

Show quickpicks if there are multiple runtime matches for targetFramework #4165

Show quickpicks if there are multiple runtime matches for targetFramework #4165

Conversation

motm32 commented Jun 3, 2024 • edited Loading

alexweininger Jun 3, 2024

Choose a reason for hiding this comment

motm32 Jun 3, 2024

Choose a reason for hiding this comment

alexweininger Jun 3, 2024

Choose a reason for hiding this comment

motm32 Jun 3, 2024

Choose a reason for hiding this comment

nturinski commented Jun 3, 2024 • edited Loading

motm32 commented Jun 3, 2024

alexweininger left a comment

Choose a reason for hiding this comment

alexweininger Jun 11, 2024

Choose a reason for hiding this comment

alexweininger Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

alexweininger Jun 26, 2024

Choose a reason for hiding this comment

alexweininger commented Jun 26, 2024 • edited Loading

motm32 commented Jun 28, 2024

nturinski Jul 3, 2024

Choose a reason for hiding this comment

Show quickpicks if there are multiple runtime matches for `targetFramework` #4165

Show quickpicks if there are multiple runtime matches for `targetFramework` #4165

motm32 commented Jun 3, 2024 •

edited

Loading

nturinski commented Jun 3, 2024 •

edited

Loading

alexweininger Jun 14, 2024 •

edited

Loading

alexweininger commented Jun 26, 2024 •

edited

Loading