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

(MODULES-1174) Puppet 3.7 compatibility #38

Conversation

Iristyle
Copy link
Contributor

  • Puppet 3.7 refactored underlying Win32 API code by changing gems and
    refactoring classes. The windows-pr gem was removed which provides
    a number of methods and constants through mixins to the
    Puppet::Util::Windows::Security class. A number of global constants
    are also defined in the namespaces ::Windows::File and
    ::Windows::Security.
  • Furthermore, some methods that used to be defined on
    Puppet::Util::Windows::Security are now in Puppet::Util::Windows::SID
  • Restructure the code to prefer Puppet 3.7 locations with a graceful
    fallback solution to 3.6.x with old gems, etc.

@Iristyle
Copy link
Contributor Author

FYI @ferventcoder I'm getting 3 fails against Puppet 3.6.2 in the tests for autorequires:
https://github.com/puppetlabs/puppetlabs-acl/blob/master/spec/unit/type/acl_spec.rb#L271-L281

I'm wondering if these tests are broken since the 3.6 release or something?

This is what I see in my catalog / reqs for the failing test

=> #<Puppet::Resource::Catalog:0x40154e8
catalog
 @aliases={},
 @classes=[],
 @downstream_from={},
 @environment="none",
 @environment_instance=none,
 @host_config=true,
 @in_to={Acl[acl]=>{}, File[c:/Temp]=>{}},
 @name=nil,
 @out_from={Acl[acl]=>{}, File[c:/Temp]=>{}},
 @relationship_graph=nil,
 @resource_table={["Acl", "acl"]=>Acl[acl], ["File", "c:/Temp"]=>File[c:/Temp]},
 @resources=[["Acl", "acl"], ["File", "c:/Temp"]],
 @upstream_from={}>

reqs
=> []

An on passing tests:

catalog
=> #<Puppet::Resource::Catalog:0x4101ae0
 @aliases={},
 @classes=[],
 @downstream_from={},
 @environment="none",
 @environment_instance=none,
 @host_config=true,
 @in_to={Acl[acl]=>{}, File[c:/temp]=>{}},
 @name=nil,
 @out_from={Acl[acl]=>{}, File[c:/temp]=>{}},
 @relationship_graph=nil,
 @resource_table={["Acl", "acl"]=>Acl[acl], ["File", "c:/temp"]=>File[c:/temp]},
 @resources=[["Acl", "acl"], ["File", "c:/temp"]],
 @upstream_from={}>

reqs
=> [{ File[c:/temp] => Acl[acl] }]

@Iristyle
Copy link
Contributor Author

I've also noticed that when testing against Puppet 3.7 x86 from source, that we need to have win32-security 0.2.5 in the Gemfile, and win32-api gem commented from the Gemfile.

Without win32-api commented from Gemfile, tests fail to load.

With the old version of win32-security, a fail is generated like:

  1) Puppet::Type::Acl::ProviderWindows :purge purge => listed_permissions when removing non-existing users should allow it to work with SIDs
     Failure/Error: user = adsi::User.create(user_name) unless adsi::User.exists?(user_name)
     Win32::Security::SID::Error:
       The security ID structure is invalid.
     # c:/source/puppet/lib/puppet/util/windows/adsi.rb:64:in `sid_uri_safe'
     # c:/source/puppet/lib/puppet/util/windows/adsi.rb:138:in `uri'
     # c:/source/puppet/lib/puppet/util/windows/adsi.rb:254:in `exists?'
     # ./spec/integration/provider/acl/windows_spec.rb:600:in `block (5 levels) in <top (required)>'

@Iristyle
Copy link
Contributor Author

Also, I did run this against Puppet 3.7 PR 2967 against x86. After ensuring that I was using Facter 2.1 / Puppet from source for the specs, I installed the bundle to a local .bundle folder. Somehow, windows-pr, windows-api and win32-api gems were present in that local directory after installing the bundle, so I manually deleted those gems before proceeding with bundle exec rspec spec

