[webkit-reviews] review denied: [Bug 223829] [build.webkit.org] Commit queue should post the identifier : [Attachment 424699] Patch

[bugzilla-daemon at webkit.org]

Aakash Jain <aakash_jain at apple.com> has denied Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 223829: [build.webkit.org] Commit queue should post the identifier
https://bugs.webkit.org/show_bug.cgi?id=223829

Attachment 424699: Patch

https://bugs.webkit.org/attachment.cgi?id=424699&action=review




--- Comment #12 from Aakash Jain <aakash_jain at apple.com> ---
Comment on attachment 424699
  --> https://bugs.webkit.org/attachment.cgi?id=424699
Patch

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

> Tools/CISupport/ews-build/steps.py:3251
> +	   if response.status_code == 200:

Need to handle the case when response is None.

> Tools/CISupport/ews-build/steps.py:3252
> +	       ref = response.json().get('identifier', ref)

Need to gracefully handle the case when response.json() returns an exception.

Better to do all this network handling in a separate method, e.g.:
get_commit_identifier_for_revision()

> Tools/CISupport/ews-build/steps.py:3254
> +	   id_string = ref if '@' in ref else '?'

Having a invalid url when commit-identifier is not available seems like a
regression. Better to have svn revision url when commit identifier is not
available.

> Tools/CISupport/ews-build/steps_unittest.py:3978
> +		   self.status_code = 200

we should also add unit-tests to test non-200 status codes. Probably a good
idea to make this status code an argument to init (with default value of 200).