diff options
27 files changed, 583 insertions, 336 deletions
diff --git a/app/assets/javascripts/raven/index.js b/app/assets/javascripts/sentry/index.js index 4dd0175e528..06e4e0aa507 100644 --- a/app/assets/javascripts/raven/index.js +++ b/app/assets/javascripts/sentry/index.js @@ -1,8 +1,8 @@ -import RavenConfig from './raven_config'; +import SentryConfig from './sentry_config'; const index = function index() { - RavenConfig.init({ - sentryDsn: gon.sentry_dsn, + SentryConfig.init({ + dsn: gon.sentry_dsn, currentUserId: gon.current_user_id, whitelistUrls: process.env.NODE_ENV === 'production' @@ -15,7 +15,7 @@ const index = function index() { }, }); - return RavenConfig; + return SentryConfig; }; index(); diff --git a/app/assets/javascripts/raven/raven_config.js b/app/assets/javascripts/sentry/sentry_config.js index 7259e0df104..bc3b2f16a6a 100644 --- a/app/assets/javascripts/raven/raven_config.js +++ b/app/assets/javascripts/sentry/sentry_config.js @@ -1,4 +1,4 @@ -import Raven from 'raven-js'; +import * as Sentry from '@sentry/browser'; import $ from 'jquery'; import { __ } from '~/locale'; @@ -26,7 +26,7 @@ const IGNORE_ERRORS = [ 'conduitPage', ]; -const IGNORE_URLS = [ +const BLACKLIST_URLS = [ // Facebook flakiness /graph\.facebook\.com/i, // Facebook blocked @@ -43,62 +43,62 @@ const IGNORE_URLS = [ /metrics\.itunes\.apple\.com\.edgesuite\.net\//i, ]; -const SAMPLE_RATE = 95; +const SAMPLE_RATE = 0.95; -const RavenConfig = { +const SentryConfig = { IGNORE_ERRORS, - IGNORE_URLS, + BLACKLIST_URLS, SAMPLE_RATE, init(options = {}) { this.options = options; this.configure(); - this.bindRavenErrors(); + this.bindSentryErrors(); if (this.options.currentUserId) this.setUser(); }, configure() { - Raven.config(this.options.sentryDsn, { - release: this.options.release, - tags: this.options.tags, - whitelistUrls: this.options.whitelistUrls, - environment: this.options.environment, - ignoreErrors: this.IGNORE_ERRORS, - ignoreUrls: this.IGNORE_URLS, - shouldSendCallback: this.shouldSendSample.bind(this), - }).install(); + const { dsn, release, tags, whitelistUrls, environment } = this.options; + Sentry.init({ + dsn, + release, + tags, + whitelistUrls, + environment, + ignoreErrors: this.IGNORE_ERRORS, // TODO: Remove in favor of https://gitlab.com/gitlab-org/gitlab/issues/35144 + blacklistUrls: this.BLACKLIST_URLS, + sampleRate: SAMPLE_RATE, + }); }, setUser() { - Raven.setUserContext({ + Sentry.setUser({ id: this.options.currentUserId, }); }, - bindRavenErrors() { - $(document).on('ajaxError.raven', this.handleRavenErrors); + bindSentryErrors() { + $(document).on('ajaxError.sentry', this.handleSentryErrors); }, - handleRavenErrors(event, req, config, err) { + handleSentryErrors(event, req, config, err) { const error = err || req.statusText; - const responseText = req.responseText || __('Unknown response text'); + const { responseText = __('Unknown response text') } = req; + const { type, url, data } = config; + const { status } = req; - Raven.captureMessage(error, { + Sentry.captureMessage(error, { extra: { - type: config.type, - url: config.url, - data: config.data, - status: req.status, + type, + url, + data, + status, response: responseText, error, event, }, }); }, - - shouldSendSample() { - return Math.random() * 100 <= this.SAMPLE_RATE; - }, }; -export default RavenConfig; +export default SentryConfig; diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 9dc4bd940d9..1f1c2db56a3 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -16,6 +16,9 @@ class MergeRequest < ApplicationRecord include ReactiveCaching include FromUnion include DeprecatedAssignee + include ShaAttribute + + sha_attribute :squash_commit_sha self.reactive_cache_key = ->(model) { [model.project.id, model.iid] } self.reactive_cache_refresh_interval = 10.minutes diff --git a/app/services/merge_requests/ff_merge_service.rb b/app/services/merge_requests/ff_merge_service.rb index 479e0fe6699..cfbee3e2ff6 100644 --- a/app/services/merge_requests/ff_merge_service.rb +++ b/app/services/merge_requests/ff_merge_service.rb @@ -20,6 +20,10 @@ module MergeRequests rescue StandardError => e raise MergeError, "Something went wrong during merge: #{e.message}" ensure + if merge_request.squash + merge_request.update_column(:squash_commit_sha, merge_request.in_progress_merge_commit_sha) + end + merge_request.update(in_progress_merge_commit_sha: nil) end end diff --git a/app/services/metrics/dashboard/custom_metric_embed_service.rb b/app/services/metrics/dashboard/custom_metric_embed_service.rb index 50f070989fc..dce713fbda7 100644 --- a/app/services/metrics/dashboard/custom_metric_embed_service.rb +++ b/app/services/metrics/dashboard/custom_metric_embed_service.rb @@ -40,7 +40,7 @@ module Metrics # All custom metrics are displayed on the system dashboard. # Nil is acceptable as we'll default to the system dashboard. def valid_dashboard?(dashboard) - dashboard.nil? || SystemDashboardService.system_dashboard?(dashboard) + dashboard.nil? || ::Metrics::Dashboard::SystemDashboardService.system_dashboard?(dashboard) end end diff --git a/app/views/layouts/_head.html.haml b/app/views/layouts/_head.html.haml index 6aebe42b74d..0c4c48447c9 100644 --- a/app/views/layouts/_head.html.haml +++ b/app/views/layouts/_head.html.haml @@ -57,7 +57,7 @@ = yield :library_javascripts = javascript_include_tag locale_path unless I18n.locale == :en - = webpack_bundle_tag "raven" if Gitlab.config.sentry.enabled + = webpack_bundle_tag "sentry" if Gitlab.config.sentry.enabled - if content_for?(:page_specific_javascripts) = yield :page_specific_javascripts diff --git a/changelogs/unreleased/26138-replace-raven-js-with-sentry-browser.yml b/changelogs/unreleased/26138-replace-raven-js-with-sentry-browser.yml new file mode 100644 index 00000000000..87db6917ab7 --- /dev/null +++ b/changelogs/unreleased/26138-replace-raven-js-with-sentry-browser.yml @@ -0,0 +1,5 @@ +--- +title: Replace raven-js with @sentry/browser +merge_request: 17715 +author: +type: changed diff --git a/changelogs/unreleased/ak-fix-undefined-value.yml b/changelogs/unreleased/ak-fix-undefined-value.yml new file mode 100644 index 00000000000..d85f5ddc23f --- /dev/null +++ b/changelogs/unreleased/ak-fix-undefined-value.yml @@ -0,0 +1,5 @@ +--- +title: Fix uninitialized constant SystemDashboardService +merge_request: 19453 +author: +type: fixed diff --git a/config/webpack.config.js b/config/webpack.config.js index 25fb6cc5f5a..1bcd8b68ac9 100644 --- a/config/webpack.config.js +++ b/config/webpack.config.js @@ -73,7 +73,7 @@ function generateEntries() { const manualEntries = { default: defaultEntries, - raven: './raven/index.js', + sentry: './sentry/index.js', }; return Object.assign(manualEntries, autoEntries); diff --git a/db/migrate/20191013100213_add_squash_commit_sha_to_merge_requests.rb b/db/migrate/20191013100213_add_squash_commit_sha_to_merge_requests.rb new file mode 100644 index 00000000000..0a58f0a89aa --- /dev/null +++ b/db/migrate/20191013100213_add_squash_commit_sha_to_merge_requests.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddSquashCommitShaToMergeRequests < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + add_column :merge_requests, :squash_commit_sha, :binary + end +end diff --git a/db/schema.rb b/db/schema.rb index 58cc20d6e40..066ae22cbd7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2334,6 +2334,7 @@ ActiveRecord::Schema.define(version: 2019_10_29_191901) do t.boolean "allow_maintainer_to_push" t.integer "state_id", limit: 2, default: 1, null: false t.string "rebase_jid" + t.binary "squash_commit_sha" t.index ["assignee_id"], name: "index_merge_requests_on_assignee_id" t.index ["author_id"], name: "index_merge_requests_on_author_id" t.index ["created_at"], name: "index_merge_requests_on_created_at" diff --git a/doc/api/commits.md b/doc/api/commits.md index 3927a4bbc62..3f2932f2666 100644 --- a/doc/api/commits.md +++ b/doc/api/commits.md @@ -670,6 +670,7 @@ Example response: "merge_status":"can_be_merged", "sha":"af5b13261899fb2c0db30abdd0af8b07cb44fdc5", "merge_commit_sha":null, + "squash_commit_sha":null, "user_notes_count":0, "discussion_locked":null, "should_remove_source_branch":null, diff --git a/doc/api/issues.md b/doc/api/issues.md index 0ddbb18ce92..ded412a7af0 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -1416,6 +1416,7 @@ Example response: "merge_status": "cannot_be_merged", "sha": "3b7b528e9353295c1c125dad281ac5b5deae5f12", "merge_commit_sha": null, + "squash_commit_sha": null, "discussion_locked": null, "should_remove_source_branch": null, "force_remove_source_branch": false, @@ -1546,6 +1547,7 @@ Example response: "merge_status": "unchecked", "sha": "5a62481d563af92b8e32d735f2fa63b94e806835", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "should_remove_source_branch": null, "force_remove_source_branch": false, diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index ef5cbeb015e..7074d0249ef 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -126,6 +126,7 @@ Parameters: "merge_status": "can_be_merged", "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -287,6 +288,7 @@ Parameters: "merge_status": "can_be_merged", "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -440,6 +442,7 @@ Parameters: "merge_status": "can_be_merged", "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -563,6 +566,7 @@ Parameters: "merge_error": null, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -769,6 +773,7 @@ Parameters: "subscribed" : true, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "changes_count": "1", "should_remove_source_branch": true, @@ -976,6 +981,7 @@ order for it to take effect: "merge_error": null, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -1129,6 +1135,7 @@ Must include at least one non-required attribute from above. "merge_error": null, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -1298,6 +1305,7 @@ Parameters: "merge_error": null, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -1470,6 +1478,7 @@ Parameters: "merge_error": null, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -1755,6 +1764,7 @@ Example response: "merge_status": "can_be_merged", "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -1900,6 +1910,7 @@ Example response: "merge_status": "can_be_merged", "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 1, "discussion_locked": null, "should_remove_source_branch": true, @@ -2061,6 +2072,7 @@ Example response: "subscribed": true, "sha": "8888888888888888888888888888888888888888", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 7, "changes_count": "1", "should_remove_source_branch": true, diff --git a/doc/api/search.md b/doc/api/search.md index f9bd5143018..8e20722052e 100644 --- a/doc/api/search.md +++ b/doc/api/search.md @@ -181,6 +181,7 @@ Example response: "merge_status": "can_be_merged", "sha": "78765a2d5e0a43585945c58e61ba2f822e4d090b", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 0, "discussion_locked": null, "should_remove_source_branch": null, @@ -583,6 +584,7 @@ Example response: "merge_status": "can_be_merged", "sha": "78765a2d5e0a43585945c58e61ba2f822e4d090b", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 0, "discussion_locked": null, "should_remove_source_branch": null, @@ -890,6 +892,7 @@ Example response: "merge_status": "can_be_merged", "sha": "78765a2d5e0a43585945c58e61ba2f822e4d090b", "merge_commit_sha": null, + "squash_commit_sha": null, "user_notes_count": 0, "discussion_locked": null, "should_remove_source_branch": null, diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index b9d55a9395c..39557cf4174 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -222,7 +222,7 @@ requirements. on the CI server. 1. Regressions and bugs are covered with tests that reduce the risk of the issue happening again. -1. Performance/scalability implications have been considered, addressed, and tested. +1. [Performance guidelines](../merge_request_performance_guidelines.md) have been followed. 1. [Documented](../documentation/index.md) in the `/doc` directory. 1. [Changelog entry added](../changelog.md), if necessary. 1. Reviewed by relevant (UX/FE/BE/tech writing) reviewers and all concerns are addressed. diff --git a/doc/development/merge_request_performance_guidelines.md b/doc/development/merge_request_performance_guidelines.md index 4456e5e6d18..2e80e813a4b 100644 --- a/doc/development/merge_request_performance_guidelines.md +++ b/doc/development/merge_request_performance_guidelines.md @@ -1,7 +1,9 @@ # Merge Request Performance Guidelines +Each new introduced merge request **should be performant by default**. + To ensure a merge request does not negatively impact performance of GitLab -_every_ merge request **must** adhere to the guidelines outlined in this +_every_ merge request **should** adhere to the guidelines outlined in this document. There are no exceptions to this rule unless specifically discussed with and agreed upon by backend maintainers and performance specialists. @@ -12,6 +14,19 @@ the following guides: - [Performance Guidelines](performance.md) - [What requires downtime?](what_requires_downtime.md) +## Definition + +The term `SHOULD` per the [RFC 2119](https://www.ietf.org/rfc/rfc2119.txt) means: + +> This word, or the adjective "RECOMMENDED", mean that there +> may exist valid reasons in particular circumstances to ignore a +> particular item, but the full implications must be understood and +> carefully weighed before choosing a different course. + +Ideally, each of these tradeoffs should be documented +in the separate issues, labelled accordingly and linked +to original issue and epic. + ## Impact Analysis **Summary:** think about the impact your merge request may have on performance @@ -44,6 +59,64 @@ should ask one of the merge request reviewers to review your changes. You can find a list of these reviewers at <https://about.gitlab.com/company/team/>. A reviewer in turn can request a performance specialist to review the changes. +## Think outside of the box + +Everyone has their own perception how the new feature is going to be used. +Always consider how users might be using the feature instead. Usually, +users test our features in a very unconventional way, +like by brute forcing or abusing edge conditions that we have. + +## Data set + +The data set that will be processed by the merge request should be known +and documented. The feature should clearly document what the expected +data set is for this feature to process, and what problems it might cause. + +If you would think about the following example that puts +a strong emphasis of data set being processed. +The problem is simple: you want to filter a list of files from +some git repository. Your feature requests a list of all files +from the repository and perform search for the set of files. +As an author you should in context of that problem consider +the following: + +1. What repositories are going to be supported? +1. How long it will take for big repositories like Linux kernel? +1. Is there something that we can do differently to not process such a + big data set? +1. Should we build some fail-safe mechanism to contain + computational complexity? Usually it is better to degrade + the service for a single user instead of all users. + +## Query plans and database structure + +The query plan can answer the questions whether we need additional +indexes, or whether we perform expensive filtering (i.e. using sequential scans). + +Each query plan should be run against substantional size of data set. +For example if you look for issues with specific conditions, +you should consider validating the query against +a small number (a few hundred) and a big number (100_000) of issues. +See how the query will behave if the result will be a few +and a few thousand. + +This is needed as we have users using GitLab for very big projects and +in a very unconventional way. Even, if it seems that it is unlikely +that such big data set will be used, it is still plausible that one +of our customers will have the problem with the feature. + +Understanding ahead of time how it is going to behave at scale even if we accept it, +is the desired outcome. We should always have a plan or understanding what it takes +to optimise feature to the magnitude of higher usage patterns. + +Every database structure should be optimised and sometimes even over-described +to be prepared to be easily extended. The hardest part after some point is +data migration. Migrating millions of rows will always be troublesome and +can have negative impact on application. + +To better understand how to get help with the query plan reviews +read this section on [how to prepare the merge request for a database review](https://docs.gitlab.com/ee/development/database_review.html#how-to-prepare-the-merge-request-for-a-database-review). + ## Query Counts **Summary:** a merge request **should not** increase the number of executed SQL @@ -172,3 +245,107 @@ Caching data per transaction can be done using `Gitlab::SafeRequestStore` to avoid having to remember to check `RequestStore.active?`). Caching data in Redis can be done using [Rails' caching system](https://guides.rubyonrails.org/caching_with_rails.html). + +## Pagination + +Each feature that renders a list of items as a table needs to include pagination. + +The main styles of pagination are: + +1. Offset-based pagination: user goes to a specific page, like 1. User sees the next page number, + and the total number of pages. This style is well supported by all components of GitLab. +1. Offset-based pagination, but without the count: user goes to a specific page, like 1. + User sees only the next page number, but does not see the total amount of pages. +1. Next page using keyset-based pagination: user can only go to next page, as we do not know how many pages + are available. +1. Infinite scrolling pagination: user scrolls the page and next items are loaded asynchronously. This is ideal, + as it has exact same benefits as the previous one. + +The ultimately scalable solution for pagination is to use Keyset-based pagination. +However, we don't have support for that at GitLab at that moment. You +can follow the progress looking at [API: Keyset Pagination +](https://gitlab.com/groups/gitlab-org/-/epics/2039). + +Take into consideration the following when choosing a pagination strategy: + +1. It is very inefficient to calculate amount of objects that pass the filtering, + this operation usually can take seconds, and can time out, +1. It is very inefficent to get entries for page at higher ordinals, like 1000. + The database has to sort and iterate all previous items, and this operation usually + can result in substantial load put on database. + +## Badge counters + +Counters should always be truncated. It means that we do not want to present +the exact number over some threshold. The reason for that is for the cases where we want +to calculate exact number of items, we effectively need to filter each of them for +the purpose of knowing the exact number of items matching. + +From ~UX perspective it is often acceptable to see that you have over 1000+ pipelines, +instead of that you have 40000+ pipelines, but at a tradeoff of loading page for 2s longer. + +An example of this pattern is the list of pipelines and jobs. We truncate numbers to `1000+`, +but we show an accurate number of running pipelines, which is the most interesting information. + +There's a helper method that can be used for that purpose - `NumbersHelper.limited_counter_with_delimiter` - +that accepts an upper limit of counting rows. + +In some cases it is desired that badge counters are loaded asynchronously. +This can speed up the initial page load and give a better user experience overall. + +## Application/misuse limits + +Every new feature should have safe usage quotas introduced. +The quota should be optimised to a level that we consider the feature to +be performant and usable for the user, but **not limiting**. + +**We want the features to be fully usable for the users.** +**However, we want to ensure that the feature will continue to perform well if used at its limit** +**and it will not cause availability issues.** + +Consider that it is always better to start with some kind of limitation, +instead of later introducing a breaking change that would result in some +workflows breaking. + +The intent is to provide a safe usage pattern for the feature, +as our implementation decisions are optimised for the given data set. +Our feature limits should reflect the optimisations that we introduced. + +The intent of quotas could be different: + +1. We want to provide higher quotas for higher tiers of features: + we want to provide on GitLab.com more capabilities for different tiers, +1. We want to prevent misuse of the feature: someone accidentially creates + 10000 deploy tokens, because of a broken API script, +1. We want to prevent abuse of the feature: someone purposely creates + a 10000 pipelines to take advantage of the system. + +Examples: + +1. Pipeline Schedules: It is very unlikely that user will want to create + more than 50 schedules. + In such cases it is rather expected that this is either misuse + or abuse of the feature. Lack of the upper limit can result + in service degredation as the system will try to process all schedules + assigned the the project. + +1. GitLab CI includes: We started with the limit of maximum of 50 nested includes. + We understood that performance of the feature was acceptable at that level. + We received a request from the community that the limit is too small. + We had a time to understand the customer requirement, and implement an additional + fail-safe mechanism (time-based one) to increase the limit 100, and if needed increase it + further without negative impact on availability of the feature and GitLab. + +## Usage of feature flags + +Each feature that has performance critical elements or has a known performance deficiency +needs to come with feature flag to disable it. + +The feature flag makes our team more happy, because they can monitor the system and +quickly react without our users noticing the problem. + +Performance deficiencies should be addressed right away after we merge initial +changes. + +Read more about when and how feature flags should be used in +[Feature flags in GitLab development](https://docs.gitlab.com/ee/development/feature_flags/process.html#feature-flags-in-gitlab-development). diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 9faae460527..93982d7eefc 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -755,6 +755,7 @@ module API end expose :diff_head_sha, as: :sha expose :merge_commit_sha + expose :squash_commit_sha expose :discussion_locked expose :should_remove_source_branch?, as: :should_remove_source_branch expose :force_remove_source_branch?, as: :force_remove_source_branch diff --git a/package.json b/package.json index 7deb8401f6d..fa070578ada 100644 --- a/package.json +++ b/package.json @@ -40,6 +40,7 @@ "@gitlab/svgs": "^1.80.0", "@gitlab/ui": "7.3.0", "@gitlab/visual-review-tools": "1.0.3", + "@sentry/browser": "^5.7.1", "apollo-cache-inmemory": "^1.5.1", "apollo-client": "^2.6.4", "apollo-link": "^1.2.11", @@ -109,7 +110,6 @@ "prosemirror-markdown": "^1.3.0", "prosemirror-model": "^1.6.4", "raphael": "^2.2.7", - "raven-js": "^3.22.1", "raw-loader": "^3.1.0", "sanitize-html": "^1.16.1", "select2": "3.5.2-browserify", diff --git a/spec/features/raven_js_spec.rb b/spec/features/raven_js_spec.rb deleted file mode 100644 index 38699f0cc1b..00000000000 --- a/spec/features/raven_js_spec.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe 'RavenJS' do - let(:raven_path) { '/raven.chunk.js' } - - it 'does not load raven if sentry is disabled' do - visit new_user_session_path - - expect(has_requested_raven).to eq(false) - end - - it 'loads raven if sentry is enabled' do - stub_sentry_settings - - visit new_user_session_path - - expect(has_requested_raven).to eq(true) - end - - def has_requested_raven - page.all('script', visible: false).one? do |elm| - elm[:src] =~ /#{raven_path}$/ - end - end -end diff --git a/spec/features/sentry_js_spec.rb b/spec/features/sentry_js_spec.rb new file mode 100644 index 00000000000..b39c4f0a0ae --- /dev/null +++ b/spec/features/sentry_js_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Sentry' do + let(:sentry_path) { '/sentry.chunk.js' } + + it 'does not load sentry if sentry is disabled' do + allow(Gitlab.config.sentry).to receive(:enabled).and_return(false) + visit new_user_session_path + + expect(has_requested_sentry).to eq(false) + end + + it 'loads sentry if sentry is enabled' do + stub_sentry_settings + + visit new_user_session_path + + expect(has_requested_sentry).to eq(true) + end + + def has_requested_sentry + page.all('script', visible: false).one? do |elm| + elm[:src] =~ /#{sentry_path}$/ + end + end +end diff --git a/spec/javascripts/raven/index_spec.js b/spec/frontend/sentry/index_spec.js index 6b9fe923624..82b6c445d96 100644 --- a/spec/javascripts/raven/index_spec.js +++ b/spec/frontend/sentry/index_spec.js @@ -1,8 +1,8 @@ -import RavenConfig from '~/raven/raven_config'; -import index from '~/raven/index'; +import SentryConfig from '~/sentry/sentry_config'; +import index from '~/sentry/index'; -describe('RavenConfig options', () => { - const sentryDsn = 'sentryDsn'; +describe('SentryConfig options', () => { + const dsn = 'https://123@sentry.gitlab.test/123'; const currentUserId = 'currentUserId'; const gitlabUrl = 'gitlabUrl'; const environment = 'test'; @@ -11,7 +11,7 @@ describe('RavenConfig options', () => { beforeEach(() => { window.gon = { - sentry_dsn: sentryDsn, + sentry_dsn: dsn, sentry_environment: environment, current_user_id: currentUserId, gitlab_url: gitlabUrl, @@ -20,14 +20,14 @@ describe('RavenConfig options', () => { process.env.HEAD_COMMIT_SHA = revision; - spyOn(RavenConfig, 'init'); + jest.spyOn(SentryConfig, 'init').mockImplementation(); indexReturnValue = index(); }); it('should init with .sentryDsn, .currentUserId, .whitelistUrls and environment', () => { - expect(RavenConfig.init).toHaveBeenCalledWith({ - sentryDsn, + expect(SentryConfig.init).toHaveBeenCalledWith({ + dsn, currentUserId, whitelistUrls: [gitlabUrl, 'webpack-internal://'], environment, @@ -38,7 +38,7 @@ describe('RavenConfig options', () => { }); }); - it('should return RavenConfig', () => { - expect(indexReturnValue).toBe(RavenConfig); + it('should return SentryConfig', () => { + expect(indexReturnValue).toBe(SentryConfig); }); }); diff --git a/spec/frontend/sentry/sentry_config_spec.js b/spec/frontend/sentry/sentry_config_spec.js new file mode 100644 index 00000000000..62b8bbd50a2 --- /dev/null +++ b/spec/frontend/sentry/sentry_config_spec.js @@ -0,0 +1,214 @@ +import * as Sentry from '@sentry/browser'; +import SentryConfig from '~/sentry/sentry_config'; + +describe('SentryConfig', () => { + describe('IGNORE_ERRORS', () => { + it('should be an array of strings', () => { + const areStrings = SentryConfig.IGNORE_ERRORS.every(error => typeof error === 'string'); + + expect(areStrings).toBe(true); + }); + }); + + describe('BLACKLIST_URLS', () => { + it('should be an array of regexps', () => { + const areRegExps = SentryConfig.BLACKLIST_URLS.every(url => url instanceof RegExp); + + expect(areRegExps).toBe(true); + }); + }); + + describe('SAMPLE_RATE', () => { + it('should be a finite number', () => { + expect(typeof SentryConfig.SAMPLE_RATE).toEqual('number'); + }); + }); + + describe('init', () => { + const options = { + currentUserId: 1, + }; + + beforeEach(() => { + jest.spyOn(SentryConfig, 'configure'); + jest.spyOn(SentryConfig, 'bindSentryErrors'); + jest.spyOn(SentryConfig, 'setUser'); + + SentryConfig.init(options); + }); + + it('should set the options property', () => { + expect(SentryConfig.options).toEqual(options); + }); + + it('should call the configure method', () => { + expect(SentryConfig.configure).toHaveBeenCalled(); + }); + + it('should call the error bindings method', () => { + expect(SentryConfig.bindSentryErrors).toHaveBeenCalled(); + }); + + it('should call setUser', () => { + expect(SentryConfig.setUser).toHaveBeenCalled(); + }); + + it('should not call setUser if there is no current user ID', () => { + jest.clearAllMocks(); + + options.currentUserId = undefined; + + SentryConfig.init(options); + + expect(SentryConfig.setUser).not.toHaveBeenCalled(); + }); + }); + + describe('configure', () => { + const sentryConfig = {}; + const options = { + dsn: 'https://123@sentry.gitlab.test/123', + whitelistUrls: ['//gitlabUrl', 'webpack-internal://'], + environment: 'test', + release: 'revision', + tags: { + revision: 'revision', + }, + }; + + beforeEach(() => { + jest.spyOn(Sentry, 'init').mockImplementation(); + + sentryConfig.options = options; + sentryConfig.IGNORE_ERRORS = 'ignore_errors'; + sentryConfig.BLACKLIST_URLS = 'blacklist_urls'; + + SentryConfig.configure.call(sentryConfig); + }); + + it('should call Sentry.init', () => { + expect(Sentry.init).toHaveBeenCalledWith({ + dsn: options.dsn, + release: options.release, + tags: options.tags, + sampleRate: 0.95, + whitelistUrls: options.whitelistUrls, + environment: 'test', + ignoreErrors: sentryConfig.IGNORE_ERRORS, + blacklistUrls: sentryConfig.BLACKLIST_URLS, + }); + }); + + it('should set environment from options', () => { + sentryConfig.options.environment = 'development'; + + SentryConfig.configure.call(sentryConfig); + + expect(Sentry.init).toHaveBeenCalledWith({ + dsn: options.dsn, + release: options.release, + tags: options.tags, + sampleRate: 0.95, + whitelistUrls: options.whitelistUrls, + environment: 'development', + ignoreErrors: sentryConfig.IGNORE_ERRORS, + blacklistUrls: sentryConfig.BLACKLIST_URLS, + }); + }); + }); + + describe('setUser', () => { + let sentryConfig; + + beforeEach(() => { + sentryConfig = { options: { currentUserId: 1 } }; + jest.spyOn(Sentry, 'setUser'); + + SentryConfig.setUser.call(sentryConfig); + }); + + it('should call .setUser', () => { + expect(Sentry.setUser).toHaveBeenCalledWith({ + id: sentryConfig.options.currentUserId, + }); + }); + }); + + describe('handleSentryErrors', () => { + let event; + let req; + let config; + let err; + + beforeEach(() => { + event = {}; + req = { status: 'status', responseText: 'Unknown response text', statusText: 'statusText' }; + config = { type: 'type', url: 'url', data: 'data' }; + err = {}; + + jest.spyOn(Sentry, 'captureMessage'); + + SentryConfig.handleSentryErrors(event, req, config, err); + }); + + it('should call Sentry.captureMessage', () => { + expect(Sentry.captureMessage).toHaveBeenCalledWith(err, { + extra: { + type: config.type, + url: config.url, + data: config.data, + status: req.status, + response: req.responseText, + error: err, + event, + }, + }); + }); + + describe('if no err is provided', () => { + beforeEach(() => { + jest.clearAllMocks(); + + SentryConfig.handleSentryErrors(event, req, config); + }); + + it('should use req.statusText as the error value', () => { + expect(Sentry.captureMessage).toHaveBeenCalledWith(req.statusText, { + extra: { + type: config.type, + url: config.url, + data: config.data, + status: req.status, + response: req.responseText, + error: req.statusText, + event, + }, + }); + }); + }); + + describe('if no req.responseText is provided', () => { + beforeEach(() => { + req.responseText = undefined; + + jest.clearAllMocks(); + + SentryConfig.handleSentryErrors(event, req, config, err); + }); + + it('should use `Unknown response text` as the response', () => { + expect(Sentry.captureMessage).toHaveBeenCalledWith(err, { + extra: { + type: config.type, + url: config.url, + data: config.data, + status: req.status, + response: 'Unknown response text', + error: err, + event, + }, + }); + }); + }); + }); +}); diff --git a/spec/javascripts/raven/raven_config_spec.js b/spec/javascripts/raven/raven_config_spec.js deleted file mode 100644 index af634a0c196..00000000000 --- a/spec/javascripts/raven/raven_config_spec.js +++ /dev/null @@ -1,254 +0,0 @@ -import Raven from 'raven-js'; -import RavenConfig from '~/raven/raven_config'; - -describe('RavenConfig', () => { - describe('IGNORE_ERRORS', () => { - it('should be an array of strings', () => { - const areStrings = RavenConfig.IGNORE_ERRORS.every(error => typeof error === 'string'); - - expect(areStrings).toBe(true); - }); - }); - - describe('IGNORE_URLS', () => { - it('should be an array of regexps', () => { - const areRegExps = RavenConfig.IGNORE_URLS.every(url => url instanceof RegExp); - - expect(areRegExps).toBe(true); - }); - }); - - describe('SAMPLE_RATE', () => { - it('should be a finite number', () => { - expect(typeof RavenConfig.SAMPLE_RATE).toEqual('number'); - }); - }); - - describe('init', () => { - const options = { - currentUserId: 1, - }; - - beforeEach(() => { - spyOn(RavenConfig, 'configure'); - spyOn(RavenConfig, 'bindRavenErrors'); - spyOn(RavenConfig, 'setUser'); - - RavenConfig.init(options); - }); - - it('should set the options property', () => { - expect(RavenConfig.options).toEqual(options); - }); - - it('should call the configure method', () => { - expect(RavenConfig.configure).toHaveBeenCalled(); - }); - - it('should call the error bindings method', () => { - expect(RavenConfig.bindRavenErrors).toHaveBeenCalled(); - }); - - it('should call setUser', () => { - expect(RavenConfig.setUser).toHaveBeenCalled(); - }); - - it('should not call setUser if there is no current user ID', () => { - RavenConfig.setUser.calls.reset(); - - options.currentUserId = undefined; - - RavenConfig.init(options); - - expect(RavenConfig.setUser).not.toHaveBeenCalled(); - }); - }); - - describe('configure', () => { - let raven; - let ravenConfig; - const options = { - sentryDsn: '//sentryDsn', - whitelistUrls: ['//gitlabUrl', 'webpack-internal://'], - environment: 'test', - release: 'revision', - tags: { - revision: 'revision', - }, - }; - - beforeEach(() => { - ravenConfig = jasmine.createSpyObj('ravenConfig', ['shouldSendSample']); - raven = jasmine.createSpyObj('raven', ['install']); - - spyOn(Raven, 'config').and.returnValue(raven); - - ravenConfig.options = options; - ravenConfig.IGNORE_ERRORS = 'ignore_errors'; - ravenConfig.IGNORE_URLS = 'ignore_urls'; - - RavenConfig.configure.call(ravenConfig); - }); - - it('should call Raven.config', () => { - expect(Raven.config).toHaveBeenCalledWith(options.sentryDsn, { - release: options.release, - tags: options.tags, - whitelistUrls: options.whitelistUrls, - environment: 'test', - ignoreErrors: ravenConfig.IGNORE_ERRORS, - ignoreUrls: ravenConfig.IGNORE_URLS, - shouldSendCallback: jasmine.any(Function), - }); - }); - - it('should call Raven.install', () => { - expect(raven.install).toHaveBeenCalled(); - }); - - it('should set environment from options', () => { - ravenConfig.options.environment = 'development'; - - RavenConfig.configure.call(ravenConfig); - - expect(Raven.config).toHaveBeenCalledWith(options.sentryDsn, { - release: options.release, - tags: options.tags, - whitelistUrls: options.whitelistUrls, - environment: 'development', - ignoreErrors: ravenConfig.IGNORE_ERRORS, - ignoreUrls: ravenConfig.IGNORE_URLS, - shouldSendCallback: jasmine.any(Function), - }); - }); - }); - - describe('setUser', () => { - let ravenConfig; - - beforeEach(() => { - ravenConfig = { options: { currentUserId: 1 } }; - spyOn(Raven, 'setUserContext'); - - RavenConfig.setUser.call(ravenConfig); - }); - - it('should call .setUserContext', function() { - expect(Raven.setUserContext).toHaveBeenCalledWith({ - id: ravenConfig.options.currentUserId, - }); - }); - }); - - describe('handleRavenErrors', () => { - let event; - let req; - let config; - let err; - - beforeEach(() => { - event = {}; - req = { status: 'status', responseText: 'responseText', statusText: 'statusText' }; - config = { type: 'type', url: 'url', data: 'data' }; - err = {}; - - spyOn(Raven, 'captureMessage'); - - RavenConfig.handleRavenErrors(event, req, config, err); - }); - - it('should call Raven.captureMessage', () => { - expect(Raven.captureMessage).toHaveBeenCalledWith(err, { - extra: { - type: config.type, - url: config.url, - data: config.data, - status: req.status, - response: req.responseText, - error: err, - event, - }, - }); - }); - - describe('if no err is provided', () => { - beforeEach(() => { - Raven.captureMessage.calls.reset(); - - RavenConfig.handleRavenErrors(event, req, config); - }); - - it('should use req.statusText as the error value', () => { - expect(Raven.captureMessage).toHaveBeenCalledWith(req.statusText, { - extra: { - type: config.type, - url: config.url, - data: config.data, - status: req.status, - response: req.responseText, - error: req.statusText, - event, - }, - }); - }); - }); - - describe('if no req.responseText is provided', () => { - beforeEach(() => { - req.responseText = undefined; - - Raven.captureMessage.calls.reset(); - - RavenConfig.handleRavenErrors(event, req, config, err); - }); - - it('should use `Unknown response text` as the response', () => { - expect(Raven.captureMessage).toHaveBeenCalledWith(err, { - extra: { - type: config.type, - url: config.url, - data: config.data, - status: req.status, - response: 'Unknown response text', - error: err, - event, - }, - }); - }); - }); - }); - - describe('shouldSendSample', () => { - let randomNumber; - - beforeEach(() => { - RavenConfig.SAMPLE_RATE = 50; - - spyOn(Math, 'random').and.callFake(() => randomNumber); - }); - - it('should call Math.random', () => { - RavenConfig.shouldSendSample(); - - expect(Math.random).toHaveBeenCalled(); - }); - - it('should return true if the sample rate is greater than the random number * 100', () => { - randomNumber = 0.1; - - expect(RavenConfig.shouldSendSample()).toBe(true); - }); - - it('should return false if the sample rate is less than the random number * 100', () => { - randomNumber = 0.9; - - expect(RavenConfig.shouldSendSample()).toBe(false); - }); - - it('should return true if the sample rate is equal to the random number * 100', () => { - randomNumber = 0.5; - - expect(RavenConfig.shouldSendSample()).toBe(true); - }); - }); -}); diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 3835b23123f..04fe985cdb5 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -185,6 +185,7 @@ MergeRequest: - merge_when_pipeline_succeeds - merge_user_id - merge_commit_sha +- squash_commit_sha - in_progress_merge_commit_sha - lock_version - milestone_id diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index c622add7f8f..3f28ad7433e 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1637,6 +1637,21 @@ describe API::MergeRequests do expect(source_repository.branch_exists?(source_branch)).to be_falsy end end + + context "performing a ff-merge with squash" do + let(:merge_request) { create(:merge_request, :rebased, source_project: project, squash: true) } + + before do + project.update(merge_requests_ff_only_enabled: true) + end + + it "records the squash commit SHA and returns it in the response" do + put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user) + + expect(response).to have_gitlab_http_status(200) + expect(json_response['squash_commit_sha'].length).to eq(40) + end + end end describe "GET /projects/:id/merge_requests/:merge_request_iid/merge_ref", :clean_gitlab_redis_shared_state do diff --git a/yarn.lock b/yarn.lock index 515ac2b3549..a5e727dae8d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1191,6 +1191,58 @@ consola "^2.3.0" node-fetch "^2.3.0" +"@sentry/browser@^5.7.1": + version "5.7.1" + resolved "https://registry.yarnpkg.com/@sentry/browser/-/browser-5.7.1.tgz#1f8435e2a325d7a09f830065ebce40a2b3c708a4" + integrity sha512-K0x1XhsHS8PPdtlVOLrKZyYvi5Vexs9WApdd214bO6KaGF296gJvH1mG8XOY0+7aA5i2A7T3ttcaJNDYS49lzw== + dependencies: + "@sentry/core" "5.7.1" + "@sentry/types" "5.7.1" + "@sentry/utils" "5.7.1" + tslib "^1.9.3" + +"@sentry/core@5.7.1": + version "5.7.1" + resolved "https://registry.yarnpkg.com/@sentry/core/-/core-5.7.1.tgz#3eb2b7662cac68245931ee939ec809bf7a639d0e" + integrity sha512-AOn3k3uVWh2VyajcHbV9Ta4ieDIeLckfo7UMLM+CTk2kt7C89SayDGayJMSsIrsZlL4qxBoLB9QY4W2FgAGJrg== + dependencies: + "@sentry/hub" "5.7.1" + "@sentry/minimal" "5.7.1" + "@sentry/types" "5.7.1" + "@sentry/utils" "5.7.1" + tslib "^1.9.3" + +"@sentry/hub@5.7.1": + version "5.7.1" + resolved "https://registry.yarnpkg.com/@sentry/hub/-/hub-5.7.1.tgz#a52acd9fead7f3779d96e9965c6978aecc8b9cad" + integrity sha512-evGh323WR073WSBCg/RkhlUmCQyzU0xzBzCZPscvcoy5hd4SsLE6t9Zin+WACHB9JFsRQIDwNDn+D+pj3yKsig== + dependencies: + "@sentry/types" "5.7.1" + "@sentry/utils" "5.7.1" + tslib "^1.9.3" + +"@sentry/minimal@5.7.1": + version "5.7.1" + resolved "https://registry.yarnpkg.com/@sentry/minimal/-/minimal-5.7.1.tgz#56afc537737586929e25349765e37a367958c1e1" + integrity sha512-nS/Dg+jWAZtcxQW8wKbkkw4dYvF6uyY/vDiz/jFCaux0LX0uhgXAC9gMOJmgJ/tYBLJ64l0ca5LzpZa7BMJQ0g== + dependencies: + "@sentry/hub" "5.7.1" + "@sentry/types" "5.7.1" + tslib "^1.9.3" + +"@sentry/types@5.7.1": + version "5.7.1" + resolved "https://registry.yarnpkg.com/@sentry/types/-/types-5.7.1.tgz#4c4c1d4d891b6b8c2c3c7b367d306a8b1350f090" + integrity sha512-tbUnTYlSliXvnou5D4C8Zr+7/wJrHLbpYX1YkLXuIJRU0NSi81bHMroAuHWILcQKWhVjaV/HZzr7Y/hhWtbXVQ== + +"@sentry/utils@5.7.1": + version "5.7.1" + resolved "https://registry.yarnpkg.com/@sentry/utils/-/utils-5.7.1.tgz#cf37ad55f78e317665cd8680f202d307fa77f1d0" + integrity sha512-nhirUKj/qFLsR1i9kJ5BRvNyzdx/E2vorIsukuDrbo8e3iZ11JMgCOVrmC8Eq9YkHBqgwX4UnrPumjFyvGMZ2Q== + dependencies: + "@sentry/types" "5.7.1" + tslib "^1.9.3" + "@types/anymatch@*": version "1.3.0" resolved "https://registry.yarnpkg.com/@types/anymatch/-/anymatch-1.3.0.tgz#d1d55958d1fccc5527d4aba29fc9c4b942f563ff" @@ -9989,11 +10041,6 @@ raphael@^2.2.7: dependencies: eve-raphael "0.5.0" -raven-js@^3.22.1: - version "3.22.1" - resolved "https://registry.yarnpkg.com/raven-js/-/raven-js-3.22.1.tgz#1117f00dfefaa427ef6e1a7d50bbb1fb998a24da" - integrity sha512-2Y8czUl5a9usbvXbpV8a+GPAiDXjxQjaHImZL0TyJWI5k5jV/6o+wceaBAg9g6RpO9OOJp0/w2mMs6pBoqOyDA== - raw-body@2.4.0: version "2.4.0" resolved "https://registry.yarnpkg.com/raw-body/-/raw-body-2.4.0.tgz#a1ce6fb9c9bc356ca52e89256ab59059e13d0332" |