Update prebuilt.py to handle private google storage uploads.

Update prebuilt.py to handle private google storage uploads.

Remove boto configuration, anyone using this script should either have ~/.boto set
or they should be starting it with BOTO_CONFIG=....

Add in support for acl xml files, If we pass an ACL xml file we first upload the file to
google storage with the canned acl of private then apply the ACL.

Automatically derive the binhost_url for private Google Storage uploads and warn
if people set --private and --binhost-base-url.

Update unittests to pass with the new private assumptions.

BUG=NA
TEST=Ran on a canary build chroot to make sure we upload and set URLs properly

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

[scottz, 2011-03-14 22:02:23]

+dparker

[davidjames, 2011-03-14 22:31:44]

LGTM w/nits

http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py
File buildbot/prebuilt.py (right):

http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py#newcode288
buildbot/prebuilt.py:288: cmd = '%s cp -a private %s %s' % (_GSUTIL_BIN,
local_file, remote_file)
Can we check that the acl file exists in this case? If it doesn't then we should
fail out...

http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py#newcode636
buildbot/prebuilt.py:636: if (options.binhost_base_url != _BINHOST_BASE_URL):
No parens needed here.

[scottz, 2011-03-14 22:57:45]

PTAL

http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py
File buildbot/prebuilt.py (right):

http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py#newcode288
buildbot/prebuilt.py:288: cmd = '%s cp -a private %s %s' % (_GSUTIL_BIN,
local_file, remote_file)
I actually had that around here and I moved it down to the optparse verification
step. Do you want it here again in case it disappears during the run?

On 2011/03/14 22:31:44, davidjames wrote:
> Can we check that the acl file exists in this case? If it doesn't then we
should
> fail out...

http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py#newcode636
buildbot/prebuilt.py:636: if (options.binhost_base_url != _BINHOST_BASE_URL):
On 2011/03/14 22:31:44, davidjames wrote:
> No parens needed here.

Done.

[davidjames, 2011-03-16 18:48:32]

Still LGTM, but a suggestion.

http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py
File buildbot/prebuilt.py (right):

http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py#newcode288
buildbot/prebuilt.py:288: cmd = '%s cp -a private %s %s' % (_GSUTIL_BIN,
local_file, remote_file)
On 2011/03/14 22:57:45, scottz wrote:
> I actually had that around here and I moved it down to the optparse
verification
> step. Do you want it here again in case it disappears during the run?

I'm not worried about it disappearing during the run. But I think it would be
good to just add an assert here to verify that we have a sane input. That is
helpful just for future-proofing the code I think.

[scottz, 2011-03-16 19:02:24]

On 2011/03/16 18:48:32, davidjames wrote:
> Still LGTM, but a suggestion.
> 
> http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py
> File buildbot/prebuilt.py (right):
> 
> http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py#newcode288
> buildbot/prebuilt.py:288: cmd = '%s cp -a private %s %s' % (_GSUTIL_BIN,
> local_file, remote_file)
> On 2011/03/14 22:57:45, scottz wrote:
> > I actually had that around here and I moved it down to the optparse
> verification
> > step. Do you want it here again in case it disappears during the run?
> 
> I'm not worried about it disappearing during the run. But I think it would be
> good to just add an assert here to verify that we have a sane input. That is
> helpful just for future-proofing the code I think.

When you say sane input do you mean you think I should validate that the file
that someone passed is an XML file? If that is the case I think that is a bit
over kill. The script asks for this type of input and the user decides not to
adhere to that, things break. 

If you just want me to assert that the file exists up top also can you explain
why that is preferred. I am asking because if there is a valid reason I just
don't understand it and I would like to.

-Scott

[scottz, 2011-03-16 21:29:01]

PTAL, per our discussion I added a bit more error checking and a message should
we end up in a state where canned acls were not specific and the acl file is not
valid.


