rubocop: bring it closer to upstream

After some discussions internally, we decided that it was good to be as
close to the default upstream rubocop configuration. For this reasons,
I have trimmed down as much as possible our own modifications for the
rubocop file, and hence we had to re-adapt a lot of code.

There are some things that have been left out:

- Some extra cops like rspec-rubocop.
- Further trimming down some configuration options that needed some
  heavy liftings...

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>

.rubocop.yml

@@ -1,75 +1,100 @@
+AllCops:
+  TargetRubyVersion: 2.4
+  TargetRailsVersion: 4.2
 
-inherit_from:
-  - ./config/rubocop-suse.yml
+  DisplayCopNames: true
+  DisplayStyleGuide: false
 
-# TODO: (mssola) only the LDAP class and portusctl require this.
-Metrics/ClassLength:
-  Max: 210
+  Exclude:
+    # Files that are out of our control and that are not excluded in the
+    # default config of rubocop.
+    - db/schema.rb
+    - db/migrate/*
+    - vendor/**/*
 
-# TODO: (mssola) Some methods are offending this cop. In the SUSE's style guide
-# the approach is to use Rubocop's default value. In the near future I will
-# fix this.
-Metrics/MethodLength:
-  Max: 252
+#
+# Layout
+#
 
-# Of course this will fail in spec blocks... Just skip it.
+# This is a remnant of old SUSE-style alignment for hashes, The table format
+# looks more human readable.
+Layout/AlignHash:
+  EnforcedHashRocketStyle: table
+  EnforcedColonStyle: table
+
+#
+# Metrics
+#
+
+# The default is just too small.
+Metrics/AbcSize:
+  Max: 30
+
+# We will skip it for tests and the Grape API.
 Metrics/BlockLength:
-  Max: 200
   Exclude:
     - spec/**/*
+    - lib/api/**/*
+
+# NOTE: this could be further trimmed down but the following refactorings are
+# needed:
+#   1. The Grape API classes should be further modularized (probably in helper
+#      modules).
+#   2. The models regarding the registry have to be modularized.
+#   3. LDAP has to be refactored.
+Metrics/ClassLength:
+  Max: 210
 
-# It's convenient to mix both. This is something that SUSE's style guide does
-# not specify, so we take the approach that we were following already.
-Style/ClassAndModuleChildren:
-  Enabled: false
+# The default is just too small. A limit of 100 looks reasonable and many other
+# projects (including inside of SUSE) are also using this value.
+Metrics/LineLength:
+  Max: 100
 
-# NOTE: (mssola) This would be nice, but there are too many errors on this for
-# now and it's not urgent. I will fix this in the near future.
-Style/Documentation:
-  Enabled: false
+# NOTE: this could be further trimmed down but some refactorings will have to be
+# done.
+Metrics/MethodLength:
+  Max: 20
 
-# This forces us to create a new object for no real reason.
-Style/MultipleComparison:
-  Enabled: false
+#
+# Rails
+#
 
-# Doesn't feel right ...
-Style/NumericPredicate:
-  Enabled: false
+Rails:
+  Enabled: true
 
-# TODO: remove this on Rails 5.
-Style/InverseMethods:
+Rails/SkipsModelValidations:
   Enabled: false
 
-# NOTE: (mssola) In versions of Ruby higher than 2.1.x, we get
-# Lint/ShadowedException in some cases where in 2.1.x it complains if we remove
-# such guards. Therefore, we have to disable this cop until we update the
-# version of Ruby to be officially supported.
-Lint/UnneededDisable:
-  Enabled: false
+#
+# Style
+#
 
-Style/FrozenStringLiteralComment:
+# It's not needed to add documentation for obvious modules or classes. The main
+# idea is that documentation will be asked during the review process if needed.
+Style/Documentation:
   Enabled: false
 
-# TODO: move to SUSE's style guide ?
-Style/SymbolArray:
+# NOTE: this is giving false positives because it assumes some Rails 5 methods
+# are available.
+Style/InverseMethods:
   Enabled: false
 
-Rails:
-  Enabled: true
-
-Rails/SkipsModelValidations:
+# This forces us to create a new object for no real reason.
+Style/MultipleComparison:
   Enabled: false
 
-AllCops:
-  TargetRubyVersion: 2.4
-  TargetRailsVersion: 4.2
+# This is a common SUSE configuration value: the performance difference between
+# single and double quotes is no longer there, and so it's better to be
+# consistent and force only double quotes.
+Style/StringLiterals:
+  EnforcedStyle: double_quotes
 
