Skip to content

Remove gitlab-shell from gitlab-rails

At one time, gitlab-rails tried to gate all its accesses to git via the gitlab-shell project. With the introduction of Gitaly, that has been dying a death. There are now two categories of operation in lib/gitlab/shell.rb:

  • Thin wrappers around Gitaly, run in sidekiq jobs and within unicorn/puma
  • Thin wrappers around the bin/gitlab-keys executable in gitlab-shell, run from sidekiq jobs

I propose that we merge the bin/gitlab-keys functionality into gitlab-rails, just as we did for GitlabProjects (this has since gone to Gitaly), and then delete lib/gitlab/shell.rb and reroute all unnecessary indirections. This will mean that Gitlab::Shell::Error will no longer exist, and can be removed from a number of sites in the codebase too.

Removing this unnecessary indirection makes the code easier to understand and reason about. I did it for the fetch_remote call in this MR: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/22635 and I think it's feasible for the remaining calls too.

With the movement of gitlab-shell's hooks into gitaly and bin/gitlab-keys into gitlab-rails, this would mean gitlab-shell would be focused solely on servicing commands from the SSH daemon, which is a very reasonable scope. We might also be able to completely remove gitlab-shell from the gitlab-rails dependencies, including in the test suite, which would be nice.

A TODO list:

  • Disentangle authorized_keys from Gitlab::Shell !26913 (merged)
  • Stop using Gitlab::Shell#create_repository in specs: !27009 (merged)
  • Bite 1 of instance methods in Gitlab::Shell: !26924 (diffs)
    • create_repository
    • url_to_repo
    • version
  • GitlabGitalyProjects !27217 (merged)
    • import_repository
    • fork_repository
  • Others
    • repository_exists?
    • remove_repository
  • Remove Gitlab::ShellAdapter

The following are used in legacy storage, and may be best handled by leaving them as-is and removing as part of &2320 . This implies we'd move this issue between epics once we get here.

  • Remove namespace instance methods from Gitlab::Shell
    • add_namespace
    • rm_namespace
    • mv_namespace
  • Remove remaining instance methods from Gitlab::Shell
    • mv_repository
  • Remove GitlabShellWorker

Here are the remaining methods, and where they're used:

Method Sync Async
mv_repository x x
remove_repository x x
add_namespace x -
rm_namespace - x
rm_directory - -
mv_namespace x -
repository_exists x -

Things that are used async need to be handled in GitlabShellWorker still, one way or another.

Edited by Igor Drozdov