Expose a top-level abstraction for all the backups functionality. #708
Conversation
ericsnowcurrently
force-pushed
the
backups-create
branch
from
365685c
to
a9512ff
Compare
ericsnowcurrently
changed the title
| var ( | ||
| getFilesToBackUp = files.GetFilesToBackUp | ||
| getDBDumper (func(db.ConnInfo) db.Dumper) = db.NewDumper | ||
| runCreate = create |
There was a problem hiding this comment.
this seems unnecessary to me (runCreate)
There was a problem hiding this comment.
It's for testing. Otherwise I agree.
|
LGTM, my review mentor is @fwereade |
| if err != nil { | ||
| return nil, errors.Annotate(err, "error listing files to back up") | ||
| } | ||
| dumper := getDBDumper(*dbInfo) |
There was a problem hiding this comment.
Manually dereferencing a pointer is almost always a sign that something is wrong. IIRC, the ConnInfo doesn't really need to be a pointer anyway (it's not a large value... and we aren't going to be passing a lot of them around regardless), so maybe just pass it in as a value?
There was a problem hiding this comment.
Fine with me.
|
LGTM. I don't think the abstraction is really necessary or beneficial, but I don't think it's the end of the world, either. |
|
LGTM. |
|
Thanks, all, for your reviews. Once we're unblocked I'll get this merged. |
| filesToBackUp, err := getFilesToBackUp("") | ||
| if err != nil { | ||
| return nil, errors.Annotate(err, "error listing files to back up") | ||
| } |
|
Thanks for the review, Jesse, and especially for pointing out the missing tests. Since I want to keep some internal types private, the solution was kind of messy. However, it's not too bad. |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Does not match ['fixes-1366802'] |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Does not match ['fixes-1366802'] |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Does not match ['fixes-1366802'] |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Expose a top-level abstraction for all the backups functionality. This patch adds a top-level backups abstraction. It also exposes the backup creation functionality through a method. The PR that will follow this one: ericsnowcurrently#4
This patch adds a top-level backups abstraction. It also exposes the backup creation functionality through a method.
The PR that will follow this one:
ericsnowcurrently#4