Bug 198415

Summary: [ews-app] Add authentication while fetching bugs
Product: WebKit Reporter: Aakash Jain <aakash_jain>
Component: Tools / TestsAssignee: Aakash Jain <aakash_jain>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, jbedard, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Patch
none
Patch none

Description Aakash Jain 2019-05-31 04:47:37 PDT
Add bugzilla authentication to Django ews-app for fetching bug list with r? flag, and the individual bugs.
Comment 1 Radar WebKit Bug Importer 2019-05-31 04:47:47 PDT
<rdar://problem/51298710>
Comment 2 Aakash Jain 2019-05-31 05:58:55 PDT
Created attachment 371052 [details]
WIP
Comment 3 Aakash Jain 2019-06-04 09:04:58 PDT
Created attachment 371288 [details]
Patch
Comment 4 Jonathan Bedard 2019-06-04 10:13:41 PDT
Comment on attachment 371288 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:110
> +        if self.authenticated:

This code concerns me.

What happens if we authenticate, and then this class sticks around for too long, and bugzilla logs us out? If that happens, we have no recourse. I would think that we would set authenticated to false at the beginning on this function.

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:136
> +                if attempts < 5:

Can we do:

if attempts >= 5:
    raise Exception(errorMessage)
_log.error(errorMessage)
time.sleep(5)
Comment 5 Aakash Jain 2019-06-04 11:34:26 PDT
Comment on attachment 371288 [details]
Patch

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

>> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:110
>> +        if self.authenticated:
> 
> This code concerns me.
> 
> What happens if we authenticate, and then this class sticks around for too long, and bugzilla logs us out? If that happens, we have no recourse. I would think that we would set authenticated to false at the beginning on this function.

Agree.. modified to remove caching.

>> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:136
>> +                if attempts < 5:
> 
> Can we do:
> 
> if attempts >= 5:
>     raise Exception(errorMessage)
> _log.error(errorMessage)
> time.sleep(5)

Modified in updated patch.
Comment 6 Aakash Jain 2019-06-04 11:37:07 PDT
Created attachment 371312 [details]
Patch
Comment 7 Aakash Jain 2019-06-04 14:40:02 PDT
Comment on attachment 371312 [details]
Patch

Clearing flags on attachment: 371312

Committed r246082: <https://trac.webkit.org/changeset/246082>
Comment 8 Aakash Jain 2019-06-04 14:40:05 PDT
All reviewed patches have been landed.  Closing bug.