Merge branch '7864-ee-routes-ee' into 'master'

Put EE routes in EE files under EE directories

Closes #7864

See merge request gitlab-org/gitlab-ee!7935

.rubocop.yml

@@ -76,6 +76,7 @@ Naming/FileName:
     - 'qa/qa/specs/**/*'
     - 'qa/bin/*'
     - 'config/**/*'
+    - 'ee/config/**/*'
     - 'lib/generators/**/*'
     - 'locale/unfound_translations.rb'
     - 'ee/locale/unfound_translations.rb'

config/initializers/routing_draw.rb

 # Adds draw method into Rails routing
-# It allows us to keep routing splitted into files
-class ActionDispatch::Routing::Mapper
-  def draw(routes_name)
-    instance_eval(File.read(Rails.root.join("config/routes/#{routes_name}.rb")))
-  end
-end
+# It allows us to keep routing split into files
+ActionDispatch::Routing::Mapper.prepend Gitlab::Patch::DrawRoute

config/routes.rb

@@ -37,13 +37,7 @@ Rails.application.routes.draw do
     match '*all', via: [:get, :post], to: proc { [404, {}, ['']] }
   end
 
-  namespace :oauth do
-    scope path: 'geo', controller: :geo_auth, as: :geo do
-      get 'auth'
-      get 'callback'
-      get 'logout'
-    end
-  end
+  draw :oauth
 
   use_doorkeeper_openid_connect
 

config/routes/admin.rb

@@ -16,17 +16,11 @@ namespace :admin do
       put :unlock
       put :confirm
       post :impersonate
-      post :reset_runners_minutes
       patch :disable_two_factor
       delete 'remove/:email_id', action: 'remove_email', as: 'remove_email'
     end
   end
 
-  ## EE-specific
-  resource :push_rule, only: [:show, :update]
-  get :instance_review, to: 'instance_review#index'
-  ## EE-specific
-
   resource :impersonation, only: :destroy
 
   resources :abuse_reports, only: [:index, :destroy]
@@ -48,7 +42,6 @@ namespace :admin do
 
     scope(as: :group) do
       put :members_update
-      post :reset_runners_minutes
       get :edit, action: :edit
       get '/', action: :show
       patch '/', action: :update
@@ -79,11 +72,6 @@ namespace :admin do
   resource :health_check, controller: 'health_check', only: [:show]
   resource :background_jobs, controller: 'background_jobs', only: [:show]
 
-  ## EE-specific
-  resource :email, only: [:show, :create]
-  resources :audit_logs, controller: 'audit_logs', only: [:index]
-  ## EE-specific
-
   resource :system_info, controller: 'system_info', only: [:show]
   resources :requests_profiles, only: [:index, :show], param: :name, constraints: { name: /.+\.html/ }
 
@@ -125,26 +113,6 @@ namespace :admin do
     get :integrations, :repository, :templates, :ci_cd, :reporting, :metrics_and_profiling, :network, :geo, :preferences
   end
 
-  ## EE-specific
-  resource :license, only: [:show, :new, :create, :destroy] do
-    get :download, on: :member
-  end
-
-  namespace :geo do
-    resources :nodes, only: [:index, :create, :new, :edit, :update]
-
-    resources :projects, only: [:index, :destroy] do
-      member do
-        post :recheck
-        post :resync
-        post :force_redownload
-      end
-    end
-  end
-
-  get '/dashboard/stats', to: 'dashboard#stats'
-  ## EE-specific
-
   resources :labels
 
   resources :runners, only: [:index, :show, :update, :destroy] do

config/routes/group.rb

+# frozen_string_literal: true
+
 resources :groups, only: [:index, :new, :create] do
   post :preview_markdown
 end
@@ -13,7 +15,6 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
       get :merge_requests, as: :merge_requests_group
       get :projects, as: :projects_group
       get :activity, as: :activity_group
-      get :subgroups, as: :subgroups_group ## EE-specific
       put :transfer, as: :transfer_group
       # TODO: Remove as part of refactor in https://gitlab.com/gitlab-org/gitlab-ce/issues/49693
       get 'shared', action: :show, as: :group_shared
@@ -55,7 +56,6 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
     resources :group_members, only: [:index, :create, :update, :destroy], concerns: :access_requestable do
       post :resend_invite, on: :member
       delete :leave, on: :collection
-      patch :override, on: :member ## EE-specific
     end
 
     resources :uploads, only: [:create] do
