| Age | Commit message (Collapse) | Author |
|---|
|
Signed-off-by: Balasankar "Balu" C <balasankar@gitlab.com>
|
|
gittest: Convert most tests that use worktrees
See merge request gitlab-org/gitaly!4666
|
|
Adjust infrastructure to use Go v1.17
See merge request gitlab-org/gitaly!4684
|
|
Disallow use of `os.Setenv()` and `os.Unsetenv()` in tests. Callers
should instead use `testhelper.ModifyEnvironment()`. Adjust existing
callers to do so. Note: the log tests cannot use the testhelper package
due to a cyclic import and thus use `t.Setenv()` directly.
|
|
When using `os.Setenv()`, we change the environment variable for the
full process. While not surprising, this means that we must avoid this
in parallel tests given that the environment variable would also be set
for any other, currently running tests.
With Go v1.17 a new `t.Setenv()` helper function has been introduced
that detects this case and that instead causes us to panic. Convert
`testhelper.ModifyEnvironment()` to use that function and fix a test
that is using `t.Parallel()`.
|
|
We have recently started to require Go 1.17 and later. Interestingly
enough, our CI has not been updated to use that new version yet. Because
of whatever reason this seems to be fine, and Go doesn't complain about
it when running with v1.16.
Let's fix this regardless and start testing with Go v1.17 and v1.18,
only.
|
|
The tags generated for GitLab build images have changed a bit due to an
upstream refactoring of how these images are built [1]. Migrate to the
new format.
[1]: https://gitlab.com/gitlab-org/gitlab-build-images/-/merge_requests/460
|
|
go: Update module gitlab.com/gitlab-org/labkit to v1.15.0
See merge request gitlab-org/gitaly!4682
|
|
go: Update module github.com/prometheus/client_golang to v1.12.2
See merge request gitlab-org/gitaly!4680
|
|
git: Add a gitversion label to command related metrics
See merge request gitlab-org/gitaly!4460
|
|
featureflags: Require version and rollout issue URL for flags
Closes #4312
See merge request gitlab-org/gitaly!4676
|
|
[ci skip]
|
|
go: Update module github.com/uber/jaeger-client-go to v2.30.0
See merge request gitlab-org/gitaly!4681
|
|
We have bumped the minimum required Go version to v1.17 recently, but
didn't update our asdf tool versions. Let's update them to the latest
available Go v1.17 and v1.18 releases.
|
|
We sometimes want to know the git version of the spawned command
Add gitversion label to metrics to increase the visibility of
git version.
Changelog: added
|
|
|
|
|
|
|
|
go: Update module github.com/kelseyhightower/envconfig to v1.4.0
See merge request gitlab-org/gitaly!4649
|
|
When creating a new feature flag it is mandatory to create a feature
flag rollout issue that keeps track of the current status of any flag.
This issue is used both to not forget about rolling out a flag, and also
for the sake of documenting whole rollout process. We have nothing in
our process though that enforces creation of any such issue, and thus it
happens from time to time that we forget to create it.
Add two new parameters to `NewFeatureFlag()` that document the version a
flag has been introduced in and the URL of the issue that's tracking the
rollout. This brings us several benefits:
- It's hard to forget about creating the issue given that you now
have to specify the link when creating the flag.
- Reviewers immediately spot a missing link and can verify that it's
referring to the correct flag. Furthermore, they may review the
rollout issue itself to see whether it documents important
details.
- When checking up on a feature flag after it has been merged it
becomes trivial to find the rollout issue as a developer.
Furthermore, this commit also adds a new version parameter to keep track
of the version a flag has been introduced in. This also makes it easier
for reviewers to see whether a flag can really already be removed or
not.
Adjust all existing feature flag definitions to have this information.
|
|
Add FullPath RPC
See merge request gitlab-org/gitaly!4642
|
|
FullPath reads the path from the repository's gitconfig under the key
"gitlab.fullpath".
|
|
FullPath will allow fetching the value previously set by the SetFullPath
RPC. FullPath assists admins to determine which repo on disk is
associated with which repository on gitlab.
Changelog: added
|
|
|
|
build: drop support for Go 1.16
See merge request gitlab-org/gitaly!4522
|
|
|
|
|
|
[ci skip]
|
|
[ci skip]
|
|
[ci skip]
|
|
'master'
command: Export environment variable to force-enable all feature flags
Closes #4318
See merge request gitlab-org/gitaly!4672
|
|
repository: Fix clone credentials leaking via command line arguments
See merge request gitlab-org/gitaly!4668
|
|
In Rails we're setting an environment variable that force-enables all
feature flags in Gitaly so that we can ensure that newly introduced code
hidden behind any flag is still compatible with the old way of doing
things in Rails. With a recent change though we have started to export
feature flags to the `gitaly-git2go` child process so that it knows
which flags have explicitly been enabled and/or disabled. In combination
both these features result to inconsistent state: we don't export the
environment variable to the child process, and neither do we inject the
force-enabled feature flags. This means that we'll now see all feature
flags as enabled in Gitaly, while we see the default values in the child
process.
Fix this by exporting the environment variable to child processes. It
should only ever be set for tests, so that should be a fine thing to do.
And by exporting it we now have the same and thus consistent view of
overridden feature flags.
Changelog: fixed
|
|
Keep context cancel suppression logic consistent
See merge request gitlab-org/gitaly!4656
|
|
operations: Honor feature flag for structured errors in UserCherryPick
See merge request gitlab-org/gitaly!4669
|
|
The gitaly-git2go command knows to return two different styles of
conflict errors depending on whether the `CherryPickStructuderErrors`
feature flag is set or not. This is done to help zero-downtime upgrades:
we can't yet return the new-style error when the old Gitaly server is
still running.
Because the server controls the binary it shouldn't ever happen that the
binary doesn't see the feature flag while the server does. It's thus
pointless to handle the old-style `HasConflictsError` on the server side
if the flag is enabled: this rather indicates an unexpected programming
error, and we should treat it as such.
Remove special handling of this error condition to clarify this.
|
|
With b253d7c82 (operations: Convert UserCherryPick to return structured
errors, 2022-06-16), we have converted the UserCherryPick RPC to return
structured errors instead of errors that are embedded into a successful
response. While some of the new errors are covered by a feature flag,
this flag currently really only covers the case where the RPC ran into a
merge conflict. All the other newly introduced structured errors don't
honor to the feature flag at all and are returned unconditionally.
This breaks backwards compatibility during zero-downtime upgrades where
Rails may not yet be prepared to handle our new structured errors, and
thus it may start to misbehave when running an old version with a new
Gitaly server. So let's fix this by properly paying attention to the
feature flag so that we continue to return the old-style errors embedded
inside of the successful response in case the flag is disabled.
Changelog: fixed
|
|
On Linux systems, users can by default observe all command line
arguments of processes that may not even be owned by the same user by
inspecting `/proc/${PID}/cmdline`. This means that we mustn't ever put
any credentials in command line arguments given that unprivileged users
can easily sniff them out. Instead, we typically use the environment to
place credentials, which by default is not readable by any other users.
While we do this in most cases, we don't in `CreateRepositoryFromURL()`.
This RPC uses `cloneFromURLCommand()`, which seemingly knows that it's
bad to put credentials on the command line and thus strips them from the
URL already. But afterwards it goes out of its way to put them on the
command line anyway by creating a set of `http.extraHeader` config
options that are passed to git-clone(1) via the `-c` switch.
Fix this by using `git.WithConfigEnv()` instead of `git.WithConfig()`,
which puts the credentials into the environment instead of the command
line.
Changelog: fixed
|
|
Simplify the code flow of `cloneFromURLCommand()` a bit so that things
that are related are located in direct succession. Also, rename the
`pwd` variable to make it harder to confuse with the parent working
directory.
|
|
The test to verify that `cloneFromURLCommand()` behaves as expected is
spawning a git-clone(1) command, but doesn't make sure that this command
terminates as expected. This has resulted in failed pipelines due to the
process leak checker detecting this escaped process.
Fix this flake by killing the process by cancelling the context and then
waiting for the process to return.
|
|
When cloning a repository via `cloneFromURLCommand()`, we strip
credentials from the URL in order to not leak them. The test we have
that verifies that the credentials are not part of the resulting
command's arguments is wrong though: it checks that the array of args
doesn't have the complete user information, but it only checks for an
exact match. What we want to check instead is that none of the args
carries the credentials.
Fix the check by looping through all arguments and verifying that none
of them contain the credentials. Also, rename the username from the
rather generic `"user"` to something that is less likely to clash with
any host information like e.g. paths.
|
|
ci: Fix FIPS jobs failing in forks of Gitaly
See merge request gitlab-org/gitaly!4665
|
|
We have two test cases for `cloneFromURLCommand()` that roughly do the
same thing, except that the way they set up credentials is different.
Merge them into a single, table-driven test.
|
|
Convert the tests for UserRebaseConfirmable to not use a worktree
anymore.
|
|
Remove the handcrafted version of `gittest.ResolveRevision()` in favor
of this new helper function. While at it, drop mentions of `SHA`: this
is an implementation detail, and for all it's worth we should just call
this an "object ID".
|
|
Adjust names of tests for the UserRebaseConfirmable RPC to match our
best practices.
|
|
Convert the test that demonstrates squashing with commits that contain a
rename and modification of the renamed file to not use a worktree.
|
|
Fix the test for squashing commits with renames to indeed squash the
correct commit range so that the rename is actually visible.
|
|
One of our tests tries to verify that we can squash a commit when there
was a rename on the target branch. This test is silently broken though
because of the way we set up the commits: while we do rename the changed
file, the commit containing that change is not used to compute the
squash.
Demonstrate this brokenness by asserting the tree's contents. As can be
seen, the resulting tree has the changed content but is still using the
original filename.
|
|
One of our `FindPage()` tests uses a worktree so that it can amend the
latest commit creating a Wiki page to have author information including
UTF8-encoded characters. Convert the test to not use a worktree by
amending the `createWikiPageOpts` to allow for specifying an author.
|
