Bug 206530

Summary:

[ews] commit-queue should check cq+ flag

Product:

WebKit

Reporter:

Aakash Jain <aakash_jain>

Component:

Tools / Tests

Assignee:

Aakash Jain <aakash_jain>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aakash_jain, ap, jbedard, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

201934

Attachments:

Description	Flags
Patch	none
Patch	none

Description Aakash Jain 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.

Comment 1 Radar WebKit Bug Importer 2020-01-21 07:07:04 PST

<rdar://problem/58758367>

Comment 2 Aakash Jain 2020-01-21 09:23:14 PST

Created attachment 388308 [details]
Patch

Comment 3 Aakash Jain 2020-01-21 10:50:31 PST

Created attachment 388320 [details]
Patch

Comment 4 Aakash Jain 2020-01-21 10:51:33 PST

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

Comment 5 Jonathan Bedard 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:
    ...

Comment 6 Aakash Jain 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?

Comment 7 Jonathan Bedard 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.

Comment 8 Aakash Jain 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>

Comment 9 Aakash Jain 2020-01-21 12:05:55 PST

All reviewed patches have been landed.  Closing bug.