Skip to content

Conversation

@Jaykul
Copy link
Member

@Jaykul Jaykul commented Jan 11, 2020

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
Copy link
Member Author

Jaykul commented Jan 11, 2020

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

@bravo-kernel
Copy link
Contributor

bravo-kernel commented Jan 11, 2020

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
Copy link
Contributor

bravo-kernel commented Jan 11, 2020

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
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
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
Copy link
Member Author

Jaykul commented Jan 14, 2020

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
Copy link
Member Author

Jaykul commented Jan 14, 2020

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
Contributor

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

@bravo-kernel
Copy link
Contributor

bravo-kernel commented Jan 14, 2020

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
Copy link
Member Author

Jaykul commented Jan 14, 2020

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
Copy link
Member Author

Jaykul commented Jan 14, 2020

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

@bravo-kernel
Copy link
Contributor

bravo-kernel commented Jan 14, 2020

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
Copy link
Member Author

Jaykul commented Jan 14, 2020

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
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
Copy link
Member Author

Jaykul commented Jan 14, 2020

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
Contributor

Any news thus far ?

@bravo-kernel
Copy link
Contributor

Friendly ping 💃

@Jaykul Jaykul merged commit 3fe63f8 into master Feb 10, 2020
@Jaykul
Copy link
Member Author

Jaykul commented Feb 10, 2020

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

4 participants