Fix parse errors caused by missing dsc resouces

[kapilmb]

Resolves #520

---

This change is

[kapilmb]

A brief summary of the changes introduced by this PR:

• Adds a ResolveDSCModuleDependency switch
• If Invoke-ScriptAnalyzer (ISA) is invoked with the -ResolveDSCModuleDepency switch then
  • Creates a PSSScriptAnalyzer directory in LOCALAPPDATA, if it doesn't exist
  • Creates a link to a temporary folder
  • Adds the temporary path to the PSModulePath environment variable
  • If ISA encouters ModuleNotFoundDuringParse for "Import-DSCResource -ModuleName SomeModule" then
    • Checks if the missing DSC Resource module is present in PSGallery
    • If present, then downloads it to the temporary path
    • Parses again
  • Restore PSModulePath environment variable to its original value

[kapilmb]

@lzybkr @raghushantha @daviwil please review. Thanks!

[lzybkr]

Review status: 0 of 6 files reviewed at latest revision, 18 unresolved discussions.

---

Engine/ScriptAnalyzer.cs, line 1315 [r1] (raw file):

                    bool parseAgain = false;
                    if (moduleHandler != null && errors != null && errors.Length > 0)

Errors will never be null.

---

Engine/Generic/ModuleDependencyHandler.cs, line 24 [r1] (raw file):

        private string localAppdataPath;
        private string pssaAppDataPath;
        private const string symLinkName = "TempModuleDir";

I find this confusing - you aren't using symbolic links, right?

---

