WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
REOPENED
39136
sheriffbot can't roll out security patches
https://bugs.webkit.org/show_bug.cgi?id=39136
Summary
sheriffbot can't roll out security patches
Eric Seidel (no email)
Reported
2010-05-14 14:16:25 PDT
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.
Attachments
Patch with unit tests
(9.72 KB, patch)
2010-09-11 16:12 PDT
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Patch with unit tests
(21.76 KB, patch)
2010-09-12 12:03 PDT
,
Daniel Bates
abarth
: review-
Details
Formatted Diff
Diff
Patch with unit tests
(22.21 KB, patch)
2010-09-14 22:36 PDT
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Patch with unit test
(21.13 KB, patch)
2010-09-21 21:15 PDT
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Daniel Bates
Comment 1
2010-09-11 16:12:57 PDT
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.
Daniel Bates
Comment 2
2010-09-11 16:17:20 PDT
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.
Adam Barth
Comment 3
2010-09-11 16:21:30 PDT
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...
Daniel Bates
Comment 4
2010-09-11 16:25:05 PDT
(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.
Daniel Bates
Comment 5
2010-09-12 11:58:12 PDT
(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?
Daniel Bates
Comment 6
2010-09-12 12:03:50 PDT
Created
attachment 67340
[details]
Patch with unit tests Updated patch based on Adam Barth's suggestions.
WebKit Review Bot
Comment 7
2010-09-12 12:09:18 PDT
Attachment 67340
[details]
did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKitTools/Scripts/webkitpy/tool/commands/download.py:166: at least two spaces before inline comment [pep8/E261] [5] WebKitTools/Scripts/webkitpy/tool/commands/download.py:179: at least two spaces before inline comment [pep8/E261] [5] WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:47: expected 2 blank lines, found 1 [pep8/E302] [5] WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:517: .has_key() is deprecated, use 'in' [pep8/W601] [5] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Adam Barth
Comment 8
2010-09-12 12:19:28 PDT
> 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.
Adam Barth
Comment 9
2010-09-12 12:27:59 PDT
Comment on
attachment 67340
[details]
Patch with unit tests View in context:
https://bugs.webkit.org/attachment.cgi?id=67340&action=prettypatch
> WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:73 > + elif (e.is_not_permitted_to_view_bug()): > + ScriptError(message="SheriffBot is not authorized to rollout a security bug.")
It's kind of too bad that we'll announce to the whole channel that this patch fixes a security bug, but I guess we're kind of doing that already... Maybe something like "SheriffBot is not authorized to view
https://bugs.webkit.org/show_bug.cgi
?...."
> WebKitTools/Scripts/webkitpy/tool/commands/download.py:166 > class ProcessAttachmentsMixin(object): > def _fetch_list_of_patches_to_process(self, options, args, tool): > - return map(lambda patch_id: tool.bugs.fetch_attachment(patch_id), args) > + all_patches = [] > + for patch_id in args: > + try: > + all_patches.append(tool.bugs.fetch_attachment(patch_id)) > + except BugzillaError, e: > + continue # Ignore this patch.
Can we print a message so the user knows that one of the attachments was skipped? Also, this behavior seems wrong when used by the bots because the command will appear to succeed when, in fact, it doesn't do anything... Maybe check for options.non_interactive and error out?
> WebKitTools/Scripts/webkitpy/tool/commands/download.py:179 > class ProcessBugsMixin(object): > def _fetch_list_of_patches_to_process(self, options, args, tool): > all_patches = [] > for bug_id in args: > - patches = tool.bugs.fetch_bug(bug_id).reviewed_patches() > - log("%s found on bug %s." % (pluralize("reviewed patch", len(patches)), bug_id)) > - all_patches += patches > + try: > + patches = tool.bugs.fetch_bug(bug_id).reviewed_patches() > + log("%s found on bug %s." % (pluralize("reviewed patch", len(patches)), bug_id)) > + all_patches += patches > + except BugzillaError, e: > + continue # Ignore the patches for this bug.
Similar comments here.
> WebKitTools/Scripts/webkitpy/tool/commands/upload.py:91 > + # This may occur if the bug becomes a security bug between the time > + # we fetched its ID from the pending commit list and now. We ignore > + # this bug. > + continue
Again, I'd print a message here to explain what just happened.
> WebKitTools/Scripts/webkitpy/tool/steps/obsoletepatches.py:56 > + try: > + patches = self._tool.bugs.fetch_bug(bug_id).patches() > + except BugzillaError, e: > + if (e.is_invalid_bug_id()): > + log("Bug %s is invalid." % bug_id) > + elif (e.is_not_found()): > + log("Bug %s does not exist." % bug_id) > + elif (e.is_not_permitted_to_view_bug()): > + log("You're not authorized to modify bug %s." % bug_id) > + return
This code seems copy/pasted three times. Is there some way we could share code between these instances?
> WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py:59 > + except BugzillaError, e: > + # We may not be authorized to retrieve the bug title if it's for > + # a security bug. Therefore, we leave the change logs unchanged. > + return
Again, we should tell the user what happened. I think we need to error out in this case. We can't really proceed with the commands that use this step without a ChangeLog. We use the ChangeLog to store important state information (like the bug id).
Daniel Bates
Comment 10
2010-09-14 22:36:24 PDT
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.
WebKit Review Bot
Comment 11
2010-09-14 22:43:05 PDT
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.
Daniel Bates
Comment 12
2010-09-21 21:15:35 PDT
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.
WebKit Review Bot
Comment 13
2010-09-21 21:21:38 PDT
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.
Adam Barth
Comment 14
2010-09-26 22:22:32 PDT
Comment on
attachment 68336
[details]
Patch with unit test View in context:
https://bugs.webkit.org/attachment.cgi?id=68336&action=review
> WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:73 > + ScriptError(message="SheriffBot is not authorized to view %s." % self._tool.bugs.bug_url_for_bug_id(e.bug_id())) > + else: > + ScriptError(message="Could not determine the corresponding bug for r%s." % svn_revision)
Did you want to raise these errors?
> WebKitTools/Scripts/webkitpy/tool/commands/download.py:167 > + raise e
You just want to say "raise" without giving the exception again. That will make the stack trace better.
> WebKitTools/Scripts/webkitpy/tool/commands/download.py:183 > + raise e
ditto
Daniel Bates
Comment 15
2010-09-27 21:34:05 PDT
(In reply to
comment #14
)
> (From update of
attachment 68336
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=68336&action=review
> > > WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:73 > > + ScriptError(message="SheriffBot is not authorized to view %s." % self._tool.bugs.bug_url_for_bug_id(e.bug_id())) > > + else: > > + ScriptError(message="Could not determine the corresponding bug for r%s." % svn_revision) > > Did you want to raise these errors?
Yes. I'll fix this before I land.
> > > WebKitTools/Scripts/webkitpy/tool/commands/download.py:167 > > + raise e > > You just want to say "raise" without giving the exception again. That will make the stack trace better.
Will change.
> > > WebKitTools/Scripts/webkitpy/tool/commands/download.py:183 > > + raise e > > ditto
Will change.
Daniel Bates
Comment 16
2010-09-27 23:39:42 PDT
Committed
r68493
: <
http://trac.webkit.org/changeset/68493
>
Daniel Bates
Comment 17
2010-09-28 00:29:24 PDT
Unfortunately, this change regressed Sheriffbot's rollout feature. With Adam's help, we discovered that this change caused Sheriffbot to raise an exception in bugzilla.py: abarth: File "/mnt/git/webkit-sheriff-bot/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py", line 547, in _parse_bug_page [11:50pm] renato joined the chat room. [11:50pm] abarth: bug_id = soup.find("bug_id").string [11:50pm] abarth: AttributeError: 'NoneType' object has no attribute 'string' Reverted changeset 68493 <
http://trac.webkit.org/changeset/68493
> in changeset 68496 <
http://trac.webkit.org/changeset/68496
>.
Daniel Bates
Comment 18
2010-09-28 00:29:50 PDT
Comment on
attachment 68336
[details]
Patch with unit test Clearing review-queue and commit-queue flags.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug