Added improved headers notifying sysadmins that the file is managed by puppet. #20

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@vStone
vStone commented Nov 28, 2012

Ref: Reordering pull request #19

@skottler skottler commented on an outdated diff Nov 28, 2012
files/config.ru
@@ -1,3 +1,7 @@
+### File provisioned by puppet ###
@skottler
skottler Nov 28, 2012 The Foreman member

Can you make this ### File managed with puppet ### so it matches the other headers?

@GregSutcliffe
Member

Ack for this once @skottler's point is resolved.

@vStone
vStone commented Nov 28, 2012

done.

@skottler skottler commented on an outdated diff Dec 4, 2012
templates/puppet.conf.erb
@@ -1,3 +1,8 @@
+### File managed with puppet ###
+## Served by: '<%= scope.lookupvar('::servername') %>'
+## Module: '<%= scope.to_hash['module_name'] %>'
+## Template source: 'MODULES<%= template_source.gsub(Regexp.new("^#{Puppet::Node::Environment.current[:modulepath].gsub(':','|')}"),"") %>'
@skottler
skottler Dec 4, 2012 The Foreman member

## Template source: 'MODULESroot/modules/./puppet/templates/puppet.conf.erb is the output of this line right now. Seems like the root value isn't correct if apply is used?

@vStone
vStone commented Dec 4, 2012

I noticed this too but gave it little notice since checking these cases would make the header too complex.

Also, using template_source directly is messy when using environments. All templated files get an update for each time you switch environments.

Another option is to /hardcode/ the paths and use $module_name.

@vStone
vStone commented Dec 4, 2012

It probably has something to do with the relative paths used. I can't reproduce when I'm using the full path to the module folders (puppet apply --modulepath /full/path/to/folder ... vs puppet apply --modulepath ./folder)

@vStone
vStone commented Dec 4, 2012

Alternative solution(s):

<% module_paths =  Puppet::Node::Environment.current[:modulepath].split(':').map{|i| File.expand_path(i) }.join("|") -%>
## Template source:  'MODULES<%= template_source.gsub(Regexp.new("^#{module_paths}"),"") %>'

Pro: Always works
Con: Adds more complexity (expanding all paths)


## Template source:  'MODULES/<%= scope.to_hash['module_name'] %>/templates/debug.txt.erb'

Con: Hardcoded, need to adjust for every template


## Template Source:  'MODULES/<%= scope.to_hash['module_name'] %>/<%= template_source.sub(Regexp.new('^.*\/(templates\/.*)$'), '\1') %>

Pro: Simpler but portable
Con: Will probably not work when there is a second /template/ segment somewhere in the path

@domcleal
Member
domcleal commented Dec 4, 2012

Could the complexity be moved into a parser function which returns the whole header? It might be less of a headache to maintain then across multiple files.

@vStone
vStone commented Dec 4, 2012

I also thought about that several times, problem with putting it in a function is that you would have to either add this function to every module that uses this header or well, you add dependencies on another module.

@vStone
vStone commented Dec 4, 2012

Alternatively, we could put this in one _header.erb template and include that everywhere using some ERB magic:

<%= ERB.new(File.read(File.expand_path("_header.erb",File.dirname(file)))).result(binding) -%>
@domcleal
Member
domcleal commented Dec 4, 2012

Another problem with a parser function is you'd have it declared multiple times, which might work unreliably or be a problem.

I like the header.erb idea, though it might be simpler to use template('puppet/_header.erb', 'template/foo.erb') where we want it.

@vStone
vStone commented Dec 4, 2012

If you call it like that, you will get the wrong template_source to work on :/

@domcleal
Member
domcleal commented Dec 4, 2012

Ugh, good point!

@domcleal
Member
domcleal commented Dec 5, 2012

Can we simplify this? My feeling talking with @GregSutcliffe is that this is potentially brittle and could be a maintenance headache. I'd like to simplify it down to just mentioning the first static line and the module name - is that ok with you?

@vStone
vStone commented Dec 6, 2012

I have a simplified version but have to craft it into a commit. Its a combination of suggested solutions.

The header is placed in a file called _header.erb and gets included using ERB magic in a one line header in each file.

Would this work for you guys?

@skottler
Member
skottler commented Dec 6, 2012

Sounds like a good approach to me.

@GregSutcliffe
Member

Tested, works for me. Anyone got any issues left, or shall we merge?

@domcleal
Member

@GregSutcliffe go for it

@GregSutcliffe
Member

I take it, back, there are still bugs here. We have:

#!/usr/bin/env ruby                                                                                               
<%= ERB.new(File.read(File.expand_path("../_header.erb",File.dirname(file)))).result(binding) -%>

in the post hook ERB, but the actual file gets deploy as:

### File managed with puppet ###                                                                                  
## Served by:        ''
## Module:           'puppet'
## Template source:  'MODULES/puppet/templates/server/post-receive.erb'

Notice how the shebang line has gone, making it not execute with ruby any more. @vStone we can't merge that :)

@vStone
vStone commented Jan 14, 2013

I stripped the full path to the template file out of it because it gave me headaches when switching one node to another environment. In those cases, each file will be detected as an update and the service restarted automatically. Services should not reboot on a environment change, only on 'true' file changes.\

Therefore: MODULES/*

@GregSutcliffe
Member

I have no objection to the content of the header - it's that the "#!/usr/bin/ruby" line has somehow been removed during template compilation, breaking the posthook.

@vStone
vStone commented Jan 14, 2013

Riiiiiiiight, allow me to fix that :) I already ran into this before but totally forgot about the pull here.

@GregSutcliffe
Member

That's nailed it. Merging. Thanks for the patch, and your patience, @vStone :)

@vStone vStone deleted the vStone:feature/template_headers branch Aug 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment