Bug 206536 - [ews] add commit-queue build step to clear flags on patch
Summary: [ews] add commit-queue build step to clear flags on patch
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords: InRadar
Depends on:
Blocks: 201934
  Show dependency tree / graph
 
Reported: 2020-01-21 08:29 PST by Aakash Jain
Modified: 2020-02-19 07:41 PST (History)
4 users (show)

See Also:

Attachments
WIP (4.61 KB, patch)
2020-02-05 12:25 PST, Aakash Jain 		no flags Details | Formatted Diff | Diff
Patch (4.98 KB, patch)
2020-02-07 06:33 PST, Aakash Jain 		jbedard: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2020-01-21 08:29:47 PST 
[ews] add commit-queue build step to clear flags on patch and close the bug.
Comment 1 Radar WebKit Bug Importer 2020-01-21 08:30:04 PST 
<rdar://problem/58759929>
Comment 2 Aakash Jain 2020-02-05 12:25:46 PST 
Created attachment 389843 [details]
WIP
Comment 3 Aakash Jain 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
Comment 4 Jonathan Bedard 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?
Comment 5 Aakash Jain 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.
Comment 6 Aakash Jain 2020-02-07 06:33:46 PST 
Created attachment 390079 [details]
Patch
Comment 8 Jonathan Bedard 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.
Comment 9 Aakash Jain 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.
Comment 10 Aakash Jain 2020-02-07 09:07:38 PST 
Committed r256026: <https://trac.webkit.org/changeset/256026>