Bug 26715 - bugzilla-tool should automate rollouts
Summary: bugzilla-tool should automate rollouts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 28193
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-25 01:22 PDT by Eric Seidel (no email)
Modified: 2009-09-11 10:12 PDT (History)
4 users (show)

See Also:


Attachments
Work in progress (14.94 KB, patch)
2009-07-02 02:24 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Working, but not ready for primetime (16.48 KB, patch)
2009-07-02 02:59 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Updated patch for rollout support (16.90 KB, patch)
2009-07-17 15:04 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Begone ye tabs! (16.92 KB, patch)
2009-07-17 15:06 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Updated, just needs testing with more rollouts (16.72 KB, patch)
2009-07-31 22:32 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Updated to ToT (9.34 KB, patch)
2009-08-11 23:16 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Update rollout support (24.72 KB, patch)
2009-08-19 14:55 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch v1 (9.13 KB, patch)
2009-09-10 14:12 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Works well enough for others to start to use (9.20 KB, patch)
2009-09-10 14:16 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Make ChangeLog less pessimistic (9.12 KB, patch)
2009-09-10 14:18 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add a bunch of testing code for abarth (12.14 KB, patch)
2009-09-10 17:29 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Final patch with a bunch of extra testing (18.86 KB, patch)
2009-09-10 17:30 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Document all the extra testing in the ChangeLog (20.08 KB, patch)
2009-09-10 17:40 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-06-25 01:22:15 PDT
bugzilla-tool should automate rollouts

1.  Should take an svn revision and know how to search through git logs as necessary. (I think git svn has a command for this.)
2.  Do the revert.
3.  Update the ChangeLog to note that no review is needed.
4.  Test that things build correctly, etc.
5.  Re-open the bug with a rollout message.
Comment 1 Eric Seidel (no email) 2009-06-25 01:31:13 PDT
Once we have a way to do automated rollbacks, then we could eventually consider adding a web interface to do so. :)  Never have a red tree again! :)
Comment 2 Eric Seidel (no email) 2009-07-02 02:24:24 PDT
Created attachment 32173 [details]
Work in progress
Comment 3 Eric Seidel (no email) 2009-07-02 02:59:40 PDT
Created attachment 32174 [details]
Working, but not ready for primetime
Comment 4 Eric Seidel (no email) 2009-07-17 15:04:40 PDT
Created attachment 32974 [details]
Updated patch for rollout support


---
 4 files changed, 189 insertions(+), 51 deletions(-)
Comment 5 Eric Seidel (no email) 2009-07-17 15:06:24 PDT
Created attachment 32976 [details]
Begone ye tabs!


---
 4 files changed, 189 insertions(+), 51 deletions(-)
Comment 6 David Levin 2009-07-17 19:42:25 PDT
Comment on attachment 32976 [details]
Begone ye tabs!

Just a few things to address.  I'm most concerned about the issue I mentioned in update_changelog_for_revert.

Two general notes:
1. It was pointed out to me that python style is to have two blank lines between top level functions: "Separate top-level function and class definitions with two blank lines."
2. Many comments don't end with periods.  Please add them.

> diff --git a/WebKitTools/Scripts/bugzilla-tool b/WebKitTools/Scripts/bugzilla-tool

> +def update_changelog_for_revert(changelog_path, revision):
> +    removing_boilerplate = False
> +    # inplace=1 creates a backup file and re-directs stdout to the file
> +    for line in fileinput.FileInput(changelog_path, inplace=1):
> +        if re.search('Reviewed by', line):

I'm confused.  How does this stop after this boilerplate change log that was just created?

It looks like it goes through the whole changelog removing all descriptions along the way.


> +            print line.replace("Reviewed by NOBODY (OOPS!)", "No review, rolling out %s" % revision),

I like to prefix revisions with an r.
Consider: ..."No review, rolling out r%s"...


> +class RolloutCommit(Command):
...
> +    def create_changelogs_for_revert(scm, revision):
...
> +        # This could move to prepare-ChangeLog by adding a --revert= option

That might be easier.  I think the current code has a bug as I noted above.


> diff --git a/WebKitTools/Scripts/modules/scm.py b/WebKitTools/Scripts/modules/scm.py

> +    @classmethod
> +    def git_commit_from_svn_revision(cls, revision):
> +        git_log_output = cls.run_command(['git', 'svn', 'log', '-r', revision, '--oneline', '--show-commit'])
> +        # Example: r45396 | b4954e7 | 2009-06-30  Antonio Gomes   <antonio.gomes@openbossa.org>
> +        matched_commit = re.search("^r(\d+) \| (\w+) \| .*$", git_log_output, re.MULTILINE)

