Switch to module approach #20

TylerLeonhardt · 2018-08-30T02:45:09Z

This PR features:

Everything gets run in local scope
the module is exposed to the user - they can do Push-OutputBinding and Get-OutputBinding
some refactoring of the PowerShellManager class to make composing powershell easier

I learned that .AddStatement() exists which basically does what a ; does in a script.

src/Azure.Functions.PowerShell.Worker/PowerShell/PowerShellManager.cs

TylerLeonhardt · 2018-08-30T02:49:27Z

src/Azure.Functions.PowerShell.Worker/PowerShell/PowerShellManager.cs

+            // Reset the runspace to the Initial Session State
+            _pwsh.Runspace.ResetRunspaceState();
+
+            // TODO: Change this to clearing the variable by running in the module


@daxian-dbw I remember you mentioning there's a way to do this but I don't remember. Can you enlighten me 😄

I think it's OK to just call ResetRunspaceState (reset the variable table) for now, and wait for feedback.
If it's necessary, we can go wild on refreshing the Runspace:

we can sweep the global function table to remove the non-built-in free functions (not from module)

we can remove all modules after a function run

we can analyze the function script to explicitly disallow using global: for variable/function declaration, and -scope global for *-Variable.

Oh I meant refreshing the module scope variable as in setting $script:OutputBindings = @{}

Right now I'm just force importing the module.

Oh, easy, add a parameter -Purge to Get-OutputBinding. When specified, it clears the internal hashtable before returning.

I don't think we should expose a -Purge to the user. We talked about this yesterday.

Something like
& $module { $script:OutputBindings = @{}}

I don't remember the exact syntax you brought up

Yes, we talked about the Clean function. I don't really have a strong opinion on the Clean function, but it might be overkill to have a separate function just for that simple operation.

We don't give the user the ability to grab the instance of the internal hashtable, but I think it's OK for them to purge it if they want. For the language worker, if we are supposed to receive something but Get-OutputBinding returns nothing, then it's an error to report to the Host.

I thought about holding a reference to the hashtable in C#. If we want to go that road, then we can make the module accept argument -- a Hashtable instance. If the argument is presetnt, then the module uses that as the internal container; if not present, then create a new one.

But I do think in the local testing scenario, user will want to purge the output key/value pairs sometimes.

daxian-dbw · 2018-08-30T17:15:20Z

src/Azure.Functions.PowerShell.Worker/PowerShell/PowerShellManager.cs

+                .AddParameter("Name", modulePath)
+                .AddParameter("Scope", "Global")
+                .AddParameter("Force")
+                .InvokeAndClearCommands();


We can choose to remove all modules but this one, because we know this will be needed for the next function run.

But for now, I think we can skip removing any modules given the fact that 1 function app maps to N language worker and 1 language worker maps to 1 function app. Let's wait for the feedback.

daxian-dbw · 2018-08-30T17:24:42Z

src/Azure.Functions.PowerShell.Worker/PowerShell/PowerShellManager.cs

@@ -60,63 +41,35 @@ internal class PowerShellManager
        }

        public static PowerShellManager Create(RpcLogger logger)


Better to avoid this factory method and use the constructor directly. This SO question has a pretty good discussion about what a factory method is mainly for.

Quoted from https://blogs.msdn.microsoft.com/kcwalina/2004/10/11/design-guidelines-update-factories-vs-constructors/:

Consider using a constructor instead of a Factory. Only after this thought process should you proceed with the implementation of a Factory.

daxian-dbw · 2018-08-30T17:27:14Z

src/Azure.Functions.PowerShell.Worker/PowerShell/PowerShellManager.cs

-                    script.Append("' = $");
-                }
-                script.AppendLine(binding.Key);
+                initialSessionState.ExecutionPolicy = Microsoft.PowerShell.ExecutionPolicy.Unrestricted;


I didn't know we can set the execution policy this way, great!
We can skip the platform check and always set to Unrestricted for all platforms (even though it execution policy doesn't work on Unix).

daxian-dbw · 2018-08-30T17:30:09Z

src/Azure.Functions.PowerShell.Worker/PowerShell/PowerShellManager.cs

+            string modulePath = System.IO.Path.Join(
+                AppDomain.CurrentDomain.BaseDirectory,
+                "Azure.Functions.PowerShell.Worker.Module",
+                "Azure.Functions.PowerShell.Worker.Module.psd1");


In my PR, I updated the .csproj file to bin-place the module in the folder Modules that sits next to the worker assembly. We should add that folder to the module path at the initialization phase.

daxian-dbw · 2018-08-30T17:38:51Z

src/Azure.Functions.PowerShell.Worker/PowerShell/PowerShellManager.cs

+                .AddCommand("Import-Module")
+                .AddParameter("Name", modulePath)
+                .AddParameter("Scope", "Global")
+                .InvokeAndClearCommands();


The rest of the initialization should be done when the WorkerInitRequest comes in. Once we add the Modules folder in the module path, we don't need to explicitly import this module.

daxian-dbw · 2018-08-30T17:47:05Z

src/Azure.Functions.PowerShell.Worker/PowerShell/PowerShellManager.cs

+                        _pwsh
+                            .AddScript($@". {scriptPath}", s_UseLocalScope)
+                            .AddStatement()
+                            .AddCommand(entryPoint, s_UseLocalScope);


You are running the script twice, which could potentially cause problems, such as some operations get to done twice and fails in the second attempt for other side effects.

When the user specify the entrypoint, we treat the script as a module, and run import-module on it (useLocalScope won't be needed). So the script as a whole only gets run once. After the function run, we remove the module in the cleanup phase.

daxian-dbw · 2018-08-30T17:56:37Z

src/Azure.Functions.PowerShell.Worker/PowerShell/PowerShellManager.cs

        readonly static string s_TriggerMetadataParameterName = "TriggerMetadata";
+        readonly static bool s_UseLocalScope = true;


They should be const if they are not changed.

The explicit modifier is always preferred.

s_UseLocalScope is not very useful, better replace it with useLocalScope: true in the method call.

daxian-dbw · 2018-08-30T18:02:50Z

src/Azure.Functions.PowerShell.Worker/PowerShell/PowerShellManager.cs

+                    Collection<PSObject> pipelineItems = _pwsh.InvokeAndClearCommands<PSObject>();
+                    foreach (var psobject in pipelineItems)
+                    {
+                        _logger.LogInformation(psobject.ToString());


The log message needs to be reworked. It should call out that this is what returned from the function run.

prepend it with FROM FUNCTION: ?

How about OUTPUT: ?

daxian-dbw · 2018-08-30T18:05:04Z

src/Azure.Functions.PowerShell.Worker/PowerShell/PowerShellManager.cs

+                if(returnObject != null)
+                {
+                    result.Add("$return", returnObject);
+                }
                ResetRunspace();
                return result;
            }
            catch(Exception e)
            {
                ResetRunspace();


ResetRunspace(); should be put in the finally block.

daxian-dbw · 2018-08-30T18:08:50Z

src/Azure.Functions.PowerShell.Worker/Requests/HandleInvocationRequest.cs

@@ -67,6 +69,7 @@ internal static class HandleInvocationRequest
            // Set out binding data and return response to be sent back to host
            foreach (KeyValuePair<string, BindingInfo> binding in functionInfo.OutputBindings)
            {
+                // TODO: How do we want to handle when a binding is not set?


Yes, we do. That is an error, we should return fail statue in the InvocationResponse message.

daxian-dbw · 2018-08-30T18:10:41Z

BTW, when using any built-in cmdlet, please use the fully qualified format, such as Microsoft.PowerShell.Core\Get-Command. This is the best practice followed by PowerShellGet and other script modules.

TylerLeonhardt · 2018-08-30T22:20:58Z