Merge branch 'ee-refactor/remove-sentry-from-app-settings' into 'master'

Port of refactor/remove-sentry-from-app-settings to EE

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

app/helpers/application_settings_helper.rb

@@ -165,8 +165,6 @@ module ApplicationSettingsHelper
       :authorized_keys_enabled,
       :auto_devops_enabled,
       :auto_devops_domain,
-      :clientside_sentry_dsn,
-      :clientside_sentry_enabled,
       :container_registry_token_expire_delay,
       :default_artifacts_expire_in,
       :default_branch_protection,
@@ -235,8 +233,6 @@ module ApplicationSettingsHelper
       :restricted_visibility_levels,
       :rsa_key_restriction,
       :send_user_confirmation_email,
-      :sentry_dsn,
-      :sentry_enabled,
       :session_expire_delay,
       :shared_runners_enabled,
       :shared_runners_text,

app/models/application_setting.rb

@@ -30,6 +30,10 @@ class ApplicationSetting < ApplicationRecord
   ignore_column :circuitbreaker_check_interval
   ignore_column :koding_url
   ignore_column :koding_enabled
+  ignore_column :sentry_enabled
+  ignore_column :sentry_dsn
+  ignore_column :clientside_sentry_enabled
+  ignore_column :clientside_sentry_dsn
 
   cache_markdown_field :sign_in_text
   cache_markdown_field :help_page_text
@@ -75,14 +79,6 @@ class ApplicationSetting < ApplicationRecord
             presence: true,
             if: :recaptcha_enabled
 
-  validates :sentry_dsn,
-            presence: true,
-            if: :sentry_enabled
-
-  validates :clientside_sentry_dsn,
-            presence: true,
-            if: :clientside_sentry_enabled
-
   validates :akismet_api_key,
             presence: true,
             if: :akismet_enabled
@@ -264,7 +260,6 @@ class ApplicationSetting < ApplicationRecord
                  encode: true
 
   before_validation :ensure_uuid!
-  before_validation :strip_sentry_values
 
   before_save :ensure_runners_registration_token
   before_save :ensure_health_check_access_token

app/models/application_setting_implementation.rb

@@ -180,27 +180,6 @@ module ApplicationSettingImplementation
     super(levels&.map { |level| Gitlab::VisibilityLevel.level_value(level) })
   end
 
-  def strip_sentry_values
-    sentry_dsn.strip! if sentry_dsn.present?
-    clientside_sentry_dsn.strip! if clientside_sentry_dsn.present?
-  end
-
-  def sentry_enabled
-    Gitlab.config.sentry.enabled || read_attribute(:sentry_enabled)
-  end
-
-  def sentry_dsn
-    Gitlab.config.sentry.dsn || read_attribute(:sentry_dsn)
-  end
-
-  def clientside_sentry_enabled
-    Gitlab.config.sentry.enabled || read_attribute(:clientside_sentry_enabled)
-  end
-
-  def clientside_sentry_dsn
-    Gitlab.config.sentry.clientside_dsn || read_attribute(:clientside_sentry_dsn)
-  end
-
   def performance_bar_allowed_group
     Group.find_by_id(performance_bar_allowed_group_id)
   end

app/views/admin/application_settings/_logging.html.haml

-= form_for @application_setting, url: reporting_admin_application_settings_path(anchor: 'js-logging-settings'), html: { class: 'fieldset-form' } do |f|
-  = form_errors(@application_setting)
-
-  %p
-    %strong
-      NOTE:
-    These settings will be removed from the UI in a GitLab 12.0 release and made available within gitlab.yml.
-    In addition, you will be able to define a Sentry Environment to differentiate between multiple deployments. For example, development, staging, and production.
-
-  %fieldset
-    .form-group
-      .form-check
-        = f.check_box :sentry_enabled, class: 'form-check-input'
-        = f.label :sentry_enabled, class: 'form-check-label' do
-          Enable Sentry
-        .form-text.text-muted
-          %p This setting requires a restart to take effect.
-          Sentry is an error reporting and logging tool which is currently not shipped with GitLab, get it here:
-          %a{ href: 'https://getsentry.com', target: '_blank', rel: 'noopener noreferrer' } https://getsentry.com
-
-    .form-group
-      = f.label :sentry_dsn, 'Sentry DSN', class: 'label-bold'
-      = f.text_field :sentry_dsn, class: 'form-control'
-
-    .form-group
-      .form-check
-        = f.check_box :clientside_sentry_enabled, class: 'form-check-input'
-        = f.label :clientside_sentry_enabled, class: 'form-check-label' do
-          Enable Clientside Sentry
-        .form-text.text-muted
-          Sentry can also be used for reporting and logging clientside exceptions.
-          %a{ href: 'https://sentry.io/for/javascript/', target: '_blank', rel: 'noopener noreferrer' } https://sentry.io/for/javascript/
-
-    .form-group
-      = f.label :clientside_sentry_dsn, 'Clientside Sentry DSN', class: 'label-bold'
-      = f.text_field :clientside_sentry_dsn, class: 'form-control'
-
-  = f.submit 'Save changes', class: "btn btn-success"