-  DisplayCopNames: true
-  DisplayStyleGuide: false
+# Same as Style/StringLiterals.
+Style/StringLiteralsInInterpolation:
+  EnforcedStyle: double_quotes
 
-  Exclude:
-    # Files that are out of our control and that are not excluded in the
-    # default config of rubocop.
-    - db/schema.rb
-    - db/migrate/*
-    - vendor/**/*
+# There are some false positives (e.g. "module ::Module", in which we want to
+# make sure there are no clashes or misunderstandings). Therefore, we just
+# disable this cop.
+Style/ClassAndModuleChildren:
+  Enabled: false

.travis.yml

@@ -45,7 +45,7 @@ script:
   - bundle exec rake portus:assets:compile
 
   # Ruby tests
-  - bundle exec rspec spec packaging/suse/portusctl/spec
+  - bundle exec rspec spec packaging/suse/portusctl/spec --tag ~integration
 
   # Style and security checks
   - bundle exec rubocop -V

Gemfile

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 source "https://rubygems.org"
 
 gem "active_record_union"

Guardfile

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 guard :rspec, all_on_start: false, cmd: "bundle exec rspec" do
   require "guard/rspec/dsl"
   dsl = Guard::RSpec::Dsl.new(self)
@@ -31,7 +33,7 @@ end
 
 guard :rubocop, all_on_start: true do
   watch(/.+\.rb\z/)
-  watch(/\Abin\/.+\z/)
+  watch(%r{\Abin/.+\z})
 
   watch(%r{(?:.+/)?\.rubocop\.yml$}) do |m|
     File.dirname(m[0])

Rakefile

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # Add your own tasks in files placed in lib/tasks ending in .rake,
 # for example lib/tasks/capistrano.rake, and they will automatically be available to Rake.
 

Vagrantfile

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # -*- mode: ruby -*-
 # vi: set ft=ruby :
 
@@ -15,7 +17,7 @@ Vagrant.configure("2") do |config|
     node.vm.box_check_update = true
     node.vm.hostname = "registry.test.lan"
     node.vm.network :private_network, ip: "192.168.1.2", virtualbox__intnet: true
