Bug 206530 – [ews] commit-queue should check cq+ flag

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

[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.

[Radar WebKit Bug Importer]

<rdar://problem/58758367>

[Aakash Jain]

Created attachment 388308 [details]
Patch

[Aakash Jain]

Created attachment 388320 [details]
Patch

[Aakash Jain]

Sample run: https://ews-build.webkit-uat.org/#/builders/26/builds/350

[Jonathan Bedard]

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 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]

(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 on attachment 388320 [details]
Patch

Clearing flags on attachment: 388320

Committed r254870: <https://trac.webkit.org/changeset/254870>

[Aakash Jain]

All reviewed patches have been landed.  Closing bug.