gaelcolas / Sampler

Module template with build pipeline and examples, including DSC elements.
MIT License
172 stars 42 forks source link

Make $BuiltModuleSubdirectory configurable in build.yaml #299

Closed gaelcolas closed 3 years ago

gaelcolas commented 3 years ago

Currently it seems to work fine when passed by argument. It would be great it we could make it work in the config file.

gaelcolas commented 3 years ago

fixed with #306 Does the existing build.ps1 need updating on each repo?

gaelcolas commented 3 years ago

Also, look at testing delta for your PR for the Set-SamplerVariable (-6%), please try to correct that.

gaelcolas commented 3 years ago

And now we need to update DscResource.Test to support this. Currently getting:

Build FAILED. 18 tasks, 1 errors, 0 warnings 00:00:11.9031330
Get-Item: C:\src\nxtools\output\RequiredModules\DscResource.Test\0.15.1\tasks\Invoke_HQRM_Tests.build.ps1:113:37
Line |
 113 |  … ModuleManifest = [string](Get-Item -Path $builtModuleManifest).FullNa …
     |                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Cannot find path 'C:\src\nxtools\output\nxtools' because it does not exist.
NicolasBn commented 3 years ago

Indeed, each build.ps1 need to be updated. If no, the value of PSModulePath will be defined to output folder.

I'll update tests for Set-SamplerVariable.

For DscResource.Test, should we open a new issue ?

gaelcolas commented 3 years ago

DscResource.Test, should we open a new issue ?

Yes please, those issues are kinda expected. Probably need to check Sampler.GithubTask, and other modules with task (DocGenerator, Test...).

gaelcolas commented 3 years ago

oh, and I can see that the output/ is still set as one $env:PSModulePath variable which should not be the case. When using subdirectory, only output/RequiredModules and output/subdirectory should be in the PSModulePath...

NicolasBn commented 3 years ago

When using subdirectory, only output/RequiredModules and output/subdirectory should be in the PSModulePath...

Yes, it's why I moved the definition of $env:PSModulePath in build.ps1 from Begin to Process bloc ;) https://github.com/NicolasBn/Sampler/blob/594da9bd6b4905f578ce838743f1b5314da24ac3/Sampler/Templates/Build/build.ps1#L229

gaelcolas commented 3 years ago

ah, I know what went wrong... As you rightly moved the block in the build.ps1 from begin to process block... you forgot to remove the bit in the template (which is what I used to try it out).

Please also update the build.ps1

NicolasBn commented 3 years ago

Oh Okay ! I will do !

NicolasBn commented 3 years ago

DscResource.Test, should we open a new issue ?

Yes please, those issues are kinda expected. Probably need to check Sampler.GithubTask, and other modules with task (DocGenerator, Test...).

After checking, DscResource.Test has no need to change. With my last PR in the repo, the tasks are based on Set-SamplerTaskVariable.ps1 script.

But Sampler.GitHubTasks need to be changed : https://github.com/gaelcolas/Sampler.GitHubTasks/blob/fd5cf1e0544a4c900e577b87e69181b483a11366/.build/tasks/New-Release.GitHub.build.ps1#L58

Same to DscResource.DocGenerator : https://github.com/dsccommunity/DscResource.DocGenerator/blob/3a8ba1112e5cba97e3daf0b1d72453f547f5186a/source/tasks/Generate_Conceptual_Help.build.ps1#L88

https://github.com/dsccommunity/DscResource.DocGenerator/blob/3a8ba1112e5cba97e3daf0b1d72453f547f5186a/source/tasks/Generate_Wiki_Content.build.ps1#L91

https://github.com/dsccommunity/DscResource.DocGenerator/blob/3a8ba1112e5cba97e3daf0b1d72453f547f5186a/source/tasks/Publish_GitHub_Wiki_Content.build.ps1#L112

johlju commented 3 years ago

@NicolasBn is this closed with the merge of PR #306?

NicolasBn commented 3 years ago

No, I forget to remove some lines in build.ps1 template.

And I must to add some pester tests to cover my changes.

It's already written. I just need to create a new PR.

johlju commented 3 years ago

Cool. Then we keep this open.

NicolasBn commented 3 years ago

Finally, I think I found a bug in my code. I based it on $PSboundParameter to find if BuiltModuleSubDirectory parameter is passed to the task. But $PSBoundParameter scope is to Set-SamplerTaskVariable script, not the task.

So when BuiltModuleSubDirectory is set in the build configuration file and the BuiltModuleSubDirectory parameter is used on build.ps1, the configuration file has the priority. For me, the parameter ought to have the biggest priority.

johlju commented 3 years ago

Yes, parameter should have priority. it cannot use $PSBoundParameters since it does not contain any default parameter that might be set by property. Must use Get-Variable to check if the parameter was set. Then if property of the parameter got the variable from parent scope or environment variable it will be set, but it is not shown in $PSBoundParameters.

NicolasBn commented 3 years ago

So, I just need to check the value of $BuiltModuleSubDirectory, the value is herited from the task scope. Is not necessary to use Get-Variable ?

And if no values set in $BuilfModuleSubDirectory or $BuildInfo['BuiltModuleSubDirectory'], I use a null value. What do you think about this ?

johlju commented 3 years ago

Yes, sounds correct. The script is dot-sourced into the task so it is in the same scope, so we should be able to use the variable directly without Get-Variable.

gaelcolas commented 3 years ago

The problem with not using Get-Variable is when we use Set-StrictMode -Version Latest. Sure, it's not in use at the moment, but it's probably best to assume we'll try to be more strict going forward.

NicolasBn commented 3 years ago

OK, I see, but $BuiltModuleSubDirectory will not be the only variable impacted by Set-StrictMode -Version Latest.

Should we open a new issue to discuss about that ?

I will create a PR with Get-Variable for $BuilModuleSubDirectory.

johlju commented 3 years ago

I think we should run the unit test with strict mode, then the tests will enforce that for us, and let us not have the strict statement when we run the code in production

I agree with @NicolasBn that we should open an issue to track this. But @NicolasBn we could fix this one so that there is one less we need to fix later. 😊

NicolasBn commented 3 years ago

Understood ! I tested to set strict mode directly at the beginning of task :

Task Build_ModuleOutput_ModuleBuilder {
    Set-StrictMode -Version Latest -Debug
    # Get the vales for task variables, see https://github.com/gaelcolas/Sampler#task-variables.
    . Set-SamplerTaskVariable -AsNewBuild

No exception was generated with $BuiltSubFolderDirectory. And with Get-Variable I can determine which scope I must to use in -Scope parameter. No value was found.

johlju commented 3 years ago

And with Get-Variable I can determine which scope I must to use in -Scope parameter. No value was found.

@NicolasBn I'm not following you. Do you mean you get a value when using $BuiltSubFolderDirectory, but not with Get-Variable -Name 'BuiltSubFolderDirectory' -ValueOnly?

NicolasBn commented 3 years ago

To resume : I got the value when I used BuiltSubFolderDirectory and Get-Variable -Name 'BuiltSubFolderDirectory' -ValueOnly

But when I added -Scope to Get-Variable, that didn't work. The scoping isn't my preference in PowerShell. I might be wrong in my way to use -Scope parameter.

johlju commented 3 years ago

Not sure you need to use scope parameter since you want the variable in the same scope. The script are dot sourced in the same scope as the task parameters. Or am missing something?

NicolasBn commented 3 years ago

No, you are right. But what's the benefit to use Get-Variable -Name 'BuiltSubFolderDirectory' -ValueOnly rather than $BuiltSubFolderDirectory?

The dot sourcing is really impacted by Strict Mode?

johlju commented 3 years ago

You did tests with strict and it didn’t impact so we could then just use BuiltSubFolderDirectory.