Support running docs builds against worktrees #2130
Conversation
The Elasticsearch convention is to use git worktrees to support multiple lines of development across different branches. A typical layout would be: - A regular checkout at `$GIT_HOME/elasticsearch` for the `master` branch - A worktree at `$GIT_HOME/elasticsearch-7.x` for the `7.x` branch - A worktree at `$GIT_HOME/elasticsearch-6.x` for the `6.x` branch This commit changes the docs build to support this structure.
tvernum
added
enhancement
|
It's been more than 10 years since I was a perl engineer, so my perl code probably looks more like Java than idiomatic perl. There's no tests, because I really wanted to check whether this was useful or not before I put any more time into it. I originally intended to just detect the worktree style, and fail the build quickly so that ES engineers don't spend time trying to debug it themselves, but then it looked like it might be possible to fix and I went down that rabbit hole. I don't know what other development practices other projects use, so it's possible this breaks something for another team. I'm happy to iterate on it if needed - or just hand it over if you'd like to run with it yourself. |
|
Related: #2039 |
There was a problem hiding this comment.
Thanks for taking the time to look at this, @tvernum. I think this is valuable and worth including; I just had a couple questions about the implementation. I'm not a Perl expert, nor a frequent user of Git worktrees. I don't think it's likely to break anyone's workflows either.
We do have an integration test suite that could probably be expanded to include this as well.
Is support for worktrees only needed for the --resource argument, or also for --sub_dir?
| } else { | ||
| $d = $p; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we get rid of the unless $toplevel below, and then just return 0?
| my $p = $d->parent; | ||
| if ($p eq $d) { | ||
| # Root of this filesystem | ||
| $d = undef; |
There was a problem hiding this comment.
Can we just break here? Or print an error and return immediately? Is there any other way $d could become undefined in this loop?
| @@ -169,7 +169,13 @@ def run_build_docs(args): | |||
| extra_name = repo_name.replace('x-pack-', '') | |||
| repo_mount = '/doc/%s-extra/%s' % (extra_name, repo_name) | |||
| else: | |||
| repo_mount = '/doc/' + repo_name | |||
| match = re.search(r'(.*)-[0-9]+\.[0-9x]+$', repo_name) | |||
There was a problem hiding this comment.
This seems like a pretty safe way to detect "repo names" that include a -<version branch> suffix, and is unlikely to inadvertently catch a non-worktree repo name; the only repo that contains a digit right now is cloud-on-k8s.
Is this pattern for worktree names (elasticsearch, elasticsearch-7.x, elasticsearch-6.x) pretty universal and documented anywhere? I wouldn't want to have a partial solution that only works for this specific setup if it's not universal; if I understand correctly, any other worktrees that don't match this pattern would still fail, right?
Would it be possible to parse the .git file (which on my machine seems to contain something like gitdir: /absolute/path/to/elasticsearch/.git/worktrees/elasticsearch-7.x and find the repo name by looking at the path component before .git?
There was a problem hiding this comment.
I don't think this would match the pattern I am using for my work trees. I am not aware of any conventions/documentation for how to create these (I have a tiny bash script for personal use)
~/workspace/elasticsearch $ ls
7.0 7.1 7.10 7.11 7.12 7.13 7.14 7.15 7.16 7.17 7.2 7.3 7.4 7.5 7.6 7.7 7.8 7.9 8.0 8.1 8.2 main
There was a problem hiding this comment.
This is the format that we recommend in the ES development docs
$ git fetch origin 7.x
$ git checkout -b 7.x origin/7.x && git checkout master
$ git worktree add ../elasticsearch-7.x 7.x
I'll try and pick this PR up again and see if I can make it agnostic to the worktree name.
There was a problem hiding this comment.
Ahh.. i had missed those. Not a big fan of that layout and the instructions needed updating anyway ... so I am taking a swing at changing the recommendation : https://github.com/elastic/elasticsearch-team/pull/882
|
@tvernum I haven't looked into why the tests are failing. Are you able to do that? |
|
Yes, now that we've branched for 7.15 I should have some cycles to look at this again. |
The Elasticsearch convention is to use git worktrees to support
multiple lines of development across different branches.
A typical layout would be:
$GIT_HOME/elasticsearchfor themasterbranch$GIT_HOME/elasticsearch-7.xfor the7.xbranch$GIT_HOME/elasticsearch-6.xfor the6.xbranchThis commit changes the docs build to support this structure.