Allow Test-ModuleManifest to work when filename is not specified #8388#8687
Conversation
5994fd0 to
33adf5e
Compare
|
@rjmholt can you please review this PR? |
There was a problem hiding this comment.
This might be a good opportunity to label the true parameter in IsValidFilePath
There was a problem hiding this comment.
Actually, this entire condition really needs to be spun into a bool method. I think it's now too long to be inline here, since it's very hard to read and determine the logic or intention of.
Something like:
...
if (!IsRootModuleValid(module.RootModule))
{
string errorMsg = StringUtil.Format(Modules.InvalidModuleManifest, module.RootModule, filePath);
...
}
...
// All module extensions except ".psd1" are valid RootModule extensions
private static readonly IReadOnlyList<string> s_validRootModuleExtensions = ModuleIntrinsics.PSModuleExtensions
.Filter(ext => !string.Equals(ext, StringLiterals.PowerShellDataFileExtension, StringComparer.OrdinalIgnoreCase))
.ToArray();
/// <summary>
/// Checks whether the RootModule field of a module is valid or not.
/// Valid root modules are:
/// - null
/// - Empty string
/// - A valid non-psd1 module file (psm1, cdxml, xaml, dll), as name with extension, name without extension, or path
/// </summary>
/// <param name="rootModule">The root module specified in the module manifest RootModule field.</param>
/// <returns>True if the root module is valid, false otherwise.</returns>
private static bool HasValidRootModule(PSModuleInfo module)
{
// Empty/null root modules are allowed
if (string.IsNullOrEmpty(module.RootModule))
{
return true;
}
// GAC assemblies are allowed
if (IsValidGacAssembly(module.RootModule))
{
return true;
}
// Check for extensions
string rootModuleExt = Path.GetExtension(module.RootModule);
if (!string.IsNullOrEmpty(rootModuleExt))
{
// Check that the root module's extension is an allowed one
if(!s_validRootModuleExtensions.Contains(rootModuleExt, StringComparison.OrdinalIgnoreCase))
{
return false;
}
// Check the file path of the full root module
return IsValidFilePath(module.RootModule, module, verifyPathScope: true);
}
// We have no extension, so need to check all of them
foreach (string extension in s_validRootModuleExtensions)
{
if (IsValidFilePath(module.RootModule + extension, module, verifyPathScope: true))
{
return true;
}
}
return false;
}I haven't executed or otherwise tested this code, so it might not even compile, but I think it's worth breaking the logic up in this PR.
There was a problem hiding this comment.
Thanks for the advice. From what I understand a root module without an extension is considered valid only if the file path with extension is valid and that means we can't just return true if the root module has no file extension as this code suggest.
There was a problem hiding this comment.
Yes, that makes sense, good catch.
I've edited the code above. Tell me if that makes more sense.
There was a problem hiding this comment.
Yep. I'll rewrite the PR on monday using this. Thanks for the help. :)
There was a problem hiding this comment.
The prevailing comment style tends to be //-prefixed blocks, rather than /* */ regions:
// RootModule can be null, empty string or point to a valid .psm1, .cdxml, .xaml or .dll. Extensions are optional.
// Anything else is invalid.If you make the change below, you probably don't need a comment here at all though.
fe1d002 to
a57506b
Compare
|
@rjmholt Hey :), I have updated the PR using your suggestions. |
There was a problem hiding this comment.
| private static IList<string> validRootModuleExtensions = ModuleIntrinsics.PSModuleExtensions | |
| private static readonly IReadOnlyList<string> s_validRootModuleExtensions = ModuleIntrinsics.PSModuleExtensions |
There was a problem hiding this comment.
nit: I can't do s_ due to CodeFactor not allowing it but I can do _ instead which also corresponds to how other parts of the repo do it.
There was a problem hiding this comment.
@rjmholt ok sounds good. I have just changed it to s_. :)
6dce2ea to
47987be
Compare
|
Hey @daxian-dbw , |
Test-ModuleManifest to work when filename is not specified
|
Hey @rjmholt, |
Test-ModuleManifest to work when filename is not specifiedTest-ModuleManifest to work when filename is not specified #8388
47987be to
7e2a84f
Compare
|
This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. |
|
@pougetat I apologize for us being neglecting this PR. I will quickly go through the changes and merge it if things look good. |
|
@daxian-dbw No worries. Thanks for merging. :) |
|
@pougetat Thank you for the contribution! |
PR Summary
Fixing 'Test-Module requires rootmodule entry to be specified with filename extension'
fixes #8388
PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.[feature]to your commit messages if the change is significant or affects feature tests