This is ok, but I always find code more readable when it uses named groups.
Comment 7 David Levin 2009-07-20 08:34:00 PDT
btw, ignore this comment: 

    > +        # This could move to prepare-ChangeLog by adding a --revert= option
   That might be easier.  I think the current code has a bug as I noted above.

as I realized I was completely wrong about it being easier and I rather like it in these files :)
Comment 8 Evan Martin 2009-07-22 11:28:31 PDT
+        git_log_output = cls.run_command(['git', 'svn', 'log', '-r', revision, '--oneline', '--show-commit'])
+        # Example: r45396 | b4954e7 | 2009-06-30  Antonio Gomes   <antonio.gomes@openbossa.org>
+        matched_commit = re.search("^r(\d+) \| (\w+) \| .*$", git_log_output, re.MULTILINE)


git svn find-rev r45396
Comment 9 Eric Seidel (no email) 2009-07-31 22:32:47 PDT
Created attachment 33930 [details]
Updated, just needs testing with more rollouts


---
 4 files changed, 186 insertions(+), 53 deletions(-)
Comment 10 Eric Seidel (no email) 2009-08-04 11:33:41 PDT
Some testing code for later:

    def _parse_revision_to_commit_mappings(self, stderr):
        revision_to_commit_mapping = {}
        for line in stderr.split_lines():
            match = line.match("^(?P<revision>r\d+) = (?P<commit_id>\w+)$")
            if match:
                revision_to_commit_mapping[match.group('revision')] = match.group('commit_id')
        return revision_to_commit_mapping
Comment 11 Eric Seidel (no email) 2009-08-11 23:16:13 PDT
Created attachment 34639 [details]
Updated to ToT


---
 4 files changed, 123 insertions(+), 3 deletions(-)
Comment 12 Eric Seidel (no email) 2009-08-19 14:55:33 PDT
Created attachment 35147 [details]
Update rollout support


---
 7 files changed, 373 insertions(+), 49 deletions(-)
Comment 13 Eric Seidel (no email) 2009-09-10 14:12:02 PDT
Created attachment 39377 [details]
Patch v1
Comment 14 Eric Seidel (no email) 2009-09-10 14:16:29 PDT
Created attachment 39379 [details]
Works well enough for others to start to use
Comment 15 Eric Seidel (no email) 2009-09-10 14:18:09 PDT
Created attachment 39381 [details]
Make ChangeLog less pessimistic
Comment 16 Adam Barth 2009-09-10 14:47:45 PDT
Comment on attachment 39381 [details]
Make ChangeLog less pessimistic

Would be nice to have some unittests for this change...  We also discussed some alternate approaches if the ChangeLog conflicts are too problematic.
Comment 17 Eric Seidel (no email) 2009-09-10 17:29:46 PDT
Created attachment 39397 [details]
Add a bunch of testing code for abarth
Comment 18 Eric Seidel (no email) 2009-09-10 17:30:54 PDT
Created attachment 39398 [details]
Final patch with a bunch of extra testing
Comment 19 Eric Seidel (no email) 2009-09-10 17:31:48 PDT
Note that I uploaded a diff from the reviewed patch which just contains the new testing changes as "Add a bunch of testing code for abarth".  The final patch (including all the added tests) is up for review again.
Comment 20 Eric Seidel (no email) 2009-09-10 17:40:18 PDT
Created attachment 39400 [details]
Document all the extra testing in the ChangeLog
Comment 21 Adam Barth 2009-09-10 21:52:59 PDT
Comment on attachment 39400 [details]
Document all the extra testing in the ChangeLog

Thanks for the tests.
Comment 22 WebKit Commit Bot 2009-09-11 01:12:24 PDT
Comment on attachment 39400 [details]
Document all the extra testing in the ChangeLog

Rejecting patch 39400 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment 23 Eric Seidel (no email) 2009-09-11 09:06:39 PDT
Comment on attachment 39400 [details]
Document all the extra testing in the ChangeLog

Bitten by bug 28845. :(
media/video-source-error.html -> timed out
Comment 24 WebKit Commit Bot 2009-09-11 09:54:51 PDT
Comment on attachment 39400 [details]
Document all the extra testing in the ChangeLog

Clearing flags on attachment: 39400

Committed r48303: <http://trac.webkit.org/changeset/48303>
Comment 25 WebKit Commit Bot 2009-09-11 09:55:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Eric Seidel (no email) 2009-09-11 10:12:13 PDT
Committed r48304: <http://trac.webkit.org/changeset/48304>