Skip to content

Conversation

eimlav
Copy link
Contributor

@eimlav eimlav commented Jun 28, 2019

No description provided.

@eimlav eimlav added the feature label Jun 28, 2019
@eimlav
Copy link
Contributor Author

eimlav commented Jun 28, 2019

if Puppet::Util::Platform.windows?
require Pathname.new(__FILE__).dirname + '../../../../' + 'puppet/type/acl/ace'
require 'puppet/util/windows/security'
require 'win32/security'
end

The reason for this change is that by putting everything inside the if block, YARD is unable to parse the comments of any of the methods. I tried a number of different things to remove this if block however every time it would result in errors. By applying it only on the requires, the tests pass fine however I'm not sure if this is the best solution. Feel free to give some other ideas of better alternatives.

Copy link
Contributor

@RandomNoun7 RandomNoun7 left a comment

Choose a reason for hiding this comment

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

I see now that I should have commented each of these one line lower to make the review comments easier to see in this view. Sorry about that.

@glennsarti
Copy link
Contributor

By applying it only on the requires, the tests pass fine however I'm not sure if this is the best solution. Feel free to give some other ideas of better alternatives.

Strictly speaking you should probably add a guard clause to every single public method e.g.

def do_something()
  raise "Only Supported on Windows" unless Puppet::Util::Platform.windows?
  ...
end

However...this whole if thing could be a hangover from the Puppet 3 days and the tests may be testing the wrong thing?

@eimlav
Copy link
Contributor Author

eimlav commented Jul 1, 2019

@glennsarti I believe the errors are to do with Windows platform specific gem dependencies that do not work on nix based systems (i.e. the gems being required in the file). The acceptance test suite also contains similar if clauses for requiring gems from puppet/util/windows/.... For now the proposed solution appears to work in adhoc however finding a cleaner solution is something that should be investigated in the future.

@RandomNoun7
Copy link
Contributor

Manually tested the module with a Puppet master deliberately trying to trigger an explosion. Even with no if clause it all, the module did not cause an error during catalog compilation, and it functioned as expected once the code ran on the Windows node.

@RandomNoun7 RandomNoun7 merged commit 9c7f45b into puppetlabs:master Jul 1, 2019
@eimlav eimlav deleted the modules-9304 branch July 2, 2019 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants