-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(MODULES-5845) Add DSC Resource ModuleSpecification support #29
(MODULES-5845) Add DSC Resource ModuleSpecification support #29
Conversation
0316042
to
059bd22
Compare
705ded8
to
7472837
Compare
assert_match(/^v2#{test_file_contents}/, result.stdout, 'PuppetFakeResource File contents incorrect!') | ||
end | ||
end | ||
# if this file exists, 'absent' didn't work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want to actually make sure the file exists here and that it contains the right data. The assertion works because the v2 type appends the string "v2" to the front of the output.
on(agent, "cat /cygdrive/c/#{fake_name}", :acceptable_exit_codes => [0]) do |result|
assert_match(/^v2#{test_file_contents}/, result.stdout, 'PuppetFakeResource File contents incorrect!')
end
README.md
Outdated
* [Using EmbeddedInstance or CimInstance](#using-dsc-resources-with-puppet) | ||
* [Specifying a DSC Resource version](#specifying-a-dsc-resource-version) | ||
* [Using PSCredential or MSFT_Credential](#using-pscredential-or-msft_credential) | ||
* [Using EmbeddedInstance or CimInstance](#using-embeddedinstance-or-ciminstance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch on the anchors
Oh oh... except that the third should actually be:
* [Using CimInstance](#using-ciminstance)
Can fix that up in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like further up description
has the same problem - we can fix in a follow-up.
And further down Learn More About DSC
as well.
README.md
Outdated
dsc {'iis_server': | ||
dsc_resource_name => 'WindowsFeature', | ||
dsc_resource_module => { | ||
name => 'C:\Windows\system32\WindowsPowershell\v1.0\Modules\PsDesiredStateConfiguration\PSDesiredStateConfiguration.psd1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will test locally as well, but is it the path to the PSD1
that's used, or the path to the directory? Or both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like DSC allows you to use a parent directory instead of the PSD1, but then that allows it to find the wrong thing.
I created a gist demonstrating all the various scenarios for side-by-side versions of the same module at:
https://gist.github.com/Iristyle/92a722ee0bfe61aa7ee5ec53dbf65b4a
Fully qualified path + version kind of doesn't make sense to me, given the above gist shows that the path already has to point to the specific version when side-by-side DSC modules are in play. And when the parent directory of all the side-by-side modules was specified, it used the wrong version and didn't generate an error!
If the example above aligns with the system I'm testing, the PSD1 has 1.1
in it, which makes it simpler for an end user to just specify dsc_resource_module
as a string, unless they really need it there for documentation purposes?
dsc_resource_module => 'C:\Windows\system32\WindowsPowershell\v1.0\Modules\PsDesiredStateConfiguration\'
Basically I'm saying I wouldn't recommend users do this given it's more to put in a manifest than is necessary and has the potential to deviate from expectations if a user puts directory instead of PSD1 file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, using your example with PSDesiredStateConfiguration
the behavior is a little different. It won't load an invalid version, but not sure if that's because it's not one of the side-by-side style installations like I used above?
Trying to load an invalid version 1.5 using the parent directory fails as you would expect:
PS C:\Users\Administrator> Invoke-DscResource -ModuleName @{ ModuleName = "C:\Windows\System32\WindowsPowerShell\v1.0\Modules\PSDesiredStateConfiguration"; ModuleVersion = '1.5' } -Name WindowsFeature -Property @{ Ensure = 'present'; Name = 'Web-Server' } -Method Set
Invoke-DscResource : Resource WindowsFeature was not found.
At line:1 char:1
+ Invoke-DscResource -ModuleName @{ ModuleName = "C:\Windows\System32\W ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : NotSpecified: (:) [Invoke-DscResource], ArgumentException
+ FullyQualifiedErrorId : System.ArgumentException,Microsoft.PowerShell.DesiredStateConfiguration.Commands.InvokeDscResourceMethodCommand
And using the correct 1.1 succeeds in executing as you would expect
PS C:\Users\Administrator> Invoke-DscResource -ModuleName @{ ModuleName = "C:\Windows\System32\WindowsPowerShell\v1.0\Modules\PSDesiredStateConfiguration"; ModuleVersion = '1.1' } -Name WindowsFeature -Property @{ Ensure = 'present'; Name = 'Web-Server' } -Method Set
WARNING: [E3HEJGHNN47UI95]: [[WindowsFeature]DirectResourceAccess] Failed to start automatic updating for installed components. Error: 0x80070422
RebootRequired
--------------
False
Note I'm not fully qualifying the PSD1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with how we do it in the dsc module, it has to be the fully qualified path to the psd1 with the psd1 specified, not just the module directory for it to work.
It was a mistake to document using the version as well as the path, as specifying the path is only really useful if the DSC Resource is not in the default paths because you can't rely on the engine to find it. The version is immaterial at this point.
Instead of investigating the vagaries of how Invoke-DscResource, I think it better we stick to documenting what the source docs show (name and version only) and leave it at that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, +1 ... that's basically what I was saying above. This is a weird / confusing usage, and we're probably better off ignoring it.
it "should not allow nil" do | ||
expect { | ||
resource[:name] = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whooooops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need some changes to get acceptance going - I've PR'd your PR at jpogran#1.
908342c
to
4bc1dd3
Compare
expect_failure('Cannot yet execute PuppetFakeResource 2.0 until MODULES-5845 implemented') do | ||
assert_match(/^v2#{test_file_contents}/, result.stdout, 'PuppetFakeResource File contents incorrect!') | ||
end | ||
# if this file exists, 'absent' didn't work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you didn't carry over the comment from the prior version / in my fixup commit jpogran@fdc8d83#diff-8ed1e98461c1f6d1b58dcb66de5b2427R118
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor change to make in the test comments on tests/acceptance/tests/dsc_type/multiple_dsc_resources_in_PSModulePath.rb ... then this is ready to go
f70e6e7
to
1117c6a
Compare
This commit changes `dsc_resource_module_name` to `dsc_resource_module`, and updates it to accept both a string or a hash with two specific key/value pairs, which loosely follows the `ModuleSpecification` format of the `Invoke-DscResource` `ModuleName` parameter. A user can now specify either the name of the DSC Resource Module *or* the name of the DSC Resource Module and the version of the module to use. The name of the DSC Resource module can either be the name of the module or the physical path to the module. This commit updates all doc and acceptance test references, adds unit tests for the type validation, as well as adding an example for usage in the readme.
1117c6a
to
fd78f60
Compare
This commit changes
dsc_resource_module_name
todsc_resource_module
, and updates it to accept both a string or a hash with two specific key/value pairs