Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
make the move-syslog-config upgrade more lenient #2476
Conversation
mjs
reviewed
Jun 3, 2015
| + if err := osRemove(oldpath); err != nil { | ||
| + // Don't fail the step if we can't get rid of the old files. | ||
| + // We don't actually care if they still exist or not. | ||
| + logger.Infof("Can't delete old config file %q: %s", oldpath, err) |
wallyworld
reviewed
Jun 3, 2015
| + | ||
| + // ensure that we don't error out if we can't remove the old file. | ||
| + // error out if one of the files exists in datadir but not logdir. | ||
| + *upgrades.OsRemove = func(string) error { return os.ErrPermission } |
mjs
reviewed
Jun 3, 2015
| + | ||
| + // should still exist in both places (i.e. check we didn't screw up the test) | ||
| + _, err = os.Stat(file) | ||
| + c.Assert(err, gc.IsNil) |
mjs
reviewed
Jun 3, 2015
| + _, err = os.Stat(file) | ||
| + c.Assert(err, gc.IsNil) | ||
| + _, err = os.Stat(filepath.Join(logdir, "logrotate.conf")) | ||
| + c.Assert(err, gc.IsNil) |
mjs
reviewed
Jun 3, 2015
| + | ||
| + ctx := fakeContext{cfg: fakeConfig{logdir: logdir, datadir: datadir}} | ||
| + err = upgrades.MoveSyslogConfig(ctx) | ||
| + c.Assert(err, gc.IsNil) |
mjs
reviewed
Jun 3, 2015
| + defer func() { *upgrades.OsRemove = os.Remove }() | ||
| + | ||
| + err := ioutil.WriteFile(file, data, 0644) | ||
| + c.Assert(err, gc.IsNil) |
wallyworld
reviewed
Jun 3, 2015
| + // should still exist in both places (i.e. check we didn't screw up the test) | ||
| + _, err = os.Stat(file) | ||
| + c.Assert(err, gc.IsNil) | ||
| + _, err = os.Stat(filepath.Join(logdir, "logrotate.conf")) |
wallyworld
Jun 3, 2015
Owner
isn't file = filepath.Join(logdir, "logrotate.conf")
so these 2 Stat()s are the same?
mjs
reviewed
Jun 3, 2015
| +func (s *steps124Suite) TestMoveSyslogConfigCantDeleteOld(c *gc.C) { | ||
| + logdir := c.MkDir() | ||
| + datadir := c.MkDir() | ||
| + data := []byte("data!") |
mjs
Jun 3, 2015
Contributor
nit: given that this is a) obvious and b) only used once, surely you don't need a variable for it
|
LGTM |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Does not match ['fixes-1461150', 'fixes-1461246'] |
|
fixes 1461246 |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Does not match ['fixes-1461150', 'fixes-1461246'] |
|
fixes-1461246 |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$nobutseriously$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
added a commit
that referenced
this pull request
Jun 3, 2015
jujubot
merged commit f479fac
into
juju:1.24
Jun 3, 2015
natefinch
deleted the
natefinch:upgrade-fix
branch
Jun 3, 2015
added a commit
that referenced
this pull request
Jul 14, 2015
natefinch commentedJun 3, 2015
…about not being able to delete old config files. We don't actually care overly much if the old ones are left around, so now we just log if we can't delete them, rather than screwing the upgrade because of it.