app/views/admin/application_settings/reporting.html.haml

@@ -23,14 +23,3 @@
       = _('Set notification email for abuse reports.')
   .settings-content
     = render 'abuse'
-
-%section.settings.as-logging.no-animate#js-logging-settings{ class: ('expanded' if expanded_by_default?) }
-  .settings-header
-    %h4
-      = _('Error Reporting and Logging')
-    %button.btn.btn-default.js-settings-toggle{ type: 'button' }
-      = expanded_by_default? ? _('Collapse') : _('Expand')
-    %p
-      = _('Enable Sentry for error reporting and logging.')
-  .settings-content
-    = render 'logging'

app/views/layouts/_head.html.haml

@@ -46,7 +46,7 @@
     = yield :library_javascripts
 
   = javascript_include_tag locale_path unless I18n.locale == :en
-  = webpack_bundle_tag "raven" if Gitlab::CurrentSettings.clientside_sentry_enabled
+  = webpack_bundle_tag "raven" if Gitlab.config.sentry.enabled
 
   - if content_for?(:page_specific_javascripts)
     = yield :page_specific_javascripts

changelogs/unreleased/refactor-sentry.yml

+---
+title: Remove Sentry from application settings
+merge_request: 28447
+author: Roger Meier
+type: added

config/initializers/sentry.rb

@@ -3,18 +3,11 @@
 require 'gitlab/current_settings'
 
 def configure_sentry
-  # allow it to fail: it may do so when create_from_defaults is executed before migrations are actually done
-  begin
-    sentry_enabled = Gitlab::CurrentSettings.current_application_settings.sentry_enabled
-  rescue
-    sentry_enabled = false
-  end
-
-  if sentry_enabled
+  if Gitlab::Sentry.enabled?
     Raven.configure do |config|
-      config.dsn = Gitlab::CurrentSettings.current_application_settings.sentry_dsn
+      config.dsn = Gitlab.config.sentry.dsn
       config.release = Gitlab.revision
-      config.current_environment = Gitlab.config.sentry.environment.presence
+      config.current_environment = Gitlab.config.sentry.environment
 
       # Sanitize fields based on those sanitized from Rails.
       config.sanitize_fields = Rails.application.config.filter_parameters.map(&:to_s)

db/post_migrate/20190625184066_remove_sentry_from_application_settings.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveSentryFromApplicationSettings < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  SENTRY_ENABLED_COLUMNS = [
+    :sentry_enabled,
+    :clientside_sentry_enabled
+  ].freeze
+
+  SENTRY_DSN_COLUMNS = [
+    :sentry_dsn,
+    :clientside_sentry_dsn
+  ].freeze
+
+  disable_ddl_transaction!
+
+  def up
+    (SENTRY_ENABLED_COLUMNS + SENTRY_DSN_COLUMNS).each do |column|
+      remove_column(:application_settings, column) if column_exists?(:application_settings, column)
+    end
+  end
+
+  def down
+    SENTRY_ENABLED_COLUMNS.each do |column|
+      add_column_with_default(:application_settings, column, :boolean, default: false, allow_null: false) unless column_exists?(:application_settings, column)
+    end
+
+    SENTRY_DSN_COLUMNS.each do |column|
+      add_column(:application_settings, column, :string) unless column_exists?(:application_settings, column)
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20190620112608) do
+ActiveRecord::Schema.define(version: 20190625184066) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -94,8 +94,6 @@ ActiveRecord::Schema.define(version: 20190620112608) do
     t.boolean "akismet_enabled", default: false
     t.string "akismet_api_key"
     t.integer "metrics_sample_interval", default: 15
