WIP: Add the client side of the backup API. #200
Conversation
| // XXXX what to do if the digest header is missing? | ||
| // just log and return? (Seems hostile to delete it.) | ||
| digest := resp.Header.Get("Digest") | ||
| if digest == "" || digest[:4] != "SHA=" { |
There was a problem hiding this comment.
Use strings.HasPrefix instead of slicing the string to check the beginning.
|
Is there a plan to merge this into 1.20 once it's merged in to trunk? It seems a shame that all of backup is in the release except for the client side part. |
|
I don't know much about our release cycle mechanics, but I certainly don't have a problem with getting this patch (and the |
| } | ||
|
|
||
| //--------------------------- | ||
| // Success tests |
There was a problem hiding this comment.
These comments are not needed. It should be evident from the function name what we are testing and if it is expected to succeed or fail.
This goes for all the 'header' type comments in this file.
There was a problem hiding this comment.
While I agree they aren't strictly necessary and the function names give sufficient information, I will argue that the comments make the code more readable. I've grouped the tests/helpers and labeled the groups. Do you find the comments to be a problem?
There was a problem hiding this comment.
The comments are just flagging the structure of the file. I don't see a
precedent for this in other test suites - all of which have a similar
structure to backup_test.go.
In state/api/backup_test.go:
- s.PatchValue(&apiserver.Backup, succeed)
+}
+func (s *clientSuite) setBackupServerError(err error) {
- fail := func(pw, user, dir, addr string) (string, string, error) {
return "", "", err- }
- s.PatchValue(&apiserver.Backup, fail)
+}
+func (s *clientSuite) removeBackupArchive(filename string) {
- s.AddCleanup(func(*gc.C) { os.Remove(filename) })
+}
+//---------------------------
+// Success testsWhile I agree they aren't strictly necessary and the function names
give sufficient information, I will argue that the comments make the
code more readable. I've grouped the tests/helpers and labeled the
groups. Do you find the comments to be a problem?—
Reply to this email directly or view it on GitHub
https://github.com/juju/juju/pull/200/files#r14457742.
There was a problem hiding this comment.
I appreciate the feedback but I'm still not clear on why it's a problem. Personally I find that the comments delineating different groups of tests are helpful, particularly since Go really doesn't seem to give a better alternative to grouping. If it's an issue of precedent, I'd be glad to set one. :)
|
At a first glance, I don't think this is worthy of a 1.20 release backport. (As it is significantly more features than just a bugfix). We want to get into a better cadence of making releases, and delaying releases because we want to get one more thing gets us into the situation where we don't have releases for a significant period of time. (All that other good work that did make it in so far ends up delayed because of yet-another improvement.) I'd rather see us make it easier to get 1.21 out the door in a reasonable timeframe. That said, if there is clamoring for the full Backup code to be available in a 1.20 series, then we could arrange for it to land into a 1.20.1. I'd really like us to be on 'new features end up in a new Minor version, only bugfixes in a Patch version'. |
| // the local filesystem. It returns the name of the file created. | ||
| // The backup can take a long time to prepare and be a large file, depending | ||
| // on the system being backed up. | ||
| func (c *Client) Backup(backupFilePath string, validate bool) (string, error) { |
There was a problem hiding this comment.
Since this didn't actually land for the 1.20 release, I'd really like to see us go with "smaller more focused facades". So rather than being Client.Backup and Client.Restore, which both don't have anything to do with all the other methods on Client, I'd rather see a Backup facade be introduced and have us using it over there.
I realize this is way too late to be introducing this concept, and I'm sorry I didn't catch it earlier.
We can still move towards that, I think, and hopefully the API versioning code will finish up in the next day or two and we can make use of that to give ourselves the API we want instead of limping along with just the one we have.
That would also end up moving the objects here from being in state/api/client.go to something more like state/api/backup/backup.go
There was a problem hiding this comment.
Could we take this up separately?
| */ | ||
| func (c *Client) getRawRPCRequest(httpMethod string, method string) (*http.Request, error) { | ||
| url := fmt.Sprintf("%s/%s", c.st.serverRoot, method) | ||
| req, err := http.NewRequest(httpMethod, url, nil) |
There was a problem hiding this comment.
I'm not sure if we captured this for compatibility purposes, but since 'backup' is a new API anyway, you really want to be posting to https://address:port/environ/UUID/backup and not just /backup
We have the latter methods for backwards compatibility, but that isn't needed here.
In fact, we probably shouldn't even expose backup at "/backup" anymore.
There was a problem hiding this comment.
That is really good to know. I'll fix it.
There was a problem hiding this comment.
So it should look like this (assuming you meant "environment" rather than "environ")?
url := fmt.Sprintf("%s/environment/%s/%s", c.st.serverRoot, uuid, method)
From where do I get uuid?
|
I have quite a few comments about how this is structured. Hopefully it is constructive feedback. You're certainly welcome to ping me on IRC, etc if you want to have a more direct conversation. |
|
I mentioned this in another comment but I'll bring it up again. Should we yank /backup from 1.20? I'm pretty sure it's new in that release. |
|
|
||
| validate := false | ||
| filename, err := client.Backup(expected, validate) | ||
| if filename != "" { |
There was a problem hiding this comment.
You'll get a panic here if client.Backup errors out . Handle err as soon as you get it: move c.Assert(err, gc.IsNil) to line 93.
There was a problem hiding this comment.
Why would it panic?
Keep in mind that at this point I want to make sure the backup file (if there is one) is queued up for removal before I do anything that would stop execution (and leave the file cluttering up your filesystem after running the test).
There was a problem hiding this comment.
Hey, I appreciate your attentiveness. :)
There was a problem hiding this comment.
Thanks for that. I'll try not to throw too many red herrings out there!
ericsnowcurrently
changed the title
|
@ericsnowcurrently: I'm contemplating adding a header to the backup API response so that the server provides the backup file name (it's already generating one which is then thrown away). It seems that having the server provide the backup filename would also simplify the changes in this PR. For upcoming Juju upgrade changes the client will take a backup before starting an upgrade and backups will be kept server side as well as being downloaded to the client. The upgrade process needs to know the name of the backup that was taken just before the upgrade started so I'd like the server to return the name to the client. It seems to me that using the same backup filename on both the server and client would be wise. What do you think? |
|
For reference, here's the PR which adds the filename to the backup API response: |
|
Some of the work from this PR can be addressed later. The core functionality has been addressed in PR #334. |
Fix learn to be highlighted when on /docs/sdk
This builds on the work of Michael Foord. I've reorganized the code a little, added tests and factored out the raw RPC code from the backup client code. (The raw RPC can later be re-used by the other 2 client methods that currently duplicate the code.)