feat: add git clone with thinpack option to support self hosted azure devops #446
Conversation
CH-Chang
changed the title
CH-Chang
force-pushed
the
dev/chchang/thinkpack
branch
from
590d4a7 to
2a22162
Compare
git/git.go
Outdated
| @@ -53,7 +54,7 @@ func CloneRepo(ctx context.Context, logf func(string, ...any), opts CloneRepoOpt | |||
| return false, fmt.Errorf("parse url %q: %w", opts.RepoURL, err) | |||
| } | |||
| logf("Parsed Git URL as %q", parsed.Redacted()) | |||
| if parsed.Hostname() == "dev.azure.com" { | |||
| if parsed.Hostname() == "dev.azure.com" || opts.ThinPack { | |||
There was a problem hiding this comment.
Thanks for this! Would you mind updating the logf message below? It still states that this is for azure specifically, which could be misleading if ThinPack were set in non Azure environments.
There was a problem hiding this comment.
I think the semantics of the option as currently defined are also somewhat misleading. Setting ENVBUILDER_GIT_CLONE_THINPACK=true will add thinpack to the unsupported capabilities of the Git transport when attempting to clone. Shouldn't it be the other way round?
There was a problem hiding this comment.
Thanks for this! Would you mind updating the logf message below? It still states that this is for azure specifically, which could be misleading if ThinPack were set in non Azure environments.
I've updated the log message to differentiate between disabling the feature via the command line and disabling it due to encountering the corresponding domain.
I think the semantics of the option as currently defined are also somewhat misleading. Setting
ENVBUILDER_GIT_CLONE_THINPACK=truewill add thinpack to the unsupported capabilities of the Git transport when attempting to clone. Shouldn't it be the other way round?
I've adjusted the definition of the option. By default, the option is set to true, which enables Git clone thin pack compatibility.
git/git.go
Outdated
| @@ -53,7 +54,7 @@ func CloneRepo(ctx context.Context, logf func(string, ...any), opts CloneRepoOpt | |||
| return false, fmt.Errorf("parse url %q: %w", opts.RepoURL, err) | |||
| } | |||
| logf("Parsed Git URL as %q", parsed.Redacted()) | |||
| if parsed.Hostname() == "dev.azure.com" { | |||
| if parsed.Hostname() == "dev.azure.com" || opts.ThinPack { | |||
There was a problem hiding this comment.
I think the semantics of the option as currently defined are also somewhat misleading. Setting ENVBUILDER_GIT_CLONE_THINPACK=true will add thinpack to the unsupported capabilities of the Git transport when attempting to clone. Shouldn't it be the other way round?
docs/env-variables.md
Outdated
| @@ -27,6 +27,7 @@ | |||
| | `--git-url` | `ENVBUILDER_GIT_URL` | | The URL of a Git repository containing a Devcontainer or Docker image to clone. This is optional. | | |||
| | `--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` | | Clone with thin pack compabilities. | | |||
There was a problem hiding this comment.
Can you add a note regarding the behaviour of thinpack here for repos hosted on dev.zaure.com?
There was a problem hiding this comment.
I have added the description regarding the behavior of thinpack for repositories hosted on dev.zaure.com, where the option remains ineffective even when enabled. Thank you!
git/git.go
Outdated
| } else { | ||
| if parsed.Hostname() == "dev.azure.com" { | ||
| // Azure DevOps requires capabilities multi_ack / multi_ack_detailed, | ||
| // which are not fully implemented and by default are included in | ||
| // transport.UnsupportedCapabilities. | ||
| // | ||
| // The initial clone operations require a full download of the repository, | ||
| // and therefore those unsupported capabilities are not as crucial, so | ||
| // by removing them from that list allows for the first clone to work | ||
| // successfully. | ||
| // | ||
| // Additional fetches will yield issues, therefore work always from a clean | ||
| // clone until those capabilities are fully supported. | ||
| // | ||
| // New commits and pushes against a remote worked without any issues. | ||
| // See: https://github.com/go-git/go-git/issues/64 | ||
| // | ||
| // This is knowingly not safe to call in parallel, but it seemed | ||
| // like the least-janky place to add a super janky hack. | ||
| thinPack = false | ||
| logf("Workaround for Azure DevOps: marking thin-pack as unsupported") | ||
| } | ||
| } |
There was a problem hiding this comment.
Thanks, I have updated the code to use else if { ... } and have run make docs/env-variables.md to update the documentation.
|
Thanks @CH-Chang ! |
|
Created coder/terraform-provider-envbuilder#89 to support this in the provider. |
The program uses only the hostname to address the issue of requiring thin pack to perform a git clone operation on Azure DevOps, as shown in the code snippet below.
envbuilder/git/git.go
Lines 56 to 78 in d045c1c
However, since Azure DevOps supports self-hosted servers, the hostname may not necessarily be dev.azure.com. Therefore, it is desired to add a command-line option to enable this functionality.