Engine/Generic/ModuleDependencyHandler.cs, line 53 [r1] (raw file):

            {
                tempPath
                    = (string.IsNullOrEmpty(value)

IsNullOrWhitespace covers the empty case as well, so this is redundant.

---

Engine/Generic/ModuleDependencyHandler.cs, line 55 [r1] (raw file):

                    = (string.IsNullOrEmpty(value)
                        || string.IsNullOrWhiteSpace(value))
                    ? Environment.GetEnvironmentVariable("TEMP")

Path.GetTempPath () is probably more reliable.

---

Engine/Generic/ModuleDependencyHandler.cs, line 73 [r1] (raw file):

            {
                localAppdataPath 
                    = (string.IsNullOrEmpty(value)

Redundant

---

Engine/Generic/ModuleDependencyHandler.cs, line 75 [r1] (raw file):

                    = (string.IsNullOrEmpty(value)
                        || string.IsNullOrWhiteSpace(value))
                    ? Environment.GetEnvironmentVariable("LOCALAPPDATA")

You should use Environment.GetFolderPath(Environment.SpecialFolder.LocalAppData)

---

Engine/Generic/ModuleDependencyHandler.cs, line 93 [r1] (raw file):

            {
                moduleRepository
                    = (string.IsNullOrEmpty(value)

Redundant

---

Engine/Generic/ModuleDependencyHandler.cs, line 112 [r1] (raw file):

            {
                var leaf
                    = (string.IsNullOrEmpty(value) || string.IsNullOrWhiteSpace(value))

IsNullOrEmpty is redundant

---

Engine/Generic/ModuleDependencyHandler.cs, line 115 [r1] (raw file):

                    ? "PSScriptAnalyzer"
                    : value;
                pssaAppDataPath = Path.Combine(LocalAppDataPath, leaf);

You have a hidden dependency on ordering of assignments with LocalAppDataPath here - it might be better to not use private setters at all and just move this logic to the constructor.

---

Engine/Generic/ModuleDependencyHandler.cs, line 145 [r1] (raw file):

            {
                throw new ArgumentNullException(name);
            }            

You have lots of trailing whitespace in this file.

---

Engine/Generic/ModuleDependencyHandler.cs, line 159 [r1] (raw file):

                    // check if the temp dir exists
                    if (tempModulePath != null

The null check is redundant - Directory.Exists will make the same check.

---

Engine/Generic/ModuleDependencyHandler.cs, line 188 [r1] (raw file):

        {
            tempModulePath = GetPSSATempDirPath();
            Directory.CreateDirectory(tempModulePath);

What happens if this fails, e.g. access denied?

---

Engine/Generic/ModuleDependencyHandler.cs, line 207 [r1] (raw file):

        private string GetTempModulePath(string symLinkPath)
        {
            var symLinkLines = File.ReadAllLines(symLinkPath);

This just looks wrong - read all lines, then forget all but the first? And it's not clear what's in the file you're reading anyway.

---

Engine/Generic/ModuleDependencyHandler.cs, line 306 [r1] (raw file):

                return modulesFound[moduleName];
            }
            var ps = System.Management.Automation.PowerShell.Create();

using ... - PowerShell implements IDisposable

---

Engine/Generic/ModuleDependencyHandler.cs, line 335 [r1] (raw file):

            try
            {
                SaveModule(moduleName);

Is this saved module ever deleted?
It's also not clear if you'll reuse it based on using random file names.

---

Engine/Generic/ModuleDependencyHandler.cs, line 356 [r1] (raw file):

                return;
            }
            var ps = System.Management.Automation.PowerShell.Create();

using ... - PowerShell implements IDisposable

---

Engine/Generic/ModuleDependencyHandler.cs, line 390 [r1] (raw file):

            // right now we handle only the following form
            // Import-DSCResource -ModuleName xActiveDirectory
            if (dynamicKywdAst.CommandElements.Count != 3)

There are other arguments that you need to worry about, e.g. the following is valid
Import-DSCResource -Name xResource -ModuleName foo -ModuleVersion 1.0

---

Engine/Generic/ModuleDependencyHandler.cs, line 422 [r1] (raw file):

        {
            runspace.Dispose();
            RestorePSModulePath();

People forget to call Dispose all the time (like you did). I'd rather see PSModulePath only changed around operations that require it, and restored afterwards.
Alternatively (probably harder to implement), you could rewrite the ast to Import-DSCResource a full path to the module you saved.

---

Comments from Reviewable

[kapilmb]

Engine/Generic/ModuleDependencyHandler.cs, line 24 [r1] (raw file):

Previously, lzybkr (Jason Shirk) wrote…

I find this confusing - you aren't using symbolic links, right?

I am creating a text file named TempModuleDir that contains a path to a directory that contains the downloaded modules, so TempModuleDir will contain something like this: C:\Users\Foo\AppData\Local\Temp\PSSAModule-abcdefgh

I manually create this "symbolic link" file because I couldn't find any c# or powershell api to create a symbolic link

---

Comments from Reviewable

[kapilmb]

Engine/Generic/ModuleDependencyHandler.cs, line 335 [r1] (raw file):

Previously, lzybkr (Jason Shirk) wrote…

Is this saved module ever deleted?
It's also not clear if you'll reuse it based on using random file names.

The module is saved in directory "$temp\pssamodule-abcdefg" and the path "$temp\pssamodule-abcdefg" is saved in a file named "TempModuleDir" located in "$localappdata\psscriptanalyzer".
So, the next time when ModuleDependencyHandler class is instantiated, it will look into TempModuleDir file to locate the temporary directory (in this case "$temp\pssamodule-abcdefg"). This temporary directory now contains all the downloaded modules and hence scriptanalyzer doesn't have to download the modules again.

---

Comments from Reviewable

[kapilmb]

Engine/Generic/ModuleDependencyHandler.cs, line 188 [r1] (raw file):

Previously, lzybkr (Jason Shirk) wrote…

What happens if this fails, e.g. access denied?

In that case the constructor will throw an exception as this method is called only from the constructor. We don't know how to handle such a failure and hence we let the constructor fail.
What would be a better approach to handle this?

---

Comments from Reviewable

[kapilmb]

Engine/Generic/ModuleDependencyHandler.cs, line 188 [r1] (raw file):

Previously, lzybkr (Jason Shirk) wrote…

What happens if this fails, e.g. access denied?

In that case the constructor will throw an exception as this method is called only from the constructor. We don't know how to handle such a failure and hence we let the constructor fail.
What would be a better approach to handle this?

---

Comments from Reviewable

[joeyaiello]

• The parameter name should probably hint at the fact that files are being downloaded and placed on the local system. Something like -DownloadDscDependency or -SaveDscDependency (I'm totally open to other options). Since this is using Save-Module, no warning gets thrown on Untrusted repositories, and people might get weird about stuff getting dropped down without them really knowing.
• If DSC stays in the parameter name, it should be Dsc per Pascal casing (from these design guidelines, which btw is what we should reconcile with the community guide at some point).
• All this PSModulePath business still freaks me out. From this, it looks like only the 3 parameter overload modifies anything outside the current process, but I would very much appreciate a CR on the PSModulePath code from a "guru".

The rest of it LGTM. :)

[kapilmb]

• Though we download the dependency to a temp folder, we do it only during the first instance when pssa encounters parse error. During pssa's next invocation, if the downloaded module is present in the temporary path it doesn't download it. Hence, specifying download in the switch name indicates that pssa downloads the module every time it is invoked with the switch.
• Will change DSC to Dsc
• PSModulePath will be changed right before pssa starts analyzing the files and will be restored right after the analysis is over.

Previously, joeyaiello (Joey Aiello) wrote…

• The parameter name should probably hint at the fact that files are being downloaded and placed on the local system. Something like -DownloadDscDependency or -SaveDscDependency (I'm totally open to other options). Since this is using Save-Module, no warning gets thrown on Untrusted repositories, and people might get weird about stuff getting dropped down without them really knowing.
• If DSC stays in the parameter name, it should be Dsc per Pascal casing (from these design guidelines, which btw is what we should reconcile with the community guide at some point).
• All this PSModulePath business still freaks me out. From [this](https://msdn.microsoft.com/en-us/library/system.environment.setenvironmentvariable(v=vs.110).aspx, it looks like only the 3 parameter overload modifies anything outside the current process, but I would very much appreciate a CR on the PSModulePath code from a "guru".

The rest of it LGTM. :)

---

Comments from Reviewable

[joeyaiello]

We should bring in @JKeithB for the naming of the parameter.