@@ -65,71 +65,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
       end
     end
 
-    ## EE-specific
-    resource :analytics, only: [:show]
-    resource :ldap, only: [] do
-      member do
-        put :sync
-      end
-    end
-
-    resources :ldap_group_links, only: [:index, :create, :destroy]
-
-    # EE-specific start
-    namespace :security do
-      resource :dashboard, only: [:show], controller: :dashboard
-      resources :vulnerabilities, only: [:index], controller: :vulnerabilities do
-        collection do
-          get :summary
-        end
-      end
-    end
-    # EE-specific end
-
-    ## EE-specific
-    resource :saml_providers, path: 'saml', only: [:show, :create, :update] do
-      post :callback, to: 'omniauth_callbacks#group_saml'
-      get :sso, to: 'sso#saml'
-    end
-
-    resource :notification_setting, only: [:update]
-    resources :audit_events, only: [:index]
-    resources :pipeline_quota, only: [:index]
-
-    resources :hooks, only: [:index, :create, :destroy], constraints: { id: /\d+/ } do
-      member do
-        post :test
-      end
-    end
-
-    resources :autocomplete_sources, only: [] do
-      collection do
-        get 'members'
-        get 'labels'
-        get 'epics'
-        get 'commands'
-      end
-    end
-
-    resources :billings, only: [:index]
-    resources :epics, concerns: :awardable, constraints: { id: /\d+/ } do
-      member do
-        get :discussions, format: :json
-        get :realtime_changes
-        post :toggle_subscription
-      end
-
-      resources :epic_issues, only: [:index, :create, :destroy, :update], as: 'issues', path: 'issues'
-
-      scope module: :epics do
-        resources :notes, only: [:index, :create, :destroy, :update], concerns: :awardable, constraints: { id: /\d+/ }
-      end
-    end
-
-    resources :todos, only: [:create]
-
-    # On CE only index and show are needed
-    resources :boards, only: [:index, :show, :create, :update, :destroy]
+    resources :boards, only: [:index, :show]
 
     resources :runners, only: [:index, :edit, :update, :destroy, :show] do
       member do
@@ -137,17 +73,6 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
         post :pause
       end
     end
-
-    legacy_ee_group_boards_redirect = redirect do |params, request|
-      path = "/groups/#{params[:group_id]}/-/boards"
-      path << "/#{params[:extra_params]}" if params[:extra_params].present?
-      path << "?#{request.query_string}" if request.query_string.present?
-      path
-    end
-    get 'boards(/*extra_params)', as: :legacy_ee_group_boards_redirect, to: legacy_ee_group_boards_redirect
-
-    ## EE-specific
-    resource :roadmap, only: [:show], controller: 'roadmap'
   end
 
   scope(path: '*id',
@@ -166,11 +91,5 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
     Gitlab::Routing.redirect_legacy_paths(self, :labels, :milestones, :group_members,
                                           :edit, :issues, :merge_requests, :projects,
                                           :activity)
-
-    ## EE-specific
-    Gitlab::Routing.redirect_legacy_paths(self, :analytics, :ldap, :ldap_group_links,
-                                          :notification_setting, :audit_events,
-                                          :pipeline_quota, :hooks, :boards)
-    ## EE-specific
   end
 end

doc/development/ee_features.md

@@ -171,7 +171,7 @@ There are a few gotchas with it:
       class Base
         def execute
           return unless enabled?
-  
+
           # ...
           # ...
         end
@@ -185,12 +185,12 @@ There are a few gotchas with it:
       class Base
         def execute
           return unless enabled?
-  
+
           do_something
         end
-  
+
         private
-  
+
         def do_something
           # ...
           # ...
@@ -204,14 +204,14 @@ There are a few gotchas with it:
     ```ruby
       module EE::Base
         extend ::Gitlab::Utils::Override
-  
+
         override :do_something
         def do_something
           # Follow the above pattern to call super and extend it
         end
       end
     ```
-  
+
     This would require updating CE first, or make sure this is back ported to CE.
 
 When prepending, place them in the `ee/` specific sub-directory, and
@@ -332,6 +332,21 @@ full implementation details.
 [ce-mr-full-private]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12373
 [ee-mr-full-private]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2199
 
+### Code in `config/routes`
+
+When we add `draw :admin` in `config/routes.rb`, the application will try to
+load the file located in `config/routes/admin.rb`, and also try to load the
+file located in `ee/config/routes/admin.rb`.
+
+In EE, it should at least load one file, at most two files. If it cannot find
+any files, an error will be raised. In CE, since we don't know if there will
+be an EE route, it will not raise any errors even if it cannot find anything.
+
+This means if we want to extend a particular CE route file, just add the same
+file located in `ee/config/routes`. If we want to add an EE only route, we
+could still put `draw :ee_only` in both CE and EE, and add
+`ee/config/routes/ee_only.rb` in EE, similar to `render_if_exists`.
+
 ### Code in `app/controllers/`
 
 In controllers, the most common type of conflict is with `before_action` that

ee/config/routes/admin.rb

+# frozen_string_literal: true
+
+namespace :admin do
+  resources :users, constraints: { id: %r{[a-zA-Z./0-9_\-]+} } do
+    member do
+      post :reset_runners_minutes
+    end
+  end
+
+  scope(path: 'groups/*id',
+        controller: :groups,
+        constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ }) do
+    scope(as: :group) do
+      post :reset_runners_minutes
+    end
+  end
+
+  get :instance_review, to: 'instance_review#index'
+
+  resource :push_rule, only: [:show, :update]
+  resource :email, only: [:show, :create]
+  resources :audit_logs, controller: 'audit_logs', only: [:index]
+
+  resource :license, only: [:show, :new, :create, :destroy] do
+    get :download, on: :member
+  end
+
+  namespace :geo do
+    resources :nodes, only: [:index, :create, :new, :edit, :update]
+
+    resources :projects, only: [:index, :destroy] do
+      member do
+        post :recheck
+        post :resync
+        post :force_redownload
+      end
+    end
+  end
+
+  get '/dashboard/stats', to: 'dashboard#stats'
+end

