Skip to content
GitLab Next
  • Menu
Projects Groups Snippets
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
  • GitLab GitLab
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
    • Locked Files
  • Issues 43,846
    • Issues 43,846
    • List
    • Boards
    • Service Desk
    • Milestones
    • Iterations
    • Requirements
  • Merge requests 1,397
    • Merge requests 1,397
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
    • Test Cases
  • Deployments
    • Deployments
    • Environments
    • Releases
  • Packages & Registries
    • Packages & Registries
    • Package Registry
    • Container Registry
    • Infrastructure Registry
  • Monitor
    • Monitor
    • Metrics
    • Incidents
  • Analytics
    • Analytics
    • Value stream
    • CI/CD
    • Code review
    • Insights
    • Issue
    • Repository
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • GitLab.org
  • GitLabGitLab
  • Issues
  • #25095
Closed
Open
Created Nov 07, 2018 by Nick Thomas@nick.thomas🆓Contributor8 of 20 tasks completed8/20 tasks

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 Mar 16, 2020 by Nick Thomas
Assignee
Assign to
Time tracking