Change _ArchiveTestResults to upload to Google Storage

Change _ArchiveTestResults to upload to Google Storage

BUG=chromium-os:8364
TEST=

Change-Id: Icecc8268fa2c12c8413caa2879785f00e4be5a0a

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=ac13647

[sosa, 2010-11-12 18:49:09]

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode404
bin/cbuildbot.py:404: def _gsutil_with_retries(command, num_retries):
Name style is wrong

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode410
bin/cbuildbot.py:410: command: Command to run
Command should be an array

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode416
bin/cbuildbot.py:416: return
No explicit return

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode418
bin/cbuildbot.py:418: Warning('Retrying command')
Print out command you are retrying.

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode446
bin/cbuildbot.py:446: RunCommand([gsutil, 'setacl', acl, archive_dir])
All these try's should be in the same try clause probably.  If gsutil is being
flaky in one try it'll prob be flaky again right after.

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode507
bin/cbuildbot.py:507: parser.add_option('-a', '--acl', default='',
Why should this be user specified?  SHouldn't we have a default value?

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode558
bin/cbuildbot.py:558: archive_dir=options.gsutil_archive + '/' +
use os.path.join here

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode559
bin/cbuildbot.py:559: str(options.buildnumber),
is str() needed here?

[scottz-goog, 2010-11-12 19:15:02]

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode404
bin/cbuildbot.py:404: def _gsutil_with_retries(command, num_retries):
This will is just a RunCommandWithRetries and you are calling it with the gsutil
binary

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode404
bin/cbuildbot.py:404: def _gsutil_with_retries(command, num_retries):
While I know this file doesn't follow it exactly we should start convering on
what we've started using as the "Standard" for docstrings

"""One line describing the function.

More indepth description if needed.

Arguments:
  command: a list of commands/args to execute
  num_retries: the number of retries to perform before dying

Returns:
  Only use this is we are returning something otherwise drop it.

Raises:
   Only use this if something is raised 

etc etc.

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode442
bin/cbuildbot.py:442: except:
We should never be catching blank exceptions. You are calling RunCommand in both
instances so we should be catching the exception it throws. When you catch
blanket exceptions you run the risk of masking problems. I've definitely created
a few "mystery" problems by doing this.

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode447
bin/cbuildbot.py:447: except:
see above

http://codereview.chromium.org/4864001/diff/1/bin/cbuildbot.py#newcode457
bin/cbuildbot.py:457: except:
See above.

[thieule, 2010-11-12 21:33:25]

I've addressed the code review feedbacks and moved the gsutil retry function to
cros_build_lib.py so it lives with RunCommand() and renamed it to
RunCommandWithRetries().  Only one comment that I couldn't fix is the str().  If
I don't use str(options.buildnumber), python complains.

--Thieu

[sosa, 2010-11-12 21:41:45]

Looks much better.  You may wanna consider adding retries as an optional option
to RunCommand rather than a whole new functions.  That'll allow you to re-use
this function much more for different use cases that RunCommand can handle.

http://codereview.chromium.org/4864001/diff/6001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4864001/diff/6001/bin/cbuildbot.py#newcode411
bin/cbuildbot.py:411: Arguments:
buildroot?

http://codereview.chromium.org/4864001/diff/6001/bin/cbuildbot.py#newcode538
bin/cbuildbot.py:538: archive_dir=archive_full_path,
You have tabs here.  This is bad.

[thieule, 2010-11-12 23:23:24]

I've added num_retries to RunCommand and deleted RunCommandWithRetries.

[sosa, 2010-11-12 23:33:08]

http://codereview.chromium.org/4864001/diff/14001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4864001/diff/14001/bin/cbuildbot.py#newcode536
bin/cbuildbot.py:536: _ArchiveTestResults(buildroot, buildconfig['board'],
Now that you are pushing to gsutil ... add a:

"if not options.debug" here so that someone with --debug (for non-buildbot calls
of this code) doesn't inadvertantly upload their test results to gs

http://codereview.chromium.org/4864001/diff/14001/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/4864001/diff/14001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:85: if print_cmd:
Little easier to read if this is in the else: of your except clause.

[scottz-goog, 2010-11-12 23:37:13]

I'd say if you wanted to fight for world peace fix the Exception that we are
generically raising. Since anyone who is catching it currently would be
(incorrectly) catching Exception it won't affect them and we can incrementally
migrate them.

If not I understand :) these updates are great, especially adding the
functionality to the library!

LGTM

http://codereview.chromium.org/4864001/diff/14001/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/4864001/diff/14001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:76: else:
No need for the else here.

http://codereview.chromium.org/4864001/diff/14001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:79: except Exception, e:
Ugg do we really not raise a RunCommand Exception? My heart tells me this should
be fixed but I realize it has been this way for ever :(

[thieule, 2010-11-13 00:21:32]

I'm trying to save the world...

http://codereview.chromium.org/4864001/diff/14001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/4864001/diff/14001/bin/cbuildbot.py#newcode536
bin/cbuildbot.py:536: _ArchiveTestResults(buildroot, buildconfig['board'],
On 2010/11/12 23:33:09, sosa wrote:
> Now that you are pushing to gsutil ... add a:
> 
> "if not options.debug" here so that someone with --debug (for non-buildbot
calls
> of this code) doesn't inadvertantly upload their test results to gs

Done.

http://codereview.chromium.org/4864001/diff/14001/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/4864001/diff/14001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:76: else:
On 2010/11/12 23:37:13, scottz-goog wrote:
> No need for the else here. 

Done.

http://codereview.chromium.org/4864001/diff/14001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:79: except Exception, e:
On 2010/11/12 23:37:13, scottz-goog wrote:
> Ugg do we really not raise a RunCommand Exception? My heart tells me this
should
> be fixed but I realize it has been this way for ever :( 

Done.

http://codereview.chromium.org/4864001/diff/14001/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:85: if print_cmd:
On 2010/11/12 23:33:09, sosa wrote:
> Little easier to read if this is in the else: of your except clause.

Done.

[scottz-goog, 2010-11-15 18:04:25]

LGTM just update the doc string :P 

Thanks for doing this!

http://codereview.chromium.org/4864001/diff/19002/lib/cros_build_lib.py
File lib/cros_build_lib.py (right):

http://codereview.chromium.org/4864001/diff/19002/lib/cros_build_lib.py#newco...
lib/cros_build_lib.py:52: Exception:  Raises generic exception on error with
optional error_message.
RunCommandException

[thieule, 2010-11-23 01:35:05]

Fix unit test to work with new _ArchiveTestResults

[scottz, 2010-11-24 18:33:45]

LGTM
On 2010/11/23 01:35:05, thieule wrote:
> Fix unit test to work with new _ArchiveTestResults