74 git submodule support #485
Conversation
Add support for initializing and updating git submodules during repository cloning.
Added GitCloneSubmodules option to support recursive submodule initialization.
Resolved issues with git submodule handling for better cloning and URL resolution.
Resolved issues with git submodule handling to ensure robust cloning and URL resolution without relying on git binary calls.
*test.go file addendum
Adding tests for Submodules and two missing
|
I have read the CLA Document and I hereby sign the CLA |
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
Thanks for working on this @bjornrobertsson !
I have some comments below, and I think it would also be worthwhile adding an integration test (see: integration/integration_test.go and testutil/gittest/gittest.go).
| { | ||
| name: "absolute", | ||
| parentURL: "https://example.com/org/main.git", | ||
| subURL: "https://github.com/other/repo.git", | ||
| expect: "https://github.com/other/repo.git", | ||
| }, | ||
| { | ||
| name: "relativeSibling", | ||
| parentURL: "https://example.com/org/main.git", | ||
| subURL: "../deps/lib.git", | ||
| expect: "https://example.com/org/deps/lib.git", | ||
| }, | ||
| { | ||
| name: "relativeChild", | ||
| parentURL: "https://example.com/org/main.git", | ||
| subURL: "./extras/tool.git", | ||
| expect: "https://example.com/org/main.git/extras/tool.git", | ||
| }, | ||
| { | ||
| name: "badParent", | ||
| parentURL: "://bad", | ||
| subURL: "./child", | ||
| expectErr: "parse parent URL", | ||
| }, |
There was a problem hiding this comment.
Please also test with the URL schemes from TestSetupRepoAuth:
- SSH with scheme (
ssh://host.tld/repo) - SSH without scheme (
git@host.tld:repo/path) - Git scheme (
git://git@host.tld:repo/path) - Absolute local path (
/path/to/repo) - Relative paths without
./..(path/to/repo)
There was a problem hiding this comment.
Testing SSH and Git revealed negative user experience if ssh:// and git_username were passed. Lead to: #486
Otherwise testing are fine.
The location when extracted leads to the need to work around relative paths, a lot of work and testing led to the current methods as working
git/git.go
Outdated
| } | ||
|
|
||
| // initSubmodules recursively initializes and updates all submodules in the repository. | ||
| func initSubmodules(ctx context.Context, logf func(string, ...any), repo *git.Repository, opts CloneRepoOptions) error { |
There was a problem hiding this comment.
I'm always a bit wary about recursive functions. I'd feel better about this if we had a maximum depth of which to recurse. I'd wager that most repos won't need more than 2 iterations. If ENVBUILDER_GIT_CLONE_SUBMODULES were changed to accept a positive integer instead, this could function as the number of times to recurse.
There was a problem hiding this comment.
Changed the acceptance to 0-10, true, false - and added a Depth testing
| parentSrv = httptest.NewServer(opts.AuthMW(NewServer(fs))) | ||
| } | ||
| return parentSrv, submoduleSrv | ||
| } |
There was a problem hiding this comment.
I'm not 100% sure this needs to be its own function. I think we could probably in-line this into the relevant test for now.
git/git.go
Outdated
| if opts.Submodules { | ||
| err = initSubmodules(ctx, logf, repo, opts) | ||
| if err != nil { | ||
| return true, fmt.Errorf("init submodules: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
I just saw this -- apaprently go-git supports this natively?
https://pkg.go.dev/github.com/go-git/go-git#CloneOptions
https://pkg.go.dev/github.com/go-git/go-git#SubmoduleRescursivity
There was a problem hiding this comment.
This seems like the preferable option to me vs bespoke implementation. 👍🏻
There was a problem hiding this comment.
That function breaks because of the relative path, and reason for handling this manually (it was confounding and took some time).
There was a problem hiding this comment.
Oh, and v6 renames it, fixes the typo but we're not at v6 yet.
There was a problem hiding this comment.
I take it you're referencing this commit? We could fork and backport this fix to v5 until v6 comes out. WDYT?
There was a problem hiding this comment.
There is nothing concrete that the relative path issues are resolved, more they're not. Checks indicate that there are known bugs with:
- HTTPS URLs losing a slash
- Multiple relative paths (../../submodule.git) not working
git/git.go
Outdated
| if opts.Submodules { | ||
| err = initSubmodules(ctx, logf, repo, opts) | ||
| if err != nil { | ||
| return true, fmt.Errorf("init submodules: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems like the preferable option to me vs bespoke implementation. 👍🏻
docs/env-variables.md
Outdated
| | `--git-clone-depth` | `ENVBUILDER_GIT_CLONE_DEPTH` | | The depth to use when cloning the Git repository. | | ||
| | `--git-clone-single-branch` | `ENVBUILDER_GIT_CLONE_SINGLE_BRANCH` | | Clone only a single branch of the Git repository. | | ||
| | `--git-clone-thinpack` | `ENVBUILDER_GIT_CLONE_THINPACK` | `true` | Git clone with thin pack compatibility enabled, ensuring that even when thin pack compatibility is activated,it will not be turned on for the domain dev.zaure.com. | | ||
| | `--git-clone-submodules` | `ENVBUILDER_GIT_CLONE_SUBMODULES` | | Recursively clone Git submodules after cloning the repository. | |
There was a problem hiding this comment.
We may also want to add --git-clone-submodules-depth to limit recursion depth.
There was a problem hiding this comment.
WDYT about --git-clone-submodules-depth=0 as "don't clone submodules (default)"?
There was a problem hiding this comment.
I just add conditional string eval, default, and 0 is 'false' and a number is Depth?
Transitional, it might be nicer if 'true' opens up a sub-menu for Depth in the template, poc:
validation {
regex = "^(true|false|[0-9]|10)$"
error = "Must be 'true', 'false', or a number from 0-10."
}
# and #
"ENVBUILDER_GIT_CLONE_SUBMODULES" : tostring(local.submodule_depth),
- Added SubmoduleDepth custom type that accepts 'true' (depth 10), 'false' (0), or positive integer - initSubmodules now tracks current depth and stops at max depth - Default is 0 (disabled) - submodules are not cloned unless explicitly enabled
3 participants
Add submodule cloning support via native Go implementation
Summary
This PR resolves/mitigates #74
Changes
Since the existing submodule support in
git.godepends on execution of the git binary, this implementation circumvents that limitation by traversing the tree to clone submodules natively in Go.Behavior