-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
Fix https://github.com/voxpupuli/puppet-archive/issues/225 #226
Conversation
require File.join archive.path, 'lib/puppet_x/bodeco/archive' | ||
require File.join archive.path, 'lib/puppet_x/bodeco/util' | ||
end | ||
require File.dirname(__FILE__) + '/../../../puppet_x/bodeco/archive.rb' |
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.
Not sure about this. The code you're removing looks like it was specifically added as a workaround for https://tickets.puppetlabs.com/browse/SERVER-973
https://github.com/voxpupuli/puppet-corosync/blob/master/lib/puppet/provider/cs_clone/crm.rb#L4 is identical? But in that module the comments also reference SERVER-973
ping @roidelapluie - Can you clarify?
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.
I am unsure here. I found PUB-3502 which states:
"2". The second issue is that your provider is accessing Puppet[:environment] which is almost certainly going to be wrong if you are using more than the production enviroment:
vmware_module = Puppet::Module.find('vmware_lib', Puppet[:environment].to_s) require File.join vmware_module.path, "lib/#{path}"`
Both the agent and master will require your vcac_appliance provider, and the use of Puppet[:environment] is likely to be wrong for both of them. For example, the agent setting will default to production but will switch to the ENC specified environment. And the value of the Puppet[:environment] setting on the master will likely not correspond to the agents' environment in which the master is supposed to be compiling a catalog. As a result, if you have different versions of your provider on the master (dev, test, production), then you could be loading helper code from an environment which doesn't match the manifest that you're compiling for the agent.
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.
Since we are ruby 1.9.3+, we can just use require_relative. The patch here might appear to 'fix' the problem, but it's likely coincidental. The core problem is https://tickets.puppetlabs.com/browse/SERVER-94.
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.
We can use require_relative to simplify some of the code path here, but I'm not sure this will address the multi-environment problem
@@ -1,6 +1,6 @@ | |||
# rubocop:disable RSpec/MultipleExpectations | |||
require 'spec_helper' | |||
require 'puppet_x/bodeco/archive' | |||
require File.dirname(__FILE__) + '/../../../../lib/puppet_x/bodeco/archive.rb' |
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.
There's really no reason to touch unit tests.
@@ -9,7 +9,7 @@ module Puppet::Parser::Functions | |||
raise(ArgumentError, "Invalid artifactory file info url #{args}") unless args.size == 1 | |||
|
|||
require 'json' | |||
require 'puppet_x/bodeco/util.rb' | |||
require File.dirname(__FILE__) + '/../../../puppet_x/bodeco/util.rb' |
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.
Since functions are pluginsynced to the libdir, this could be loading an arbitrary version of util.rb.
require File.join archive.path, 'lib/puppet_x/bodeco/archive' | ||
require File.join archive.path, 'lib/puppet_x/bodeco/util' | ||
end | ||
require File.dirname(__FILE__) + '/../../../puppet_x/bodeco/archive.rb' |
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.
Since we are ruby 1.9.3+, we can just use require_relative. The patch here might appear to 'fix' the problem, but it's likely coincidental. The core problem is https://tickets.puppetlabs.com/browse/SERVER-94.
@juniorsysadmin Could you explain why this issue won't be fixed ? I am seeing other 'supported' modules doing the same thing : |
@olivierHa My understanding after reading the linked issue is that the issue is related to either an EOL Ruby version or Puppet's handing of environments or an older Puppet master version - it's not something that this module should be trying to accommodate. Additionally, the code changes in this PR don't seem to be fixing the underlying issues that were reported. Happy to be corrected and re-open this if it is made clearer the reason for this PR. |
Fix Issue #225