Upgrade rsyslog config files

Upgrade rsyslog config files

Add upgrade capability for rsyslog config
files. A chunk of code refactoring was also
done. Previously committed code to upgrade 
the uniter lock directory was moved to its
own file, plus code to test rsyslog config
contents was pulled out so it could be
reused.

https://code.launchpad.net/~wallyworld/juju-core/rsyslogconf-upgrader-118/+merge/206090

(do not edit description out of merge proposal)

[wallyworld, 2014-02-13 06:01:47]

Please take a look.

[axw, 2014-02-13 07:49:45]

https://codereview.appspot.com/62950043/diff/1/upgrades/file.go
File upgrades/file.go (right):

https://codereview.appspot.com/62950043/diff/1/upgrades/file.go#newcode13
upgrades/file.go:13: // contents, allowing for the fact that filename may
already exist.
what should happen if it exists? should perms/ownership be copied?

https://codereview.appspot.com/62950043/diff/1/upgrades/file.go#newcode19
upgrades/file.go:19: tempDir, err := ioutil.TempDir(confDir, "")
why not just create a TempFile in confDir?

https://codereview.appspot.com/62950043/diff/1/upgrades/lockdirectory.go
File upgrades/lockdirectory.go (right):

https://codereview.appspot.com/62950043/diff/1/upgrades/lockdirectory.go#newc...
upgrades/lockdirectory.go:28: command := fmt.Sprintf(""+
Why shell out for this?

[dimitern, 2014-02-13 09:38:10]

Diff is missing, repropose please.

[thumper, 2014-02-13 21:13:12]

https://codereview.appspot.com/62950043/diff/1/log/syslog/testing/syslogconf.go
File log/syslog/testing/syslogconf.go (right):

https://codereview.appspot.com/62950043/diff/1/log/syslog/testing/syslogconf....
log/syslog/testing/syslogconf.go:33: $FileCreateMode 0640
If these are just copies of the templates in the log/syslog package, why
duplicate them here?

We should just be referring to the originals, otherwise we just have to update
the file in two places when we change it.

https://codereview.appspot.com/62950043/diff/1/upgrades/lockdirectory.go
File upgrades/lockdirectory.go (right):

https://codereview.appspot.com/62950043/diff/1/upgrades/lockdirectory.go#newc...
upgrades/lockdirectory.go:1: // Copyright 2014 Canonical Ltd.
Ian, you were talking about moving these into sub-packages.

Still going to do this?  I think it would be good.

https://codereview.appspot.com/62950043/diff/1/upgrades/lockdirectory.go#newc...
upgrades/lockdirectory.go:28: command := fmt.Sprintf(""+
On 2014/02/13 07:49:45, axw wrote:
> Why shell out for this?

This was my one, we shell out because we can't identify whether the ubuntu user
exists and what the user id / group id would be for the chown.  Seemed easier
this way.

[wallyworld, 2014-02-13 21:53:28]

https://codereview.appspot.com/62950043/diff/1/log/syslog/testing/syslogconf.go
File log/syslog/testing/syslogconf.go (right):

https://codereview.appspot.com/62950043/diff/1/log/syslog/testing/syslogconf....
log/syslog/testing/syslogconf.go:33: $FileCreateMode 0640
On 2014/02/13 21:13:13, thumper wrote:
> If these are just copies of the templates in the log/syslog package, why
> duplicate them here?
> 
> We should just be referring to the originals, otherwise we just have to update
> the file in two places when we change it.

This is deliberate - it means that if the actual content of the generated files
change, the tests will break.
The stuff in this new file is simply transplanted (ie moved) from an existing
_test file to a testing package so that it can be shared between tests in
different packages.

https://codereview.appspot.com/62950043/diff/1/upgrades/file.go
File upgrades/file.go (right):

https://codereview.appspot.com/62950043/diff/1/upgrades/file.go#newcode13
upgrades/file.go:13: // contents, allowing for the fact that filename may
already exist.
On 2014/02/13 07:49:45, axw wrote:
> what should happen if it exists? should perms/ownership be copied?

I think it's correct to override any existing perms/ownership. This method is
called when doing an upgrade. The upgrade may create the new file with new
ownership/perms and we want to retain these.

https://codereview.appspot.com/62950043/diff/1/upgrades/file.go#newcode19
upgrades/file.go:19: tempDir, err := ioutil.TempDir(confDir, "")
On 2014/02/13 07:49:45, axw wrote:
> why not just create a TempFile in confDir?

This code was copied from elsewhere. Not sure. It can be changed.

https://codereview.appspot.com/62950043/diff/1/upgrades/lockdirectory.go
File upgrades/lockdirectory.go (right):

https://codereview.appspot.com/62950043/diff/1/upgrades/lockdirectory.go#newc...
upgrades/lockdirectory.go:1: // Copyright 2014 Canonical Ltd.
On 2014/02/13 21:13:13, thumper wrote:
> Ian, you were talking about moving these into sub-packages.
> 
> Still going to do this?  I think it would be good.

a. I want to land this branch first cause it's big enough already
b. Let's see how it looks with 2 or 3 upgrade plugins so we can get a picture of
how best to do any sub packaging.

[axw, 2014-02-14 01:02:53]

https://codereview.appspot.com/62950043/diff/1/upgrades/lockdirectory.go
File upgrades/lockdirectory.go (right):

https://codereview.appspot.com/62950043/diff/1/upgrades/lockdirectory.go#newc...
upgrades/lockdirectory.go:28: command := fmt.Sprintf(""+
On 2014/02/13 21:13:13, thumper wrote:
> On 2014/02/13 07:49:45, axw wrote:
> > Why shell out for this?
> 
> This was my one, we shell out because we can't identify whether the ubuntu
user
> exists and what the user id / group id would be for the chown.  Seemed easier
> this way.

It's not too heinous the way it is, but just FYI it is very possible to do in
Go: http://golang.org/pkg/os/user/

[axw, 2014-02-14 01:09:27]

On 2014/02/13 21:53:28, wallyworld wrote:
>
https://codereview.appspot.com/62950043/diff/1/log/syslog/testing/syslogconf.go
> File log/syslog/testing/syslogconf.go (right):
> 
>
https://codereview.appspot.com/62950043/diff/1/log/syslog/testing/syslogconf....
> log/syslog/testing/syslogconf.go:33: $FileCreateMode 0640
> On 2014/02/13 21:13:13, thumper wrote:
> > If these are just copies of the templates in the log/syslog package, why
> > duplicate them here?
> > 
> > We should just be referring to the originals, otherwise we just have to
update
> > the file in two places when we change it.
> 
> This is deliberate - it means that if the actual content of the generated
files
> change, the tests will break.
> The stuff in this new file is simply transplanted (ie moved) from an existing
> _test file to a testing package so that it can be shared between tests in
> different packages.
> 
> https://codereview.appspot.com/62950043/diff/1/upgrades/file.go
> File upgrades/file.go (right):
> 
> https://codereview.appspot.com/62950043/diff/1/upgrades/file.go#newcode13
> upgrades/file.go:13: // contents, allowing for the fact that filename may
> already exist.
> On 2014/02/13 07:49:45, axw wrote:
> > what should happen if it exists? should perms/ownership be copied?
> 
> I think it's correct to override any existing perms/ownership. This method is
> called when doing an upgrade. The upgrade may create the new file with new
> ownership/perms and we want to retain these.

Okay, fair enough. May be good to have explicit mode then, but it can wait.

> https://codereview.appspot.com/62950043/diff/1/upgrades/file.go#newcode19
> upgrades/file.go:19: tempDir, err := ioutil.TempDir(confDir, "")
> On 2014/02/13 07:49:45, axw wrote:
> > why not just create a TempFile in confDir?
> 
> This code was copied from elsewhere. Not sure. It can be changed.
> 
> https://codereview.appspot.com/62950043/diff/1/upgrades/lockdirectory.go
> File upgrades/lockdirectory.go (right):
> 
>
https://codereview.appspot.com/62950043/diff/1/upgrades/lockdirectory.go#newc...
> upgrades/lockdirectory.go:1: // Copyright 2014 Canonical Ltd.
> On 2014/02/13 21:13:13, thumper wrote:
> > Ian, you were talking about moving these into sub-packages.
> > 
> > Still going to do this?  I think it would be good.
> 
> a. I want to land this branch first cause it's big enough already
> b. Let's see how it looks with 2 or 3 upgrade plugins so we can get a picture
of
> how best to do any sub packaging.