Merge lp://staging/~stub/charms/precise/postgresql/bug-1281600-log_temp_files into lp://staging/charms/postgresql

Proposed by Stuart Bishop
Status: Superseded
Proposed branch: lp://staging/~stub/charms/precise/postgresql/bug-1281600-log_temp_files
Merge into: lp://staging/charms/postgresql
Prerequisite: lp://staging/~stub/charms/precise/postgresql/bug-1329816-backup-log-spurious-alert
Diff against target: 31 lines (+11/-0)
2 files modified
config.yaml (+8/-0)
templates/postgresql.conf.tmpl (+3/-0)
To merge this branch: bzr merge lp://staging/~stub/charms/precise/postgresql/bug-1281600-log_temp_files
Reviewer Review Type Date Requested Status
Cory Johns (community) Needs Information
Chris Glass (community) Approve
charmers Pending
Review via email: mp+224250@code.staging.launchpad.net

This proposal has been superseded by a proposal from 2014-09-08.

Description of the change

Address Bug #1281600 by adding the log_temp_files configuration option.

To post a comment you must log in.
144. By Stuart Bishop

Merged bug-1329816-backup-log-spurious-alert into bug-1281600-log_temp_files.

145. By Stuart Bishop

Merged bug-1329816-backup-log-spurious-alert into bug-1281600-log_temp_files.

146. By Stuart Bishop

Merged bug-1329816-backup-log-spurious-alert into bug-1281600-log_temp_files.

Revision history for this message
Chris Glass (tribaal) wrote :

Looks good to me! Clean addition of a charm parameter, it does what is written on the tin.

review: Approve
147. By Stuart Bishop

Merged bug-1329816-backup-log-spurious-alert into bug-1281600-log_temp_files.

148. By Stuart Bishop

Merged bug-1329816-backup-log-spurious-alert into bug-1281600-log_temp_files.

149. By Stuart Bishop

Merged bug-1329816-backup-log-spurious-alert into bug-1281600-log_temp_files.

150. By Stuart Bishop

Merged bug-1329816-backup-log-spurious-alert into bug-1281600-log_temp_files.

Revision history for this message
Cory Johns (johnsca) wrote :

+1 LGTM

review: Approve
Revision history for this message
Cory Johns (johnsca) wrote :

> +1 LGTM

Upon further review, there are quite a few more changes being included in this MP from the prerequisite branch. There was a merged MP on that branch from around the time this MP was submitted, and the subsequent changes to the prerequisite branch don't necessarily seem related to this change. Those changes (changing and resyncing the charm-helpers branch) seem fine, but the ambiguity makes me less sure about recommending this MP.

Additionally, I wasn't able to get the tests passing, even after doing `make testdeps`. This doesn't seem related to this change, so I'm not certain it should block this MP, but it definitely should be addressed.

Ideally, I would recommend removing the prerequisite branch and including any applicable changes explicitly in this MP. Switching my review to "Needs Information" for clarification and / or removing the prerequisite branch. If you can get `make test` working on a fresh env, as well, that would be even better.

review: Needs Information
Revision history for this message
Stuart Bishop (stub) wrote :

@johnsca

I'm using pipelines and dependant branches to keep the merge proposals to managable size. I'm more than happy to handle landing once the merge proposal and all its dependencies have been approved. In case you would rather review everything as one big diff, I've created https://code.launchpad.net/~stub/charms/precise/postgresql/integration/+merge/233666 .

The tests should run, and do run here with lxc. Important points:
- The default juju environment needs to be bootstrapped
- Bad host keys need to be cleaned out of ~/.ssh/known_hosts and ~root/.ssh/known_hosts (per the outstanding juju bugs)
- Strict host key checking needs to be enabled (or SSH implicitly disables port forwarding, which the tests need).

Revision history for this message
Stuart Bishop (stub) wrote :

Unexpected changes come from:

 - Another charmhelpers update I neglected to land earlier ( http://pastebin.ubuntu.com/8287937/ )
 - Some dead code removal removal ( http://pastebin.ubuntu.com/8287920/ )

151. By Stuart Bishop

Merged bug-1329816-backup-log-spurious-alert into bug-1281600-log_temp_files.

152. By Stuart Bishop

Merged bug-1329816-backup-log-spurious-alert into bug-1281600-log_temp_files.

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches