| Age | Commit message (Collapse) | Author |
|---|
|
We have flaky tests[1] that keep causing false alarms in the CI, so
people have to waste time looking at the same issue and click "retry"
manually.
Let's do that retry automatically. We'll waste less time just having
it retry so you'll potentially wait a bit more for a real failure,
v.s. the false alarms.
This is an interim solution for[1]. We should also fix those tests, a
smarter way to do this would be to quickly annotate known failed
tests, or to have "make test" et al smartly retry just the failing
tests, but just doing this is simpler and better than the status quo.
1. https://gitlab.com/gitlab-org/gitaly/-/issues?label_name[]=failure%3A%3Aflaky-test
|
|
Remove on-by-default go_user_delete_{branch,tag} feature flags
See merge request gitlab-org/gitaly!3033
|
|
Remove calls to the Ruby code for the go_user_delete_{branch,tag}
flags on by default since a96949aef (Enable feature flag by default,
2021-01-12), and on in production since 2021-01-14:
- https://gitlab.com/gitlab-org/gitaly/-/issues/3412#note_485304117
- https://gitlab.com/gitlab-org/gitaly/-/issues/3413#note_485303898
I'm removing less code than I did in fbc9f83ab (Remove Ruby code for
100% on user_delete_{branch,tag} in Go feature, 2021-01-15) per a
comment from Patrick[1]:
We can't remove feature flag and Ruby code at the same time
due to there being a race condition. That race even exists if
the feature flag is default-enabled as customers may have
disabled the flag.
See [2] for past discussion and conflicting information on the
issue. I'll open a MR to the feature flag docs to clear this up.
This brings the runtime of:
time go test -v ./internal/gitaly/service/operations/ -run 'Tag|Branch' -count=1
From ~25 seconds to ~15 second.
1. https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3033#note_492850106
2. https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3010#note_488175228
|
|
Git command factory injected in to objectpool
See merge request gitlab-org/gitaly!3047
|
|
Improve test coverage of line ending normalization
See merge request gitlab-org/gitaly!3026
|
|
repository: Fix regressions in FetchRemote
See merge request gitlab-org/gitaly!3043
|
|
User(Branch|Tag|Merge) tests: fix lint issues
See merge request gitlab-org/gitaly!3046
|
|
In continuation of the removing dependency on the global
config.Config we now replace usages of the git.NewCommand
func with injected factory method designed for the same purpose.
As the calls are deeply nested we are required to make a lot of
changes in other components as well in order to pass the dependency
throw all the layers.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
|
|
|
|
The FetchRemote RPC accepts a NoPrune parameter, which if set should
cause us to not prune remote references. We fail to pay attention to
that parameter though and always invoke git-fetch(1) with the "--prune"
parameter and inject "remote.$REMOTE.prune=true".
Let's fix this bug by passing the NoPrune parameter to git-fetch(1). We
can also get rid of the injected config entry as it's essentially
duplicating the "--prune" parameter.
|
|
in case a RemoteParam as passed to FetchRemote, we intend do use its
MirrorRefmaps to determine how references should be mirrored, as there
is no configured remote which could have a refspec. In case no map is
provided, we should fall back to just doing a mirror-fetch, if one goes
by what the Ruby RPC did back when it still existed. But in fact we
don't set up a refspec at all in that case, which instead causes us to
simply fetch the remote's HEAD reference.
Fix this issue by using "refs/*:refs/*" in case no refspec is given.
|
|
We're currently only injecting the "http.followRedirects" option in case
the FetchRemote RPC was invoked with RemoteParams, but not in case
remote name was passed. This is different to the original Ruby behaviour
and opens us up for unwanted redirects.
Fix this issue by unconditionally passing `http.followRedirects=false`.
|
|
When calling FetchRemote with RemoteParams set, we create an ad-hoc
remote and configure it as requested by the RPC's parameters. One of the
config entries we inject at runtime only is "remote.$REMOTE.mirror",
which shouldn't have any effect at all considering it's only used when
pushing to a remote.
Let's drop this useless entry, it's only confusing.
|
|
The FetchRemoteRequest RPC doesn't have any docmuentation right now.
Let's fix it by documenting both the RPC itself as well as its
parameters.
|
|
Gitaly normalizes line endings by setting `core.autocrlf=input` globally
for every git invocation. This commit improves test coverage around this
in WriteBlob and UserCommitFiles.
|
|
git: Add support for options which always get injected
See merge request gitlab-org/gitaly!3028
|
|
Fix a few lint issues added to the exclusions list in 6e7eea01e (Lint:
turn "errcheck" on by default, 2021-01-19).
|
|
[ci skip]
|
|
operations: Wire up AllowConflicts handling for Go
See merge request gitlab-org/gitaly!2997
|
|
git: Split out LocalRepository implementation
See merge request gitlab-org/gitaly!3048
|
|
Originally, handling of the "AllowConflicts" parameter for the
UserMergeToRef RPC was only implemented in Ruby and not in Go because
efforts to port to Go and to implement this new flag crossed. This has
changed with the previous commit so that we can now switch over to fully
use the Go port instead of the Ruby code in all cases.
Note that this directly removes the Ruby codepath without any kind of
feature flag. This is done because first, the UserMergeToRef RPC has
been tested already and rolled out by default. And second, all callers
in GitLab Rails which make use of "AllowConflicts" are already hidden
behind the "display_merge_conflicts_in_diff" feature flag which is
currently disabled by default. Introducing another feature flag for this
parameter thus doesn't seem necessary.
|
|
While we ported the UserMergeToRef RPC to Go, a new flag
"allow_conflicts" was added to its request. What it does is that instead
of raising an error when there's any conflict, the RPC will now commit
the conflicting file together with its conflict markers.
Given that both efforts kind of crossed, the parameter isn't supported
by the Go implementation nowadays. This commit lays the groundwork to
support it.
|
|
In order to compute the list of conflicts, we have a function which
takes an `IndexConflict` as input and computes a merged file which
contains the conflicts as part of its merged contents, including
conflict markers. While this function is currently only used in a single
location, we're about to extend the UserMergeToRef RPC to also
optionally commit conflict markers, where this function will be useful.
Let's expose it up front in order to make it available outside of its
current module.
|
|
The tests for `FormatTag()` is currently called
`TestLocalRepository_FormatTag()`, even though it's not a receiver
function of the `LocalRepository` structure. Let's rename it to
`TestFormatTag()` instead according to our test name policy.
|
|
The `LocalRepository.HasBranches()` function is missing a comment. Let's
add it to silence the linter.
|
|
The RepositoryConfig type is only implemented for the LocalRepository
type, and it can only ever be used for local repositories in the first
place. Let's make it obvious that this is the case by moving the
RepositoryConfig implementation into a "localrepo" file and renaming it
to LocalRepositoryConfig.
|
|
The RepositoryRemote type is only implemented for the LocalRepository
type, and it can only ever be used for local repositories in the first
place. Let's make it obvious that this is the case by moving the
RepositoryRemote implementation into a "localrepo" file and renaming it
to LocalRepositoryRemote.
|
|
The LocalRepository implementation has grown quite a lot recently, but
is still mostly contained in a single file only. This makes it really
hard to understand, refactor and extend its implementation. This commit
thus moves the abstraction into its own set of files to fix this issue.
|
|
Make gitaly_ruby_json.log work again
See merge request gitlab-org/gitaly!3052
|
|
https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2900 dropped
`GITALY_LOG_DIR` from the environment, causing gitaly-ruby to log
everything to /dev/null.
Relates to
https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/822
|
|
featureflag: Remove GoFetchSourceBranch feature flag
See merge request gitlab-org/gitaly!3050
|
|
Remove server scope from Praefect
Closes #3122
See merge request gitlab-org/gitaly!3015
|
|
hooks: Drop payload fallback code
See merge request gitlab-org/gitaly!2988
|
|
The name of the hooks payload environment variable names' variable is
somehow got named `ErrHooksPayloadNotFound`. It's been called like that
ever since I originally implemented it in 807efa204 (git: Implement a
new hooks payload structure, 2020-12-02), but my mind really must have
been clouded to call it such.
Let's rename it to `EnvHooksPayload` to make it less confusing.
|
|
The GoFetchSourceBranch feature flag has been default-enabled in commit
8443b3333 (featureflag: Enable Go implementation of FetchSourceBranch by
default, 2021-01-13), which was released with v13.8.0. We can thus
remove this feature flag altogether now.
|
|
We're using a set of "real" feature flags to test the feature set
implementation. This has the obvious downside that as each feature flag
is eventually being removed, the test cases need to be adjusted to use a
different feature flag instead.
Let's convert this to instead use a set of dummy feature flags.
|
|
featureflags: Remove GoUserMergeBranch feature flag
See merge request gitlab-org/gitaly!3049
|
|
Back when we were still using standalone environment variables to pass
down both transaction and Praefect server information to gitaly-hooks,
we had to serialize and deserialize both structures into and from
environment variables. We've since converted the code to use a single
serialized `HooksPayload` struct, so in fact we don't need this legacy
code anymore.
Remove `PraefectFromEnv()`, `TransactionFromEnv()` and their `Env()`
functions.
|
|
In release 13.7, we have migrated hooks away from using multiple
environment variables to instead use a single serialized `HooksPayload`
structure. In order to allow zero-downtime upgrades, we had to keep some
fallback code though to still work with the old environment variables
such that an old server continues to work with a new gitaly-hooks
binary.
Now that 13.7 is out, let's drop the fallback code. We only allow for
zero-downtime upgrades between consecutive releases, so this is safe to
do.
|
|
Traditionally, the gitconfig is not managed by the Gitaly project but by
the various projects which deploy Gitaly or by the administrator himself
if he chooses to do a source-based installation. Admins may have to
potentially update the gitconfig on each upgrade of Gitaly while we as
the Gitaly team need to ensure that all projects which deploy Gitaly
(Docker, CNG, Omnibus and the likes) have the same gitconfig. Going by
Murphy's law, this means that the git configuration for ways of
installing Gitaly is going to diverge or has already diverged.
Running in an environment where we don't know that a set of config
entries is set can be dangerous and may lead to incorrect results,
failures or even data loss in the case of "core.fsyncObjectFiles" and
"gc.auto".
This commit thus moves the global configuration into the Gitaly project
to avoid any inconsistent environments. This allows us to ensure that
required options are set while being quicker to iterate in case we need
to change the configuration.
The injection is done via the `git.ConfigPair{}` mechanism, which will
put all config entries on the command line via `git -c <key>=<value>`
pairs. While it has the downside of being verbose, the current set of
entries we inject is quite limited and contains only five different
options, so we shouldn't be near any command line limits. Furthermore,
we're currently in the process of upstreaming a new way of injecting
config entries via the environment, which we can migrate to as soon as
this git version has been released.
The list of injected configuration entries is taken from our source
installation instructions.
|
|
The GoUserMergeBranch feature flag has been default-enabled in eeab751f2
(featureflags: Enable Go implementation of UserMergeBranch by default,
2021-01-06), which has been released with v13.8.0. We can thus remove
the feature flag completely now.
|
|
[ci skip]
|
|
Remove unused Ruby code not used in almost a year
See merge request gitlab-org/gitaly!3045
|
|
transactions: Optionally use timestamps for deterministic results
See merge request gitlab-org/gitaly!3036
|
|
The ResolveConflicts RPC is currently not deterministic with regards to
the created commits because they contain a timestamp, which is by
default the creation time. This breaks transactions, as when multiple
Gitalies now try to process the same request in parallel, the likelihood
is very high that they'll end up with different commit IDs.
Fix the issue by introducing a new optional "timestamp" field to the
request. If set, it will be used instead of the current system time.
This is only the first half of the fix, the second one will require us
to inject a single timestamp in Rails.
|
|
The UserUpdateSubmodule RPC is currently not deterministic with regards to
the created commits because they contain a timestamp, which is by
default the creation time. This breaks transactions, as when multiple
Gitalies now try to process the same request in parallel, the likelihood
is very high that they'll end up with different commit IDs.
Fix the issue by introducing a new optional "timestamp" field to the
request. If set, it will be used instead of the current system time.
This is only the first half of the fix, the second one will require us
to inject a single timestamp in Rails.
|
|
The UserApplyPatch RPC is currently not deterministic with regards to
the created commits because they contain a timestamp, which is by
default the creation time. This breaks transactions, as when multiple
Gitalies now try to process the same request in parallel, the likelihood
is very high that they'll end up with different commit IDs.
Fix the issue by introducing a new optional "timestamp" field to the
request. If set, it will be used instead of the current system time.
This is only the first half of the fix, the second one will require us
to inject a single timestamp in Rails.
|
|
The UserSquash RPC is currently not deterministic with regards to
the created commits because they contain a timestamp, which is by
default the creation time. This breaks transactions, as when multiple
Gitalies now try to process the same request in parallel, the likelihood
is very high that they'll end up with different commit IDs.
Fix the issue by introducing a new optional "timestamp" field to the
request. If set, it will be used instead of the current system time.
This is only the first half of the fix, the second one will require us
to inject a single timestamp in Rails.
|
|
The UserRevert RPC is currently not deterministic with regards to
the created commits because they contain a timestamp, which is by
default the creation time. This breaks transactions, as when multiple
Gitalies now try to process the same request in parallel, the likelihood
is very high that they'll end up with different commit IDs.
Fix the issue by introducing a new optional "timestamp" field to the
request. If set, it will be used instead of the current system time.
This is only the first half of the fix, the second one will require us
to inject a single timestamp in Rails.
|
|
The UserCommitFiles RPC is currently not deterministic with regards to
the created commits because they contain a timestamp, which is by
default the creation time. This breaks transactions, as when multiple
Gitalies now try to process the same request in parallel, the likelihood
is very high that they'll end up with different commit IDs.
Fix the issue by introducing a new optional "timestamp" field to the
request. If set, it will be used instead of the current system time.
This is only the first half of the fix, the second one will require us
to inject a single timestamp in Rails.
|