-    t.boolean "sentry_enabled", default: false
-    t.string "sentry_dsn"
     t.boolean "email_author_in_body", default: false
     t.integer "default_group_visibility"
     t.boolean "repository_checks_enabled", default: false
@@ -147,8 +145,6 @@ ActiveRecord::Schema.define(version: 20190620112608) do
     t.decimal "polling_interval_multiplier", default: "1.0", null: false
     t.boolean "elasticsearch_experimental_indexer"
     t.integer "cached_markdown_version"
-    t.boolean "clientside_sentry_enabled", default: false, null: false
-    t.string "clientside_sentry_dsn"
     t.boolean "check_namespace_plan", default: false, null: false
     t.integer "mirror_max_delay", default: 300, null: false
     t.integer "mirror_max_capacity", default: 100, null: false

doc/api/settings.md

@@ -152,8 +152,6 @@ are listed in the descriptions of the relevant settings.
 | `auto_devops_domain`                     | string           | no                                   | Specify a domain to use by default for every project's Auto Review Apps and Auto Deploy stages. |
 | `auto_devops_enabled`                    | boolean          | no                                   | Enable Auto DevOps for projects by default. It will automatically build, test, and deploy applications based on a predefined CI/CD configuration. |
 | `check_namespace_plan`                   | boolean          | no                                   | **(Premium)** Enabling this will make only licensed EE features available to projects if the project namespace's plan includes the feature or if the project is public. |
-| `clientside_sentry_dsn`                  | string           | required by: `clientside_sentry_enabled` | Clientside Sentry Data Source Name. |
-| `clientside_sentry_enabled`              | boolean          | no                                   | (**If enabled, requires:** `clientside_sentry_dsn`) Enable Sentry error reporting for the client side. |
 | `container_registry_token_expire_delay`  | integer          | no                                   | Container Registry token duration in minutes. |
 | `default_artifacts_expire_in`            | string           | no                                   | Set the default expiration time for each job's artifacts. |
 | `default_branch_protection`              | integer          | no                                   | Determine if developers can push to master. Can take: `0` _(not protected, both developers and maintainers can push new commits, force push, or delete the branch)_, `1` _(partially protected, developers and maintainers can push new commits, but cannot force push or delete the branch)_ or `2` _(fully protected, developers cannot push new commits, but maintainers can; no-one can force push or delete the branch)_ as a parameter. Default is `2`. |
@@ -249,8 +247,6 @@ are listed in the descriptions of the relevant settings.
 | `restricted_visibility_levels`           | array of strings | no                                   | Selected levels cannot be used by non-admin users for groups, projects or snippets. Can take `private`, `internal` and `public` as a parameter. Default is `null` which means there is no restriction. |
 | `rsa_key_restriction`                    | integer          | no                                   | The minimum allowed bit length of an uploaded RSA key. Default is `0` (no restriction). `-1` disables RSA keys. |
 | `send_user_confirmation_email`           | boolean          | no                                   | Send confirmation email on sign-up. |
-| `sentry_dsn`                             | string           | required by: `sentry_enabled`        | Sentry Data Source Name. |
-| `sentry_enabled`                         | boolean          | no                                   | (**If enabled, requires:** `sentry_dsn`) Sentry is an error reporting and logging tool which is currently not shipped with GitLab, available at <https://sentry.io>. |
 | `session_expire_delay`                   | integer          | no                                   | Session duration in minutes. GitLab restart is required to apply changes |
 | `shared_runners_enabled`                 | boolean          | no                                   | (**If enabled, requires:** `shared_runners_text` and `shared_runners_minutes`) Enable shared runners for new projects. |
 | `shared_runners_minutes`                 | integer          | required by: `shared_runners_enabled` | **(Premium)** Set the maximum number of pipeline minutes that a group can use on shared Runners per month. |

lib/api/settings.rb

@@ -36,10 +36,6 @@ module API
       given akismet_enabled: ->(val) { val } do
         requires :akismet_api_key, type: String, desc: 'Generate API key at http://www.akismet.com'
       end
-      optional :clientside_sentry_enabled, type: Boolean, desc: 'Sentry can also be used for reporting and logging clientside exceptions. https://sentry.io/for/javascript/'
-      given clientside_sentry_enabled: ->(val) { val } do
-        requires :clientside_sentry_dsn, type: String, desc: 'Clientside Sentry Data Source Name'
-      end
       optional :container_registry_token_expire_delay, type: Integer, desc: 'Authorization token duration (minutes)'
       optional :default_artifacts_expire_in, type: String, desc: "Set the default expiration time for each job's artifacts"
       optional :default_project_creation, type: Integer, values: ::Gitlab::Access.project_creation_values, desc: 'Determine if developers can create projects in the group'
@@ -114,10 +110,6 @@ module API
       end
       optional :restricted_visibility_levels, type: Array[String], desc: 'Selected levels cannot be used by non-admin users for groups, projects or snippets. If the public level is restricted, user profiles are only visible to logged in users.'
       optional :send_user_confirmation_email, type: Boolean, desc: 'Send confirmation email on sign-up'
-      optional :sentry_enabled, type: Boolean, desc: 'Sentry is an error reporting and logging tool which is currently not shipped with GitLab, get it here: https://getsentry.com'
-      given sentry_enabled: ->(val) { val } do
-        requires :sentry_dsn, type: String, desc: 'Sentry Data Source Name'
-      end
       optional :session_expire_delay, type: Integer, desc: 'Session duration in minutes. GitLab restart is required to apply changes.'
       optional :shared_runners_enabled, type: Boolean, desc: 'Enable shared runners for new projects'
       given shared_runners_enabled: ->(val) { val } do

lib/gitlab/gon_helper.rb

@@ -16,8 +16,8 @@ module Gitlab
       gon.shortcuts_path         = Gitlab::Routing.url_helpers.help_page_path('shortcuts')
       gon.user_color_scheme      = Gitlab::ColorSchemes.for_user(current_user).css_class
 
-      if Gitlab::CurrentSettings.clientside_sentry_enabled
-        gon.sentry_dsn           = Gitlab::CurrentSettings.clientside_sentry_dsn
+      if Gitlab.config.sentry.enabled
+        gon.sentry_dsn           = Gitlab.config.sentry.clientside_dsn
         gon.sentry_environment   = Gitlab.config.sentry.environment
       end
 

lib/gitlab/sentry.rb

@@ -4,7 +4,7 @@ module Gitlab
   module Sentry
     def self.enabled?
       (Rails.env.production? || Rails.env.development?) &&
-        Gitlab::CurrentSettings.sentry_enabled?
+        Gitlab.config.sentry.enabled
     end
 
     def self.context(current_user = nil)

locale/gitlab.pot

@@ -4733,9 +4733,6 @@ msgstr ""
 msgid "Enable SAML authentication for this group"
 msgstr ""
 
-msgid "Enable Sentry for error reporting and logging."
-msgstr ""
-
 msgid "Enable access to the Performance Bar for a given group."
 msgstr ""
 
@@ -5066,9 +5063,6 @@ msgstr ""
 msgid "Error"
 msgstr ""
 
-msgid "Error Reporting and Logging"
-msgstr ""
-
 msgid "Error Tracking"
 msgstr ""
 

spec/features/raven_js_spec.rb

@@ -10,7 +10,7 @@ describe 'RavenJS' do
   end
 
   it 'loads raven if sentry is enabled' do
-    stub_application_setting(clientside_sentry_dsn: 'https://key@domain.com/id', clientside_sentry_enabled: true)
+    stub_sentry_settings
 
     visit new_user_session_path
 

spec/models/application_setting_spec.rb

@@ -354,36 +354,6 @@ describe ApplicationSetting do
     end
   end
 
-  describe 'setting Sentry DSNs' do
-    context 'server DSN' do
-      it 'strips leading and trailing whitespace' do
-        subject.update(sentry_dsn: ' http://test ')
-
-        expect(subject.sentry_dsn).to eq('http://test')
-      end
-
-      it 'handles nil values' do
-        subject.update(sentry_dsn: nil)
-
-        expect(subject.sentry_dsn).to be_nil
-      end
-    end
-
-    context 'client-side DSN' do
-      it 'strips leading and trailing whitespace' do
-        subject.update(clientside_sentry_dsn: ' http://test ')
-
-        expect(subject.clientside_sentry_dsn).to eq('http://test')
-      end
-
-      it 'handles nil values' do
-        subject.update(clientside_sentry_dsn: nil)
-
-        expect(subject.clientside_sentry_dsn).to be_nil
-      end
-    end
-  end
-
   describe '#disabled_oauth_sign_in_sources=' do
     before do
       allow(Devise).to receive(:omniauth_providers).and_return([:github])