-    node.vm.network :forwarded_port, host: 44242, guest: 80
+    node.vm.network :forwarded_port, host: 44_242, guest: 80
 
     node.vm.provision(
       "shell",

app/controllers/admin/activities_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require "csv"
 
 class Admin::ActivitiesController < Admin::BaseController

app/controllers/admin/base_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class Admin::BaseController < ApplicationController
   before_action :ensure_admin!
 

app/controllers/admin/dashboard_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class Admin::DashboardController < Admin::BaseController
   def index
     @recent_activities = PublicActivity::Activity

app/controllers/admin/namespaces_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class Admin::NamespacesController < Admin::BaseController
   def index
     @special_namespaces = Namespace.where(global: true)

app/controllers/admin/registries_controller.rb

@@ -1,8 +1,10 @@
+# frozen_string_literal: true
+
 # Allows the creation of exactly one registry. It also allows updating the
 # "use_ssl" attribute of a given registry. Doing all this is safe because only
 # admin users will be able to reach this controller.
 class Admin::RegistriesController < Admin::BaseController
-  before_action :registry_created, only: [:new, :create]
+  before_action :registry_created, only: %i[new create]
 
   # GET /admin/registries/
   def index

app/controllers/admin/teams_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class Admin::TeamsController < Admin::BaseController
   def index
     @teams = Team.all_non_special.page(params[:page])

app/controllers/admin/users_controller.rb

@@ -1,6 +1,8 @@
+# frozen_string_literal: true
+
 class Admin::UsersController < Admin::BaseController
   respond_to :html, :js
-  before_action :another_user_access, only: [:edit, :update, :destroy]
+  before_action :another_user_access, only: %i[edit update destroy]
 
   def index
     @users = User.not_portus.order(:username).page(params[:page])
@@ -32,7 +34,7 @@ def edit; end
   def update
     return if @user.nil?
 
-    attr = params.require(:user).permit([:email, :display_name])
+    attr = params.require(:user).permit(%i[email display_name])
 
     if @user.update_attributes(attr)
       redirect_to admin_users_path,
@@ -72,7 +74,7 @@ def toggle_admin
   private
 
   def user_create_params
-    permitted = [:username, :email, :password, :password_confirmation]
+    permitted = %i[username email password password_confirmation]
     params.require(:user).permit(permitted)
   end
 

app/controllers/api/base_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class Api::BaseController < ActionController::Base
   class ScopeNotHandled < StandardError; end
   class RegistryNotHandled < StandardError; end

app/controllers/api/v2/events_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # This is the endpoint being used to handle notifications from the Registry.
 class Api::V2::EventsController < Api::BaseController
   # A new notification is coming, register it if valid.

app/controllers/api/v2/ping_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class Api::V2::PingController < Api::BaseController
   def ping
     authenticate_user!

app/controllers/api/v2/tokens_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # TokensController is used to deliver the token that the docker client should
 # use in order to perform operation into the registry. This is the last step in
 # the authentication process for Portus' point of view.
@@ -49,27 +51,11 @@ def authorize_scopes(registry)
     scopes.each do |scope|
       auth_scope, actions = scope_handler(registry, scope)
 
-      actions.each do |action|
-        # It will try to check if the current user is authorized to access the
-        # scope given in this iteration. If everything is fine, then nothing will
-        # happen, otherwise there are two possible exceptions that can be raised:
-        #
-        #   - NoMethodError: the targeted resource does not handle the scope that
-        #     is being checked. It will raise a ScopeNotHandled.
-        #   - Pundit::NotAuthorizedError: the targeted resource unauthorized the
-        #     given user for the scope that is being checked. In this case this
-        #     scope gets removed from `auth_scope.actions`.
-        begin
-          authorize auth_scope.resource, "#{action}?".to_sym
-        rescue NoMethodError, Pundit::NotAuthorizedError, Portus::AuthScope::ResourceNotFound
-          logger.debug "action #{action} not handled/authorized, removing from actions"
-          auth_scope.actions.delete_if { |a| match_action(action, a) }
-        end
-      end
-
+      actions.each { |action| triage_action!(auth_scope, action) }
       next if auth_scope.actions.empty?
-      # if there is already a similar scope (type and resource name),
-      # we combine them into one:
+
+      # If there is already a similar scope (type and resource name), we combine
+      # them into one:
       # e.g. scope=repository:busybox:push&scope=repository:busybox:pull
       #      -> access=>[{:type=>"repository", :name=>"busybox", :actions=>["push", "pull"]}
       k = [auth_scope.resource_type, auth_scope.resource_name]
@@ -82,6 +68,22 @@ def authorize_scopes(registry)
     auth_scopes.values
   end
 
+  # It will try to check if the current user is authorized to access the
+  # scope given in this iteration. If everything is fine, then nothing will
+  # happen, otherwise there are two possible exceptions that can be raised:
+  #
+  #   - NoMethodError: the targeted resource does not handle the scope that
+  #     is being checked. It will raise a ScopeNotHandled.
+  #   - Pundit::NotAuthorizedError: the targeted resource unauthorized the
+  #     given user for the scope that is being checked. In this case this
+  #     scope gets removed from `auth_scope.actions`.
+  def triage_action!(auth_scope, action)
+    authorize auth_scope.resource, "#{action}?".to_sym
+  rescue NoMethodError, Pundit::NotAuthorizedError, Portus::AuthScope::ResourceNotFound
+    logger.debug "action #{action} not handled/authorized, removing from actions"
+    auth_scope.actions.delete_if { |a| match_action(action, a) }
+  end
+
   # Returns true if the given item matches the given action.
   def match_action(action, item)
     action = "*" if action == "all"

app/controllers/application_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require "portus/auth_from_token"
 
 class ApplicationController < ActionController::Base

app/controllers/application_tokens_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # ApplicationTokensController manages the creation/removal of application tokens
 class ApplicationTokensController < ApplicationController
   respond_to :js

app/controllers/auth/omniauth_callbacks_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class Auth::OmniauthCallbacksController < Devise::OmniauthCallbacksController
   skip_before_action :verify_authenticity_token
   before_action :check_user, except: [:failure]
@@ -49,7 +51,7 @@ def check_domain
       true
     else
       redirect_to new_user_session_url,
-        alert: "Email addresses on the domain #{d} aren't allowed."
+                  alert: "Email addresses on the domain #{d} aren't allowed."
       false
     end
   end

app/controllers/auth/omniauth_registrations_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class Auth::OmniauthRegistrationsController < ApplicationController
   layout "authentication"
   skip_before_action :authenticate_user!