Puppet - Bug #12371pluginsync should warn when files are not in a special directory02/01/2012 04:21 pm - Patrick Carlisle

Status: Accepted Start date: 02/01/2012Priority: Normal Due date:Assignee: Patrick Carlisle % Done: 0%Category: plug-ins Estimated time: 0.00 hourTarget version:Affected Puppet version: Branch:Keywords:Description

If you put files in a module in a directory that doesn’t get autoloaded there should be some sort of warning.

History#1 - 02/01/2012 04:22 pm - Patrick Carlisle

Maybe pluginsync is the wrong place, not sure yet

#2 - 02/01/2012 04:39 pm - Anonymous- Status changed from Unreviewed to Needs Decision- Assignee set to Anonymous

Randall, I think you will have some valuable input on this:

When a module results in something being synced to an “unusual” location, should we warn about it?

It seems like there are some valuable uses, like shipping support code to the client, where stuff would fall entirely outside the “puppet”Ruby-level namespace.

On the other hand, under “puppet” we could reasonably warn, and be confident that most files in odd places were mistakes …unless a newer version of Puppet supported them, and the older version considered them “odd”.

#3 - 02/01/2012 04:48 pm - Patrick Carlisle

We have a lot of magical conventions that you have to follow in a module directory, and if you make a mistake you get no feedback except that yourcode just doesn’t work at the other end. The case I just ran into with Chris today was actually forgetting the “puppet” componentof the path and just writing lib/feature/something.rb. If we want to support people including non-puppet code maybe we should have it be explicit, makeit put in a library subdirectory or something.

#4 - 02/15/2012 12:00 pm - Anonymous

Proposal:

- when evaluated on the master this should fail hard - when evaluated on the agent this should warn

05/01/2016 1/4

In both cases with a good and useful error.

#5 - 02/15/2012 12:49 pm - Anonymous

I am happy with that proposal. We need to be careful:

- if the change is under a path we “own” like puppet or facter, we should warn. - we should support something like puppet/util that you can throw random junk into as a user. - we should not warn for foo or other top level namespaces we don’t own.

We should also keep in mind changes to this list over time: we always have a master version >= the agent version, but the reverse is not true. Thatmeans if we introduce a new “supported” path in 4.0, a 3.0 agent might warn even once the master was upgraded.

#6 - 02/15/2012 12:50 pm - Anonymous- Category set to plug-ins- Status changed from Needs Decision to Accepted- Assignee changed from Anonymous to Patrick Carlisle

#7 - 02/15/2012 12:52 pm - Anonymous

Daniel Pittman wrote:

We should also keep in mind changes to this list over time: we always have a master version >= the agent version, but the reverse is not true. That means if we introduce a new “supported” path in 4.0, a 3.0 agent might warn even once the master was upgraded.

That sounds like a good addition to the test suite :)

#8 - 02/18/2012 10:41 am - Nigel Kersten

Shouldn’t we fix the autoloading rather than implementing a whitelist?

I know in the past I’ve used modulepath/module/lib/foo to sync arbitrary code that something related to my Puppet infrastructure was using,even if I had to manually load things from there.

It feels like we should be able to just consistently load from the libdir, and that’s the more consistent approach.

#9 - 02/18/2012 10:42 am - James Turnbull

What Nigel said.

#10 - 02/20/2012 10:10 am - Anonymous

Nigel Kersten wrote:

Shouldn’t we fix the autoloading rather than implementing a whitelist?

05/01/2016 2/4

Sorry, fix it how? Should we assume that something in puppet/functions was intended to be in puppet/parser/functions? Spell-check filenames andsee if that makes it possible to autoload a file? Use the hamming distance between the name of a file and the name of a file we could autoload tofigure out the user meant it to work, but got it wrong?

I know in the past I’ve used modulepath/module/lib/foo to sync arbitrary code that something related to my Puppet infrastructure was using,even if I had to manually load things from there.

…..yes.

It feels like we should be able to just consistently load from the libdir, and that’s the more consistent approach.

So, how is this different from the proposal that:

- if the change is under a path we “own” like puppet or facter, we should warn. - we should support something like puppet/util that you can throw random junk into as a user. - we should not warn for foo or other top level namespaces we don’t own.

#11 - 02/20/2012 11:22 am - Chris Price

FYI: I added some debug logging to plugin loading here:

https://github.com/cprice-puppet/puppet/commit/eb70e49cf73253773314f1a657fa56cf63fc325e

It does not entirely address this ticket, and it may end up being reworked slightly to meet the eventual goals of this ticket. However, it would havesaved me several hours of debugging time so I thought it was worth getting in place as a first step.

#12 - 02/20/2012 02:37 pm - Nigel Kersten

Daniel Pittman wrote:

Nigel Kersten wrote:

Shouldn’t we fix the autoloading rather than implementing a whitelist?

Sorry, fix it how? Should we assume that something in puppet/functions was intended to be in puppet/parser/functions? Spell-check filenamesand see if that makes it possible to autoload a file? Use the hamming distance between the name of a file and the name of a file we could autoloadto figure out the user meant it to work, but got it wrong?

I wasn’t implying anything like that and it sounds like I’ve gotten the wrong impression about what the problem actually is in this ticket.

If the problem is lack of visibility into module/lib directories where people have used the wrong paths, I’m not sure how:

“we should not warn for foo or other top level namespaces we don’t own.”

is going to fix someone using “lib/features” rather than “lib/puppet/features”.

If the problem is people using: “lib/puppet/foo” rather than “lib/puppet/features/foo”, it sounds like the proposal is thatpeople won’t be able to extend puppet with arbitrary directories under “puppet” and deliver those extensions with pluginsync

05/01/2016 3/4

without getting a warning right?

I’m a little concerned about that aspect, of us maintaining a whitelist of sanctioned directories underneath “puppet”. Do we havesignal that people make mistakes in that direction? where they have “puppet” in the path, but get the rest wrong?

#13 - 02/20/2012 02:40 pm - Patrick Carlisle

I’ve certainly made that mistake.

#14 - 02/20/2012 02:59 pm - Nigel Kersten

OK, I misunderstood the situation and proposed response, so please ignore me :)

05/01/2016 4/4