sheriffbot can't roll out security patches
When it tries, all we get is:
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'create-rollout', '--force-clean', '--parent-command=sheriff-bot', REVISION, 'REASON (Requested by abarth on #webkit).']" exit_code: 1
Either we should giver sheriffbot access, or have him do something smarter.

Created attachment 67318[details]
Patch with unit tests
Patch to make Sheriffbot and webkit-patch print human-readable error messages when the person (or bot) cannot view a bug.
I'm open to naming suggestions.

I should note that for the unit tests included in this patch, I explicitly tested the exception as there does not seem to be analogous unitTest.TestCase.assertRaises() that can be used as a context manager in Python < 2.7. I am open to suggestions.

Comment on attachment 67318[details]
Patch with unit tests
View in context: https://bugs.webkit.org/attachment.cgi?id=67318&action=prettypatch> WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:516
> + bug_element = soup.find("bug")
> + if bug_element.has_key("error"):
> + raise BugzillaError(bug_element["error"])
Nice.
> WebKitTools/Scripts/webkitpy/tool/multicommandtool.py:320
> + except BugzillaError, e:
> + if e.is_invalid_bug_id():
> + log("Invalid bug number.")
> + if e.does_bug_exist():
> + log("The bug does not exist.")
> + elif e.is_not_permitted_to_view_bug():
> + log("You are not authorized to view this bug.")
> + sys.exit(1)
This seems like a very high level to catch this error. Also, exit(1) is pretty severe... Would it make sense to move this exception block lower down to where we actually fetch the bugs? That might give us more context about what to do.
> WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:71
> + if (e.is_not_permitted_to_view_bug()):
> + raise ScriptError(message="SheriffBot is not authorized to rollout a security bug.")
Instead of raising a ScriptError, should we just send a message to IRC? I see that in other places in this file we raise ScriptErrors, so maybe this is the best thing...

(In reply to comment #3)
> > WebKitTools/Scripts/webkitpy/tool/multicommandtool.py:320
> > + except BugzillaError, e:
> > + if e.is_invalid_bug_id():
> > + log("Invalid bug number.")
> > + if e.does_bug_exist():
> > + log("The bug does not exist.")
> > + elif e.is_not_permitted_to_view_bug():
> > + log("You are not authorized to view this bug.")
> > + sys.exit(1)
> This seems like a very high level to catch this error. Also, exit(1) is pretty severe... Would it make sense to move this exception block lower down to where we actually fetch the bugs? That might give us more context about what to do.
Will look into this.
>
> > WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:71
> > + if (e.is_not_permitted_to_view_bug()):
> > + raise ScriptError(message="SheriffBot is not authorized to rollout a security bug.")
> Instead of raising a ScriptError, should we just send a message to IRC? I see that in other places in this file we raise ScriptErrors, so maybe this is the best thing...
I was not familiar with how to send a message to IRC directly. As you notice, other places seem to use this approach. I'll look into this as well.

(In reply to comment #4)
> > > WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:71
> > > + if (e.is_not_permitted_to_view_bug()):
> > > + raise ScriptError(message="SheriffBot is not authorized to rollout a security bug.")
> > Instead of raising a ScriptError, should we just send a message to IRC? I see that in other places in this file we raise ScriptErrors, so maybe this is the best thing...
>
> I was not familiar with how to send a message to IRC directly. As you notice, other places seem to use this approach. I'll look into this as well.
The Rollout class (in irc_command.py), which implements the SheriffBot rollout command, has the necessary infrastructure to catch a ScriptError exception and post its message to IRC. You seem to be implying that we should explicitly post an IRC message and return with value None (or some other error code) and modify the caller Rollout.execute() to handle this instead of raising an exception in Sheriff.post_rollout_patch() with the error message. I'm unclear of the advantages of this approach as I would envision using a similar error message prefix of "...: Failed to create rollout patch: ..." for the Bugzilla error message (for consistency with the other failed rollout error messages). Can you elaborate on you how you envision this to work?

> The Rollout class (in irc_command.py), which implements the SheriffBot rollout command, has the necessary infrastructure to catch a ScriptError exception and post its message to IRC.
Yeah. It just looks kind of ugly. I guess we could make it less ugly instead.

Created attachment 67642[details]
Patch with unit tests
Changed to raise an exception in ProcessAttachmentsMixin, and ProcessBugsMixin if we are running in non-interactive mode.
Added comment in PrepareChangeLog and just let the BugzillaError exception bubble instead of explicitly catching it.

Attachment 67642[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:535: .has_key() is deprecated, use 'in' [pep8/W601] [5]
WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py:58: at least two spaces before inline comment [pep8/E261] [5]
Total errors found: 2 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 68336[details]
Patch with unit test
Updated patch as <https://bugs.webkit.org/attachment.cgi?id=67642> became stale.
Also fixed style issue: "WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py:58: at least two spaces before inline comment"
After talking with Eric, decided to use has_key as the BeautifulSoup element object is not a native dictionary.

Attachment 68336[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:548: .has_key() is deprecated, use 'in' [pep8/W601] [5]
Total errors found: 1 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.