New programming model #849

andystaples · 2022-08-22T22:48:26Z

Issue describing the changes in this PR

Adds support for worker indexing to the functions worker

Pull request checklist

My changes do not require documentation changes
- Otherwise: Documentation issue linked to PR
My changes should not be added to the release notes for the next release
- Otherwise: I've added my notes to release_notes.md
My changes do not need to be backported to a previous version
- Otherwise: Backport tracked by issue/PR #issue_or_pr
I have added all required tests (Unit tests, E2E tests)

andystaples · 2022-08-22T22:51:16Z

@Francisco-Gamino I think this PR is correct. The rebase was awful and doubled the amount of commits, not sure why. Lots still to do here, gotta bundle the module with the worker and figure out how to get the cmdlet available in order to write tests for the new programming model

Francisco-Gamino

Thank you @andystaples for sending this out! I've left a few comments. Let me know if you have any questions.

src/Microsoft.Azure.Functions.PowerShellWorker.csproj

src/RequestProcessor.cs

src/WorkerIndexing/BindingInformation.cs

src/WorkerIndexing/FunctionInformation.cs

src/WorkerIndexing/WorkerIndexingHelper.cs

src/worker.config.json

src/WorkerIndexing/WorkerIndexingHelper.cs

Francisco-Gamino · 2022-08-24T23:27:08Z

src/resources/PowerShellWorkerStrings.resx

@@ -352,4 +352,10 @@
  <data name="DependencySnapshotDoesNotContainAcceptableModuleVersions" xml:space="preserve">
    <value>Dependency snapshot '{0}' does not contain acceptable module versions.</value>
  </data>
+  <data name="GetFunctionsMetadataMultipleResultsError" xml:space="preserve">


Could you please ping the Team channel to get feedback on the new string resources you added?

src/WorkerIndexing/BindingInformation.cs

davidmrdavid

Some minor thoughts from skimming the PR. Looking good!

davidmrdavid · 2022-08-25T16:33:58Z

src/Microsoft.Azure.Functions.PowerShellWorker.csproj

-<!--
+<!--


nipickt: did anything change in this file? Seems like nothing changed. If so - can we instruct git to "revert this file" so that it doesn't register a change in the PR?

davidmrdavid · 2022-08-25T16:35:25Z

src/RequestProcessor.cs

+                // Failure that happens during this step is terminating and we will need to return a failure response to
+                // all subsequent 'FunctionLoadRequest'. Cache the exception so we can reuse it in future calls.


A suggestion on wording:

Suggested change

// Failure that happens during this step is terminating and we will need to return a failure response to

// all subsequent 'FunctionLoadRequest'. Cache the exception so we can reuse it in future calls.

// This is a terminating failure: we will need to return a failure response to

// all subsequent 'FunctionLoadRequest'. Cache the exception so we can reuse it in future calls.

src/RequestProcessor.cs

davidmrdavid · 2022-08-25T16:37:40Z

src/RequestProcessor.cs

+            // WorkerStatusResponse type says that it is not used but this will create an empty one anyway to return to the host
+            StreamingMessage response = NewStreamingMessageTemplate(
+                request.RequestId,
+                StreamingMessage.ContentOneofCase.FunctionMetadataResponse,
+                out StatusResult status);
+
+            response.FunctionMetadataResponse.FunctionMetadataResults.AddRange(WorkerIndexingHelper.IndexFunctions(request.FunctionsMetadataRequest.FunctionAppDirectory));


Without knowing the specifics of the contract between the Host and worker, I can't decipher what the comment at the top of this block is trying to say. Can you please add a bit more context? What exactly "says that it is not used"?, and what is "it" in this context? Thanks!

I have no idea what this comment is about either. My guess - some misguided note I made while working on my first prototype and forgot to remove. I have removed it

davidmrdavid · 2022-08-25T16:39:20Z

src/WorkerIndexing/WorkerIndexingHelper.cs

