Skip to content

Commit a28a83b

Browse files
authored
Fancy --sub_dir and --keep_hash (#1262)
This adds a fancy interaction between `--sub-dir` and `--keep_hash` that is meant to make sure we build *exactly* the right docs for pull requests. When we first deploy pull request builds we targeted them at the tip of the pull request branch. This caused trouble because the tip of the pull request branch was often behind the last successful build. Building the docs against that would often cause spurious failure from out of date links. Merging `master` into the PR branch usually fixed it but it was annoying. We fixed this problem by switching our target to the result of merging the pull request branch into the target branch. This was *better* because it had fewer spurious failures, but often caused us to think that the docs changes were larger than they actually were because the target branch often had changes in it that hadn't yet been picked up by the docs build. This was annoying because the `diff`s generated by the docs builds had changes that weren't part of the PR. Which made it difficult to track down what actually changed. This solves the problem by first merging the `--sub_dir`ed branch into the last successful docs build. Thus we only build *exactly* what is in the PR. We trigger this behavior by checking for both the `--keep_hash` and `--sub_dir` flags *and* checking for outstanding changes in the `--sub_dir`ed branch. This behavior might end up being slower sometimes because merging can be expensive, but it is usually less expensive than building a book we don't have to build. So maybe it'll all come out in the wash.
1 parent 1329469 commit a28a83b

File tree

8 files changed

+353
-37
lines changed

8 files changed

+353
-37
lines changed

README.asciidoc

+11-3
Original file line numberDiff line numberDiff line change
@@ -231,13 +231,21 @@ To check links before you merge your changes:
231231
into the master branch of the `elasticsearch` repo, run:
232232
+
233233
----------------------------
234-
./docs/build_docs --all --target_repo [email protected]:elastic/built-docs.git --open --sub_dir elasticsearch:master:./elasticsearch
234+
./docs/build_docs --all --target_repo [email protected]:elastic/built-docs.git \
235+
--open --keep_hash --sub_dir elasticsearch:master:./elasticsearch
235236
----------------------------
236237

237-
To run a full build to mimic the website build, omit the `--sub_dir` option:
238+
NOTE: If there are no outstanding changes in the `../elasticsearch` directory
239+
then this will build against the result of merging the last successful
240+
docs build and the contents of `../elasticsearch`. If there *are*
241+
outstanding changes then it'll just build against the contents of
242+
`../elasticsearch`.
243+
244+
To run a full build to mimic the website build, omit the `--sub_dir` and
245+
`--keep_hash` options:
238246

239247
----------------------------
240-
build_docs --all --target_repo [email protected]:elastic/built-docs.git --open
248+
./build_docs --all --target_repo [email protected]:elastic/built-docs.git --open
241249
----------------------------
242250

243251
The first time you run a full build is slow as it needs to:

build_docs.pl

+2-2
Original file line numberDiff line numberDiff line change
@@ -607,7 +607,7 @@ sub init_repos {
607607
tracker => $tracker,
608608
url => $url,
609609
reference => $reference_dir,
610-
keep_hash => $Opts->{keep_hash},
610+
keep_hash => $Opts->{keep_hash} || 0,
611611
);
612612
delete $child_dirs{ $repo->git_dir->absolute };
613613

@@ -644,7 +644,7 @@ sub init_repos {
644644
ES::DocsRepo->new(
645645
tracker => $tracker,
646646
dir => $conf->{docs} || '/docs_build',
647-
keep_hash => $Opts->{keep_hash}
647+
keep_hash => $Opts->{keep_hash} || 0
648648
);
649649

650650
return $tracker;

integtest/spec/all_books_change_detection_spec.rb

+5-2
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,7 @@ def chapter(index)
401401
before_second_build: lambda do |src, config|
402402
repo = src.repo 'repo'
403403
repo.write 'dummy', 'dummy'
404+
repo.commit 'dummy'
404405

405406
config.extra do |conversion|
406407
conversion.keep_hash.sub_dir(repo, 'master')
@@ -619,7 +620,9 @@ def chapter(index)
619620
book = src.book 'Test'
620621
book.source repo2, 'not_used_actually'
621622
repo = src.repo 'repo'
623+
repo.switch_to_new_branch 'subbed'
622624
repo.write 'index.asciidoc', TWO_CHAPTERS + "\nmore words"
625+
repo.commit 'sub'
623626
config.extra do |conversion|
624627
conversion.keep_hash.sub_dir(repo, 'master')
625628
end
@@ -722,9 +725,9 @@ def self.add_branch(src)
722725
context 'the second build' do
723726
let(:out) { outputs[1] }
724727
include_examples 'commits changes'
725-
it "doesn't print that it is building the original branch" do
728+
it "doesn't print that it is building any branch" do
726729
# The original book hasn't changed so we don't rebuild it
727-
expect(out).not_to include('Test: Building master...')
730+
expect(out).not_to include('Test: Building')
728731
end
729732
end
730733
end
+210
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,210 @@
1+
# frozen_string_literal: true
2+
3+
RSpec.describe 'building all books' do
4+
describe '--sub_dir' do
5+
##
6+
# Setups up a repo that looks like:
7+
# master sub_me
8+
# original master-------->subbed
9+
# |
10+
# new master
11+
# |
12+
# too new master
13+
#
14+
# Optionally runs the build once on the commit "new master". Then it always
15+
# runs the build, substituting sub_me for the master branch. If:
16+
# * --keep_hash isn't specified or
17+
# * the sub_me branch has outstanding changes or
18+
# * there was a merge conflict or
19+
# * the build wasn't run against "new master"
20+
# then --sub_dir will pick up the contents of the directory and the build
21+
# won't have "new maser" because it was forked from "original master".
22+
# Otherwise, --keep_hash and --sub_dir will cause the build to merge
23+
# "new master" and "subbed" and build against *that*.
24+
def self.convert_with_sub(keep_hash: true, commit_sub: true,
25+
build_with_init: true,
26+
cause_merge_conflict: false, premerge: false)
27+
convert_before do |src, dest|
28+
repo = setup_repo src
29+
setup_book src, repo
30+
dest.prepare_convert_all(src.conf).convert if build_with_init
31+
modify_master_after_build repo
32+
setup_sub repo, commit_sub, cause_merge_conflict, premerge
33+
convert src, repo, dest, keep_hash
34+
end
35+
end
36+
37+
def self.setup_repo(src)
38+
repo = src.repo 'repo'
39+
repo.write 'docs/index.adoc', index
40+
repo.write 'docs/from_master.adoc', 'original master'
41+
repo.write 'docs/from_subbed.adoc', 'unsubbed'
42+
repo.commit 'original master'
43+
repo.write 'docs/from_master.adoc', 'new master'
44+
repo.write 'docs/conflict', 'from master'
45+
repo.commit 'new master'
46+
repo
47+
end
48+
49+
def self.setup_book(src, repo)
50+
book = src.book 'Test'
51+
book.index = 'docs/index.adoc'
52+
book.source repo, 'docs'
53+
end
54+
55+
def self.modify_master_after_build(repo)
56+
repo.write 'docs/from_master.adoc', 'too new master'
57+
repo.commit 'too new master'
58+
end
59+
60+
def self.setup_sub(repo, commit_sub, cause_merge_conflict, premerge)
61+
repo.switch_to_branch 'HEAD~2'
62+
repo.switch_to_new_branch 'sub_me'
63+
repo.write 'docs/from_subbed.adoc', 'now subbed'
64+
repo.write 'docs/conflict', 'from subbed' if cause_merge_conflict
65+
repo.commit 'subbed' if commit_sub
66+
repo.merge 'master' if premerge
67+
end
68+
69+
def self.convert(src, repo, dest, keep_hash)
70+
builder = dest.prepare_convert_all src.conf
71+
builder.sub_dir repo, 'master'
72+
builder.keep_hash if keep_hash
73+
builder.convert
74+
dest.checkout_conversion
75+
end
76+
77+
def self.index
78+
<<~ASCIIDOC
79+
= Title
80+
81+
[[chapter]]
82+
== Chapter
83+
84+
include::from_master.adoc[]
85+
include::from_subbed.adoc[]
86+
ASCIIDOC
87+
end
88+
89+
let(:logs) { outputs[-1] }
90+
91+
shared_examples 'examples' do |master|
92+
file_context 'raw/test/master/chapter.html' do
93+
it "contains the #{master} master changes" do
94+
expect(contents).to include("<p>#{master} master</p>")
95+
end
96+
it 'contains the subbed changes' do
97+
expect(contents).to include('<p>now subbed</p>')
98+
end
99+
end
100+
end
101+
shared_examples 'contains the original master and subbed changes' do
102+
include_examples 'examples', 'original'
103+
end
104+
shared_examples 'contains the new master and subbed changes' do
105+
include_examples 'examples', 'new'
106+
end
107+
shared_examples 'contains the too new master and subbed changes' do
108+
include_examples 'examples', 'too new'
109+
end
110+
shared_examples 'log merge' do |path|
111+
it "log that it started merging [#{path}]" do
112+
expect(logs).to include(<<~LOGS)
113+
Test: Merging the subbed dir for [repo][master][#{path}] into the last successful build.
114+
LOGS
115+
end
116+
it "log that it merged [#{path}]" do
117+
expect(logs).to include(<<~LOGS)
118+
Test: Merged the subbed dir for [repo][master][#{path}] into the last successful build.
119+
LOGS
120+
end
121+
end
122+
123+
describe 'without --keep_hash' do
124+
convert_with_sub keep_hash: false
125+
it "doesn't log that it won't merge because of uncommitted changes" do
126+
expect(logs).not_to include(<<~LOGS)
127+
Test: Not merging the subbed dir for [repo][master][docs] because it has uncommitted changes.
128+
LOGS
129+
end
130+
include_examples 'contains the original master and subbed changes'
131+
end
132+
describe 'with --keep_hash' do
133+
describe 'when there are uncommitted changes' do
134+
convert_with_sub commit_sub: false
135+
it "logs that it won't merge because of uncommitted changes" do
136+
expect(logs).to include(<<~LOGS)
137+
Test: Not merging the subbed dir for [repo][master][docs] because it has uncommitted changes.
138+
LOGS
139+
end
140+
include_examples 'contains the original master and subbed changes'
141+
end
142+
describe 'when the source is new' do
143+
convert_with_sub build_with_init: false
144+
it "log that it won't merge because the source is new" do
145+
expect(logs).to include(<<~LOGS)
146+
Test: Not merging the subbed dir for [repo][master][docs] because it is new.
147+
LOGS
148+
end
149+
include_examples 'contains the original master and subbed changes'
150+
end
151+
describe 'when the subbed dir can be merged' do
152+
convert_with_sub
153+
include_examples 'log merge', 'docs'
154+
include_examples 'contains the new master and subbed changes'
155+
end
156+
describe 'when the subbed dir has already been merged' do
157+
# This simulates what github will do if you ask it to build the "sha"
158+
# of the merged PR instead of the "head" of the branch.
159+
convert_with_sub premerge: true
160+
include_examples 'log merge', 'docs'
161+
include_examples 'contains the too new master and subbed changes'
162+
end
163+
describe 'when there is a conflict merging the subbed dir' do
164+
convert_with_sub cause_merge_conflict: true
165+
it 'logs that it failed to merge' do
166+
expect(logs).to include(<<~LOGS)
167+
Test: Failed to merge the subbed dir for [repo][master][docs] into the last successful build:
168+
LOGS
169+
end
170+
it 'logs the conflict' do
171+
expect(logs).to include(<<~LOGS)
172+
CONFLICT (add/add): Merge conflict in docs/conflict
173+
LOGS
174+
end
175+
include_examples 'contains the original master and subbed changes'
176+
end
177+
describe 'when there is more than one source using the same repo' do
178+
def self.setup_book(src, repo)
179+
book = src.book 'Test'
180+
book.index = 'docs/index.adoc'
181+
book.source repo, 'docs/index.adoc'
182+
book.source repo, 'docs/from_master.adoc'
183+
book.source repo, 'docs/from_subbed.adoc'
184+
end
185+
convert_with_sub
186+
include_examples 'log merge', 'docs/index.adoc'
187+
include_examples 'log merge', 'docs/from_master.adoc'
188+
include_examples 'log merge', 'docs/from_subbed.adoc'
189+
include_examples 'contains the new master and subbed changes'
190+
end
191+
describe 'when more than one book uses the same source' do
192+
def self.setup_book(src, repo)
193+
%w[Test Test2].each do |name|
194+
book = src.book name
195+
book.index = 'docs/index.adoc'
196+
book.source repo, 'docs'
197+
end
198+
end
199+
convert_with_sub
200+
include_examples 'log merge', 'docs'
201+
it 'logs only one merge' do
202+
# This asserts that we log a single merge. We *should* be using the
203+
# cache instead.
204+
expect(logs).not_to match(/Merged.+Merged/m)
205+
end
206+
include_examples 'contains the new master and subbed changes'
207+
end
208+
end
209+
end
210+
end

integtest/spec/helper/repo.rb

+7
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ def switch_to_new_branch(new_branch)
9393
##
9494
# Checks out a branch.
9595
def switch_to_branch(branch)
96+
# TODO: rename to checkout?
9697
Dir.chdir @root do
9798
sh "git checkout #{branch}"
9899
end
@@ -139,6 +140,12 @@ def push_to(dest)
139140
end
140141
end
141142

143+
def merge(ref)
144+
Dir.chdir @root do
145+
sh "git merge #{ref}"
146+
end
147+
end
148+
142149
def copy_shared_conf
143150
FileUtils.mkdir_p @root
144151
sh "cp -r /docs_build/shared #{@root}"

lib/ES/BaseRepo.pm

+13
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,19 @@ sub _reference_args {
141141
return ();
142142
}
143143

144+
#===================================
145+
# Write a sparse checkout config for the repo.
146+
#===================================
147+
sub _write_sparse_config {
148+
#===================================
149+
my ( $self, $root, $config ) = @_;
150+
151+
my $dest = $root->subdir( '.git' )->subdir( 'info' )->file( 'sparse-checkout' );
152+
open(my $sparse, '>', $dest) or dir("Couldn't write sparse config");
153+
print $sparse $config;
154+
close $sparse;
155+
}
156+
144157
#===================================
145158
sub name { shift->{name} }
146159
sub git_dir { shift->{git_dir} }

0 commit comments

Comments
 (0)