Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Make build.psd1 optional and fix bugs in 1.5.2#89

Merged
Jaykul merged 17 commits into
masterPoshCode/ModuleBuilder:masterfrom
feature/NoBuildDataPoshCode/ModuleBuilder:feature/NoBuildDataCopy head branch name to clipboard
Feb 10, 2020
Merged

Make build.psd1 optional and fix bugs in 1.5.2#89
Jaykul merged 17 commits into
masterPoshCode/ModuleBuilder:masterfrom
feature/NoBuildDataPoshCode/ModuleBuilder:feature/NoBuildDataCopy head branch name to clipboard

Conversation

@Jaykul

@Jaykul Jaykul commented Jan 11, 2020

Copy link
Copy Markdown
Member

Fixes #87 by not using ModuleManifest explicitly anymore
Fixes #88 by not loosing the value of the prefix
Address #44 because #87 was so darn close. The build.psd1 is optional

I don't think this will break anything, but I'd like some reassurance.
If there's any doubt, I'll do a 2.0.0-alpha01 and we can beg people to test it.

Regardless, even if nobody finds any bugs, I'm willing to bump major because of the build.psd1 change if y'all think it's necessary

@Jaykul

Jaykul commented Jan 11, 2020

Copy link
Copy Markdown
Member Author

Wow, that test run was surprising. These work on my box!
I'll have to try again tomorrow. Sorry.

@bravo-kernel

bravo-kernel commented Jan 11, 2020

Copy link
Copy Markdown
Contributor

Either one is fine with me and I'm available to test whatever is needed, just let me know.

IMO a major version bump would only be justified if this introduces a breaking change.

If we are going 2.x perhaps we can use that to consider introducing more breaking changes. I don't know if there are on the todo-list here but for example... changing the default upper-cased directories to lower-case (e.g. Source to source, Private to private). Would IMO better accommodate platform-agnostic use besides resulting in properly sorted Github directory views.

image

@bravo-kernel

bravo-kernel commented Jan 11, 2020

Copy link
Copy Markdown
Contributor

ps: the failing build is probably due to the artifact.

1.5.2 produces an artifact without the module name as root folder inside the zip file (instead version number is now root folder inside the zip). This broke my ci as well.

@gaelcolas

Copy link
Copy Markdown
Contributor

I agree with @bravo-kernel 's statement regarding breaking change. 1.5.2 was already one for us so... We're still pinned to 1.0.0, the issue with publish module's different interpretation of semver is a pain.

But while we're here, I'd like to suggest a different way to detect module manifest (to deduct the module name). I found that finding psd1s that were mostly valid with test-modulemanifest (ignoring specific errors such as rootmodule NotFound & no version set) is a more reliable approach.
Guessing from the parent folder's name is not reliable when some tools (azDO) git fetch in a dir that's just the first later than it should be...

@bravo-kernel

Copy link
Copy Markdown
Contributor

@Jaykul are you confused about the statement or the fact that the artifacts have changed?

I thought Get-Module would error if it could not parse the manifest. Instead it returns an empty ModuleInfo object.

This wrapper will correct that while preserving the old error handling...
After discussion on #89 it seems this will "just work" in more scenarios, without breaking things that already work.
@Jaykul

Jaykul commented Jan 14, 2020

Copy link
Copy Markdown
Member Author

Yeah, I'm not sure when that changed -- or how!

There's code in the Build-Module step in the Azure pipeline to make sure the name shows up:

# Build-Module currently doesn't guarantee the module name will be in the output
$manifest = Import-LocalizedData -Base $module.Directory.FullName -File $module.Name
$destination = Join-Path "${{ parameters.destination }}" ([IO.Path]::GetFileNameWithoutExtension( $manifest.Path ))

Build-Module -SourcePath $module -Destination $destination -SemVer "${{ parameters.semVer }}" -Verbose

You can see from the build logs that it worked on 1.1.5 and did not work on 1.5.3.

But as far as I can tell Build-Module-step.yml hasn't changed to speak of.

EDIT:

OMG. I just realized why this happened. I changed the build.psd1 to use a different alias for SourcePath (ModuleManifest) instead of Path -- but the pipeline yaml has Path hardcoded.

@Jaykul

Jaykul commented Jan 14, 2020

Copy link
Copy Markdown
Member Author

You're definitely right that's why it's failing too. I see now that the failed tests show errors coming from ModuleBuilder 1.5.2, not 1.5.3

@bravo-kernel

Copy link
Copy Markdown
Contributor

That's great news, once identified... 👍

@bravo-kernel

bravo-kernel commented Jan 14, 2020

Copy link
Copy Markdown
Contributor

I hope I'm not starting to annoy you folks but this was the main reason I once suggested moving all CI templates over to this repository. Cross-repo changes become near-impossible to detect using shared templates and will (al)most certainly break "search and replace"/refactoring.

Perhaps this can be reconsidered. If wanted, I can move them right into here.

@Jaykul

Jaykul commented Jan 14, 2020

Copy link
Copy Markdown
Member Author

It would still have broken even if all the build files were copied in here. The only difference is that if they were in here, I would have just changed the hardcoded value in the yaml file just now instead of the psd1 -- meaning I would then encounter the same problem in every other repo one at a time as I touch them.

I'm not convinced 😛

Honestly, if including it as a subrepo would work, I would be happy to do that (it would solve both problems). But the last time I tested, Azure's build system can't handle it -- or at least, you can't use yaml files from the sub-repo.

I guess I could include it as a subrepo and still reference the build files from the remote -- that would let you see the problems. But that might cause confusion because we'd have to manage pinning the commit of the other repo twice...

@Jaykul

Jaykul commented Jan 14, 2020

Copy link
Copy Markdown
Member Author

Bah. I forgot. The bug in 1.5.2 prevents using 1.5.2 to build it with Path -- LOL

@bravo-kernel

bravo-kernel commented Jan 14, 2020

Copy link
Copy Markdown
Contributor

Agreed that the error would still have occurred. I'm convinced it would have been found sooner though, using simple repo search. Anyway, it don't really matter much to me, as long as the builds are succeeding I'm happy, thanks for diving into this 💃

Off to work

This one handles ModuleManifest and SourcePath as well as Path
@Jaykul Jaykul force-pushed the feature/NoBuildData branch from 4dcbc5b to 55ce9c8 Compare January 14, 2020 07:21
@Jaykul

Jaykul commented Jan 14, 2020

Copy link
Copy Markdown
Member Author

There we go. That's much better. I'm sure I've broken something that's not covered by tests, but at least now you can try it?

@bravo-kernel

Copy link
Copy Markdown
Contributor

Of course but I wonder how to test your patched version against/using my CI as that will probably just download 1.5.2 from the gallery.

@Jaykul

Jaykul commented Jan 14, 2020

Copy link
Copy Markdown
Member Author

I could push it to the gallery as a pre-release, but first I want a little more confirmation. I'm using it at work today, seems ok so far.

@bravo-kernel

Copy link
Copy Markdown
Contributor

Any news thus far ?

@bravo-kernel

Copy link
Copy Markdown
Contributor

Friendly ping 💃

@Jaykul Jaykul merged commit 3fe63f8 into master Feb 10, 2020
@Jaykul

Jaykul commented Feb 10, 2020

Copy link
Copy Markdown
Member Author

Ok, fine. I've published it as 1.6.0-beta since nobody has reported either success or failure with the PR build.

@Jaykul Jaykul deleted the feature/NoBuildData branch February 12, 2020 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Version 1.5.2 doesn't copy in the prefix file Version 1.5.2 cannot read the manifest file

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.