Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(111)

Issue 90660043: Look for mongo* executables when not in PATH

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 11 months ago by hduran
Modified:
9 years, 11 months ago
Reviewers:
mp+216920, jameinel
Visibility:
Public.

Description

Look for mongo* executables when not in PATH Backup is performed by a bash script that, among other steps, dumps the mongodb, this makes use of mongodump and mongoexport binaries which are provided by the package juju-mongodb and placed in a path not included in PATH. The backup works on some versions of juju that use mongodb-server instead, I added a check to make sure that, if present, we use the binaries provided by juju-mongodb https://code.launchpad.net/~hduran-8/juju-core/1305780_backup_fails_on_trusty/+merge/216920 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -2 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/plugins/juju-backup/juju-backup View 1 chunk +14 lines, -2 lines 1 comment Download

Messages

Total messages: 3
hduran
Please take a look.
9 years, 11 months ago (2014-04-23 16:04:35 UTC) #1
jameinel
Provided you tested this live LGTM Though I do wonder about -x vs -f. https://codereview.appspot.com/90660043/diff/1/cmd/plugins/juju-backup/juju-backup ...
9 years, 11 months ago (2014-04-23 17:23:03 UTC) #2
hduran
9 years, 11 months ago (2014-04-23 18:20:33 UTC) #3
On 2014/04/23 17:23:03, jameinel wrote:
> Provided you tested this live LGTM
> Though I do wonder about -x vs -f.
> 
>
https://codereview.appspot.com/90660043/diff/1/cmd/plugins/juju-backup/juju-b...
> File cmd/plugins/juju-backup/juju-backup (right):
> 
>
https://codereview.appspot.com/90660043/diff/1/cmd/plugins/juju-backup/juju-b...
> cmd/plugins/juju-backup/juju-backup:46: if [ -f /usr/lib/juju/bin/mongodump ];
> then
> I wonder if we actually want to use "-x" instead of "-f" since we really do
need
> those to be executable, though if they are just bad files in that location
that
> would be pretty bad for us.

Well, -f checks that they are there and are files, which is something already.
We should see the whole thing break if we try to execute them and the files are
not executable, which is implicitly the same as testing for -x as we would get 
Permission denied so that covers pretty much all possible and not so much
situations.

I have tested this with the latest trunk merged.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b