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

Adding ability to select the .NET language on create #2667

Merged
merged 11 commits into from Dec 7, 2021

Conversation

aaronpowell
Copy link
Contributor

When adding the F# templates for dotnet-isolated (see: Azure/azure-functions-templates#1074) I dug around to figure out just how the tools worked to create new projects.

This led me to figure out how the dotnet setup was done and that adding a language selector into the func init pipeline wouldn't be that hard. So now we have the ability to select which .NET language we want to use for both in-proc and isolated models.

As a result, F# projects can be managed using the func CLI rather than having to go via the dotnet cli.

This will allow the creation of F# projects from the func init command
@aaronpowell
Copy link
Contributor Author

I've done some local testing of this, using the new templates, from the above mentioned pack, and it was able to create both C# and F# across dotnet and isolated.

@@ -41,7 +41,7 @@ public static class WorkerRuntimeLanguageHelper
private static readonly IDictionary<WorkerRuntime, string> workerToDefaultLanguageMap = new Dictionary<WorkerRuntime, string>
{
{ WorkerRuntime.dotnet, Constants.Languages.CSharp },
{ WorkerRuntime.dotnetIsolated, Constants.Languages.CSharp },
{ WorkerRuntime.dotnetIsolated, Constants.Languages.CSharpIsolated },
Copy link
Member

Choose a reason for hiding this comment

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

Does this need entries for F#?

Copy link
Member

Choose a reason for hiding this comment

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

I see it's the defaults

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, figured that the default of C# makes the most sense

@anthonychu
Copy link
Member

@fabiocav Adding it to the next sprint to see if we can have someone review this. It's only going into v4. I think @aaronpowell can help create a PR to back port it to v3 as well if this is approved.

@anthonychu anthonychu added this to the Functions Sprint 108 milestone Aug 18, 2021
@soninaren soninaren self-assigned this Aug 25, 2021
@fabiocav
Copy link
Member

fabiocav commented Sep 8, 2021

Apologies for the delay here. Reassigning this for review.

@aaronpowell would it be possible to update this PR against the latest code? Thanks!

@aaronpowell
Copy link
Contributor Author

@fabiocav done

@michaelpeng36 michaelpeng36 self-assigned this Nov 17, 2021
@@ -25,7 +25,7 @@ public static class WorkerRuntimeLanguageHelper
private static readonly IDictionary<WorkerRuntime, IEnumerable<string>> availableWorkersRuntime = new Dictionary<WorkerRuntime, IEnumerable<string>>
{
{ WorkerRuntime.dotnet, new [] { "c#", "csharp", "f#", "fsharp" } },
{ WorkerRuntime.dotnetIsolated, new [] { "dotnet-isolated" } },
{ WorkerRuntime.dotnetIsolated, new [] { "dotnet-isolated", "c#-isolated", "csharp-isolated", "f#-isolated", "fsharp-isolated" } },
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need dotnet-isolated in the array containing c#-isolated and csharp-isolated here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have to have a second look at it, but I think the reason I kept dotnet-isolated in there was for backwards compatibility, as prior to this change that's how things were identified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've had a bit of a poke around and I don't think it can be removed with a lot of other cascading changes.

Removing dotnet-isolated will cause existing projects to fail to launch using func start as it's unable to detect the worker type. I also tested changing the worker type in local.settings.json to c#-isolated but that then errors within the WebJobs dependency as it couldn't find the functions language c#-isolated, so there'd be a downstream impact.

Additionally, all templates would need to be updated to either say c#-isolated or f#-isolated in them before the change would usable.

Ultimately, the additional worker types are probably not doing anything of value at present, they are more future proofing 😅

@@ -55,7 +55,7 @@ public static class WorkerRuntimeLanguageHelper
{ Constants.Languages.TypeScript, new [] { "ts" } },
{ Constants.Languages.Python, new [] { "py" } },
{ Constants.Languages.Powershell, new [] { "pwsh" } },
{ Constants.Languages.CSharp, new [] { "csharp", "dotnet", "dotnet-isolated", "dotnetIsolated" } },
{ Constants.Languages.CSharpIsolated, new [] { "csharp", "dotnet", "dotnet-isolated", "dotnetIsolated" } },
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe there should be separate entries here: one with { Constants.Languages.CSharp, new [] { "csharp", "dotnet" } } and another with { Constants.Languages.CSharpIsolated, new [] { "dotnet-isolated", "dotnetIsolated" } }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's probably true, otherwise it won't detect in-proc properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done but I've had to move csharp out of the CSharpIsolated constant, otherwise it'll explode later on in WorkerRuntimeStringToLanguage, but I'm not quite sure how they all hang together to know if that's a problem (hasn't seem to be in my testing)

Copy link
Contributor

@michaelpeng36 michaelpeng36 left a comment

Choose a reason for hiding this comment

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

@aaronpowell this PR seems mostly correct, but there are a few minor issues that need to be addressed, which I have added in the comments.

Copy link
Contributor

@michaelpeng36 michaelpeng36 left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelpeng36
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@michaelpeng36 michaelpeng36 merged commit eab5619 into Azure:v4.x Dec 7, 2021
@dsyme
Copy link
Member

dsyme commented Dec 7, 2021

Yay!!!!

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.

None yet

7 participants