[webkit-reviews] review denied: [Bug 39136] sheriffbot can't roll out security patches : [Attachment 67340] Patch with unit tests

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 39136: sheriffbot can't roll out security patches
https://bugs.webkit.org/show_bug.cgi?id=39136

Attachment 67340: Patch with unit tests
https://bugs.webkit.org/attachment.cgi?id=67340&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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).