| Age | Commit message (Collapse) | Author |
|---|
|
On some machines we see that the PostUploadPack tests are flaky because
the responses we receive sometimes contain `0005\01` prefixes. As it
turns out, these are keep-alive packets that Git starts to send over the
sideband every 5 seconds after it has received the full packfile from
the client. So these are legit and can totally be expected on machines
that are sufficiently slow.
Implement a new helper function `requireSideband()` that parses the
response into separate sideband lines while stripping out any keep-alive
packets to fix this flakiness.
|
|
Group together test helper functions at the bottom of our
PostReceivePack tests to make them easier to find.
|
|
Adjust test names for PostReceivePack tests to match our best practices.
|
|
This is a set of random small refactors for the PostReceivePack tests to
align them better with our current best practices with regards to tests.
|
|
The tests which exercise our request validation for PostReceivePack only
assert error codes. This is quite fragile though as it could easily be
that the error code matches what we'd expect, while the actual error is
still caused by something different.
Refactor the test to instead verify that the actual error matches.
|
|
Rename `doPush()` to `performPush()` to clarify the intent of this
helper function. While at it, change its return type from `[]byte` to
`string`: all callers cast the result to a string anyway.
|
|
The `newTestPush()` helper is not really matching our current best
practices with regards to how we write tests. It's hard to understand
what this function does by just taking a look at its name.
Refactor the code to better match our best practices and rename the
helper to `createPushRequest()`.
|
|
Convert tests to use gittest helpers to write pktlines instead of using
our own, hand-coded implementation.
|
|
We're currently creating a worktree in some of smarthttp's tests in
order to be able to write commits. Convert `newTestPush()` to instead
use `gittest.WriteCommit()`, which doesn't require a worktree.
|
|
The `ExecOpts()` function accepts a configuration that allows the caller
to set various options like the standard streams. As it turns out though
setting `Stdout` in that configuration is broken and will cause us to
fail immediately. The root cause is that we call `exec.Cmd.Output()`,
which is not allowed when you set the command's `Stdout` variable.
Fix this by calling `Run()` instead when `Stdout` is passed by the
caller. Add tests to catch any future regressions.
|
|
Add function to write formatted pktlines to a writer.
|
|
go: Update golang.org/x/sys digest to 4f61da8
See merge request gitlab-org/gitaly!4636
|
|
go: Update module github.com/google/go-cmp to v0.5.8
See merge request gitlab-org/gitaly!4639
|
|
go: Update module go.uber.org/goleak to v1.1.12
See merge request gitlab-org/gitaly!4641
|
|
go: Update module github.com/stretchr/testify to v1.7.2
See merge request gitlab-org/gitaly!4640
|
|
git2go: Pass feature flags into gitaly-git2go binary
See merge request gitlab-org/gitaly!4601
|
|
featureflag: Remove concurrency_queue_enforce_max feature flag
See merge request gitlab-org/gitaly!4620
|
|
Currently when we call gitaly-git2go, we lose feature flags since they
are in the context inside the request that comes from Rails. In order to
allow gitaly-git2go to benefit from feature flags, we can pass it in
through an environment variable much like we do for gitaly-hooks.
In order to test this, add a new subcommand for gitaly-git2go that
simply returns which feature flags are set in the context. Since we only
gets built for tests, build this in only with the `test` build tag.
Changelog: changed
|
|
|
|
|
|
|
|
[ci skip]
|
|
ssh: Refactor SSHUploadPack tests to match modern best practices
See merge request gitlab-org/gitaly!4614
|
|
|
|
FindChangedPaths: Add requests field
See merge request gitlab-org/gitaly!4597
|
|
Add glId, glUser, glRepo, glProtocol to PackObjectsRequest
See merge request gitlab-org/gitaly!4599
|
|
Now that we have access to the user_id, username, and protocol, we can
pass these values into the request to PackObjectsHookWithSidechannel
|
|
Add user_id, username, protocol to the payload we send to the hook so
that it can call the PackObjectsHookWithSidechannel with these values.
|
|
The ReceiveHooksPayload contains information like the UserID, Username,
Protocol that would also be useful for UploadPack. Rename this to
UserDetails to be more generic, so that we can share this in both places.
However, in order to keep upgrades compatible, keep the
ReceiveHooksPayload and the receive_hooks_payload json tag. During an
upgrade, the gitaly-hooks binary will be replaced before the gitaly
binary, so there will be a short period of time when an old gitaly will
be sending to a newer gitaly-hooks. For this reason, we want to allow
the newer gitaly-hooks to still recognize a receive_hooks_payload key.
|
|
praefect: Add list-storages subcommand
Closes #3239
See merge request gitlab-org/gitaly!4609
|
|
go: Update golang.org/x/sys digest to 6c1b26c
See merge request gitlab-org/gitaly!4628
|
|
|
|
go: Update golang.org/x/time digest to 579cf78
See merge request gitlab-org/gitaly!4629
|
|
|
|
ci: Fix Danger job being broken because of duplicate Gemfiles
See merge request gitlab-org/gitaly!4633
|
|
'master'
operations: Return structured errors on conflict in UserMergeBranch
Closes #4066
See merge request gitlab-org/gitaly!4623
|
|
Recently our Danger job has started failing with the following error
message:
```
[!] There was an error parsing `injected gems`: You cannot specify the same gem twice with different version requirements.
You specified: gitlab-dangerfiles (~> 3.1.0) and gitlab-dangerfiles (>= 0). Gem already added. Bundler cannot continue.
```
As it turns out, this breakage is caused by the upstream change 9e3eff4
(Don't require projects to contain Gemfile when using danger-review job,
2022-06-10) in the common pipeline configurations: if the CI job detects
that there is no Gemfile, it has now started to write one for us that
contains the `gitlab-dangerfiles` version. And because our Gemfile is
not contained in the root directory but instead in the `danger/` sub
directory we now end up with two Gemfiles.
Fix this issue by making use of the new feature: instead of carrying our
own Gemfile, we can now simply rely on the auto-generated Gemfile. While
this forces us to get rid of the Ruby cache which was keyed by our own
Gemfile, this is really not an issue at all: execution of the whole job
only takes about 40 seconds without the cache, which is fast enough.
|
|
While we already handle some of the errors in UserMergeBranch specially
via structured errors, we don't yet do so for merge conflicts. Amend
this error handling so that callers can know that the error is indeed
caused by a merge conflict, and so that they can find our exactly which
files have been conflicting.
Note that this is not done behind a feature flag: there aren't any
callers which parse the error, and neither should there be any. So given
that we retain the same error code as before but only amend the error
message and add some error details this should not be a breaking change.
Changelog: added
|
|
In UserMergeBranch we're verifying whether the error message returned by
`gitaly-git2go` contains a specific string to be able to discern the
case where the merge failed because of a merge conflict. This check is
not required anymore though given that we now always get a specific
`ConflictingFilesError{}` in that case.
Remove the check to clean up the logic.
|
|
The structured `MergeConflictError` provides information about the files
that have been conflicting, but it doesn't provide any information about
the revisions that caused the merge conflict. While that info should in
the general case already be available on the calling-side, it may not
necessarily be in case the caller e.g. only specified branch names
instead of commit IDs.
Add a new `ConflictingCommitIDs` field to the message that contains the
object IDs of the conflicting commits to provide additional context.
|
|
chore(deps): update golang.org/x/sync digest to 0de741c
See merge request gitlab-org/gitaly!4627
|
|
Adjust names of SSHUploadPack tests to match modern best practices.
|
|
One of our tests that ought to verify whether cloning fails when we pass
an invalid repository only checked that an error was returned, but it
never checked what kind of error was returned. And sure enough, nowadays
we don't fail because of an invalid request, but because the test setup
is incomplete and missing the `gitaly-ssh` binary.
Fix the test setup to build this binary and verify that the error
matches what we expect so we don't regress again in the future.
|
|
There is no reason for us to set the `PATH` environment variable
explicitly in our tests, so let's stop doing that.
|
|
The tests which exercise whether we can pass Git configuration to
SSHUploadPack with hidden tags as a proxy use the same setup as our
other tests which verify that this RPC works alright. Merge both tests
into a single one to reduce code duplication.
|
|
The tests we have which exercise whether we can indeed use Git protocol
v2 as expected use the same setup as our other tests which exercise
successful invocations of SSHUploadPack, with the only exception being
that they don't use a protocol-detecting command factory.
Let's merge these tests, which also increases our test coverage for the
Git protocol to now also verify with sidechannels and the pack-objects
cache as well as without protocol v2.
|
|
The tests which exercise our partial clone filters for SSHUploadPack
have the exact same setup as our tests which verify that we can do other
successful requests, as well. Let's merge both tests into one to avoid
some code duplication.
|
|
Our testcase that exercises SSHUploadPack in the context of Git failures
is using quite dated code patterns. Modernize it to match our current
best practices.
While at it, refactor `testPostUploadPackFailedResponse()`: it's not at
all obvious what this function does just by looking at its name, and the
looping logic is a tad weird, too. This is both fixed by updating the
code style and renaming it to `recvUntilError()`, which should hopefully
be easier to understand.
|
|
The tests verifying that request validation works as expected for
SSHUploadPack are currently only asserting the error code. This is
fragile and may easily lead to failures that we assume to be what we
expected, but which are actually caused by something different.
Refactor the test to instead verify the full error to tighten it.
|
|
Because SSHUploadPack can either be invoked with or without sidechannel
and with and without the pack-objects cache we need to re-run some tests
multiple times with all combinations. This is solved via separate tests
which call into the same function, but in a rather weird way where
`TestFn()` calls `testFn()`, which then again calls `testFn2()`. This is
mighty confusing.
Refactor the test to instead be table-driven to simplify it.
|