On 2011/03/16 19:02:24, scottz wrote:
> On 2011/03/16 18:48:32, davidjames wrote:
> > Still LGTM, but a suggestion.
> > 
> > http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py
> > File buildbot/prebuilt.py (right):
> > 
> >
http://codereview.chromium.org/6677023/diff/1/buildbot/prebuilt.py#newcode288
> > buildbot/prebuilt.py:288: cmd = '%s cp -a private %s %s' % (_GSUTIL_BIN,
> > local_file, remote_file)
> > On 2011/03/14 22:57:45, scottz wrote:
> > > I actually had that around here and I moved it down to the optparse
> > verification
> > > step. Do you want it here again in case it disappears during the run?
> > 
> > I'm not worried about it disappearing during the run. But I think it would
be
> > good to just add an assert here to verify that we have a sane input. That is
> > helpful just for future-proofing the code I think.
> 
> When you say sane input do you mean you think I should validate that the file
> that someone passed is an XML file? If that is the case I think that is a bit
> over kill. The script asks for this type of input and the user decides not to
> adhere to that, things break. 
> 
> If you just want me to assert that the file exists up top also can you explain
> why that is preferred. I am asking because if there is a valid reason I just
> don't understand it and I would like to.
> 
> -Scott

[davidjames, 2011-03-16 21:29:39]

LGTM w/nits

http://codereview.chromium.org/6677023/diff/5002/buildbot/prebuilt.py
File buildbot/prebuilt.py (right):

http://codereview.chromium.org/6677023/diff/5002/buildbot/prebuilt.py#newcode292
buildbot/prebuilt.py:292: 'unknown canned acl. %s aborting upload') % acl
Could you line up the quotes here? Also, I don't understand what "%s aborting
upload" means. Maybe this instead?

 You are specifying either a file that does not exist or an
 unknown canned acl: %s. Aborting upload.

http://codereview.chromium.org/6677023/diff/5002/buildbot/prebuilt.py#newcode293
buildbot/prebuilt.py:293: # Since we are already a different process  exiting
seems to be the most
Could you add a comma between "process" and "exiting" and get rid of the double
space?

http://codereview.chromium.org/6677023/diff/5002/buildbot/prebuilt.py#newcode295
buildbot/prebuilt.py:295: sys.exit(1)
Did you test whether the parent process catch this error and exit appropriately?

[scottz, 2011-03-16 21:36:05]

PTAL good point about the exiting portion, I decided to just make it fail as if
the upload in general failed.

http://codereview.chromium.org/6677023/diff/5002/buildbot/prebuilt.py
File buildbot/prebuilt.py (right):

http://codereview.chromium.org/6677023/diff/5002/buildbot/prebuilt.py#newcode292
buildbot/prebuilt.py:292: 'unknown canned acl. %s aborting upload') % acl
On 2011/03/16 21:29:39, davidjames wrote:
> Could you line up the quotes here? Also, I don't understand what "%s aborting
> upload" means. Maybe this instead?
> 
>  You are specifying either a file that does not exist or an
>  unknown canned acl: %s. Aborting upload.

Done.

http://codereview.chromium.org/6677023/diff/5002/buildbot/prebuilt.py#newcode293
buildbot/prebuilt.py:293: # Since we are already a different process  exiting
seems to be the most
On 2011/03/16 21:29:39, davidjames wrote:
> Could you add a comma between "process" and "exiting" and get rid of the
double
> space?

Done.

http://codereview.chromium.org/6677023/diff/5002/buildbot/prebuilt.py#newcode295
buildbot/prebuilt.py:295: sys.exit(1)
Actually that is a good point I am moving this to just return as if the upload
failed for each object since that is basically what is happening

On 2011/03/16 21:29:39, davidjames wrote:
> Did you test whether the parent process catch this error and exit
appropriately?

[scottz, 2011-03-16 22:13:53]

PTAL modified the approach according to our discussion this is definitely the
preferred way for us to specify the xml since we don't have to then :)

[davidjames, 2011-03-16 22:17:06]

LGTM