Bug 191943 – [ews-app] Add support to download Patch from Bugzilla

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

We should add support to download Patch from Bugzilla. We should use the Bugzilla REST API.

[Aakash Jain]

Created attachment 355661 [details]
Proposed patch

Part of patch series. Therefore wouldn't apply to ToT without applying other patches first.

[Lucas Forschler]

Comment on attachment 355661 [details]
Proposed patch

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

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:36
> +        patch = Bugzilla._fetch_attachment(patch_id)

I'm curious what this does with multiple patches?
What does it do if there are patches marked obsolete?

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:48
> +        return patch

We are returning a patch here, but the function name doesn't really imply that we are expecting to return one.
I'm not sure how to make this better... retrieve_and_save_attachment doesn't sound great.  get_and_save_attachment isn't ideal either.
maybe the download portion of the logic should be it's own function?

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:57
> +        patch = util.fetch_data_from_url(attachment_url)

it feels like we are swapping out the name of the object here... from attachment to patch.
is there any reason not to use 'attachment' instead of 'patch' inside this function?

looking ahead to the code below, perhaps it's best to replace 'patch' with bug_attachment_data or attachments_json?

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:63
> +        return attachments.get(str(attachment_id))

could there be more than one attachment_id here?

> Tools/BuildSlaveSupport/ews-app/ews/config.py:27
> +PATCH_FOLDER = '/tmp/'

what is the plan for cleaning all of these patches up? 
Do they expire?
Will there be a cron job that occasionally runs?

[Aakash Jain]

(In reply to Lucas Forschler from comment #2)
> I'm curious what this does with multiple patches?
Note that we are fetching the patch itself, not the bug. Each patch is unique with a unique patch_id.
This method is for fetching single patch. If we want to fetch multiple patch, we simply call this method multiple times.

> What does it do if there are patches marked obsolete?
The patch object (json) it returns contains a field 'is_obsolete', which would be True.

> We are returning a patch here, but the function name doesn't really imply that we are expecting to return one.
> I'm not sure how to make this better... retrieve_and_save_attachment doesn't
> sound great.  get_and_save_attachment isn't ideal either.
> maybe the download portion of the logic should be it's own function?
Renamed to retrieve_attachment, created a separate method: save_attachment.

> is there any reason not to use 'attachment' instead of 'patch' inside this function?
No, will use 'attachment'.

> could there be more than one attachment_id here?
No, the API we use is for a single attachment, e.g.: https://bugs.webkit.org/rest/bug/attachment/355661

> Will there be a cron job that occasionally runs?
Yeah, will setup a  cron job.

[Aakash Jain]

Created attachment 355702 [details]
Updated patch

[EWS Watchlist]

Attachment 355702 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:48:  [Bugzilla.save_attachment] Undefined variable 'attachment_id'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:68:  [Bugzilla.file_path_for_patch] Undefined variable 'os'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:69:  [Bugzilla.file_path_for_patch] Undefined variable 'os'  [pylint/E0602] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Aakash Jain]

Created attachment 355703 [details]
Updated patch

[EWS Watchlist]

Comment on attachment 355703 [details]
Updated patch

Attachment 355703 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10162198

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html

[EWS Watchlist]

Created attachment 355712 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Aakash Jain]

Committed r238631: <http://trac.webkit.org/changeset/238631>

[Radar WebKit Bug Importer]

<rdar://problem/46319334>