Bug 196599 - EWS should have a way to retry a patch
Summary: EWS should have a way to retry a 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:
 
Reported: 2019-04-04 07:17 PDT by Aakash Jain
Modified: 2019-10-21 11:34 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.12 KB, patch)
2019-10-18 15:49 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff
Patch (8.53 KB, patch)
2019-10-21 10:24 PDT, Aakash Jain
jbedard: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2019-04-04 07:17:02 PDT
For example, if a patch depended on another patch, and EWS ran it too early, it shouldn't be necessary to re-upload it.

Also, sometimes EWS is just wrong because of flaky trunk.
Comment 1 Aakash Jain 2019-04-04 07:17:37 PDT
<rdar://problem/22918898>
Comment 2 Radar WebKit Bug Importer 2019-04-04 08:55:08 PDT
<rdar://problem/49608501>
Comment 3 Aakash Jain 2019-10-18 15:49:18 PDT
Created attachment 381340 [details]
Patch
Comment 4 Aakash Jain 2019-10-18 16:46:11 PDT
Tested in: https://ews.webkit-uat.org/status-bubble/380828/

The button UI would need slight polishing.
Comment 5 Jonathan Bedard 2019-10-21 07:53:40 PDT
Comment on attachment 381340 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381340&action=review

> Tools/BuildSlaveSupport/ews-app/ews/models/build.py:70
>          _log.info('Saved build {} in database for patch_id: {}'.format(uid, patch_id))

Do we have to use *args here? Until this false, everything was using a variable with a descriptive name. I think we either want a named variable or start using kwargs in this call.

> Tools/BuildSlaveSupport/ews-app/ews/templates/statusbubble.html:108
> +{% endif %}

Were we intending the EWS button style to be improved in this patch or a future one?

For what it's worth, the button seems pretty consistent with the existing style.

> Tools/BuildSlaveSupport/ews-app/ews/views/retrypatch.py:72
> +        return redirect('/status-bubble/{}'.format(patch_id))

Why weren't we redirecting before?
Comment 6 Aakash Jain 2019-10-21 10:23:09 PDT
Comment on attachment 381340 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381340&action=review

>> Tools/BuildSlaveSupport/ews-app/ews/models/build.py:70
>>          _log.info('Saved build {} in database for patch_id: {}'.format(uid, patch_id))
> 
> Do we have to use *args here? Until this false, everything was using a variable with a descriptive name. I think we either want a named variable or start using kwargs in this call.

Moved 'retried' variable declaration below complete_at in updated patch, so that we don't have to pass this optional variable while instantiating this class.

>> Tools/BuildSlaveSupport/ews-app/ews/templates/statusbubble.html:108
>> +{% endif %}
> 
> Were we intending the EWS button style to be improved in this patch or a future one?
> 
> For what it's worth, the button seems pretty consistent with the existing style.

Improved one UI aspect in updated patch, the button was rendering in new line sometimes.

>> Tools/BuildSlaveSupport/ews-app/ews/views/retrypatch.py:72
>> +        return redirect('/status-bubble/{}'.format(patch_id))
> 
> Why weren't we redirecting before?

It was work-in-progress earlier. It's a better UX to simply refresh the status-bubble on clicking the retry button. Builds which would be retried would turn blue (in-progress), (handled by the changes in statusbubble.py code).
Comment 7 Aakash Jain 2019-10-21 10:24:23 PDT
Created attachment 381416 [details]
Patch
Comment 8 Jonathan Bedard 2019-10-21 10:29:45 PDT
Comment on attachment 381416 [details]
Patch

I like this, do we have an example or a screenshot for the UI change?
Comment 9 Aakash Jain 2019-10-21 10:49:21 PDT
(In reply to Jonathan Bedard from comment #8)
> Comment on attachment 381416 [details]
> Patch
> 
> I like this, do we have an example or a screenshot for the UI change?
https://ews.webkit-uat.org/status-bubble/380828/ the 'Retry failed builds' button was being rendered in new-line earlier., Now it's rendered in same line along-with the status-bubbles.
Comment 10 Aakash Jain 2019-10-21 11:34:48 PDT
Committed r251368: <https://trac.webkit.org/changeset/251368>