Bug 206774 – [ews] Add method to fetch cq+ patches from Bugzilla

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

Add method to fetch cq+ patches from Bugzilla in the EWS django app. This is needed for Bug 206534 'EWS django app should send cq+ patches to commit-queue'.

[Aakash Jain]

Created attachment 388733 [details]
Patch

[Jonathan Bedard]

Comment on attachment 388733 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:82
> +        commit_queue_patches = []

Seems like this should be moved down so it's near the loop that's populating it

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:84
> +        api_key = os.getenv('BUGZILLA_API_KEY', None)

Is this for security bugs? What happens if it's undefined?

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:95
> +            if Bugzilla._is_patch_cq_plus(attachment_json):

Shouldn't this be Bugzilla._is_patch_cq_plus(attachment_json) == 1? It would be true for -1 too.

Also, I think this should be 'cls._is_patch_cq_plus'

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:106
> +            if flag.get('name') == 'commit-queue' and flag.get('status') == '+':

Is the patch data unicode or not? On line 94, you're treating it like unicode, here, you're treating it like a string.

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:188
> +    def fetch_bug_ids_for_commit_queue(self):

Confused why we have two different functions here. Seems like they should be the same.

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:196
> +        return [int(bug_link_cell.find("a").string)

If you event hit something that isn't an integer, this is going to blow up.

Do you have a good example of what this output actually looks like?

[Aakash Jain]

Created attachment 388943 [details]
Patch

[Aakash Jain]

Comment on attachment 388733 [details]
Patch

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

>> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:82
>> +        commit_queue_patches = []
> 
> Seems like this should be moved down so it's near the loop that's populating it

Done in updated patch.

>> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:84
>> +        api_key = os.getenv('BUGZILLA_API_KEY', None)
> 
> Is this for security bugs? What happens if it's undefined?

Yes. If api_key is undefined, it wouldn't fetch security patches (would fetch just the regular patches).

>> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:95
>> +            if Bugzilla._is_patch_cq_plus(attachment_json):
> 
> Shouldn't this be Bugzilla._is_patch_cq_plus(attachment_json) == 1? It would be true for -1 too.
> 
> Also, I think this should be 'cls._is_patch_cq_plus'

good catch. fixed in updated patch.

>> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:106
>> +            if flag.get('name') == 'commit-queue' and flag.get('status') == '+':
> 
> Is the patch data unicode or not? On line 94, you're treating it like unicode, here, you're treating it like a string.

changed line 94 from unicode to str. Was basically converting from int to str/unicode.

>> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:188
>> +    def fetch_bug_ids_for_commit_queue(self):
> 
> Confused why we have two different functions here. Seems like they should be the same.

Copied from webkitpy. Merged into one function in updated patch.

>> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:196
>> +        return [int(bug_link_cell.find("a").string)
> 
> If you event hit something that isn't an integer, this is going to blow up.
> 
> Do you have a good example of what this output actually looks like?

bug_link_cell:

<td class="first-child bz_id_column">
<a href="show_bug.cgi?id=201737">201737</a>
<span class="bz_default_hidden"></span>
</td>

bug_link_cell.find("a"):
<a href="show_bug.cgi?id=201737">201737</a>

bug_link_cell.find("a").string:
201737

Also, this code is simply copied from webkitpy where it has been working fine for years. Until we change bugzilla version (and newer bugzilla version changes this), I don't see any reason for this format to change.

[Jonathan Bedard]

(In reply to Aakash Jain from comment #4)
> ...
> 
> Also, this code is simply copied from webkitpy where it has been working
> fine for years. Until we change bugzilla version (and newer bugzilla version
> changes this), I don't see any reason for this format to change.

The fact that it's been this way in webkitpy for years means it clearly won't be blowing up, so I'm good with it as is.

[Aakash Jain]

Committed r255218: <https://trac.webkit.org/changeset/255218>

[Radar WebKit Bug Importer]

<rdar://problem/58941855>