| Age | Commit message (Collapse) | Author |
|---|
|
|
|
|
|
|
|
I don't like this for the reasons noted in
https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2857#note_457436954
Let's try to get rid of it, by making this die very noisily. Then I
can look at the CI fail whale at my leisure and fix any issues if that
seems to make sense, as I did for one file in a commit leading up to
this.
The rest of ErrInvalidArgument() etc. helpers just become a plain
wrapper for the equivalent status/code API use, which (if they don't
fail) we can eventually just use those APIs directly instead.
|
|
Change this code added in dca53eabd (SearchFilesBy{Content,Name}
Server Implementation, 2018-05-04). I'm seeing if I can get us rid of
DecorateError magic. In this case we're re-throwing our own errors,
which we can just more explicitly cast here.
|
|
The only consumer of the "Unimplemented" variable is this one
response, let's just move it over there.
|
|
'master'
UserCreateBranch: unify API responses between Go & Ruby response paths
See merge request gitlab-org/gitaly!2857
|
|
style: Document best practices for commit hygiene
See merge request gitlab-org/gitaly!2826
|
|
Change code added in [1] to use GetReference() instead of GetBranch(),
and make it return the same error messages & codes as the Ruby code.
This resolves numerous bugs in [1] which weren't spotted due to
inadequate test coverage. Those are now fixed.
Other bugs in the code added in [2] remain, but I'm still working on
fixing those.
See also [3] for the initial discarded MR to fix issues in this code
for both UserCreate and UserDelete at the same time (that WIP itself
has bugs...).
Notes:
A. The 08f22f2 and c642fe9 SHAs aren't special here. They just happen
to be the next commits after 1e292f8 in the test repo[4]. I just
needed some test commits.
B. Converting away from helper.ErrPreconditionFailed(err) &
helper.ErrPreconditionFailedf(fmt, err) here is intentional. See
[5].
1. c1e3ccca (Initial go implementation of UserCreateBranch, 2020-09-30)
2. c3b32722 (Initial go implementation of UserDeleteBranch, 2020-09-29)
3. https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2838
4. https://gitlab.com/gitlab-org/gitlab-test
5. https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2857#note_457436954
|
|
While our style document currently contains mostly advice about code
hygiene, commit hygiene is also important for a project to ensure it
ends up with understandable commits. It ensures that it is easily
possible to dig up the reasoning behind changes in the future, making
the project easier to maintain in the long run.
This commit thus documents various best practices around commit hygiene.
|
|
Tests that rely on Postgres instance are flaky
Closes #3208
See merge request gitlab-org/gitaly!2836
|
|
danger: No changelog requirement for doc changes
Closes #3322
See merge request gitlab-org/gitaly!2844
|
|
Fix TestErrorThreshold flaking
Closes #3134
See merge request gitlab-org/gitaly!2835
|
|
git: Strict verification of subcommands
See merge request gitlab-org/gitaly!2837
|
|
Update reference to command.GitPath()
See merge request gitlab-org/gitaly!2855
|
|
Print host assignments and primary per repository in `praefect dataloss`
See merge request gitlab-org/gitaly!2843
|
|
In 6cda774b5 (Removal of command.GitPath(), 2020-11-26) there was a
mass migration away from command.GitPath() which missed this one help
message. Update it accordingly.
|
|
Removal of command.GitPath()
See merge request gitlab-org/gitaly!2831
|
|
Function 'command.GitPath()' depends on the global 'config.Config' variable
and uses internal call to change the state of it in case it is not
yet initialized properly. To break this dependency we remove the function
and replaces it with direct access to the configured value.
It could be set from the config.toml file or from env using GITALY_TESTING_GIT_BINARY.
If none used the value will be resolved from the system.
In the tests the value is set on the configuration stage and point to the temporary
directory.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
|
|
To break dependency between SafeCmd and similar the CommandFactory
was introduced. In includes config.Cfg in it and provides a base
factory methods used inside of SafeCmd and others. This is
initial work in order to break dependency completely afterwards.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
|
|
'check' subcommand is not a real hook and should not write to the
same log used by actual gitaly-hook subcommands.
The 'GitlabNetClient.Check' method of the gitlab-shell lib expects
to get a JSON with 'message' field in case of an error. The mock
server aligned with that expectation. As a result the error message
has another content.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
|
|
'check' subcommand is not a real hook and should not write to the
same log used by actual gitaly-hook subcommands.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
|
|
command.GitPath() depends on the configuration provided from outside
and that configuration is not provided to the gitaly-debug executable.
The only possible case if GITALY_TESTING_GIT_BINARY env var is set to
a particular git executable. But the same could be configured using an
updated PATH.
The 'ReadIndex' function only used from the 'gitaly-debug', so it should
rely on the same 'git' binary.
Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/2699
|
|
transactions: Allow disabling with an env var
See merge request gitlab-org/gitaly!2853
|
|
Logic-wise this comments is intended to achieve the same as reverting:
ca8e2de58193775a62599b3a4de682c511046033:
(featureflag: Remove reference transaction feature flag), just without
conflicts and a more concentrated diff.
The use case is that a customer turned the feature flag for reference
transactions off, which wasn't honoured anymore. This change allows for
disabling transactions again.
The root problem lies in the fact that Gitaly needs to
connect with one Praefect that coordinates the current transaction.
Given this is an IP based connection, not to a domain name, this
violates some security policies.
|
|
Remove records of invalid repositories
Closes #3248
See merge request gitlab-org/gitaly!2833
|
|
'avar/test-both-branch-and-tag-creation-and-deletion-with-hooks' into 'master'
updateReferenceWithHooks: test both branch/tag creation and deletion
See merge request gitlab-org/gitaly!2840
|
|
Amend the tests I added in 9931076ba (updateReferenceWithHooks: add
tests for hook stdout/stderr output, 2020-11-18) to test both creation
and deletion.
The immediate reason I need this is to port UserDeleteTag to Go[1],
but more generally this improves our coverage. So let's do it in both
the branch and tag case.
1. https://gitlab.com/gitlab-org/gitaly/-/issues/3064
|
|
A nit noted in
https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2840#note_455235080
|
|
[ci skip]
|
|
When running commands via the Git DSL, we'll verify that the passed
Git subcommand looks sane. This really is only a weak sanity check as we
simply use a regular expression to do that verification, so it'll in
fact let a lot of commands pass which aren't a proper Git command.
We nowadays have the subcommand map which already tracks most of the
subcommands which we use in our codebase, so we can do a lot better than
just using a weak regular expression by just using information we
already got at hand. While typos in any subcommand would be easily
catched by any test already, having this additional verification allows
us to make sure that we never execute any Git commands for which we
don't have an entry in our subcommands map. Which in turn allows us to
assure that the map is in fact complete and is not missing any important
subcommand flags defining its characteristics. It can also serve as a
single location of truth for all commands we may run in our codebase.
So let's add the few remaining commands to that map. All of those
commands are in fact modifying references and do support end-of-options,
so none of them require any flags. With that, we can now enable strict
verification of subcommands.
|
|
The git-gc(1) command will clean up repositories by repacking both
objects and references, but in fact it shouldn't ever change any
existing references. This isn't reflected in our subcommand map and thus
requires us to pass an ultimately unused `git.WithRefTxHook()` option to
that command.
Fix this inaccuracy by labeling it as not updating references.
|
|
The git-commit-graph(1) command will never update any references. This
isn't reflected in our subcommand map and thus requires us to pass an
ultimately unused `git.WithRefTxHook()` option to that command.
Fix this inaccuracy by labeling it as not updating references.
|
|
While git-apply(1) may change worktree contents, it won't ever update
any references inside of a Git repository. This isn't reflected in our
subcommand map and thus requires us to pass an ultimately unused
`git.WithRefTxHook()` option to that command.
Fix this inaccuracy by labeling it as not updating references.
|
|
Our safecmd tests currently have a set of tests which verify command
line parsing by creating non-existing commands like `git meow`. While
cute, the plan is to make our command verification stricter by
disallowing any commands which are not in our map of subcommands. "woof"
and "meow" unfortunately aren't going to be part of that map.
Let's replace them with git-update-ref(1) commands to prepare for that
change.
|
|
'avar/remove-copy-pasted-when-an-error-happens-updateRefError-code' into 'master'
User{Branch,Submodule}: remove erroneously copy/pasted error handling
See merge request gitlab-org/gitaly!2841
|
|
Now that we have a single location where we keep track of subcommands
and their characteristics which has been introduced with the preceding
commit, this commit moves `supportsEndOfOption()` into that map by
adding a new flag `scNoEndOfOptions`. This consolidates one more
location where we track subcommands.
|
|
During the ongoing work to globally enable the reference-transaction
hook, we have grown multiple maps which track different Git subcommands
in order to categorize by different characteristics. Right now those
include whether the command is read-only or never changes references,
but there are additional characteristics which are of interest.
Let's refactor the two maps we have into a single map which holds all
subcommands, where each subcommand has a bitfield of flags. This allows
us to more easily add additional characteristics without having to
create new maps for each of them.
|
|
Tell Git where to find reference-transaction hooks
See merge request gitlab-org/gitaly!2834
|
|
When a merge request has the documentation label it does not need Danger
to trigger a warning to create a changelog entry.
|
|
The `WithRefTxHook()` function has the purpose of setting up any Git
commands which may potentially update any references such that they can
use the reference-transaction hook. While it's already passing in most
of the required environment variables, it doesn't in fact tell Git where
it shall look for any hooks. As a result, Git wouldn't ever execute them
at all.
Fix the issue by configuring `core.HooksPath` as a global Git option.
Note that while this change will cause us to execute hooks a _lot_ more
frequently, it shouldn't in fact cause any additional transactions to be
used. This is due to the fact that we don't yet pass transaction or
Praefect server info to the hook.
|
|
Our hooks are currently a set of shell scripts which use the
GITALY_BIN_DIR environment variable to locate the real gitaly-hook
binary's location. As a result, if that variable isn't set in Git's
environment, then we aren't able to execute any hooks at all and will
thus fail most Git operations.
While most places actually do set up this variable correctly, the
`WithRefTxHook()` option doesn't. This is being fixed by this commit.
|
|
The CmdOpt type can currently only modify a command's environment as
well as its standard streams. As we'll need to extend the
`WithRefTxHook()` option to also pass global options to Git, this commit
extends `cmdCfg` to also carry and apply such global options.
|
|
The function `git.unsafeBareCmdWithoutRepo()` is a direct wrapper of
`unsafeBareCmd()`, just without an additional environment parameter.
Given that we'll have to switch its only non-test user to the variant
which does have an environment argument, let's just inline this
function. This is required in order to implement global usage of
reference-transactions.
|
|
Our tests in `internal/git` are independent of any GRPC server setups as
they're a lot more low-level than that. As our hooks depend on the Hook
GRPC service to be accessible, the conclusion is that we cannot test
hooks in this package.
Override the hook path so that we don't try to use them. While this is
currently not an issue, this is in preparation of an extended
`WithRefTxHook()` option which also correctly sets `core.hooksPath`.
|
|
While the ssh service uses Git hooks, they have currently been overriden
via `hooks.Override` as the hook service wasn't set up correctly. This
commit fixes the setup and removes the hook override. This is a
preparatory commit for globally executing the reference-transaction
hook.
As we've started validating hook paramaters starting with b163cddf5
(hook: Assert that required environment variables are present,
2020-11-19), enabling hooks also requires a change to a test which was
missing the glRepository parameter.
|
|
While the smarthttp service uses Git hooks, they have currently been
overriden via `hooks.Override` as the hook service wasn't set up
correctly. This commit fixes the setup and removes the hook override.
This is a preparatory commit for globally executing the
reference-transaction hook.
|
|
Praefect uses a complete Gitaly server setup, which will make use of Git
hooks as soon as we start to globally enable the reference-transaction
hook for most of our Git commands. Currently, this would fail Praefect
tests though as we're not configuring the gitaly-hooks binary as a part
of our test setup.
Fix the issue by calling `testhelper.ConfigureGitalyHooksBinary()`.
|
|
Setup the internal socket listener for the operations service in order
to allow for hooks to funciton correctly. This is a preparatory commit
for globally executing the reference-transaction hook.
|
|
The repository tests currently don't execute any hooks. This is about to
change when we globally enable execution of the reference-transaction
hook, but as we don't have the hook service correctly set up this would
cause tests to fail for this service.
Fix the issue by setting up the hook service on Gitaly's internal
socket and installing the gitaly-hooks binary. Ideally, we'd just set up
the internal socket once and for all in `runRepoServer()`. But for
reasons I wasn't able to figure out, this would cause test failures with
Praefect due to the internal listening socket not being closed
somewhere.
|
