911270 - fix up warnings from puppetmasters

Status: RESOLVED FIXED

(Infrastructure & Operations :: RelOps: Puppet, task)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

We have a number of variables in ERB that don't start with @, warnings from concatfragments about empty chunks, etc.  Nothing harmful, but they should get fixed.

Comment 2

[Amy Rich [:arr] [:arich]]

Attachment: clean up variable syntax to add missing @ in erb tempates

I'm going to try to break this up in multiple patches and do the bits I can with the knowledge I have. 

I'm not quite sure about the reference to screen_depth.to_i and screen_depth in modules/gui/templates/xvfb.conf.erb, so please validate those, especially.

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8408498 [details] [diff] [review]
clean up variable syntax to add missing @ in erb tempates

Looks good.  I made a test run on Ubuntu and didn't see anything change.

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

something here is in production

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

backed out

[[e-mail] ~]# apt-get update
E: Malformed line 2 in source list /etc/apt/sources.list.d/precise-security.list (dist)
E: The list of sources could not be read.
[[e-mail] ~]# cat /etc/apt/sources.list.d/precise-security.list
# Managed by puppet
deb http://puppetagain-apt.pvt.build.mozilla.org/repos/apt/ubuntu

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8408498 [details] [diff] [review]
clean up variable syntax to add missing @ in erb tempates

Review of attachment 8408498 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/packages/templates/sources.list.erb
@@ +6,3 @@
>  -%>
>  # Managed by puppet
> +deb http://<%= @apt_repo_server %>/<%= @url_path -%> <%= @suffix %>

suffix here is a template-local variable, so it doesn't get the @

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug911270.patch

updated version

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Merged to prod, and I'm seeing

Notice: /Stage[packagesetup]/Packages::Setup/Packages::Aptrepo[precise]/File[/etc/apt/sources.list.d/precise.list]/content:
--- /etc/apt/sources.list.d/precise.list        2014-04-18 09:28:03.830800999 -0700
+++ /tmp/puppet-file20140418-2910-1tgxesu-0     2014-04-18 09:40:02.178986589 -0700
@@ -1,2 +1,2 @@
 # Managed by puppet
-deb http://puppetagain-apt.pvt.build.mozilla.org/repos/apt/ubuntu
+deb http://puppetagain-apt.pvt.build.mozilla.org/repos/apt/ubuntu precise main restricted universe

so this looks good.

Comment 9

[Amy Rich [:arr] [:arich]]

Attachment: a few more variable syntax fixes

I missed a few variable warnings in the first round.  This should get more (hopefully the rest).

Comment 10

[Amy Rich [:arr] [:arich]]

Attachment: fixes more variable syntax errors and adds a default motd to concat to.

fixes more variable syntax errors and adds a default motd to concat to.

Comment 11

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8409117 [details] [diff] [review]
fixes more variable syntax errors and adds a default motd to concat to.

Review of attachment 8409117 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/motd/manifests/base.pp
@@ +21,2 @@
>              target => $motd::settings::motd_file,
> +            content => "Unauthorized access prohibited"

This will need a trailing newline, or the next line will end up on top of it

::: modules/users/templates/homeclean.sh.erb
@@ +38,5 @@
>  {
>  	mkdir -p ${BASE}
>  	chmod 0700 ${BASE}
> + 	# OS X doesn't update mtime on a chmod
> +	touch ${BASE}

Did you mean to mix this hunk in, too?  If so, it should be mentioned in the commit message.

Comment 12

[Amy Rich [:arr] [:arich]]

newlins added, homeclean change already checked into default as part of a different patch (but that's what I get for not having multiple repos checked out).

This bit is now checked into default as well.

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

merged to production