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

Remove Sentry settings from application settings

See merge request gitlab-org/gitlab-ce!28447

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"
@@ -93,8 +93,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
@@ -135,8 +133,6 @@ ActiveRecord::Schema.define(version: 20190620112608) do
     t.string "uuid"
     t.decimal "polling_interval_multiplier", default: "1.0", null: false
     t.integer "cached_markdown_version"
-    t.boolean "clientside_sentry_enabled", default: false, null: false
-    t.string "clientside_sentry_dsn"
     t.boolean "prometheus_metrics_enabled", default: true, null: false
     t.boolean "help_page_hide_commercial_content", default: false
     t.string "help_page_support_url"

doc/api/settings.md

@@ -142,8 +142,6 @@ are listed in the descriptions of the relevant settings.
 | `authorized_keys_enabled`                | boolean          | no                                   | By default, we write to the `authorized_keys` file to support Git over SSH without additional configuration. GitLab can be optimized to authenticate SSH keys via the database file. Only disable this if you have configured your OpenSSH server to use the AuthorizedKeysCommand. |
 | `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. |
-| `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`. |
@@ -212,8 +210,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`) Enable shared runners for new projects. |
 | `shared_runners_text`                    | string           | required by: `shared_runners_enabled` | Shared runners text. |

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

@@ -3804,9 +3804,6 @@ msgstr ""
 msgid "Enable HTML emails"
 msgstr ""
-msgid "Enable Sentry for error reporting and logging."
-msgstr ""
-
 msgid "Enable access to the Performance Bar for a given group."
 msgstr ""
@@ -4026,9 +4023,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