| Age | Commit message (Collapse) | Author |
|---|
|
The `short` formatting option for `gotestsum` will usually suppress
panics and their stack traces, making it difficult to understand why a
panicking test failed.
Using `standard-verbose` will display the full trace, but this also
gives us the full output of `go test -v`, removing the readability
improvements that `gotestsum` provides.
To work around this, we use the `--jsonfile` option to write out the
full output of `go test -json`, then search for panics in an
`after_script` section with results in a pre-collapsed section.
To avoid unused log files piling up in local testing, by default the
full job output is written to `/dev/null`, with this setting overriden
in the CI config.
|
|
ci: Hopefully decrease flakiness in `test-with-praefect`
See merge request gitlab-org/gitaly!4583
|
|
streamcache: Unlock waiters after cache keys have been pruned
See merge request gitlab-org/gitaly!4589
|
|
ci: Fix test reports not being uploaded
See merge request gitlab-org/gitaly!4594
|
|
With 1c112bb7c (ci: Move test reports into temporary directory,
2022-01-13), we have moved our test reports into a temporary directory
to prepare for running tests as an unprivileged user. This change broke
our ability to upload these test reports as artifacts though because
GitLab only supports artifacts which are relative to the build root.
Fix this issue by instead writing test reports into a new directory in
our build root that's writeable by the unprivileged test user. While at
it, pull out the test user ID into a separate variable so that it can be
documented better.
|
|
We regularly get CI failures when running with Praefect, which is most
likely caused by an exhaustion of the database's connection pool.
Increase the limit so that we can hopefully get to a more stable state.
Note that we cannot set `PGOPTIONS` here as that causes the Postgres
services to not come up. Instead, we work around this by manually adding
the configuration to the executed command.
|
|
gitaly-lfs-smudge: Refactor code to be more readily extensible
See merge request gitlab-org/gitaly!4587
|
|
Remove the unused `to io.Writer` parameter from `handleSmudge()`. We
alreay return an `io.ReadCloser` that the contents can be read from, so
there is no need to receive a writer.
|
|
The `GetArchive()` RPC can optionally convert LFS pointers to their
actual contents via the gitaly-lfs-smudge filter. The configuration of
this filter happens via a set of environment variables, which we have
replaced with a single structure in this commit series.
Convert `GetArchive()` to use the new `smudge.Config` to inject the
environment.
|
|
Move the configuration-related code of gitaly-lfs-smudge into a separate
package so that we can access and inject it in Gitaly's services.
|
|
The gitaly-lfs-smudge binary accepts its configuration as a set of
environment variables, which is required so that we can pass it through
Git. Using separate environment variables doesn't scale though: there is
no easily accessible documentation about what is accepted and what is
not, and furthermore every new configuration needs another environment
variable.
Refactor our code to accept a single `GITALY_LFS_SMUDGE_CONFIG` variable
that contains everything needed by gitaly-lfs-smudge to operate. The old
environment variables still exist for backwards-compatibility reasons,
but are overridden in case the new environment variable exists.
|
|
The gitaly-lfs-smudge binary accepts a set of environment variables to
configure it. Because of this the configuration of the binary is kind of
hard to extend: for every piece of information that is required, we need
to add a new environment variable. Ultimately, this leads to a design
where we have lots of magic environment variables injected with no clear
indicator about which ones are used and which ones aren't, similar to
the case we had with gitaly-hooks before introducing the hooks payload.
Refactor the code and create a central `Config` struct which holds all
the configuration required. This prepares us for a fix where we can do
the same like we did for the hooks payload where we only inject a single
JSON-encoded environment variable that can then be parsed into this
struct.
|
|
Add a helper function that can extract environment variables by their
key from an array of environment variables.
|
|
We're about to refactor the way the configuration is handled, which also
includes moving the configuration code into a separate package. Split it
out into a separate file for now to prepare for this move.
|
|
We're about to move loading the configuration out of `smudge()` so that
it doesn't need to depend on any global state anymore. Prepare for this
by pulling out a separate `run()` function so that we can continue to
log any errors and exit with an error code in a single location, only.
|
|
The way `smudge()` handles errors is a bit convoluted right now: in many
paths it will both log the error and return it to the caller, where the
caller should then use it as an indicator to decide whether to exit with
an error code or whether it should exit successfully.
Refactor the code so that we only log any such errors in our `main()`
function to simplify this logic and make it more Go-like. Also, now that
logging is mostly done in the `main()` function, move the initialization
of the logging system close to it.
|
|
We're currently verifying that some of our low-level functions log
certain events into the logfile. This makes it hard to refactor the
code, and ultimately we really should only exercise the system under
test instead of also verifying that some other components like logging
work as expected. As another hurdle, this is effectively testing global
state as the loggers are configured globally.
Remove these assertions from the lower-level unit tests to allow for
easier refactoring of the code. Logging is tested already via our new
high-level tests which verify that executing the binary does the right
thing.
|
|
Because gitaly-lfs-smudge is executed by Git instead of by us we can't
directly log to either stdout or stderr. Instead, we're currently forced
to log into a separate file. The location of that file is controlled by
an environment variable: if present we set up logging, if not we skip
over the configuration.
There's a bug here though: we return a `nil` closer in case we skip
configuring the log and don't initialize the logger at all. First, this
leads to a panic because we unconditionally try to close the closer. And
second, even if this worked, because we don't configure the logger we'd
now log to stdout directly. The result is that the logs would end up in
the final blob's contents as seen by Git.
Fix this bug by always initializing the logger. In case no file is
given, we ask it to simply write to `io.Discard`.
|
|
While we do have tests which verify that parts of the gitaly-lfs-smudge
command run as expected, we don't have any which test the final binary.
This makes it hard to test for some specific scenarios and doesn't give
us an easy way to verify that it works as expected.
Add such tests to improve our test coverage.
|
|
Move functions required for the test setup into a separate file
according to our coding style.
|
|
gitaly/config: Add option to ignore gitconfig files
Closes #4242
See merge request gitlab-org/gitaly!4588
|
|
Parallelize CI jobs
Closes #3123
See merge request gitlab-org/gitaly!4571
|
|
The verify step currently depends on the build step as it will fail
if the Go modules are not downloaded prior to the job executing. If
the modules are not present, golang-ci will fail with a timeout as
it exceeds the timeout while downloading the dependencies. This commit
removes the dependency on build and instead downloads the modules
prior to executing golang-ci. In the general case when the cache is
available, this will allow the verify step to execute in parallel with
the build steps.
|
|
gosec-sast is adding some packages that don't seem necessary. This
commit removes them.
|
|
secret_detection unnecessarily disables inheritance from default.
This commit removes the unnecessary config.
|
|
License scanning is adding some packages that don't seem necessary.
This commit removes them.
|
|
gosec-sast job moves the GOPATH and the cached modules outside the
project directory as the scan will otherwise also scan through the
sources of all the dependencies. This leads to the runtime of the
scan growing massively. The job is currently dependent on the cache
existing from the build step as it unconditionally moves the cache
folder, failing if it doesn't exist. This commit prevents the job
from failing if the cache didn't extract the modules, breaking the
dependency on the build jobs and adds the missing documentation for
the hack.
|
|
The CI drops privileges while running tests. This prevents creating
the required directories to store the test coverage files, thus the
test:coverage job doesn't work without the cache that already
contains the required directories.
To allow the test:coverage job to also run in parallel with the build
steps, this commit overrides the output directory of the coverage
files in the test jobs so they can be created even when the privileges
are dropped, similarly to what is being done with the test result
output with TEST_REPORT variable.
|
|
COVERAGE_DIR in our Makefile controls where the test coverage reports
are outputted. The other variables that control test artifact output
directories begin with TEST_ and are grouped together. This commit
renames COVERAGE_DIR to TEST_COVERAGE_DIR to align it with all the
other variable names. Additionally, it defines the variable with ?=
so the value is only set if it is not already defined. This allows
for the variable to be configured via environment variables.
|
|
This commit adds a regexp to parse the total coverage from the
test:coverage job so GitLab can show it gets shown in the UI and
included in the coverage history of the project.
|
|
Some of the jobs unnecessarily declare that they don't use a cache.
Jobs don't have caches by default, so let's remove the redundant
declarations.
|
|
The testing stage doesn't actually depend on any of the build stage's
artifacts. It only uses the cache which the build stage writes. The
caches are only expired if the relevant files changes like go.sum or
the Gemfile.lock. These caches merely speed up the testing jobs when
they are present. As such, we can remove the build dependency from the
test jobs. Most of the time, the files that make up the cache key are
not changed and thus the both the build step and the test step end up
using the cache. This speeds up the general case by running the steps
in parallel and making test failures visible earlier.
In the rarer case where the cache keys are changed, the test steps will
end up performing some extra work that the cache avoids. This seems like
a fair trade-off to speed up the general case.
There are three exceptions to this though:
'test:coverage' fails without the cache in place as it fails with permission
errors creating the directory for the test coverage report.
'verify' fails with a timeout if the cache is not in place.
'gosec_sast' depends on some files from the cache being moved into the right
place.
As the above jobs fail without the cache, the dependencies are still left
in place for them.
|
|
license_scanning, gemnasium-dependency_scanning and secret_detection
don't use the caches from the earlier build stage, yet they depend
currently on the build stage. Remove the unnecessary dependencies so
they can be executed earlier.
|
|
The QA jobs in our pipeline do not depend on any of the other stages.
The QA stage has been the last one which has thus delayed the execution
of jobs in that stage. Since we are now modeling the dependencies with
the 'needs', this commit removes the unneeded dependency.
|
|
GitLab CI supports a feature that allows for explicitly marking job
dependencies. This allows for visualizing the job dependency graph
and running CI jobs in parallel that do not depend on each other.
This commit adds the needs key where needed to denote job dependencies.
For now, we just model the existing dependency graph by having each stage
depend on the earlier. Later on, we can remove some of the unnecessary
dependencies to speed up the pipeline execution.
|
|
Make Git build with SHA1 and SHA256 routines in FIPS mode
Closes #4210
See merge request gitlab-org/gitaly!4570
|
|
When running our tests, we want Git to use a well-known configuration.
It is thus mandatory that it never picks up the user's gitconfig files,
or otherwise tests may fail due to misconfiguration. To fix this, we
have added a hack that checks whether the currently executing binary has
a `.test` suffix: if it does we assume to be running our tests, and thus
inject a set of environment variable to ignore the gitconfig files.
We now have a better alternative with the newly introduced configuration
to ignore gitconfig files. So let's remove the hack and set this entry
as required. This also proves that the configuration works as expected.
|
|
Fix a cyclic dependency in the gittest package's commit tests so that we
can use the `setup()` function. This change prepares us to drop the test
hack where we inject `GIT_CONFIG_SYSTEM` et al. based on whether the
suffix of the current binary is `.test`.
|
|
We're deprecating use of gitconfig files which exist in the filesystem
in favor of adding `[[git.config]]` entries to Gitaly's `config.toml`.
Add a new option that allows distributions to opt-in to this new
behaviour so that they can migrate earlier if they decide to. This will
become the default with release 16.0.
Changelog: added
|
|
We're in the process of making Gitaly gitconfig-clean: if it is told to
ignore gitconfig files, neither Git nor libgit2 should ever read any
gitconfig file except for the repository-scoped gitconfig.
Add known-broken gitconfigs to our intercepted home directory. Both Git
and libgit2 will fail to parse these files and thus return an error,
which means that we can easily detect if the gitconfig is read by
accident via our testing infrastructure.
|
|
Previous to a40ff540f (Remove HOME overriding hack from tests,
2021-12-15), we were intercepting the home directory of users so that
we didn't pick up their gitconfig. This was required so that none of our
tests change behaviour based on what the user has configured. With that
commit though we had to remove this logic though because some users
require the home directory to be set up correctly to get some auxiliary
tools like asdf work correctly.
We're currently in the process of trying to get Gitaly gitconfig-clean
though, where we want to ensure that Gitaly never picks up any gitconfig
at all if told to do so. And naturally, we want to verify that this is
the case automatically via tests.
Reintroduce intercepting the home directory. In order to not break any
user's workflow, this setup is now opt-in, where our CI will enable it
by default. This allows us to put known-broken gitconfig files into the
home directory that would cause Git invocations to fail.
|
|
When creating a new cache entry the writer is running in a Goroutine to
populate the cache file. After it has ran we then unlock any waiters
which have been waiting for the writer to finish. If the writer failed,
we also remove its respective cache key from the cache so that it's not
used by anything else anymore.
The current order is that we first unblock waiters and then delete the
key. This is creating a race in our tests when checking whether we try
to reuse the failed cache entry because the cache key still exists for a
brief period after the original failing writer has exited.
Fix this flake by only unblocking waiters after the cache key has been
removed. This also feels like the right thing to do outside of this
flaky test: it's inconsistent to treat the write as finished but failed,
but to still have the cache entry around at that point in time.
Changelog: fixed
|
|
gitaly: Become gitconfig-clean
See merge request gitlab-org/gitaly!4578
|
|
Changelog: added
Signed-off-by: Balasankar "Balu" C <balasankar@gitlab.com>
|
|
While we make sure to override `HOME` in our Ruby specs so that Git
wouldn't pick up the gitconfig, libgit2 in fact parses the passwd file
to obtain the user's home directory. Consequentially, overriding the
environment variable is not sufficient to keep libgit2 from reading the
user's gitconfig files.
Fix this issue by overriding Rugged's gitconfig search paths.
|
|
We're using Git2go in our tests for `gitaly-git2go` to access repos as
part of the test itself. And while we do already tell Git2go to ignore
any gitconfig files as part of `gitaly-git2go`'s main function, we don't
in our tests themselves.
Fix this by configuring the gitconfig search path in our tests.
|
|
The `gitaly-git2go` tests are using a Git2go-based test helper to write
commits. That test helper is duplicating functionality we already have
in `gittest.WriteCommit()`, and furthermore it is not configuring Git2go
to not use gitconfig found in the system.
Convert tests to instead use `gittest.WriteCommit()`. Note that commit
IDs are now different because the author and committer is different now.
|
|
libgit2 is by default reading Git configuration files from their
standard locations. This is nothing we want though: the configuration
should either be explicitly set by us, or not at all.
Fix this by explicitly overriding the gitconfig locations in both Git2go
and Rugged. There is only a single Git configuration that we care about,
which is `core.fsyncObjectfiles`: it must always be set to `true` or we
may cause repository corruption. We already do enable it in Git2go via
`git.EnableFsyncGitDir(true)`, and in Rugged we inject a configuration
that contains this key. So ultimately, this change shouldn't change the
behaviour of libgit2 in any way.
|
|
The git2go executor is not injecting Git environment variables when
spawning `gitaly-git2go`. This is a strict requirement though because
we're sometimes spawning git-apply(1) from it, and Git commands must
have the environment variables of the Git execution environment set up.
Fix this by injecting the environment variables.
|
|
Our command factory is injecting a set of environment variables that
override the location of Git's configuration files when running tests so
that we are running in a well-defined environment that doesn't depend on
what the user has configured. This workaround doesn't cover the XDG
config though, which is located in `$XDG_CONFIG_HOME/git/config`, where
`$XDG_CONFIG_HOME` is typically `$HOME/.config`.
Fix this oversight by also overriding `XDG_CONFIG_HOME`, as documented
in git(1). Unfortunately, there isn't a more direct way to override
this. On the other hand it doesn't matter much: previously, we always
filtered out this environment variable in case it was set, so there
cannot be any users that depend on it being set.
|