ee/config/routes/group.rb

+# frozen_string_literal: true
+
+constraints(::Constraints::GroupUrlConstrainer.new) do
+  scope(path: 'groups/*id',
+        controller: :groups,
+        constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom|ics)/ }) do
+    scope(path: '-') do
+      get :subgroups, as: :subgroups_group
+    end
+  end
+
+  scope(path: 'groups/*group_id/-',
+        module: :groups,
+        as: :group,
+        constraints: { group_id: Gitlab::PathRegex.full_namespace_route_regex }) do
+    resources :group_members, only: [], concerns: :access_requestable do
+      patch :override, on: :member
+    end
+
+    resource :analytics, only: [:show]
+    resource :ldap, only: [] do
+      member do
+        put :sync
+      end
+    end
+
+    resource :notification_setting, only: [:update]
+
+    resources :ldap_group_links, only: [:index, :create, :destroy]
+    resources :audit_events, only: [:index]
+    resources :pipeline_quota, only: [:index]
+
+    resources :hooks, only: [:index, :create, :destroy], constraints: { id: /\d+/ } do
+      member do
+        post :test
+      end
+    end
+
+    resources :autocomplete_sources, only: [] do
+      collection do
+        get 'members'
+        get 'labels'
+        get 'epics'
+        get 'commands'
+      end
+    end
+
+    resources :billings, only: [:index]
+    resources :epics, concerns: :awardable, constraints: { id: /\d+/ } do
+      member do
+        get :discussions, format: :json
+        get :realtime_changes
+        post :toggle_subscription
+      end
+
+      resources :epic_issues, only: [:index, :create, :destroy, :update], as: 'issues', path: 'issues'
+
+      scope module: :epics do
+        resources :notes, only: [:index, :create, :destroy, :update], concerns: :awardable, constraints: { id: /\d+/ }
+      end
+    end
+
+    resources :todos, only: [:create]
+    resources :boards, only: [:create, :update, :destroy]
+
+    namespace :security do
+      resource :dashboard, only: [:show], controller: :dashboard
+      resources :vulnerabilities, only: [:index], controller: :vulnerabilities do
+        collection do
+          get :summary
+        end
+      end
+    end
+
+    resource :saml_providers, path: 'saml', only: [:show, :create, :update] do
+      post :callback, to: 'omniauth_callbacks#group_saml'
+      get :sso, to: 'sso#saml'
+    end
+
+    resource :roadmap, only: [:show], controller: 'roadmap'
+
+    legacy_ee_group_boards_redirect = redirect do |params, request|
+      path = "/groups/#{params[:group_id]}/-/boards"
+      path << "/#{params[:extra_params]}" if params[:extra_params].present?
+      path << "?#{request.query_string}" if request.query_string.present?
+      path
+    end
+    get 'boards(/*extra_params)', as: :legacy_ee_group_boards_redirect, to: legacy_ee_group_boards_redirect
+  end
+
+  scope(path: 'groups/*group_id') do
+    Gitlab::Routing.redirect_legacy_paths(self, :analytics, :ldap, :ldap_group_links,
+                                          :notification_setting, :audit_events,
+                                          :pipeline_quota, :hooks, :boards)
+  end
+end

