Libgit2 ED25519 check & clone respect context #445
Conversation
pkg/git/libgit2/checkout.go
Outdated
| select { | ||
| case err := <-errCh: | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to clone '%s', error: %w", url, gitutil.LibGit2Error(err)) |
There was a problem hiding this comment.
Some of the checkouts didn't use LibGit2Error() with the received error. I think that was missed. Now all of the checkout implementations will have this.
pkg/git/libgit2/checkout_test.go
Outdated
| g := NewWithT(t) | ||
|
|
||
| timeout := 5 * time.Second | ||
| url := "ssh://[email protected]/projectcontour/contour" |
There was a problem hiding this comment.
Can the test git server not be used here, rather than relying on the internet? I've used it in the image-automation-controller tests.
There was a problem hiding this comment.
+1 for getting rid of the references to "real" Git repositories, and replacing them with mocks.
There was a problem hiding this comment.
The issues with mocks is that we can't tell if our code works on large repos with a consistent history. If we remove it from here please add it to e2e tests, using nodejs would be best as it has many GB in .git.
There was a problem hiding this comment.
What do you mean by "consistent history" that's lacking in a local server?
There was a problem hiding this comment.
Though yes, integration tests are the place to test "real world" scenarios. But this particular test is for correctness, so using a local server is not just fine, it's preferred, since it helps isolate the property under test.
There was a problem hiding this comment.
I should've said "large history", as in hundreds of tags and thousands of commits.
There was a problem hiding this comment.
That should be handled as a separate test case, most likely with a specific (Go) tag attached to it (e.g. integration), so that it only runs in CI.
There was a problem hiding this comment.
I've refactored the test to use a test git server.
This adds a test to detect any regression in libgit2's ED25519 key support. go-git supports ED25519 but not the current version of libgit2 used in flux. The updates to libgit2 in v1.2.0 adds support for ED25519. This test would help ensure the right version of libgit2 is used. Signed-off-by: Sunny <[email protected]>
darkowlzz
force-pushed
the
libgit2-ed25519-check
branch
from
cc7789e
to
4f29459
Compare
Update libgit2-git2go checkout with a clone wrapper to respect the context passed to it. go-git accepts context passed to it during cloning but the git2go does not accept the context passed to it. This makes the GitRepository.Spec.Timeout ineffective when using libgit2. This change updates all the different libgit2 checkouts to use the new clone function. This is also needed to shorten the wait time for tests that depend on failure behavior and should fail early. TestCheckout_ED25519 test would take 30s without a context with timeout. This helps shorten the time for such tests. Signed-off-by: Sunny <[email protected]>
darkowlzz
force-pushed
the
libgit2-ed25519-check
branch
from
4f29459
to
5130752
Compare
|
The failure in CI is due to a different libgit2 version. The new build changes aren't in |
This adds a test to detect any regression in libgit2's ED25519 key
support. go-git supports ED25519 but not the current version of
libgit2 used in flux. The updates to libgit2 in v1.2.0 adds support
for ED25519. This test would help ensure the right version of libgit2
is used by enforcing test failure.
Refer #399
The test expects the failure for now. But the condition must be flipped
once #437 lands in
reconcilers-devbranch.It also updates libgit2 checkout implementations to respect the context
passed to it during cloning. This is needed to shorten the wait time for
tests that depend on failure behavior and should fail early.
TestCheckout_ED25519test would take 30s without a context withtimeout. This helps shorten the time for such tests.
This also makes the
GitRepository.Spec.Timeoutproperty effectivewhen using libgit2.
Some more details in the commits.
reconcilers-dev.