Support --target_branch (#877)

nik9000 · web-flow · commit efcb008a2e65 · 2019-05-15T13:16:18.000-04:00
Adds support for specifying the branch to which to build docs in the
target repo. This will make release times smoother because we can
continue to build the docs but send them to the `staging` branch. It
*might* allow more fancy pull request testing by allowing us to send
pull requests to their own branch.

If the branch already exists we just check it out and build the docs
onto it and then push it. If it doesn't exist then things are a little
more complicated. First we check if the `master` branch exists. It won't
in an entirely fresh repo which we often use for testing. If it doesn't
exist we create an empty commit in the master branch to "anchor" it.
Once we're sure that the master branch exists we fork the new target
branch from the master branch.

Pushing the built docs gets a bit more complex in two ways:
1. If the target branch is new we always push it, even if there haven't
been any changes. This keeps things consistent, both for easy testing
and for the potential PR testing work that I mentioned above.
2. If we had to create an empty master branch we push that too. This
makes sure that non-empty repos have a master branch which is a fairly
useful invariant.
diff --git a/build_docs.pl b/build_docs.pl
@@ -29,7 +29,6 @@ BEGIN
     run $Opts
     build_chunked build_single build_pdf
     proc_man
-    sha_for
     timestamp
     write_html_redirect
     write_nginx_redirects
@@ -462,6 +461,7 @@ sub init_repos {
         url         => $Opts->{target_repo},
         reference   => $reference_dir,
         destination => dir( $target_repo_checkout ),
+        branch      => $Opts->{target_branch} || 'master',
     );
     delete $child_dirs{ $target_repo->git_dir->absolute };
     $tracker_path = "$target_repo_checkout/$tracker_path";
@@ -555,30 +555,23 @@ sub push_changes {
     $build_dir->file('revision.txt')
         ->spew( iomode => '>:utf8', ES::Repo->all_repo_branches );
 
-    say 'Preparing commit';
-    run qw(git add -A);
-
-    if ( run qw(git status -s --) ) {
+    # Check if there are changes before committing and save that because if
+    # there are changes we'll commit them. Once we've committed them checking
+    # if there are changes will say "no" because there aren't changes
+    # *any more*. Thus we build $has_changes and $should_push here and not
+    # in the `if` statements below.
+    my $has_changes = $target_repo->outstanding_changes;
+    my $should_push = $has_changes || $target_repo->new_branch;
+    if ( $has_changes ) {
+        say 'Preparing commit';
         build_sitemap($build_dir);
-        run qw(git add -A);
         say "Commiting changes";
-        run qw(git commit -m), 'Updated docs';
+        $target_repo->commit;
     }
-
-    my $remote_sha = eval {
-        my $remote = run qw(git rev-parse --symbolic-full-name @{u});
-        chomp $remote;
-        return sha_for($remote);
-    } || '';
-
-    if ( sha_for('HEAD') ne $remote_sha ) {
-        say "Pushing changes to bare repo";
-        run qw(git push origin HEAD );
-        local $ENV{GIT_DIR} = $target_repo->git_dir if $target_repo;
+    if ( $should_push ) {
         say "Pushing changes";
-        run qw(git push origin HEAD );
-    }
-    else {
+        $target_repo->push_changes;
+    } else {
         say "No changes to push";
     }
 }
@@ -760,6 +753,7 @@ sub command_line_opts {
         'toc',
         # Options only compatible with --all
         'all',
+        'target_branch=s',
         'target_repo=s',
         'keep_hash',
         'linkcheckonly',
@@ -820,6 +814,7 @@ sub usage {
           --sub_dir         Use a directory as a branch of some repo
                             (eg --sub_dir elasticsearch:master:~/Code/elasticsearch)
           --target_repo     Repository to which to commit docs
+          --target_branch   Branch to which to commit docs
           --user            Specify which GitHub user to use, if not your own
 
     General Opts:
@@ -868,6 +863,7 @@ sub check_opts {
         die('--reference not compatible with --doc') if $Opts->{reference};
         die('--skiplinkcheck not compatible with --doc') if $Opts->{skiplinkcheck};
         die('--sub_dir not compatible with --doc') if $Opts->{sub_dir};
+        die('--target_branch not compatible with --doc') if $Opts->{target_branch};
         die('--target_repo not compatible with --doc') if $Opts->{target_repo};
         die('--user not compatible with --doc') if $Opts->{user};
     } else {
diff --git a/integtest/spec/all_books_change_detection_spec.rb b/integtest/spec/all_books_change_detection_spec.rb
@@ -4,42 +4,50 @@
 # Assertions about when books are rebuilt based on changes in source
 # repositories or the book's configuration.
 RSpec.describe 'building all books' do
+  class Config
+    attr_accessor :target_branch
+
+    def initialize
+      @target_branch = nil
+    end
+  end
   describe 'change detection' do
     def self.build_twice(
         before_first_build:,
-        before_second_build: ->(src) {}
+        before_second_build:
       )
       convert_before do |src, dest|
+        config = Config.new
         # Allow the caller to customize the source.
-        before_first_build.call(src)
+        before_first_build.call(src, config)
 
         # Convert the first time. This should build the docs.
-        dest.convert_all src.conf
+        dest.convert_all src.conf, target_branch: config.target_branch
 
         # Take some action between the builds.
-        before_second_build.call(src)
+        before_second_build.call(src, config)
 
         # Convert the second time.
-        dest.convert_all src.conf
+        dest.convert_all src.conf, target_branch: config.target_branch
 
         # Checkout the files so we can assert about them.
-        dest.checkout_conversion
+        dest.checkout_conversion branch: config.target_branch
       end
       include_context 'build one book twice'
     end
 
     def self.build_one_book_out_of_one_repo_twice(
-        before_first_build: ->(src) {},
-        before_second_build: ->(src) {}
+        before_first_build: ->(src, config) {},
+        before_second_build: ->(src, config) {}
       )
       build_twice(
-        before_first_build: lambda do |src|
+        before_first_build: lambda do |src, config|
           repo = src.repo_with_index 'repo', 'Some text.'
           book = src.book 'Test'
           book.source repo, 'index.asciidoc'
 
           # Allow the caller to customize the source
-          before_first_build.call(src)
+          before_first_build.call src, config
         end,
         before_second_build: before_second_build
       )
@@ -50,13 +58,15 @@ def self.build_one_book_out_of_two_repos_twice(
         before_second_build: ->(src) {}
       )
       build_twice(
-        before_first_build: lambda do |src|
+        before_first_build: lambda do |src, _config|
           src.simple_include
 
           # Allow the caller to customize the source
-          before_first_build.call(src)
+          before_first_build.call src
         end,
-        before_second_build: before_second_build
+        before_second_build: lambda do |src, _config|
+          before_second_build.call src
+        end
       )
     end
 
@@ -100,25 +110,51 @@ def self.build_one_book_out_of_two_repos_twice(
           build_one_book_out_of_one_repo_twice
           include_examples 'second build is noop'
         end
-
-        context 'because there are unrelated changes source repo' do
+        context 'even when there are unrelated changes source repo' do
           build_one_book_out_of_one_repo_twice(
-            before_second_build: lambda do |src|
+            before_second_build: lambda do |src, _config|
               repo = src.repo 'repo'
               repo.write 'garbage', 'junk'
               repo.commit 'adding junk'
             end
           )
           include_examples 'second build is noop'
         end
+        context 'even when there is a target_branch' do
+          build_one_book_out_of_one_repo_twice(
+            before_first_build: lambda do |_src, config|
+              config.target_branch = 'new_target'
+            end
+          )
+          include_examples 'second build is noop'
+        end
+        context 'even when there is a new target branch' do
+          # Adding a new target branch will cause us to fork it from the
+          # master branch which so we won't have to rebuild the book *but*
+          # we push anyway so the new target branch is available.
+          build_one_book_out_of_one_repo_twice(
+            before_second_build: lambda do |_src, config|
+              config.target_branch = 'new_target'
+            end
+          )
+          context 'the second build' do
+            let(:out) { outputs[1] }
+            it "doesn't print that it is building any books" do
+              expect(out).not_to include(': Building ')
+            end
+            it "doesn't print that it is commiting changes" do
+              expect(out).not_to include('Commiting changes')
+            end
+            it 'prints that it is pushing changes' do
+              expect(out).to include('Pushing changes')
+            end
+          end
+        end
       end
       context "when the second build isn't a noop" do
-        let(:latest_revision) { 'changed text' }
-        let(:new_text) { 'New text.' }
-
         context 'because the source repo changes' do
           build_one_book_out_of_one_repo_twice(
-            before_second_build: lambda do |src|
+            before_second_build: lambda do |src, _config|
               repo = src.repo 'repo'
               repo.write 'index.asciidoc', <<~ASCIIDOC
                 = Title
@@ -130,35 +166,49 @@ def self.build_one_book_out_of_two_repos_twice(
               repo.commit 'changed text'
             end
           )
+          let(:latest_revision) { 'changed text' }
+          let(:new_text) { 'New text.' }
           include_examples 'second build is not a noop'
         end
         context 'because the book changes from asciidoc to asciidoctor' do
           build_one_book_out_of_one_repo_twice(
-            before_first_build: lambda do |src|
+            before_first_build: lambda do |src, _config|
               book = src.book 'Test'
               book.asciidoctor = false
             end,
-            before_second_build: lambda do |src|
+            before_second_build: lambda do |src, _config|
               book = src.book 'Test'
               book.asciidoctor = true
             end
           )
-          # We didn't change the revision....
           let(:latest_revision) { 'init' }
-          # And the text hasn't changed
           let(:new_text) { 'Some text.' }
           include_examples 'second build is not a noop'
         end
         context 'because the book changes from asciidoctor to asciidoc' do
           build_one_book_out_of_one_repo_twice(
-            before_second_build: lambda do |src|
+            before_second_build: lambda do |src, _config|
               book = src.book 'Test'
               book.asciidoctor = false
             end
           )
-          # We didn't change the revision....
           let(:latest_revision) { 'init' }
-          # And the text hasn't changed
+          let(:new_text) { 'Some text.' }
+          include_examples 'second build is not a noop'
+        end
+        context 'because we remove the target_branch' do
+          # Removing the target branch causes us to build into the *empty*
+          # master branch. Being empty, there aren't any books in it to
+          # consider "already built".
+          build_one_book_out_of_one_repo_twice(
+            before_first_build: lambda do |_src, config|
+              config.target_branch = 'new_target'
+            end,
+            before_second_build: lambda do |_src, config|
+              config.target_branch = nil # nil means don't override
+            end
+          )
+          let(:latest_revision) { 'init' }
           let(:new_text) { 'Some text.' }
           include_examples 'second build is not a noop'
         end
diff --git a/integtest/spec/all_books_spec.rb b/integtest/spec/all_books_spec.rb
@@ -112,4 +112,16 @@ def self.single_book_built_by_two_repos
     let(:latest_revision) { 'init' }
     include_examples 'book basics', 'Test', 'test'
   end
+  context 'when target_branch is specified' do
+    convert_all_before_context target_branch: 'new_branch' do |src|
+      repo = src.repo_with_index 'repo', 'Some text.'
+      book = src.book 'Test'
+      book.source repo, 'index.asciidoc'
+    end
+    let(:latest_revision) { 'init' }
+    include_examples 'book basics', 'Test', 'test'
+    it 'prints that it is forking the new branch from master' do
+      expect(out).to include('target_repo: Forking <new_branch> from master')
+    end
+  end
 end
diff --git a/integtest/spec/helper/dest.rb b/integtest/spec/helper/dest.rb
@@ -49,20 +49,23 @@ def convert_single(from, to,
 
   ##
   # Convert a conf file worth of books and check it out.
-  def convert_all(conf, expect_failure: false)
+  def convert_all(conf, expect_failure: false, target_branch: nil)
     cmd = %W[
       --all
       --push
       --target_repo #{bare_repo}
       --conf #{conf}
     ]
+    cmd += ['--target_branch', target_branch] if target_branch
     run_convert(cmd, expect_failure)
   end
 
   ##
   # Checks out the results of the last call to convert_all
-  def checkout_conversion
-    sh "git clone #{bare_repo} #{@dest}"
+  def checkout_conversion(branch: nil)
+    branch_cmd = ''
+    branch_cmd = "--branch #{branch} " if branch
+    sh "git clone #{branch_cmd}#{bare_repo} #{@dest}"
   end
 
   private
diff --git a/integtest/spec/helper/dsl/convert_all.rb b/integtest/spec/helper/dsl/convert_all.rb
@@ -8,11 +8,12 @@ module ConvertAll
     # uses it to:
     # 1. Create source repositories and write them
     # 2. Configure the books that should be built
-    def convert_all_before_context(relative_conf: false)
+    def convert_all_before_context(relative_conf: false, target_branch: nil)
       convert_before do |src, dest|
         yield src
-        dest.convert_all src.conf(relative_path: relative_conf)
-        dest.checkout_conversion
+        dest.convert_all src.conf(relative_path: relative_conf),
+                         target_branch: target_branch
+        dest.checkout_conversion branch: target_branch
       end
       include_examples 'convert all'
     end
diff --git a/lib/ES/TargetRepo.pm b/lib/ES/TargetRepo.pm
