Issue 2471 - Update our Discourse instance

This configuration finally seems to be working but I am still testing it.

[Felix Dahlke, 2015-05-08 20:16:43]

Did a high level review of this, LGTM.

[Wladimir Palant, 2015-05-08 22:05:15]

Updated patchset, now with more dependencies that turned up in testing - I
didn't realize that my test system had them installed already.

[Felix Dahlke, 2015-05-08 22:08:17]

That's a lot of dependencies :D LGTM

[mathias, 2015-05-09 15:34:19]

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
File modules/discourse/manifests/init.pp (right):

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
modules/discourse/manifests/init.pp:44: path =>
'/bin:/usr/bin:/usr/sbin:/usr/local/bin:/home/discourse/.rvm/bin',
Do you know why the directory precedence here is inverted, in contrast to what
one would expect? I do realize that the former version did the same, but there's
no obvious reason for either one - and it looks like it can quickly become an
issue.

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
modules/discourse/manifests/init.pp:57: command => 'curl -sSL https://get.rvm.io
| bash -s stable --ruby=2.1.2',
Note that this does not cover upgrading to future Ruby versions, a specific
patch or similar. Also, it introduces yet another major (but avoidable)
dependency on external resources. However, it's fine for now that we want to fix
Discourse logins, but we have to re-visit it afterwards at least.

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
modules/discourse/manifests/init.pp:65: exec {'install-bundler':
Note that this would be better handled as a Package with provider => gem --
Though our Puppet version may not be compatible with this version of Gem itself.
So gain this may be fine for now, but should become revisited at least.

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
modules/discourse/manifests/init.pp:129: onlyif => "test ! -d
/opt/discourse/.hg"
(Not part of this patch-set, but a bug that needs to be addressed as well;
cloning into a non-empty directory does not work. Though in practice that should
not happen with the code here, it's still a footgun.)

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
modules/discourse/manifests/init.pp:329: require => Exec['init-discourse']
Wouldn't it make more sense to make it subscribe to all
Discourse::Sitesetting[]s? (Then Exec['init-discourse'] is covered as well.)

[Wladimir Palant, 2015-05-11 15:00:05]

Outstanding issue: client-side error `template not defined` affecting test VMs,
Matze wants to look into this himself.

[Wladimir Palant, 2015-05-11 15:00:42]

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
File modules/discourse/manifests/init.pp (right):

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
modules/discourse/manifests/init.pp:44: path =>
'/bin:/usr/bin:/usr/sbin:/usr/local/bin:/home/discourse/.rvm/bin',
On 2015/05/09 15:34:19, matze wrote:
> Do you know why the directory precedence here is inverted, in contrast to what
> one would expect? I do realize that the former version did the same, but
there's
> no obvious reason for either one - and it looks like it can quickly become an
> issue.

Done.

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
modules/discourse/manifests/init.pp:57: command => 'curl -sSL https://get.rvm.io
| bash -s stable --ruby=2.1.2',
On 2015/05/09 15:34:19, matze wrote:
> Note that this does not cover upgrading to future Ruby versions, a specific
> patch or similar.

Yes, I had RVM and Ruby installation separate initially but dropped that for
reasons of simplicity. I guess we'll want to reintroduce that separation.

I verified that updating Ruby actually works correctly now. However, I didn't
update to Ruby 2.2.2 yet because in my VM it produced out of memory errors.

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
modules/discourse/manifests/init.pp:65: exec {'install-bundler':
On 2015/05/09 15:34:19, matze wrote:
> Note that this would be better handled as a Package with provider => gem --

We need two different Ruby versions: one for Puppet/Hiera (1.9), another one for
Discourse (2.0+). So we don't touch the system Ruby version and install a
separate version for discourse user only.

Using Package will only install bundler in the system Ruby - that's what we did
before but it won't be usable with the Ruby installation in the discourse
account. So Exec seems to be the only way.

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
modules/discourse/manifests/init.pp:129: onlyif => "test ! -d
/opt/discourse/.hg"
On 2015/05/09 15:34:19, matze wrote:
> (Not part of this patch-set, but a bug that needs to be addressed as well;
> cloning into a non-empty directory does not work. Though in practice that
should
> not happen with the code here, it's still a footgun.)

Why would it be non-empty? It's created right before cloning - and it's only a
separate step because the discourse user doesn't have permission to write to
/opt.

http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
modules/discourse/manifests/init.pp:329: require => Exec['init-discourse']
On 2015/05/09 15:34:19, matze wrote:
> Wouldn't it make more sense to make it subscribe to all
> Discourse::Sitesetting[]s? (Then Exec['init-discourse'] is covered as well.)

Not sure whether the services should really be considered dependent on the
settings. Given that these settings are non-essential (the essential ones are in
discourse.conf) and can be changed via web UI - I think they shouldn't prevent
the services from starting up.

[Wladimir Palant, 2015-05-12 10:15:38]

Turns out that the test VM was checking out the wrong branch, hence the blank
page. I made the repository update an explicit step now, this will make updating
easier as well.

[Wladimir Palant, 2015-05-12 10:16:59]

Turns out that the test VM was checking out the wrong branch, hence the blank
page. I made the repository update an explicit step now, this will make updating
easier as well.

[mathias, 2015-05-18 11:14:39]

LGTM.

On 2015/05/11 15:00:42, Wladimir Palant wrote:

>
http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
> Yes, I had RVM and Ruby installation separate initially but dropped that for
> reasons of simplicity. I guess we'll want to reintroduce that separation.

OK, I'll take care of a follow-up.

>
http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
> 
> We need two different Ruby versions: one for Puppet/Hiera (1.9), another one
for
> Discourse (2.0+). So we don't touch the system Ruby version and install a
> separate version for discourse user only.
> 
> Using Package will only install bundler in the system Ruby - that's what we
did
> before but it won't be usable with the Ruby installation in the discourse
> account. So Exec seems to be the only way.

Fair enough, still we may want to try to improve here in a future version. But
as I wrote before, I hardly doubt that this will be trivial - so there should be
an actual requirement (e.g. another, future service requiring a recent Ruby)
before we invest the time in a more general approach, and a distinct module.

>
http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
> 
> Why would it be non-empty? It's created right before cloning - and it's only a
> separate step because the discourse user doesn't have permission to write to
> /opt.

While the Puppet code is in fact straightforward (which is why I have no
objections for pushing), the real world is not: When cloning fails, for example,
one may not succeed with a second run due to this code. I am aware that modern
SCM attempt to roll-back at this point, but still that's too many "given that"
in my opinion. The point is that this is not stable behavior or, at least, code
based on undocumented assumptions. I just prefer taking any precautions
possible. However, no need to discuss this here.

>
http://codereview.adblockplus.org/4924880143253504/diff/5639274879778816/modu...
> 
> Not sure whether the services should really be considered dependent on the
> settings. Given that these settings are non-essential (the essential ones are
in
> discourse.conf) and can be changed via web UI - I think they shouldn't prevent
> the services from starting up.

Well, this assumption - even if tested - may change with any future version.
However, since we've discussed this last week and saw that the settings are
indeed notifying the service, this point is obsolete anyway.

[Felix Dahlke, 2015-05-26 09:13:44]

LGTM, the new patch set.

[mathias, 2015-05-26 10:02:16]

http://codereview.adblockplus.org/4924880143253504/diff/5673385510043648/modu...
File modules/discourse/manifests/init.pp (right):

http://codereview.adblockplus.org/4924880143253504/diff/5673385510043648/modu...
modules/discourse/manifests/init.pp:73: unless => "rvm list | grep
$ruby_version",
Please use shellquote() to ensure $ruby_version cannot break anything here --

[Wladimir Palant, 2015-05-26 10:41:17]

http://codereview.adblockplus.org/4924880143253504/diff/5673385510043648/modu...
File modules/discourse/manifests/init.pp (right):

http://codereview.adblockplus.org/4924880143253504/diff/5673385510043648/modu...
modules/discourse/manifests/init.pp:73: unless => "rvm list | grep
$ruby_version",
On 2015/05/26 10:02:16, matze wrote:
> Please use shellquote() to ensure $ruby_version cannot break anything here --

Given that $ruby_version is a constant set above, do you consider that
necessary? Also, if we do it for $ruby_version then we should do it for
$hg_revision as well.

[mathias, 2015-05-26 10:54:36]

http://codereview.adblockplus.org/4924880143253504/diff/5673385510043648/modu...
File modules/discourse/manifests/init.pp (right):

http://codereview.adblockplus.org/4924880143253504/diff/5673385510043648/modu...
modules/discourse/manifests/init.pp:73: unless => "rvm list | grep
$ruby_version",
On 2015/05/26 10:41:17, Wladimir Palant wrote:
> On 2015/05/26 10:02:16, matze wrote:
> > Please use shellquote() to ensure $ruby_version cannot break anything here
--
> 
> Given that $ruby_version is a constant set above, do you consider that
> necessary? Also, if we do it for $ruby_version then we should do it for
> $hg_revision as well.

Indeed, it should be done there as well.

And yes I believe it's necessary. One should always assume the next programmer
to be less careful and wise than oneself (even if it will be oneself most
likely). And such a guy may forget to check for the use of $ruby_version when
migrating it to become a parameter one day (for example, but not even unlikely),
which can also easily be overseen in a code-review.

However, please note that we agreed last year on making use of shell_escape()
wherever it may make sense - just to have an easy but secure guideline that does
not require the same decision ("is it necessary?") over and over again..