Bug 197914 – [ews-build] Enabling uploading EWS archives to S3

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Aakash Jain]

Add script to enable uploading EWS archives to S3.

[Aakash Jain]

Created attachment 369949 [details]
Patch

[Kocsen Chung]

Comment on attachment 369949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369949&action=review

> Tools/BuildSlaveSupport/ews-build/transfer-ews-archive-to-s3:5
> +import os.path

We're you looking for: 

```python
from os import path
```

I say this because i see uses of `os.path` down there, and if you're using `os` then there is no need for this line.

> Tools/BuildSlaveSupport/ews-build/transfer-ews-archive-to-s3:20
> +    if archive:

args is already making sure this is not None, so this is a superfluous check.

> Tools/BuildSlaveSupport/ews-build/transfer-ews-archive-to-s3:27
> +parser = argparse.ArgumentParser(add_help=True)

Even though it's a simple script I think it may be adequate to put the argument parsing and the main routing into a main() function.

> Tools/BuildSlaveSupport/ews-build/transfer-ews-archive-to-s3:34
> +head, tail = os.path.split(str(args.archive))

I think we can trust `args.archive` will be a string right? This would make the `str()` call unnecessary.

Also, I know the documentation says this is a head and the tail, but this makes the reader assume the tail is longer. I think it may be more descriptive to rename these to `dirname, basename` respectively.

> Tools/BuildSlaveSupport/ews-build/transfer-ews-archive-to-s3:35
> +minifiedArchive = head + '/minified-' + tail

`os.path.join()` could be safer here, since it seems head is not guaranteed to have the trailing slash stripped out of it.

[Jonathan Bedard]

Comment on attachment 369949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369949&action=review

> Tools/BuildSlaveSupport/ews-build/transfer-ews-archive-to-s3:5
> +import os.path

As Kocsen pointed out, we don't need os.path, just import os, since we use os.path

> Tools/BuildSlaveSupport/ews-build/transfer-ews-archive-to-s3:27
> +parser = argparse.ArgumentParser(add_help=True)

We usually guard actual script code with __main__

>> Tools/BuildSlaveSupport/ews-build/transfer-ews-archive-to-s3:27
>> +parser.add_argument('--patch_id', action="store", required=True, help='patch_id of the patch')
> 
> Even though it's a simple script I think it may be adequate to put the argument parsing and the main routing into a main() function.

If not that, at least guard this with __main__

>> Tools/BuildSlaveSupport/ews-build/transfer-ews-archive-to-s3:34
>>  minifiedArchive = head + '/minified-' + tail
> 
> I think we can trust `args.archive` will be a string right? This would make the `str()` call unnecessary.
> 
> Also, I know the documentation says this is a head and the tail, but this makes the reader assume the tail is longer. I think it may be more descriptive to rename these to `dirname, basename` respectively.

Can we trust it's not a unicode string, though?

[Aakash Jain]

Created attachment 369970 [details]
Patch

[Aakash Jain]

Incorporated review comments.

Also created a new 'Shared' folder, and moved transfer-archive-to-s3 to Shared folder (instead of creating another copy).

[Aakash Jain]

For some reason svn-apply is not able to handle the file move. I will look separately. Meanwhile, please review the patch.

[Aakash Jain]

Created attachment 369977 [details]
Patch

[Jonathan Bedard]

Comment on attachment 369977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369977&action=review

This patch is super broken....'Tools/BuildSlaveSupport/Shared/transfer-archive-to-s3' is in the patch 3 times, what's going on?

> Tools/BuildSlaveSupport/Shared/transfer-archive-to-s3:34
> +    parser.add_argument('--ews', action='store_true')

This argument feels especially weird since it's implied by --patch-id, right?

[Aakash Jain]

Created attachment 369991 [details]
Patch

[Aakash Jain]

> This argument feels especially weird since it's implied by --patch-id, right?
Removed '--ews' parameter. --patch_id / --revision determine the appropriate S3 bucket now.

[Jonathan Bedard]

Comment on attachment 369991 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369991&action=review

> Tools/BuildSlaveSupport/Shared/transfer-archive-to-s3:43
> +    S3_BUCKET = S3_DEFAULT_BUCKET

Seems like S3_BUCKET shouldn't be capitalized now that it isn't a constant.

[Aakash Jain]

Committed r245359: <https://trac.webkit.org/changeset/245359>

[Radar WebKit Bug Importer]

<rdar://problem/50829090>