[webkit-reviews] review denied: [Bug 46764] Record bot ID when updating queue status : [Attachment 69129] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Mihai Parparita
<mihaip at chromium.org>'s request for review:
Bug 46764: Record bot ID when updating queue status
https://bugs.webkit.org/show_bug.cgi?id=46764

Attachment 69129: Patch
https://bugs.webkit.org/attachment.cgi?id=69129&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69129&action=review

Working with you is such a pleasure, Mihai.  Thank you for the patch!

r- because I think we'd end up with Browser() shared between all StatusServer
instances (not that there is normally more than one).  Otherwise looks great.

> WebKitTools/QueueStatusServer/model/queuestatus.py:34
> +    bot_id = db.StringProperty()

Will AppEngine automatically migrate the datastore correctly?

> WebKitTools/Scripts/webkitpy/common/net/statusserver.py:45
> +    def __init__(self, host=default_host, browser=Browser(),
bot_id=socket.gethostname()):

This is subtly wrong in 2 ways.

1.  foo=bar should be avoided in __init__ because bar will be shared across all
instances of the object (at least that's my understanding).  The recommended
pattern is to use foo=None and then in teh body if foo is None set it to the
default value.
2.  I think we want to avoid giving the entire dns name?  Or maybe I'm
understanding gethostname() incorrectly?  Will that just return
amazon-ews-12345 or will it return the full DNS address?

I think we might want to add a global command-line option to set the bot name? 
There is already a status-server-host= option which is global and passed to the
StatusServer at creation time.

> WebKitTools/Scripts/webkitpy/common/net/statusserver_unittest.py:36
> +class StatusServerTest(unittest.TestCase):

I love that you added StatusServerTest.  We've needed this for a long time, but
I think been too lazy to mock Browser.

We have a bunch of our mocks dumped in mocktool.py, I can't remember if there i
a MockBrowser there.