-
Notifications
You must be signed in to change notification settings - Fork 469
Change GetModuleFilePaths() to include a modules directory in root of Function App #906
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
Changes from all commits
d6dbef6
96a3616
a478b40
e69dd06
0781847
f1aacf9
7f148f8
f680ed6
32d4726
25f7fa0
e54f4ad
98629d0
9c742aa
ea7f409
da35501
3fb4187
62390bc
eb824c0
bc0b1af
fae756b
733b7ca
4babc58
7bc6206
8d579e0
e996fb8
6b6904c
1eaecd9
55f06a1
d55e774
8d170b1
75e915e
b61a4be
7598e6f
5e757d2
350a413
96b7f62
936f00b
8d97263
1f6179c
c6b1af5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,7 +87,9 @@ private async Task<PSDataCollection<ErrorRecord>> InvokePowerShellScript(Diction | |
System.Management.Automation.PowerShell.Create()) | ||
{ | ||
powerShellInstance.Runspace = runspace; | ||
_moduleFiles = GetModuleFilePaths(_host.ScriptConfig.RootScriptPath, _functionName); | ||
_moduleFiles = FindDuplicateModules(GetModuleFilePaths(_host.ScriptConfig.RootScriptPath, _functionName), _host.ScriptConfig.RootScriptPath, _functionName); | ||
// Remove duplicates from _moduleFiles and only keep the ones closest to the Function in moduleDirectory | ||
|
||
if (_moduleFiles.Any()) | ||
{ | ||
powerShellInstance.AddCommand("Import-Module").AddArgument(_moduleFiles); | ||
|
@@ -261,18 +263,58 @@ internal static List<string> GetModuleFilePaths(string rootScriptPath, string fu | |
{ | ||
List<string> modulePaths = new List<string>(); | ||
string functionFolder = Path.Combine(rootScriptPath, functionName); | ||
string rootModuleDirectory = Path.Combine(rootScriptPath, PowerShellConstants.ModulesFolderName); | ||
string moduleDirectory = Path.Combine(functionFolder, PowerShellConstants.ModulesFolderName); | ||
if (Directory.Exists(moduleDirectory)) | ||
modulePaths.AddRange(AddToModulePaths(rootModuleDirectory, moduleDirectory)); | ||
return modulePaths; | ||
} | ||
|
||
internal static List<string> AddToModulePaths(params string[] directories) | ||
{ | ||
List<string> paths = new List<string>(); | ||
foreach (string directory in directories) | ||
{ | ||
modulePaths.AddRange(Directory.GetFiles(moduleDirectory, | ||
PowerShellConstants.ModulesManifestFileExtensionPattern)); | ||
modulePaths.AddRange(Directory.GetFiles(moduleDirectory, | ||
PowerShellConstants.ModulesBinaryFileExtensionPattern)); | ||
modulePaths.AddRange(Directory.GetFiles(moduleDirectory, | ||
PowerShellConstants.ModulesScriptFileExtensionPattern)); | ||
if (Directory.Exists(directory)) | ||
{ | ||
paths.AddRange(Directory.GetFiles(directory, | ||
PowerShellConstants.ModulesManifestFileExtensionPattern, | ||
SearchOption.AllDirectories)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is the file and code for SearchOption? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kindly also add tests for the following,
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tohling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fabiocav, ah, I missed Directory.GetFiles. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tohling How would you want to handle "duplicates"? Usually I would say "closest to the function" wins. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, added some logic to the tests and In regards to duplicates: |
||
paths.AddRange(Directory.GetFiles(directory, | ||
PowerShellConstants.ModulesBinaryFileExtensionPattern, | ||
SearchOption.AllDirectories)); | ||
paths.AddRange(Directory.GetFiles(directory, | ||
PowerShellConstants.ModulesScriptFileExtensionPattern, | ||
SearchOption.AllDirectories)); | ||
} | ||
} | ||
return paths; | ||
} | ||
|
||
return modulePaths; | ||
internal static List<string> FindDuplicateModules(List<string> foundModules, string rootScriptPath, string functionName) | ||
{ | ||
List<string> moduleFileNames = new List<string>(); | ||
string functionFolder = Path.Combine(rootScriptPath, functionName); | ||
string rootModuleDirectory = Path.Combine(rootScriptPath, PowerShellConstants.ModulesFolderName); | ||
string moduleDirectory = Path.Combine(functionFolder, PowerShellConstants.ModulesFolderName); | ||
|
||
foreach (string entry in foundModules) | ||
{ | ||
moduleFileNames.Add(Path.GetFileName(entry)); | ||
} | ||
|
||
List<string> distinctModuleFiles = moduleFileNames.Distinct().ToList(); | ||
|
||
foreach (string distinctModuleFile in distinctModuleFiles) | ||
{ | ||
string potentialModule = Path.Combine(moduleDirectory, distinctModuleFile); | ||
string result = foundModules.SingleOrDefault(s => s == potentialModule); | ||
if (result != null) | ||
{ | ||
foundModules.Remove(Path.Combine(rootModuleDirectory, distinctModuleFile)); | ||
} | ||
} | ||
return foundModules; | ||
} | ||
|
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mathewc fair enough. If you like to cherry-pick the commits for the subdirectory code then I'm fine with that.
I'm also going to create a PR to remove auto-import altogether as I don't think that this is a great idea in general. This will address #897 then.