ee/config/routes/oauth.rb

+# frozen_string_literal: true
+
+namespace :oauth do
+  scope path: 'geo', controller: :geo_auth, as: :geo do
+    get 'auth'
+    get 'callback'
+    get 'logout'
+  end
+end

ee/lib/ee/gitlab/patch/draw_route.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Patch
+      module DrawRoute
+        extend ::Gitlab::Utils::Override
+
+        override :draw_ee
+        def draw_ee(routes_name)
+          draw_route(route_path("ee/config/routes/#{routes_name}.rb"))
+        end
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/patch/draw_route_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+describe Gitlab::Patch::DrawRoute do
+  subject do
+    Class.new do
+      include Gitlab::Patch::DrawRoute
+
+      def route_path(route_name)
+        File.expand_path("../../../../../#{route_name}", __dir__)
+      end
+    end.new
+  end
+
+  before do
+    allow(subject).to receive(:instance_eval)
+  end
+
+  it 'evaluates EE only routes' do
+    subject.draw(:oauth)
+
+    expect(subject).to have_received(:instance_eval)
+      .with(File.read(subject.route_path('ee/config/routes/oauth.rb')))
+      .once
+
+    expect(subject).to have_received(:instance_eval)
+      .once
+  end
+
+  it 'evaluates CE and EE routes' do
+    subject.draw(:admin)
+
+    expect(subject).to have_received(:instance_eval)
+      .with(File.read(subject.route_path('config/routes/admin.rb')))
+      .once
+
+    expect(subject).to have_received(:instance_eval)
+      .with(File.read(subject.route_path('ee/config/routes/admin.rb')))
+      .once
+  end
+
+  it 'raises an error when nothing is drawn' do
+    expect { subject.draw(:non_existing) }
+      .to raise_error(described_class::RoutesNotFound)
+  end
+end

lib/gitlab/patch/draw_route.rb

+# frozen_string_literal: true
+
+# We're patching `ActionDispatch::Routing::Mapper` in
+# config/initializers/routing_draw.rb
+module Gitlab
+  module Patch
+    module DrawRoute
+      prepend EE::Gitlab::Patch::DrawRoute
+
+      RoutesNotFound = Class.new(StandardError)
+
+      def draw(routes_name)
+        drawn_any = draw_ce(routes_name) | draw_ee(routes_name)
+
+        drawn_any || raise(RoutesNotFound.new("Cannot find #{routes_name}"))
+      end
+
+      def draw_ce(routes_name)
+        draw_route(route_path("config/routes/#{routes_name}.rb"))
+      end
+
+      def draw_ee(_)
+        true
+      end
+
+      def route_path(routes_name)
+        Rails.root.join(routes_name)
+      end
+
+      def draw_route(path)
+        if File.exist?(path)
+          instance_eval(File.read(path))
+          true
+        else
+          false
+        end
+      end
+    end
+  end
+end

spec/fast_spec_helper.rb

@@ -8,3 +8,4 @@ require_relative 'support/rspec'
 require 'active_support/all'
 
 ActiveSupport::Dependencies.autoload_paths << 'lib'
+ActiveSupport::Dependencies.autoload_paths << 'ee/lib'

spec/lib/gitlab/patch/draw_route_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+describe Gitlab::Patch::DrawRoute do
+  subject do
+    Class.new do
+      include Gitlab::Patch::DrawRoute
+
+      def route_path(route_name)
+        File.expand_path("../../../../#{route_name}", __dir__)
+      end
+    end.new
+  end
+
+  before do
+    allow(subject).to receive(:instance_eval)
+  end
+
+  it 'evaluates CE only route' do
+    subject.draw(:help)
+
+    expect(subject).to have_received(:instance_eval)
+      .with(File.read(subject.route_path('config/routes/help.rb')))
+      .once
+
+    expect(subject).to have_received(:instance_eval)
+      .once
+  end
+end