feat: implement storage management commands #1587
Conversation
sweatybridge
force-pushed
the
cmd-storage
branch
from
de5b3fa to
d44445c
Compare
Pull Request Test Coverage Report for Build 6613802796
💛 - Coveralls |
sweatybridge
force-pushed
the
cmd-storage
branch
2 times, most recently
from
c62e744 to
d9f94cc
Compare
sweatybridge
force-pushed
the
cmd-storage
branch
2 times, most recently
from
1ce3edc to
41b1273
Compare
sweatybridge
force-pushed
the
cmd-storage
branch
2 times, most recently
from
861309c to
490439d
Compare
sweatybridge
force-pushed
the
cmd-storage
branch
from
490439d to
9b14b1f
Compare
sweatybridge
force-pushed
the
cmd-storage
branch
from
ccc4373 to
e2f9cb6
Compare
| } | ||
| ) | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
I'm not sure if moving directory shouldn require a recursive call tbh, as this is not the behavior people are used to. Doing mv foo bar if foo is a directory will work in any unix environment. Only cp and rm require this flag.
There was a problem hiding this comment.
Agree on the difference. I was a bit reluctant to make mv recursive by default because it would mean at least 2 network calls to copy a file, ie. first call tries to move as a file and then second call to move as a directory.
I find adding a -r flag isn't too bad in this case because it's more consistent with other commands. Also we are only supporting moving within the same bucket (no local -> remote) so I hope the expectation doesn't need to be exactly like unix.
If it turns out to be a big inconvenience, reverting to unix behaviour would not be a breaking change for those who already use the -r flag. So this is probably ok for now.
| } | ||
| fmt.Fprintln(os.Stderr, "Uploading:", filePath, "=>", dstPath) | ||
| err = client.UploadStorageObject(ctx, projectRef, dstPath, filePath, fsys) | ||
| if err != nil && strings.Contains(err.Error(), `"error":"Bucket not found"`) { |
There was a problem hiding this comment.
Tying ourselves to the exact error message is not great imo. Can we at least assert on http code instead? It's also available from what I see. Same for 2 different places.
There was a problem hiding this comment.
404 could either be thrown from http path not found or bucket doesn't exist. We can add the status code as an additional check but eventually we need a more specific error code to distinguish the two.
| } | ||
| fmt.Fprintln(os.Stderr, "Uploading:", filePath, "=>", dstPath) | ||
| err = client.UploadStorageObject(ctx, projectRef, dstPath, filePath, fsys) | ||
| if err != nil && strings.Contains(err.Error(), `"error":"Bucket not found"`) { |
There was a problem hiding this comment.
I'm not sure if we should always create a bucket without any additional flag, but from UX perspective it sounds ok.
sweatybridge
force-pushed
the
cmd-storage
branch
from
9a3c75a to
a7e473a
Compare
| @@ -49,6 +49,10 @@ func Run(ctx context.Context, paths []string, recursive bool, fsys afero.Fs) err | |||
| return err | |||
| } | |||
| for bucket, prefixes := range groups { | |||
| confirm := fmt.Sprintf("Confirm deleting files in bucket %v?", utils.Bold(bucket)) | |||
There was a problem hiding this comment.
I like that it handles non-TTY by default 😌
|
This is just what I need (I think), are there any docs? |
What kind of change does this PR introduce?
feature
closes #1388
What is the new behavior?
Implements storage
ls/cp/mv/rmas proposed.Additional context
Not implemented in this PR: