Merge lp:~letterj/swift/st_stream into lp:~hudson-openstack/swift/trunk

[Jay Payne]

Added "-o" option to the st tool

The option "-o <filename>" will send the object data to a file named <filename>
The option "-o -" will send the object data to standard out

Special Note: This option only works with a single file download.

[clayg (clay-gerrard)]

This is a great enhancement, nice work letterj. Also, you had mentioned some problems when you were setting up the branch, but I didn't have any issues merging these changes against trunk, very clean.

I found the help string for the -o option confusing at first:
"For a single file download stream the output other location"

I read it as "file download stream" - might be easier to read with a comma:
"For a single file download, stream the output to other location"

But I'm not sure about limiting all stream downloads to only a single file at a time? I think there's a use case for multi-file download to stdout (like if you want to pipe a stream of multiple log files to grep).

Whatever you want to do, I definitely think that we should return an error if the user supplies incompatible input (i.e. multiple file download single out_file filename) instead of guessing (i.e. silently ignore the -o option). Maybe you could add a test for it right at the top of st_download next to the "if (not args ..." bit?

Last thing, an implementation detail... I had to re-read the section at the top of _download_container where you're "inferring the behavior based on the length of the item coming off the queue" a couple of times before I got what was going on. That may not mean much tho, I'm kinda dense and tend to struggle with trixy python code, plus I'm not that familiar with st - so take this with a grain of salt - but I think it would be more readable to *me* if you just added an optional keyword argument to _download_container (i.e. http://paste.openstack.org/show/140/)

Let me know if you have any questions.

[Chuck Thier (cthier)]

works for me

review: Approve

[Chuck Thier (cthier)]

> This is a great enhancement, nice work letterj. Also, you had mentioned some
> problems when you were setting up the branch, but I didn't have any issues
> merging these changes against trunk, very clean.
>
> I found the help string for the -o option confusing at first:
> "For a single file download stream the output other location"
>
> I read it as "file download stream" - might be easier to read with a comma:
> "For a single file download, stream the output to other location"
>
> But I'm not sure about limiting all stream downloads to only a single file at
> a time? I think there's a use case for multi-file download to stdout (like if
> you want to pipe a stream of multiple log files to grep).
>
> Whatever you want to do, I definitely think that we should return an error if
> the user supplies incompatible input (i.e. multiple file download single
> out_file filename) instead of guessing (i.e. silently ignore the -o option).
> Maybe you could add a test for it right at the top of st_download next to the
> "if (not args ..." bit?
>
> Last thing, an implementation detail... I had to re-read the section at the
> top of _download_container where you're "inferring the behavior based on the
> length of the item coming off the queue" a couple of times before I got what
> was going on. That may not mean much tho, I'm kinda dense and tend to
> struggle with trixy python code, plus I'm not that familiar with st - so take
> this with a grain of salt - but I think it would be more readable to *me* if
> you just added an optional keyword argument to _download_container (i.e.
> http://paste.openstack.org/show/140/)
>
> Let me know if you have any questions.

Streaming several files we can add as a future feature request.

=== modified file 'bin/st'
--- bin/st	2010-10-07 16:28:59 +0000
+++ bin/st	2010-11-11 23:43:56 +0000
@@ -821,7 +821,7 @@
 from os import environ, listdir, makedirs, utime
 from os.path import basename, dirname, getmtime, getsize, isdir, join
 from Queue import Empty, Queue
-from sys import argv, exit, stderr
+from sys import argv, exit, stderr, stdout
 from threading import enumerate as threading_enumerate, Thread
 from time import sleep
 
@@ -969,14 +969,23 @@
 st_download_help = '''
 download --all OR download container [object] [object] ...
     Downloads everything in the account (with --all), or everything in a
-    container, or a list of objects depending on the args given.'''.strip('\n')
+    container, or a list of objects depending on the args given. Use
+    the -o [--output] <filename> option to redirect the output to a file 
+    or if "-" then the just redirect to stdout.  '''.strip('\n')
 def st_download(options, args):
     if (not args and not options.yes_all) or (args and options.yes_all):
         options.error_queue.put('Usage: %s [options] %s' %
                                 (basename(argv[0]), st_download_help))
         return
     object_queue = Queue(10000)
-    def _download_object((container, obj), conn):
+    def _download_object(queue_arg, conn):
+        if len(queue_arg) == 2: 
+            container, obj = queue_arg
+            out_file = None
+        elif len(queue_arg) == 3:
+            container, obj, out_file = queue_arg
+        else:
+            raise Exception("Invalid queue_arg length of %s" % len(queue_arg)) 
         try:
             headers, body = \
                 conn.get_object(container, obj, resp_chunk_size=65536)
@@ -998,7 +1007,12 @@
                 dirpath = dirname(path)
                 if dirpath and not isdir(dirpath):
                     mkdirs(dirpath)
-                fp = open(path, 'wb')
+                if out_file == "-":
+                    fp = stdout
+                elif out_file:
+                    fp = open(out_file, 'wb')
+                else:
+                    fp = open(path, 'wb')
                 read_length = 0
                 md5sum = md5()
                 for chunk in body :
@@ -1013,7 +1027,7 @@
                 options.error_queue.put(
                     '%s: read_length != content_length, %d != %d' %
                     (path, read_length, content_length))
-            if 'x-object-meta-mtime' in headers:
+            if 'x-object-meta-mtime' in headers and not options.out_file:
                 mtime = float(headers['x-object-meta-mtime'])
                 utime(path, (mtime, mtime))
             if options.verbose:
@@ -1070,8 +1084,12 @@
     elif len(args) == 1:
         _download_container(args[0], create_connection())
     else:
-        for obj in args[1:]:
-            object_queue.put((args[0], obj))
+        if len(args) == 2:
+            obj = args[1]
+            object_queue.put((args[0], obj, options.out_file))
+        else:
+            for obj in args[1:]:
+                object_queue.put((args[0], obj))
     while not container_queue.empty():
         sleep(0.01)
     for thread in container_threads:
@@ -1438,10 +1456,14 @@
                       help='User name for obtaining an auth token')
     parser.add_option('-K', '--key', dest='key',
                       help='Key for obtaining an auth token')
+    parser.add_option('-o', '--output', dest='out_file',
+                      help='For a single file download stream the output other location ')
     args = argv[1:]
     if not args:
         args.append('-h')
     (options, args) = parser.parse_args(args)
+    if options.out_file == '-':
+        options.verbose = 0 
 
     required_help = '''
 Requires ST_AUTH, ST_USER, and ST_KEY environment variables be set or