Bug 71690 – Add cc-bugs group to watch changes in chromium graphics

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

Add cc-bugs group to watch changes in chromium graphics

[Dana Jansens]

Created attachment 113868 [details]
Patch

[WebKit Review Bot]

Attachment 113868 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/common/config/watchlist:0:  The email alias [e-mail] which is in the watchlist is not listed as a contributor in committers.py  [watchlist/general] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 113868 [details]
Patch

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

> Tools/Scripts/webkitpy/common/config/watchlist:118
> +        "ChromiumGraphics": [ "[e-mail]", "[e-mail]" ],

Does cc-bugs have a bugs.webkit.org account?

[Dana Jansens]

Yes, made one for it.

[WebKit Review Bot]

Comment on attachment 113868 [details]
Patch

Clearing flags on attachment: 113868

Committed r99470: <http://trac.webkit.org/changeset/99470>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

This broke test-webkitpy. :(

[Eric Seidel (no email)]

The test seems poorly designed:


FAIL: test_watch_list_load (webkitpy.common.watchlist.watchlistloader_unittest.WatchListLoaderTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Projects/WebKit/Tools/Scripts/webkitpy/common/watchlist/watchlistloader_unittest.py", line 45, in test_watch_list_load
    OutputCapture().assert_outputs(self, WatchListLoader(filesystem.FileSystem()).load, expected_logs="")
  File "/Projects/WebKit/Tools/Scripts/webkitpy/common/system/outputcapture.py", line 79, in assert_outputs
    testcase.assertEqual(logs_string, expected_logs)
AssertionError: 'The email alias [e-mail] which is in the watchlist is not listed as a contributor in committers.py\n' != ''

Señor Levin?

[David Levin]

Rolled out here due to test breakage (which the style bot flagged): http://trac.webkit.org/changeset/99494

There are two options: 
1. Add that to the contributors or 
2. Add a new section to committers.py (known accounts) that I can query.

The problem is that it is super bad if an email gets added to this file without having a bugzilla account and this is my best way of checking that. (fwiw, if they don't have a bugzilla account, then the bug update fails and no cc, message. etc. gets added.)

#2 is a good option if you don't need autocomplete.

I'm willing to advise if you need help. 

Thanks for understanding!

[Eric Seidel (no email)]

There must be a better way to solve this.  Either by some sort of cc-attempt fallback code, or some way to query bugzilla about known accounts.  We have a way today with webkit-patch find-users, but it requires you to have admin privileges on bugs.webkit.org

[Dana Jansens]

I will go with #2 and add a new list to committers.py, named watcher_accounts, for bugzilla accounts that watch bugs but don't contribute.

[Dana Jansens]

Created attachment 114077 [details]
add cc-bugs to a new committers.py list "watcher_accounts"

[Dana Jansens]

This will still fail style/break the test until it grabs the watcher_accounts list and deals with it accordingly.

[WebKit Review Bot]

Attachment 114077 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/common/config/watchlist:0:  The email alias [e-mail] which is in the watchlist is not listed as a contributor in committers.py  [watchlist/general] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

(In reply to comment #13)
> This will still fail style/break the test until it grabs the watcher_accounts list and deals with it accordingly.

I misspoke "Add a new section to committers.py (known accounts) that I can query." I meant "Add a new section to committers.py (known accounts) that the tool can query." but then rest shouldn't be hard.

Hook up your new list so that it can be queried like contributor_by_email. Perhaps accounts_by_email? and change this line to call it:    http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py#L155

Then, everything will be fixed. If you need more verbose instructions, let me know and I can walk you through it.

[Dana Jansens]

Created attachment 114319 [details]
create non-contributor account list and make check-webkit-style use the full account list

[Dana Jansens]

Thanks for the thoughtful instructions.  I've followed through them and made the requested changes.  Now you query the list of "accounts" instead of "contributors" in the style check.

[Eric Seidel (no email)]

Comment on attachment 114319 [details]
create non-contributor account list and make check-webkit-style use the full account list

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

> Tools/Scripts/webkitpy/common/config/committers.py:90
> +# This is a list of email addresses who have bugzilla accounts but are not

A list is not a "who", is it?

> Tools/Scripts/webkitpy/common/config/committers.py:95
> +    Contributor("Chromium Compositor Bugs", ["[e-mail]"], "cc-bugs"),

This should be Account(...)

> Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py:158
> -                if not contributors.contributor_by_email(email):
> +                if not accounts.account_by_email(email):
>                      cc_rule.remove_instruction(email)
>                      self._log_error("The email alias %s which is in the watchlist is not listed as a contributor in committers.py" % email)
>                      continue

This code should additionally check that it's the first email, the bugzilla_email in the account.  account_by_email will give you any account with that email, which might not be the bugzilla email for the account.

[David Levin]

(In reply to comment #18)
> (From update of attachment 114319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114319&action=review
> 
> > Tools/Scripts/webkitpy/common/config/committers.py:90
> > +# This is a list of email addresses who have bugzilla accounts but are not
> 
> A list is not a "who", is it?
> 
> > Tools/Scripts/webkitpy/common/config/committers.py:95
> > +    Contributor("Chromium Compositor Bugs", ["[e-mail]"], "cc-bugs"),
> 
> This should be Account(...)
> 
> > Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py:158
> > -                if not contributors.contributor_by_email(email):
> > +                if not accounts.account_by_email(email):
> >                      cc_rule.remove_instruction(email)
> >                      self._log_error("The email alias %s which is in the watchlist is not listed as a contributor in committers.py" % email)
> >                      continue
> 
> This code should additionally check that it's the first email, the bugzilla_email in the account.  account_by_email will give you any account with that email, which might not be the bugzilla email for the account.

And please add a simple test to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers_unittest.py?


Thanks so much for doing this!

[Dana Jansens]

Created attachment 114384 [details]
Patch

[Dana Jansens]

Made an additional account_by_login() that returns an Account for a login email address.  Added unit tests, and addressed nits.

[WebKit Review Bot]

Comment on attachment 114384 [details]
Patch

Clearing flags on attachment: 114384

Committed r99786: <http://trac.webkit.org/changeset/99786>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.