Bug 206536 – [ews] add commit-queue build step to clear flags on patch

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Aakash Jain]

[ews] add commit-queue build step to clear flags on patch and close the bug.

[Radar WebKit Bug Importer]

<rdar://problem/58759929>

[Aakash Jain]

Created attachment 389843 [details]
WIP

[Aakash Jain]

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

Created attachment 390079 [details]
Patch

[Aakash Jain]

Sample runs:
Success: https://ews-build.webkit-uat.org/#/builders/26/builds/425
Failure: https://ews-build.webkit-uat.org/#/builders/26/builds/423

[Jonathan Bedard]

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

Committed r256026: <https://trac.webkit.org/changeset/256026>