+{
+    internal class WorkerIndexingHelper
+    {
+        const string GetFunctionsMetadataCmdletName = "AzureFunctions.PowerShell.SDK\\Get-FunctionsMetadata";


Should we move this string to the strings-resource file?

This is the way that other places in the code which call cmdlets deal with it. Strings-resources are for customer-facing error/warning messages. I propose creating a separate issue on the worker to track all occurrences and move them to some global constants file.

davidmrdavid · 2022-08-25T16:41:11Z

src/WorkerIndexing/WorkerIndexingHelper.cs

+            List<RpcFunctionMetadata> indexedFunctions = new List<RpcFunctionMetadata>();
+
+            InitialSessionState initial = InitialSessionState.CreateDefault();
+            Runspace runspace = RunspaceFactory.CreateRunspace(initial);
+            runspace.Open();
+            System.Management.Automation.PowerShell ps = System.Management.Automation.PowerShell.Create();
+            ps.Runspace = runspace;
+
+            ps.AddCommand(GetFunctionsMetadataCmdletName).AddArgument(baseDir);
+            string outputString = string.Empty;


I think this block would benefit from a small comment explaining what data your runspace is achieving, and how exactly this is helping the indexing process.

Done. This is a much larger issue, actually, and without a better approach, will lead to the AzureFunctions.PowerShell.SDK module never being available to the worker in Azure, even if we bundle it with the worker. I have left a long comment explaining the issue and some proposed solutions.

michaelpeng36 · 2022-08-26T19:54:08Z

src/resources/PowerShellWorkerStrings.resx

+    <value>Multiple results from metadata cmdlet</value>
+  </data>
+  <data name="InvalidBindingInfoDirection" xml:space="preserve">
+    <value>The bindingInfo's Direction is not valid</value>


Would we be able to specify which binding name this corresponds to?

AnatoliB · 2022-08-26T21:26:19Z

src/WorkerIndexing/BindingInformation.cs

@@ -0,0 +1,54 @@
+using Microsoft.Azure.WebJobs.Script.Grpc.Messages;


Please add the standard copyright/license header to all the new files - just copy it from other files.

andystaples requested a review from Francisco-Gamino August 22, 2022 22:50

Francisco-Gamino reviewed Aug 23, 2022

View reviewed changes

Francisco-Gamino requested review from AnatoliB, davidmrdavid and michaelpeng36 August 23, 2022 23:17

Francisco-Gamino reviewed Aug 24, 2022

View reviewed changes

davidmrdavid reviewed Aug 25, 2022

View reviewed changes

michaelpeng36 reviewed Aug 26, 2022

View reviewed changes

michaelpeng36 approved these changes Aug 26, 2022

View reviewed changes

andystaples marked this pull request as ready for review August 26, 2022 20:45

AnatoliB reviewed Aug 26, 2022

View reviewed changes

Francisco-Gamino force-pushed the new-programming-model branch from ab737ee to 3e50a6e Compare October 5, 2022 22:17

Francisco-Gamino added 2 commits October 6, 2022 11:18

Add support for the New PowerShell Programming Model

887c882

Update tests

5e38ae4

Francisco-Gamino force-pushed the new-programming-model branch from 3e50a6e to 5e38ae4 Compare October 6, 2022 18:20

Francisco-Gamino added 2 commits October 6, 2022 15:18

Update worker.config.json

85d64cb

Use the cmdlet name instead of the module qualified cmdlet name

797fe7e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New programming model #849

New programming model #849

andystaples commented Aug 22, 2022

andystaples commented Aug 22, 2022

Francisco-Gamino left a comment

Francisco-Gamino Aug 24, 2022

davidmrdavid left a comment

davidmrdavid Aug 25, 2022

davidmrdavid Aug 25, 2022

andystaples Aug 26, 2022

davidmrdavid Aug 25, 2022

andystaples Aug 26, 2022

davidmrdavid Aug 25, 2022

andystaples Aug 26, 2022

davidmrdavid Aug 25, 2022

andystaples Aug 26, 2022

michaelpeng36 Aug 26, 2022

andystaples Aug 26, 2022

AnatoliB Aug 26, 2022

andystaples Aug 26, 2022

		// Failure that happens during this step is terminating and we will need to return a failure response to
		// all subsequent 'FunctionLoadRequest'. Cache the exception so we can reuse it in future calls.

		@@ -0,0 +1,54 @@
		using Microsoft.Azure.WebJobs.Script.Grpc.Messages;

		<!--
		<!--

New programming model #849

Are you sure you want to change the base?

New programming model #849

Conversation

andystaples commented Aug 22, 2022

Issue describing the changes in this PR

Pull request checklist

andystaples commented Aug 22, 2022

Francisco-Gamino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmrdavid left a 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