Skip to content
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

Merged

Conversation

tvpartytonight
Copy link
Contributor

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.

@tvpartytonight tvpartytonight requested a review from a team as a code owner August 31, 2023 18:01
@kenyon
Copy link
Contributor

kenyon commented Aug 31, 2023

I guess this explains why auto doesn't seem to be working on Windows for us, ending up with various agent versions on Windows.

@tvpartytonight
Copy link
Contributor Author

@joshcooper before I dig into the spec failures here, do you think this approach is correct? The bundle exec rake beaker passes on windows with these changes.

@@ -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' }
Copy link
Contributor

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

Suggested change
$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.

Copy link
Contributor

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.

@joshcooper
Copy link
Contributor

Yeah makes sense. I think the failures are because we have to stub out the VERSION file when testing the windows osfamily class.

@tvpartytonight
Copy link
Contributor Author

I added a mock similar to the one already existing for /opt/puppetlabs/puppet/VERSION, but it doesn't fix the unit test failure.

@joshcooper
Copy link
Contributor

joshcooper commented Sep 1, 2023

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 class_spec to specify the package_version and collection when applying the class, something like this in before(:all):

      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 file_contents_on doesn't seem to like paths with spaces when it calls powershell, so you can shell out to cmd instead and use the builtin type command (similar to cat). But you have to use backslashes in the path otherwise cmd thinks it's an command line switch.

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.
@tvpartytonight
Copy link
Contributor Author

@joshcooper I was able to keep using the auto value, I realized I just wasn't mocking enough previously.

Copy link
Contributor

@joshcooper joshcooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah nice!

@joshcooper joshcooper merged commit e80c795 into puppetlabs:main Sep 5, 2023
17 checks passed
@AriaXLi AriaXLi added the maintenance Maintenance chores are typically excluded from changelogs label Sep 20, 2023
@kenyon
Copy link
Contributor

kenyon commented Sep 27, 2023

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 "${facts['env_windows_installdir']}\\VERSION" from the primary server, which of course is not running Windows.

@joshcooper
Copy link
Contributor

It appears to be trying to get the content of "${facts['env_windows_installdir']}\VERSION" from the primary server, which of course is not running Windows.

Gah... I forgot the module is using the file function... Right so when running puppet apply it will retrieve the VERSION file from the currently running agent. But when puppetserver is compiling a catalog for a windows node, the file function will fail. I think we need to conditional logic to the module.

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 puppet apply

@Rewerson
Copy link

So, any news on it? Also getting errors on Windows hosts like:
Error 500 on SERVER: Server Error: Evaluation Error: Error while evaluating a Function Call, Could not find any files from C:\Program Files\Puppet Labs\Puppet\VERSION (file: /etc/puppetlabs/code/environments/production/modules/puppet_agent/manifests/init.pp, line: 170, column: 42)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintenance chores are typically excluded from changelogs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants