puppetlabs / puppetlabs-concat

File concatenation system for Puppet
Apache License 2.0
171 stars 303 forks source link

/var might be mounted with noexec options #130

Closed ghost closed 2 years ago

ghost commented 10 years ago

Since /var might be mounted with the noexec option there should be a way to set the variable $script_path to a different value. I'm not sure if the following diff is the best way to do it, though:

diff --git a/manifests/setup.pp b/manifests/setup.pp index 8d36478..0707467 100644 --- a/manifests/setup.pp +++ b/manifests/setup.pp @@ -25,7 +25,11 @@ class concat::setup { default => 'concatfragments.sh' }

jhoblitt commented 10 years ago

The best approach would likely be to override the concat_basedir fact. https://github.com/puppetlabs/puppetlabs-concat/blob/master/lib/facter/concat_basedir.rb

ghost commented 10 years ago

Obviosly something like cat /etc/facter/facts.d/concat_basedir.sh

!/bin/sh

echo 'concat_basedir'='/usr/local/puppet'

works like a "charm". It just feels wrong to me to move everything to another destination because a script is not executable. What's wrong with my suggested / sketched solution where one can just set a line in a manifest like this:

$concat_script_path = '/usr/local/puppet'

jhoblitt commented 10 years ago

The current incarnation of concat::setup is not intended to be explicitly included by the end user into a manifest. That is why it prints a warning message if you do so. That class is planned to be removed completely when this module is rewritten as a native type/provider, which is why it's been deprecated as a public entry point/API. This module is widely used and thus is very sensitive to API changes.

Setting an alternative location for the aggregation script would best be done as a [new] fact but I'm not particularly keen on the idea as:

However, I'd be willing to bless such a feature if there was a compelling argument for it.

igalic commented 10 years ago

I think it's most sensible to install the script in /usr/local/bin or /usr/local/sbin/

apenney commented 10 years ago

I feel we should hold of adapting the API considering the plans for a type/provider (And I will attempt to write this if nothing happens to stop me in late Feb). It's uncommon enough to use noexec that I'm ok with just adjusting basedir for now. I would hate to start trying to guess other places we could safely write into (not everything even has a /usr/local/ out of the box). I'd rather document the workaround of overwriting concat_basedir.

jhoblitt commented 10 years ago

Also note that ./bin & ./sbin are for scripts/executables that are intended for public consumption -- this is the equivalent of making an API as public. The aggregation script has absolutely no CLI API and/or behaviour guarantees and is private to the concat module. 'private' applications typically are installed under ./share or ./libexec.

igalic commented 10 years ago

libexec can't be found on Ubuntu systems. share should be locatable on pretty much every (Unix) system.

chelnak commented 2 years ago

Hello! We are doing some house keeping and noticed that this issue has been open for a long time.

We're going to close it but please do raise another issue if the issue still persists. 😄