Change GetModuleFilePaths() to include a modules directory in root of Function App #906
Conversation
|
Hi @davidobrien1985, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
Not entirely sure why ALL the tests are failing here. Strange. @tohling , maybe something that you can quickly check?! Sorry if I broke something. |
There was a problem hiding this comment.
Hey @davidobrien1985, thank you for sending this!
There are some issues with that CI run (the default CI project), and the failing tests are not related to your changes, so no worries there 😃
| PowerShellConstants.ModulesScriptFileExtensionPattern)); | ||
| } | ||
|
|
||
| if (Directory.Exists(rootModuleDirectory)) |
There was a problem hiding this comment.
Can we introduce a method to add a directory to modulePaths so we can combine this new code and the block above to eliminate duplication?
There was a problem hiding this comment.
Good idea. Will get to that now and push asap.
|
@fabiocav This should look cleaner now. (c# is cool :-) ) |
…e calling method.
…e calling method.
…avidobrien1985/azure-webjobs-sdk-script into feature/powershell_module_root
|
Changed AddToModulePaths() to search the |
…e calling method.
…e calling method.
…avidobrien1985/azure-webjobs-sdk-script into feature/powershell_module_root
|
@davidobrien1985 could you rebase your work on the dev branch? |
|
@fabiocav done |
… into feature/powershell_module_root
There was a problem hiding this comment.
@davidobrien1985 I've added a couple of small nit comments (simple stuff). But this looks good!
It would be nice to update the GetModuleFilePaths test to account for this new logic as well.
@tohling, since this is in the PowerShell invoker, could you take a quick look at this and give your blessing?
| return modulePaths; | ||
| } | ||
|
|
||
| internal static List<string> AddToModulePaths(string[] directories) |
There was a problem hiding this comment.
nit: change this to use params (will simplify the calling code)
There was a problem hiding this comment.
Overall, I would say that you should add tests for this and ensure that loading of modules works without errors when loading from both rootModuleDirectory and moduleDirectory. Verify that if a DLL is duplicated in both directories, the load behavior is as expected.
There was a problem hiding this comment.
@fabiocav changed to params now.
| internal static List<string> AddToModulePaths(string[] directories) | ||
| { | ||
| List<string> paths = new List<string>(); | ||
| for (int i = 0; i < directories.Length; i++) |
There was a problem hiding this comment.
nit: simplify by using a foreach
There was a problem hiding this comment.
that's done now.
|
@fabiocav cool, thanks. Will do that tonight when I'm back at the hotel. Good feedback. |
| { | ||
| paths.AddRange(Directory.GetFiles(currentDirectory, | ||
| PowerShellConstants.ModulesManifestFileExtensionPattern, | ||
| SearchOption.AllDirectories)); |
There was a problem hiding this comment.
Where is the file and code for SearchOption?
There was a problem hiding this comment.
Kindly also add tests for the following,
- nested folders
- ensure that resolution in case of duplicates is as expected.
There was a problem hiding this comment.
@tohling SearchOption is part of System.IO
There was a problem hiding this comment.
@fabiocav, ah, I missed Directory.GetFiles. Thanks!
There was a problem hiding this comment.
@tohling How would you want to handle "duplicates"? Usually I would say "closest to the function" wins.
However, PowerShell can import multiple versions of the same module into one environment. It is then up to the user to decide which one he/she wants to use. That said, because Azure Functions right now imports everything automatically, this won't necessarily work.
Hence #897
There was a problem hiding this comment.
@davidobrien1985 Yes, I agree that the correct change is to allow the customer to decide which one to use even when there are duplicates. This is potentially a breaking change to existing customers who already have their scripts written with the current default behavior. There is a broader issue on how we can absorb your changes. We are working on addressing this. Kindly give us some time and I will get back to you.
There was a problem hiding this comment.
@tohling do you want me to continue working on this here then? Do you have an issue for this already here somewhere?
There was a problem hiding this comment.
@davidobrien1985, yes you may continue to work on this. I don't have an issue opened yet. I really like the changes that you are making and my previous comment was just me thinking ahead on how soon they can be merged into the repo.
We truly value the work from contributors like yourself. We are discussing how to incorporate your changes in a timely and seamless manner for both you and customers who have already deployed their PowerShell Functions.
There was a problem hiding this comment.
Okay, added some logic to the tests and GetModuleFilePaths() is still passing.
In regards to duplicates:
I will assume, for now, that if there are duplicates (by filename) that the one closest in functionroot/modules is supposed to be imported.
| return modulePaths; | ||
| } | ||
|
|
||
| internal static List<string> AddToModulePaths(string[] directories) |
There was a problem hiding this comment.
Overall, I would say that you should add tests for this and ensure that loading of modules works without errors when loading from both rootModuleDirectory and moduleDirectory. Verify that if a DLL is duplicated in both directories, the load behavior is as expected.
|
@davidobrien1985, looks good except the test Microsoft.Azure.WebJobs.Script.Tests.PowerShellInvokerTests.GetModuleFilePaths is still failing. Could you take a look? |
…etModuleFilePaths() now passes again.
|
@tohling fixed. Totally missed that one test. Hooray for tests! |
|
@tohling indicated that she has reviewed this changes and she is good with them. @davidobrien1985 can you confirm that you have no other pending changes here? If so, we'll spend some time reviewing this so we can have this work merged. |
|
|
||
| if (_moduleFiles.Any()) | ||
| { | ||
| powerShellInstance.AddCommand("Import-Module").AddArgument(_moduleFiles); |
There was a problem hiding this comment.
So we run an auto import on every single module we discover. For a modules folder scoped to a single function (the way we work currently) that is OK, since you only add modules you want to use in your function anyways.
However, in this PR you're adding a shared modules folder that sits above all functions, and all the modules in there will be added to all functions. That seems problematic. Can't you do what you're trying to do today by adding explicit import statements to pull in the shared modules you need from a shared folder? That avoids the code duplication issue, and also allows you to pull only a subset of shared modules in as needed.
There was a problem hiding this comment.
@mathewc this would be #897
Right now I just want to have a "super modules" directory that does not sit within one function.
Let's say I have one module that I need to share across 5 functions in one Function App I only want it in one place, not in 5.
The next step will be to add implicit import as described in above issue which will address the issue you are having right now.
There was a problem hiding this comment.
But can't you simply put your shared module in a shared folder, and add implicit import statement to each function already? As you pointed out in #897 auto import has performance implications. While the problem is not too bad for function level modules (since you have to explicitly add them to your function dir so intent is pretty clear), for cross function modules auto importing into every function doesn't feel right. Isn't simply adding the proper import statements to each of your functions a cleaner approach? Are there any issues blocking you from doing that today?
There was a problem hiding this comment.
Well, I'd prefer to not have any auto-import, at all, not even on a function level.
I was told that auto-import should stay so to not break current behaviour.
I would either opt for removing auto-import everywhere or add my approach here and auto-import.
How does it work in other languages on Functions?
Having implicit imports in one case and explicit in other cases makes it quite confusing, unless we start working with PSModulePath.
PowerShell users will either expect auto loading to just work, wherever they put stuff or not.
One option would be to add the two "modules" locations to PSModulePath and use PowerShell's auto-loading of modules, but this again would have performance implications.
In the end, PowerShell on Functions will never perform as well as the other languages.
There was a problem hiding this comment.
I think we should hold off on doing any auto-import from a shared folder across functions. People can get the desired behavior in a better more predictable way today by using explicit references.
That said, I do think the small fix you mad to make the module resolution handle sub directories is good. Your call on whether you want to scope to just that and get this in, or set that as a different PR. Thanks for your help and patience :)
|
I am currently in Thailand on vacation, so if you have some more questions for this before merging, keep in mind that I'm mostly offline. Just to be clear with this PR: |
|
David, I've merged your other PR. I'm thinking we should close this PR at this point? |
|
woops, missed your comment @mathewc . Closing. Thanks. |
Revert "Python public preview cli update"
This commit should address #896
I'm not really a C# dev, just starting and coming from PowerShell, so hopefully I haven't violated too many C# best practices here ;)
The code will now check for a folder named
modulesin the root of the Function App and search for all available module files, adding results to themodulePathsoutput variable.