Crescendo icon indicating copy to clipboard operation
Crescendo copied to clipboard

Allow for module manifest adaptations

Open tbergstedt opened this issue 3 years ago • 10 comments

When using the command Export-CrescendoModule to generate an updated module, the module manifest (.psd1) file is also rewritten from scratch. This file often contains customizations and adaptations to the module that are outside of the scope for Crescendo: e.g. custom formats, default command prefixes. All such adaptations must then be re-inserted from scratch.

I can understand that Crescendo maintains "full ownership" of the .psm1 part of the module, but the manifest needs to be customizable at time of export, e.g. through parameters. Either that, or one should be given the option to keep it unchanged.

tbergstedt avatar Dec 15 '21 14:12 tbergstedt

+1 to this issue.

My custom module manifests getting overwritten is the primary reason why my pipelines are pinned to Crescendo 0.4.1, because otherwise I'd have to rewrite them to back up and restore my custom module manifest every time the module is built.

ethanbergstrom avatar Jul 23 '22 13:07 ethanbergstrom

@tbergstedt and @ethanbergstrom - Thank you for reporting this issue. We are investigating the best implementation to handle this - I will provide our thinking here as the investigation progresses.

theJasonHelmick avatar Aug 09 '22 23:08 theJasonHelmick

@tbergstedt @ethanbergstrom

It looks like this is a incredibly tricky problem that I don't think I can address by inspecting and then rewriting the manifest (the variability in what a manifest can contain can't be created with New-ModuleManifest). However, it would certainly be possible to not create a manifest at all. How about a switch -NoUpdateManifest and the manifest will not be overwritten. This means that the burden would fall on the user to update versions, etc. Behaviorally, if you use -Force -NoUpdateManifest I would rewrite the .psm but not the .psd1. If you use -Force only, I'll rewrite both. That makes this a non-breaking change as well :).

Any attempt to rewrite the manifest with string reading/writing is apt to be incredibly fragile and bug laden. It's the reason I went with New-ModuleManifest in the implementation (that and all the commentary it makes). If PS had an Update-ModuleManifest I might be able to use it, but it's a really tricky problem. I tried implementations in few ways;

  1. an object approach read the manifest with Import-PowerShellDataFile, then package up new-modulemanifest arguments

  2. dotsource the manifest, spelunk the object and package up arguments to new-modulemanifest

  3. parse the file, spelunk the AST and package up arguments to new-modulemanifest while I can read the data just fine, it boils down toNew-ModuleManifest doesn't provide for the allowed variability of a manifest contents.

No1 failed immediately because you can have branch logic for some elements so you can't even import the psd1 with import-powershellDataFile No2 failed immediately because you're left with the result of the branch logic not the code of the branch No3 was looking pretty good, but I couldn't convince new-modulemanifest to take my branch logic as an argument and you can still put stuff in a manifest which you can't express as an argument to new-modulemanifest.

JamesWTruher avatar Aug 20 '22 00:08 JamesWTruher

and if you have a different suggestion for the parameter name, let me know.

JamesWTruher avatar Aug 20 '22 01:08 JamesWTruher

I get the problem, and that sounds like a reasonable workaround. And I'm not that familiar with the Powershell grammar to think of any better parameter name. But since I don't know what the Export-CrescendoModule usually want to enter into the manifest file (apart from the obvious .psm1 reference, etc.), I think it would be helpful if that was described in the help text for the parameter. E.g. "When using this parameter, be sure to check that the following items still are correct in your manifest file: ..."

Does that sound doable?

tbergstedt avatar Aug 20 '22 11:08 tbergstedt

I'm also good with a simple flag to not overwrite an existing manifest.

As far as naming the parameter, I don't have any strong opinions, but NoClobber is a standard PowerShell parameter.

However, that is intended for a cmdlet that only writes a single resource, whereas here we're not wanting to overwrite just a piece of that resource (the manifest of the module to export), so perhaps NoClobberManifest?

ethanbergstrom avatar Aug 20 '22 11:08 ethanbergstrom

NoClobberManifest it is as for the parameter help - I'm thinking a more pithy Do not overwrite the module manifest and we'll update the documentation to be more expansive.

as an aside, I've never been a fan of "clobber" as I've thought it to be not as universal as other terms, but it's not a strong aversion

JamesWTruher avatar Aug 20 '22 17:08 JamesWTruher

Fair enough.

tbergstedt avatar Aug 20 '22 19:08 tbergstedt

Thanks @JamesWTruher for the update. I also vote for -noClobberManifest

theJasonHelmick avatar Aug 22 '22 20:08 theJasonHelmick

fixed with #171

JamesWTruher avatar Aug 23 '22 21:08 JamesWTruher

Thank you! Fixed/closed - in release: https://www.powershellgallery.com/packages/Microsoft.PowerShell.Crescendo/1.1.0-RC1

theJasonHelmick avatar Aug 08 '23 16:08 theJasonHelmick