Bug 26715 – bugzilla-tool should automate rollouts

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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! :)

[Eric Seidel (no email)]

Created attachment 32173 [details]
Work in progress

[Eric Seidel (no email)]

Created attachment 32174 [details]
Working, but not ready for primetime

[Eric Seidel (no email)]

Created attachment 32974 [details]
Updated patch for rollout support


---
 4 files changed, 189 insertions(+), 51 deletions(-)

[Eric Seidel (no email)]

Created attachment 32976 [details]
Begone ye tabs!


---
 4 files changed, 189 insertions(+), 51 deletions(-)

[David Levin]

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   <[e-mail]>
> +        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.

[David Levin]

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 :)

[Evan Martin]

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


git svn find-rev r45396

[Eric Seidel (no email)]

Created attachment 33930 [details]
Updated, just needs testing with more rollouts


---
 4 files changed, 186 insertions(+), 53 deletions(-)

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Created attachment 34639 [details]
Updated to ToT


---
 4 files changed, 123 insertions(+), 3 deletions(-)

[Eric Seidel (no email)]

Created attachment 35147 [details]
Update rollout support


---
 7 files changed, 373 insertions(+), 49 deletions(-)

[Eric Seidel (no email)]

Created attachment 39377 [details]
Patch v1

[Eric Seidel (no email)]

Created attachment 39379 [details]
Works well enough for others to start to use

[Eric Seidel (no email)]

Created attachment 39381 [details]
Make ChangeLog less pessimistic

[Adam Barth]

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.

[Eric Seidel (no email)]

Created attachment 39397 [details]
Add a bunch of testing code for abarth

[Eric Seidel (no email)]

Created attachment 39398 [details]
Final patch with a bunch of extra testing

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 39400 [details]
Document all the extra testing in the ChangeLog

[Adam Barth]

Comment on attachment 39400 [details]
Document all the extra testing in the ChangeLog

Thanks for the tests.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 39400 [details]
Document all the extra testing in the ChangeLog

Bitten by bug 28845. :(
media/video-source-error.html -> timed out

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Committed r48304: <http://trac.webkit.org/changeset/48304>