-
Notifications
You must be signed in to change notification settings - Fork 193
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
(maint) Make spec tests pass on windows #665
(maint) Make spec tests pass on windows #665
Conversation
18710f6
to
9ca99a2
Compare
I guess this explains why |
9ca99a2
to
253c32f
Compare
@joshcooper before I dig into the spec failures here, do you think this approach is correct? The |
manifests/init.pp
Outdated
@@ -126,6 +126,7 @@ | |||
$wait_for_pxp_agent_exit = undef, | |||
$wait_for_puppet_run = undef, | |||
Array[Puppet_agent::Config] $config = [], | |||
$version_file_path = $facts['os']['family'] ? { 'windows' => 'C:\Program Files\Puppet Labs\Puppet\VERSION', default => '/opt/puppetlabs/puppet/VERSION' } |
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.
On Windows the install dir is user-configurable/relocatable, but there's a core fact you can use https://www.puppet.com/docs/puppet/7/core_facts.html#env-windows-installdir
$version_file_path = $facts['os']['family'] ? { 'windows' => 'C:\Program Files\Puppet Labs\Puppet\VERSION', default => '/opt/puppetlabs/puppet/VERSION' } | |
$version_file_path = $facts['os']['family'] ? { 'windows' => "${facts['env_windows_installdir']}\\VERSION", default => '/opt/puppetlabs/puppet/VERSION' } |
One tricky thing, you'll need to double escape the backslash in the double quoted string (since we're interpolating). I don't know if that syntax works exactly, but something like it should.
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.
You can also use forward slashes on Windows.
Yeah makes sense. I think the failures are because we have to stub out the VERSION file when testing the windows osfamily class. |
I added a mock similar to the one already existing for |
Maybe one option is to not try to fix auto on windows and puppet apply (and file a ticket to fix it). Instead just modify version = if %r{windows}i.match?(default['platform'])
version_dir = fact_on(default, 'env_windows_installdir')
on(default, "cmd /c type '#{version_dir}\\VERSION'").stdout.chomp
else
file_contents_on(default, "/opt/puppetlabs/puppet/VERSION").chomp
end
collection = "puppet#{version.split('.').first}"
pp = "class { 'puppet_agent': package_version => '#{version}', collection => '#{collection}'}" The beaker helper |
Windows requires the current puppet collection to be specified in the puppet_agent class, so that is refactored to be globally specified in the `spec_helper_acceptance`. Additionally, logic and mocking was added to specifiy default install paths for the VERSION file for both linux and windows.
253c32f
to
58901da
Compare
@joshcooper I was able to keep using the |
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.
ah nice!
I'm testing puppet_agent 4.15.0. I think the change in this PR is causing catalog compilation to fail on Windows agents (PE 2019.8.11, Puppet 6.27.0). It appears to be trying to get the content of |
Gah... I forgot the module is using the We might be able to detect which mode we're in based on server variables https://www.puppet.com/docs/puppet/7/lang_facts_builtin_variables#server-variables I think they're omitted in |
So, any news on it? Also getting errors on Windows hosts like: |
Windows requires the current puppet collection to be specified in the puppet_agent class, so that is refactored to be globally specified in the
spec_helper_acceptance
. Additionally, logic was added to specifiy default install paths for the VERSION file for both linux and windows.