-
Notifications
You must be signed in to change notification settings - Fork 194
(PA-5826) Only read Windows VERSION file during puppet apply #677
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
Conversation
ba7758d
to
4a1467e
Compare
4a1467e
to
fb1cd29
Compare
Should fix #665. |
manifests/init.pp
Outdated
@@ -167,6 +167,9 @@ | |||
# The AIO package version and Puppet version can, on rare occasion, diverge. | |||
# This logic checks for the AIO version of the server, since that's what the package manager cares about. | |||
if $package_version == 'auto' { | |||
if $facts['os']['family'] == 'windows' and $facts['puppet_runmode'] == 'user' { | |||
$version_file_path = "${facts['env_windows_installdir']}\\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.
Isn't this a reassignment of the $version_file_path
variable, since it's already been assigned a value from the class parameter list?
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, I’ll fix that. Weird that all the tests passed. Or would it only show up when running a specific puppet-lint check?
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.
puppet-lint does not catch that. you need an rspec-puppet test that hits this if condition and checks for it should compile
.
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 yeah, my test coverage was lacking. I added a test for apply mode on Windows and it triggered the issue:
1) puppet_agent supported Windows x64 environment with auto when applying is expected to contain File[C:\ProgramData\Puppetlabs]
Failure/Error: it { is_expected.to contain_file("#{appdata}\\Puppetlabs") }
Puppet::PreformattedError:
Evaluation Error: Cannot reassign variable '$version_file_path' (file: /home/josh/work/modules/puppetlabs-puppet_agent/spec/fixtures/modules/puppet_agent/manifests/init.pp, line: 171, column: 28) on node xxx
# /home/josh/.rbenv/versions/3.2.2/lib/ruby/3.2.0/benchmark.rb:311:in `realtime'
# ./spec/classes/puppet_agent_osfamily_windows_spec.rb:106:in `block (5 levels) in <top (required)>'
Previously, puppetserver attempted to load the wrong VERSION file when managing `package_version => auto` and compiling catalogs for Windows nodes as can be seen by the new test: Evaluation Error: Error while evaluating a Function Call, Could not find any files from C:\Program Files\Puppet Labs\Puppet\VERSION (file: path/to/init.pp, line: 170, column: 42) This adds a `puppet_runmode` fact. When running as puppet agent, the run mode will be `agent`. When running as puppet apply, it will be `user`. If it's the latter and we're a Windows host, then look for the VERSION file locally. Since the default class parameter is no longer dependent on the agent fact, I regenerated the reference using rake strings:generate:reference
fb1cd29
to
b2cc0eb
Compare
Previously, puppetserver attempted to load the wrong VERSION file when managing
package_version => auto
and compiling catalogs for Windows nodes as can be seen by the new test:This adds a
puppet_runmode
fact. When running as puppet agent, the run mode will beagent
. When running as puppet apply, it will beuser
. If it's the latter and we're a Windows host, then look for the VERSION file locally.