[Webkit-unassigned] [Bug 39136] sheriffbot can't roll out security patches

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39136


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #67340|review?                     |review-
               Flag|                            |




--- Comment #9 from Adam Barth <abarth at webkit.org>  2010-09-12 12:27:59 PST ---
(From update of attachment 67340)
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).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.