spec/requests/api/helpers_spec.rb

@@ -226,10 +226,8 @@ describe API::Helpers do
       allow_any_instance_of(self.class).to receive(:rack_response)
       allow(Gitlab::Sentry).to receive(:enabled?).and_return(true)
 
-      stub_application_setting(
-        sentry_enabled: true,
-        sentry_dsn: "dummy://12345:67890@sentry.localdomain/sentry/42"
-      )
+      stub_sentry_settings
+
       configure_sentry
       Raven.client.configuration.encoding = 'json'
     end

spec/support/helpers/stub_configuration.rb

@@ -81,6 +81,12 @@ module StubConfiguration
     allow(Gitlab.config.repositories).to receive(:storages).and_return(Settingslogic.new(messages))
   end
 
+  def stub_sentry_settings
+    allow(Gitlab.config.sentry).to receive(:enabled).and_return(true)
+    allow(Gitlab.config.sentry).to receive(:dsn).and_return('dummy://b44a0828b72421a6d8e99efd68d44fa8@example.com/42')
+    allow(Gitlab.config.sentry).to receive(:clientside_dsn).and_return('dummy://b44a0828b72421a6d8e99efd68d44fa8@example.com/43')
+  end
+
   def stub_kerberos_setting(messages)
     allow(Gitlab.config.kerberos).to receive_messages(to_settings(messages))
   end

spec/support/shared_examples/application_setting_examples.rb

@@ -249,43 +249,4 @@ RSpec.shared_examples 'application settings examples' do
 
     expect(setting.password_authentication_enabled_for_web?).to be_falsey
   end
-
-  describe 'sentry settings' do
-    context 'when the sentry settings are not set in gitlab.yml' do
-      it 'fallbacks to the settings in the database' do
-        setting.sentry_enabled = true
-        setting.sentry_dsn = 'https://b44a0828b72421a6d8e99efd68d44fa8@example.com/40'
-        setting.clientside_sentry_enabled = true
-        setting.clientside_sentry_dsn = 'https://b44a0828b72421a6d8e99efd68d44fa8@example.com/41'
-
-        allow(Gitlab.config.sentry).to receive(:enabled).and_return(false)
-        allow(Gitlab.config.sentry).to receive(:dsn).and_return(nil)
-        allow(Gitlab.config.sentry).to receive(:clientside_dsn).and_return(nil)
-
-        expect(setting.sentry_enabled).to eq true
-        expect(setting.sentry_dsn).to eq 'https://b44a0828b72421a6d8e99efd68d44fa8@example.com/40'
-        expect(setting.clientside_sentry_enabled).to eq true
-        expect(setting.clientside_sentry_dsn). to eq 'https://b44a0828b72421a6d8e99efd68d44fa8@example.com/41'
-      end
-    end
-
-    context 'when the sentry settings are set in gitlab.yml' do
-      it 'does not fallback to the settings in the database' do
-        setting.sentry_enabled = false
-        setting.sentry_dsn = 'https://b44a0828b72421a6d8e99efd68d44fa8@example.com/40'
-        setting.clientside_sentry_enabled = false
-        setting.clientside_sentry_dsn = 'https://b44a0828b72421a6d8e99efd68d44fa8@example.com/41'
-
-        allow(Gitlab.config.sentry).to receive(:enabled).and_return(true)
-        allow(Gitlab.config.sentry).to receive(:dsn).and_return('https://b44a0828b72421a6d8e99efd68d44fa8@example.com/42')
-        allow(Gitlab.config.sentry).to receive(:clientside_dsn).and_return('https://b44a0828b72421a6d8e99efd68d44fa8@example.com/43')
-
-        expect(setting).not_to receive(:read_attribute)
-        expect(setting.sentry_enabled).to eq true
-        expect(setting.sentry_dsn).to eq 'https://b44a0828b72421a6d8e99efd68d44fa8@example.com/42'
-        expect(setting.clientside_sentry_enabled).to eq true
-        expect(setting.clientside_sentry_dsn). to eq 'https://b44a0828b72421a6d8e99efd68d44fa8@example.com/43'
-      end
-    end
-  end
 end