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

(#14149) Reserve the Puppet::Modules namespace #704

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@jeffmccune
Contributor

jeffmccune commented Apr 24, 2012

Without this patch there is no reserved place for 3rd party modules to
define utility classes or ruby module objects. This results in a free
for all situation where it is common for Puppet to accidentally monkey
patch 3rd party code or vice-versa.

This patch fixes the problem by reserving the Puppet::Modules namespace
for use by 3rd party Puppet modules. From this point forward, no change
to Puppet core should introduce objects into the Puppet::Modules
namespace.

(#14149) Reserve the Puppet::Modules namespace
Without this patch there is no reserved place for 3rd party modules to
define utility classes or ruby module objects.  This results in a free
for all situation where it is common for Puppet to accidentally monkey
patch 3rd party code or vice-versa.

This patch fixes the problem by reserving the Puppet::Modules namespace
for use by 3rd party Puppet modules.  From this point forward, no change
to Puppet core should introduce objects into the Puppet::Modules
namespace.
it 'should block Puppet::Modules as a Class' do
lambda { module Puppet; class Modules; end; end; }.should raise_error TypeError, /Modules is not a class/
end
it 'should allow monkey patching' do

This comment has been minimized.

@slippycheeze

slippycheeze Apr 25, 2012

Contributor

That feels like "is Ruby still Ruby"; are you sure it adds value?

@slippycheeze

slippycheeze Apr 25, 2012

Contributor

That feels like "is Ruby still Ruby"; are you sure it adds value?

This comment has been minimized.

@jeffmccune

jeffmccune Jun 5, 2012

Contributor

Yeah, this is to ensure the module is still a module.

I'm anticipating the situation where someone comes in and changes module Modules to class Modules and I want the specs to fail in that situation.

@jeffmccune

jeffmccune Jun 5, 2012

Contributor

Yeah, this is to ensure the module is still a module.

I'm anticipating the situation where someone comes in and changes module Modules to class Modules and I want the specs to fail in that situation.

@slippycheeze

This comment has been minimized.

Show comment
Hide comment
@slippycheeze

slippycheeze Apr 25, 2012

Contributor

It doesn't feel like this is the right direction to me.

It adds extra terms to the namespace, but does nothing to make it harder for code to collide under the hood - you can just as well defined Puppet::Modules::Util in conflicting ways, or add random code in other ways that cause problems.

It also doesn't exempt you from breakage around other changes in Puppet; there are lots of ways other than conflicting names that we can break things for these extensions. They really do need to keep up with changes in the core, rather than hoping that by pushing things off to the side they can stay "working" while the world changes radically around them.

This wouldn't really have helped in the current case, either, since the initial merge was done by taking the existing module and making as few changes as possible to it. There is no reason someone would think that they shouldn't put it under this namespace any more than they would have noticed the conflict when it wasn't present.

Contributor

slippycheeze commented Apr 25, 2012

It doesn't feel like this is the right direction to me.

It adds extra terms to the namespace, but does nothing to make it harder for code to collide under the hood - you can just as well defined Puppet::Modules::Util in conflicting ways, or add random code in other ways that cause problems.

It also doesn't exempt you from breakage around other changes in Puppet; there are lots of ways other than conflicting names that we can break things for these extensions. They really do need to keep up with changes in the core, rather than hoping that by pushing things off to the side they can stay "working" while the world changes radically around them.

This wouldn't really have helped in the current case, either, since the initial merge was done by taking the existing module and making as few changes as possible to it. There is no reason someone would think that they shouldn't put it under this namespace any more than they would have noticed the conflict when it wasn't present.

@jeffmccune

This comment has been minimized.

Show comment
Hide comment
@jeffmccune

jeffmccune Jun 5, 2012

Contributor

@daniel-pittman What is an alternative you suggest?

We're already opening up Puppet::Modules as if it were a module inside of 3rd party puppet modules. I want to make sure Puppet::Modules doesn't get declared as anything inside of Puppet core. What other way exists to accomplish this?

Should every 3rd party puppet module define and re-define Puppet::Modules as a module?

Contributor

jeffmccune commented Jun 5, 2012

@daniel-pittman What is an alternative you suggest?

We're already opening up Puppet::Modules as if it were a module inside of 3rd party puppet modules. I want to make sure Puppet::Modules doesn't get declared as anything inside of Puppet core. What other way exists to accomplish this?

Should every 3rd party puppet module define and re-define Puppet::Modules as a module?

@jeffmccune

This comment has been minimized.

Show comment
Hide comment
@jeffmccune

jeffmccune Jun 5, 2012

Contributor

Daniel, are you confusing this patch with the merge of the puppet-module tool into core Puppet?

This patch isn't really related to that at all. It's really only meant to reserve a namespace where "anything goes" for 3rd party puppet modules.

Contributor

jeffmccune commented Jun 5, 2012

Daniel, are you confusing this patch with the merge of the puppet-module tool into core Puppet?

This patch isn't really related to that at all. It's really only meant to reserve a namespace where "anything goes" for 3rd party puppet modules.

@slippycheeze

This comment has been minimized.

Show comment
Hide comment
@slippycheeze

slippycheeze Jun 5, 2012

Contributor

@jeffmccune - third party modules have an almost infinitely large space to put their things; everything that doesn't start with "::Puppet" as the top level is entirely available to them. Even inside Puppet they have an almost infinitely large space available too - anything that isn't used. "::Puppet::Hiera", or "::Puppet::RandomStuff" are just as safe as the top level, or something in your namespace.

This doesn't solve anything of value: if "anything goes", third party work can tread on third party work, without any more assurance than we had before.

Ultimately, this isn't a direction I think is of any long term value to the platform. It doesn't address enough to justify the cost, and doesn't actually solve the problem highlighted - just makes it more unlikely.

Much better that third parties are aware of this and build their code outside our namespace, just like every other Ruby project works in the wild.

Contributor

slippycheeze commented Jun 5, 2012

@jeffmccune - third party modules have an almost infinitely large space to put their things; everything that doesn't start with "::Puppet" as the top level is entirely available to them. Even inside Puppet they have an almost infinitely large space available too - anything that isn't used. "::Puppet::Hiera", or "::Puppet::RandomStuff" are just as safe as the top level, or something in your namespace.

This doesn't solve anything of value: if "anything goes", third party work can tread on third party work, without any more assurance than we had before.

Ultimately, this isn't a direction I think is of any long term value to the platform. It doesn't address enough to justify the cost, and doesn't actually solve the problem highlighted - just makes it more unlikely.

Much better that third parties are aware of this and build their code outside our namespace, just like every other Ruby project works in the wild.

@jeffmccune jeffmccune reopened this Jun 14, 2012

@jeffmccune

This comment has been minimized.

Show comment
Hide comment
@jeffmccune

jeffmccune Jun 15, 2012

Contributor

Reclosing this request. Andy's second opinion is leading us towards something outside of the Puppet namespace.

Contributor

jeffmccune commented Jun 15, 2012

Reclosing this request. Andy's second opinion is leading us towards something outside of the Puppet namespace.

@jeffmccune jeffmccune closed this Jun 15, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment