Bug 206536

Summary: [ews] add commit-queue build step to clear flags on patch
Product: WebKit Reporter: Aakash Jain <aakash_jain>
Component: Tools / TestsAssignee: Aakash Jain <aakash_jain>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, jbedard, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=206511
https://bugs.webkit.org/show_bug.cgi?id=207387
Bug Depends on:    
Bug Blocks: 201934    
Attachments:
Description Flags
WIP
none
Patch jbedard: review+

Aakash Jain
Reported 2020-01-21 08:29:47 PST
[ews] add commit-queue build step to clear flags on patch and close the bug.
Attachments
WIP (4.61 KB, patch)
2020-02-05 12:25 PST, Aakash Jain 		no flags
DetailsFormatted DiffDiff
Patch (4.98 KB, patch)
2020-02-07 06:33 PST, Aakash Jain 		jbedard: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-01-21 08:30:04 PST
<rdar://problem/58759929>
Aakash Jain
Comment 2 2020-02-05 12:25:46 PST
Created attachment 389843 [details] WIP
Aakash Jain
Comment 3 2020-02-05 12:34:38 PST
This would also use Bugzilla API key similar to Bug 206511. I think it would be better to store the API key in the passwords.json instead of environment variable as discussed in https://bugs.webkit.org/show_bug.cgi?id=206511#c10
Jonathan Bedard
Comment 4 2020-02-05 13:22:54 PST
Comment on attachment 389843 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=389843&action=review > Tools/BuildSlaveSupport/ews-build/steps.py:432 > + def _remove_flags_on_patch(self, patch_id): Strikes me as weird that you're having the Mixin own this function. Which actions, other than RemoveFlagsOnPatch, would use it?
Aakash Jain
Comment 5 2020-02-07 06:26:38 PST
Comment on attachment 389843 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=389843&action=review >> Tools/BuildSlaveSupport/ews-build/steps.py:432 >> + def _remove_flags_on_patch(self, patch_id): > > Strikes me as weird that you're having the Mixin own this function. Which actions, other than RemoveFlagsOnPatch, would use it? Other build-steps like Validating Committer/Reviewer would need to modify the flags (set cq-), which would use very similar logic. I think it's better to keep all the bugzilla logic in one place.
Aakash Jain
Comment 6 2020-02-07 06:33:46 PST
Created attachment 390079 [details] Patch
Jonathan Bedard
Comment 8 2020-02-07 09:01:14 PST
Comment on attachment 390079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390079&action=review > Tools/BuildSlaveSupport/ews-build/steps.py:534 > + rc = self.remove_flags_on_patch(patch_id) Nit: not sure rc is actually helpful here, I'd probably just put this call inside self.finished. I don't feel particularly strongly about this, though.
Aakash Jain
Comment 9 2020-02-07 09:05:01 PST
Comment on attachment 390079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390079&action=review >> Tools/BuildSlaveSupport/ews-build/steps.py:534 >> + rc = self.remove_flags_on_patch(patch_id) > > Nit: not sure rc is actually helpful here, I'd probably just put this call inside self.finished. I don't feel particularly strongly about this, though. I like the function call being explicit for better readability, as it's the most important part of this class/method.
Aakash Jain
Comment 10 2020-02-07 09:07:38 PST
Committed r256026: <https://trac.webkit.org/changeset/256026>
Note You need to log in before you can comment on or make changes to this bug.