Bug 197914 - [ews-build] Enabling uploading EWS archives to S3
Summary: [ews-build] Enabling uploading EWS archives to S3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-15 07:14 PDT by Aakash Jain
Modified: 2019-05-15 15:22 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.89 KB, patch)
2019-05-15 07:16 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff
Patch (11.72 KB, patch)
2019-05-15 10:40 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff
Patch (12.29 KB, patch)
2019-05-15 11:53 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff
Patch (12.51 KB, patch)
2019-05-15 14:20 PDT, Aakash Jain
jbedard: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2019-05-15 07:14:41 PDT
Add script to enable uploading EWS archives to S3.
Comment 1 Aakash Jain 2019-05-15 07:16:09 PDT
Created attachment 369949 [details]
Patch
Comment 2 Kocsen Chung 2019-05-15 09:45:47 PDT
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.
Comment 3 Jonathan Bedard 2019-05-15 10:13:22 PDT
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?
Comment 4 Aakash Jain 2019-05-15 10:40:12 PDT
Created attachment 369970 [details]
Patch
Comment 5 Aakash Jain 2019-05-15 10:41:45 PDT
Incorporated review comments.

Also created a new 'Shared' folder, and moved transfer-archive-to-s3 to Shared folder (instead of creating another copy).
Comment 6 Aakash Jain 2019-05-15 11:27:54 PDT
For some reason svn-apply is not able to handle the file move. I will look separately. Meanwhile, please review the patch.
Comment 7 Aakash Jain 2019-05-15 11:53:36 PDT
Created attachment 369977 [details]
Patch
Comment 8 Jonathan Bedard 2019-05-15 13:46:01 PDT
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?
Comment 9 Aakash Jain 2019-05-15 14:20:11 PDT
Created attachment 369991 [details]
Patch
Comment 10 Aakash Jain 2019-05-15 14:22:43 PDT
> 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.
Comment 11 Jonathan Bedard 2019-05-15 14:24:11 PDT
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.
Comment 12 Aakash Jain 2019-05-15 15:21:27 PDT
Committed r245359: <https://trac.webkit.org/changeset/245359>
Comment 13 Radar WebKit Bug Importer 2019-05-15 15:22:41 PDT
<rdar://problem/50829090>