The PR code at puppetlabs/puppet#2967 introduces a few new constants. Without that code, the specs will fail when unable to find constants that used to be in the windows-pr gem.

I have not run acceptance tests or verified against x64.

@puppetcla
Copy link

CLA signed by all contributors.

@Iristyle
Copy link
Contributor Author

Don't merge quite yet.. wrapping up a fix to something dumb I did.

Updated.

yield sid_ptr
end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be outside the constraint for Puppet.version < '3.6.0'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given for Puppet < 3.6, string_to_sid_ptr lives only on Puppet::Util::Windows::Security, and safe_string_to_sid_ptr is only called in this < 3.6 monkey patch, I think safe_string_to_sid_ptr can just be removed completely.

I think when I put it in there I completely missed that the entire thing is for < 3.6

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in mind that you would need to apply your changes to less than 3.7.0 (this area would not cover 3.6.1+).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah.. we have three cases here

  • < 3.6
  • = 3.7

  • in between

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just a new section with < 3.7.0

@cyberious
Copy link
Contributor

Passed 3.7 testing, waiting on results against PE 3.2 and 3.3

@Iristyle
Copy link
Contributor Author

@cyberious hold off on a merge for this one... want to test removing a little more code that I think is unnecessary.

@cyberious
Copy link
Contributor

Sounds good. Passed 3.3 and 3.4 PE

-- 

Travis Fields

travis@puppetlabs.com 

Forge Module Engineer

Join us at PuppetConf 2014, September 22-24 in San Francisco

Register by May 30th to take advantage of the Early Adopter discount —save $349!

On Wed, Aug 20, 2014 at 7:51 AM, Ethan J. Brown notifications@github.com
wrote:

@cyberious hold off on a merge for this one... want to test removing a little more code that I think is unnecessary.

Reply to this email directly or view it on GitHub:
#38 (comment)

@Iristyle Iristyle force-pushed the ticket/master/MODULES-1174-Puppet-37-compatibility branch from 210f5ae to 8b0303c Compare August 20, 2014 18:36
 - Puppet 3.7 refactored underlying Win32 API code by changing gems and
   refactoring classes.  The windows-pr gem was removed which provides
   a number of methods and constants through mixins to the
   Puppet::Util::Windows::Security class.  A number of global constants
   are also defined in the namespaces ::Windows::File and
   ::Windows::Security.
 - Furthermore, some methods that used to be defined on
   Puppet::Util::Windows::Security are now in Puppet::Util::Windows::SID
 - Restructure the code to prefer Puppet 3.7 method locations with a
   graceful fallback solution to 3.6.x with older code, etc.
 - Define all constants locally in base class, remove need to require
   'windows/security' and 'windows/file'
 - Add definitions for FILE_WRITE_DATA, FILE_APPEND_DATA, FILE_READ_DATA
   FILE_WRITE_ATTRIBUTES and FILE_EXECUTE to Base class for use in tests
 - Previous exclusion affected only x86 and not x64
@Iristyle Iristyle force-pushed the ticket/master/MODULES-1174-Puppet-37-compatibility branch from 8b0303c to 09f6068 Compare August 20, 2014 18:38
@Iristyle
Copy link
Contributor Author

Ran specs locally against Puppet 3.4.3 / Facter 1.7.5, Puppet 3.5.1 / Facter 2.0.1, Puppet 3.6.2 / Facter 2.0.1, Puppet 3.7 HEAD on master / Facter 2.1.0 for Ruby 1.9.3 x86 -- all tests pass.

Also ran specs against Puppet 3.7 HEAD / Facter 2.1.0 for Ruby 2.0.0 x64 -- all tests pass.

@Iristyle
Copy link
Contributor Author

In other words, I think we're safe to merge this now @cyberious

@cyberious
Copy link
Contributor

Sounds good, merging and testing again.

cyberious added a commit that referenced this pull request Aug 20, 2014
…t-37-compatibility

(MODULES-1174) Puppet 3.7 compatibility
@cyberious cyberious merged commit 2810f7a into puppetlabs:master Aug 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants