Bug 206530

Summary: [ews] commit-queue should check cq+ flag
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   
Bug Depends on:    
Bug Blocks: 201934    
Attachments:
Description Flags
Patch
none
Patch none

Aakash Jain
Reported 2020-01-21 07:06:42 PST
[ews] commit-queue should check cq+ flag on the patch while processing it. It should skip the patch if patch doesn't/no-longer have cq+ flag. It is possible that the patch was cq+ed, and later someone found an issue with the patch and cq- it. commit-queue should be smart about not committing such patches.
Attachments
Patch (5.98 KB, patch)
2020-01-21 09:23 PST, Aakash Jain 		no flags
DetailsFormatted DiffDiff
Patch (10.23 KB, patch)
2020-01-21 10:50 PST, Aakash Jain 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-01-21 07:07:04 PST
<rdar://problem/58758367>
Aakash Jain
Comment 2 2020-01-21 09:23:14 PST
Created attachment 388308 [details] Patch
Aakash Jain
Comment 3 2020-01-21 10:50:31 PST
Created attachment 388320 [details] Patch
Aakash Jain
Comment 4 2020-01-21 10:51:33 PST
Sample run: https://ews-build.webkit-uat.org/#/builders/26/builds/350
Jonathan Bedard
Comment 5 2020-01-21 10:56:49 PST
Comment on attachment 388320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388320&action=review > Tools/BuildSlaveSupport/ews-build/steps.py:-332 > - def __init__(self, verifyObsolete=True, verifyBugClosed=True, verifyReviewDenied=True, addURLs=True, **kwargs): We removed kwargs here. Was that deliberate? > Tools/BuildSlaveSupport/ews-build/steps.py:481 > + cq_plus = self._is_patch_cq_plus(patch_id) if self.verifycqplus else 1 Why the intermediate variable and not: if self.verifycqplus and self._is_patch_cq_plus(patch_id) != 1: ...
Aakash Jain
Comment 6 2020-01-21 11:08:23 PST
Comment on attachment 388320 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388320&action=review >> Tools/BuildSlaveSupport/ews-build/steps.py:-332 >> - def __init__(self, verifyObsolete=True, verifyBugClosed=True, verifyReviewDenied=True, addURLs=True, **kwargs): > > We removed kwargs here. Was that deliberate? Yeah, minor cleanup. although it doesn't make any difference. >> Tools/BuildSlaveSupport/ews-build/steps.py:481 >> + cq_plus = self._is_patch_cq_plus(patch_id) if self.verifycqplus else 1 > > Why the intermediate variable and not: > > if self.verifycqplus and self._is_patch_cq_plus(patch_id) != 1: > ... I was just keeping it consistent with the code above (e.g.: review_denied), but I can change it to the one you suggested. Which one do you prefer?
Jonathan Bedard
Comment 7 2020-01-21 11:51:51 PST
(In reply to Aakash Jain from comment #6) > Comment on attachment 388320 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388320&action=review > > >> Tools/BuildSlaveSupport/ews-build/steps.py:-332 > >> - def __init__(self, verifyObsolete=True, verifyBugClosed=True, verifyReviewDenied=True, addURLs=True, **kwargs): > > > > We removed kwargs here. Was that deliberate? > > Yeah, minor cleanup. although it doesn't make any difference. > > >> Tools/BuildSlaveSupport/ews-build/steps.py:481 > >> + cq_plus = self._is_patch_cq_plus(patch_id) if self.verifycqplus else 1 > > > > Why the intermediate variable and not: > > > > if self.verifycqplus and self._is_patch_cq_plus(patch_id) != 1: > > ... > > I was just keeping it consistent with the code above (e.g.: review_denied), > but I can change it to the one you suggested. Which one do you prefer? I prefer putting it without the intermediate variable, but I don't feel very strongly about that.
Aakash Jain
Comment 8 2020-01-21 12:05:52 PST
Comment on attachment 388320 [details] Patch Clearing flags on attachment: 388320 Committed r254870: <https://trac.webkit.org/changeset/254870>
Aakash Jain
Comment 9 2020-01-21 12:05:55 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.