diff options
51 files changed, 1001 insertions, 320 deletions
diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index ef66c8aeb2e..16f8783c9a0 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -446,6 +446,8 @@ .frontend:rules:bundle-size-review: rules: + - <<: *if-not-canonical-namespace + when: never - if: '$DANGER_GITLAB_API_TOKEN && $CI_MERGE_REQUEST_IID && ($CI_MERGE_REQUEST_TARGET_BRANCH_NAME == "master" || $CI_MERGE_REQUEST_TARGET_BRANCH_NAME == "main")' changes: *frontend-patterns allow_failure: true diff --git a/app/assets/javascripts/environments/mixins/environments_pagination_api_mixin.js b/app/assets/javascripts/environments/mixins/environments_pagination_api_mixin.js index b62fe196a6f..a76c8e445ed 100644 --- a/app/assets/javascripts/environments/mixins/environments_pagination_api_mixin.js +++ b/app/assets/javascripts/environments/mixins/environments_pagination_api_mixin.js @@ -16,6 +16,7 @@ export default { let params = { scope, page: '1', + nested: true, }; params = this.onChangeWithFilter(params); @@ -27,6 +28,7 @@ export default { /* URLS parameters are strings, we need to parse to match types */ let params = { page: Number(page).toString(), + nested: true, }; if (this.scope) { diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5f14d95ffed..379da90827a 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -29,7 +29,6 @@ class ApplicationController < ActionController::Base before_action :validate_user_service_ticket! before_action :check_password_expiration, if: :html_request? before_action :ldap_security_check - around_action :sentry_context before_action :default_headers before_action :default_cache_headers before_action :add_gon_variables, if: :html_request? @@ -171,7 +170,12 @@ class ApplicationController < ActionController::Base end def log_exception(exception) - Gitlab::ErrorTracking.track_exception(exception) + # At this point, the controller already exits set_current_context around + # block. To maintain the context while handling error exception, we need to + # set the context again + set_current_context do + Gitlab::ErrorTracking.track_exception(exception) + end backtrace_cleaner = request.env["action_dispatch.backtrace_cleaner"] application_trace = ActionDispatch::ExceptionWrapper.new(backtrace_cleaner, exception).application_trace @@ -528,10 +532,6 @@ class ApplicationController < ActionController::Base .execute end - def sentry_context(&block) - Gitlab::ErrorTracking.with_context(current_user, &block) - end - def allow_gitaly_ref_name_caching ::Gitlab::GitalyClient.allow_ref_name_caching do yield diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb index 152f07b4c16..53064041ab8 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -4,6 +4,8 @@ class GraphqlController < ApplicationController # Unauthenticated users have access to the API for public data skip_before_action :authenticate_user! + WHITELIST_HEADER = 'HTTP_X_GITLAB_QUERY_WHITELIST_ISSUE' + # If a user is using their session to access GraphQL, we need to have session # storage, since the admin-mode check is session wide. # We can't enable this for anonymous users because that would cause users using @@ -21,6 +23,7 @@ class GraphqlController < ApplicationController before_action(only: [:execute]) { authenticate_sessionless_user!(:api) } before_action :set_user_last_activity before_action :track_vs_code_usage + before_action :whitelist_query! # Since we deactivate authentication from the main ApplicationController and # defer it to :authorize_access_api!, we need to override the bypass session @@ -59,6 +62,14 @@ class GraphqlController < ApplicationController private + # Tests may mark some queries as exempt from query limits + def whitelist_query! + whitelist_issue = request.headers[WHITELIST_HEADER] + return unless whitelist_issue + + Gitlab::QueryLimiting.whitelist(whitelist_issue) + end + def set_user_last_activity return unless current_user @@ -66,7 +77,8 @@ class GraphqlController < ApplicationController end def track_vs_code_usage - Gitlab::UsageDataCounters::VSCodeExtensionActivityUniqueCounter.track_api_request_when_trackable(user_agent: request.user_agent, user: current_user) + Gitlab::UsageDataCounters::VSCodeExtensionActivityUniqueCounter + .track_api_request_when_trackable(user_agent: request.user_agent, user: current_user) end def execute_multiplex diff --git a/app/models/iterations/cadence.rb b/app/models/iterations/cadence.rb deleted file mode 100644 index 7945fb1785a..00000000000 --- a/app/models/iterations/cadence.rb +++ /dev/null @@ -1,8 +0,0 @@ -# frozen_string_literal: true - -# Placeholder class for model that is implemented in EE -class Iterations::Cadence < ApplicationRecord - self.table_name = 'iterations_cadences' -end - -Iterations::Cadence.prepend_if_ee('::EE::Iterations::Cadence') diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 6bd31e26748..317cd11a69d 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -181,7 +181,7 @@ module MergeRequests } if exception - Gitlab::ErrorTracking.with_context(current_user) do + Gitlab::ApplicationContext.with_context(user: current_user) do Gitlab::ErrorTracking.track_exception(exception, data) end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 38df3ee0e43..217679e733c 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -47,6 +47,7 @@ module MergeRequests handle_draft_status_change(merge_request, changed_fields) track_title_and_desc_edits(changed_fields) + track_discussion_lock_toggle(merge_request, changed_fields) notify_if_labels_added(merge_request, old_labels) notify_if_mentions_added(merge_request, old_mentioned_users) @@ -95,6 +96,16 @@ module MergeRequests end end + def track_discussion_lock_toggle(merge_request, changed_fields) + return unless changed_fields.include?('discussion_locked') + + if merge_request.discussion_locked + merge_request_activity_counter.track_discussion_locked_action(user: current_user) + else + merge_request_activity_counter.track_discussion_unlocked_action(user: current_user) + end + end + def notify_if_labels_added(merge_request, old_labels) added_labels = merge_request.labels - old_labels diff --git a/changelogs/unreleased/292824-track-mr-lock-changes.yml b/changelogs/unreleased/292824-track-mr-lock-changes.yml new file mode 100644 index 00000000000..89102528aed --- /dev/null +++ b/changelogs/unreleased/292824-track-mr-lock-changes.yml @@ -0,0 +1,5 @@ +--- +title: Track usage pings when MR gets locked/unlocked +merge_request: 55069 +author: +type: other diff --git a/changelogs/unreleased/expose_project_container_registry_url.yml b/changelogs/unreleased/expose_project_container_registry_url.yml new file mode 100644 index 00000000000..923d14ed5a6 --- /dev/null +++ b/changelogs/unreleased/expose_project_container_registry_url.yml @@ -0,0 +1,5 @@ +--- +title: Expose container_registry_image_prefix to project API +merge_request: 54090 +author: Mathieu Parent +type: added diff --git a/changelogs/unreleased/qmnguyen0711-846-sentry-merge-sentry-s-contexts-and-users-into-applicatio.yml b/changelogs/unreleased/qmnguyen0711-846-sentry-merge-sentry-s-contexts-and-users-into-applicatio.yml new file mode 100644 index 00000000000..bd4a97d264f --- /dev/null +++ b/changelogs/unreleased/qmnguyen0711-846-sentry-merge-sentry-s-contexts-and-users-into-applicatio.yml @@ -0,0 +1,5 @@ +--- +title: Merge Sentry's contexts into Gitlab::ApplicationContext +merge_request: 53691 +author: +type: changed diff --git a/changelogs/unreleased/show-nested-env.yml b/changelogs/unreleased/show-nested-env.yml new file mode 100644 index 00000000000..7305cf17975 --- /dev/null +++ b/changelogs/unreleased/show-nested-env.yml @@ -0,0 +1,5 @@ +--- +title: Show nested environments when change tab, page +merge_request: 55167 +author: +type: changed diff --git a/config/feature_flags/development/usage_data_i_code_review_user_mr_discussion_locked.yml b/config/feature_flags/development/usage_data_i_code_review_user_mr_discussion_locked.yml new file mode 100644 index 00000000000..e6e81f48028 --- /dev/null +++ b/config/feature_flags/development/usage_data_i_code_review_user_mr_discussion_locked.yml @@ -0,0 +1,8 @@ +--- +name: usage_data_i_code_review_user_mr_discussion_locked +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55069 +rollout_issue_url: +milestone: '13.10' +type: development +group: group::code review +default_enabled: true diff --git a/config/feature_flags/development/usage_data_i_code_review_user_mr_discussion_unlocked.yml b/config/feature_flags/development/usage_data_i_code_review_user_mr_discussion_unlocked.yml new file mode 100644 index 00000000000..03ec6cde34b --- /dev/null +++ b/config/feature_flags/development/usage_data_i_code_review_user_mr_discussion_unlocked.yml @@ -0,0 +1,8 @@ +--- +name: usage_data_i_code_review_user_mr_discussion_unlocked +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55069 +rollout_issue_url: +milestone: '13.10' +type: development +group: group::code review +default_enabled: true diff --git a/config/metrics/counts_28d/20210301103859_i_code_review_user_mr_discussion_locked_monthly.yml b/config/metrics/counts_28d/20210301103859_i_code_review_user_mr_discussion_locked_monthly.yml new file mode 100644 index 00000000000..9ad7ff81231 --- /dev/null +++ b/config/metrics/counts_28d/20210301103859_i_code_review_user_mr_discussion_locked_monthly.yml @@ -0,0 +1,20 @@ +--- +key_path: redis_hll_counters.code_review.i_code_review_user_mr_discussion_locked_monthly +description: Count of unique users per month who locked a MR +product_section: dev +product_stage: create +product_group: group::code review +product_category: code_review +value_type: number +status: implemented +milestone: "13.10" +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55069 +time_frame: 28d +data_source: redis_hll +distribution: +- ce +- ee +tier: +- free +- premium +- ultimate diff --git a/config/metrics/counts_28d/20210301103925_i_code_review_user_mr_discussion_unlocked_monthly.yml b/config/metrics/counts_28d/20210301103925_i_code_review_user_mr_discussion_unlocked_monthly.yml new file mode 100644 index 00000000000..707a2fc76d1 --- /dev/null +++ b/config/metrics/counts_28d/20210301103925_i_code_review_user_mr_discussion_unlocked_monthly.yml @@ -0,0 +1,20 @@ +--- +key_path: redis_hll_counters.code_review.i_code_review_user_mr_discussion_unlocked_monthly +description: Count of unique users per month who unlocked a MR +product_section: dev +product_stage: create +product_group: group::code review +product_category: code_review +value_type: number +status: implemented +milestone: "13.10" +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55069 +time_frame: 28d +data_source: redis_hll +distribution: +- ce +- ee +tier: +- free +- premium +- ultimate diff --git a/config/metrics/counts_7d/20210302105258_i_code_review_user_mr_discussion_unlocked_weekly.yml b/config/metrics/counts_7d/20210302105258_i_code_review_user_mr_discussion_unlocked_weekly.yml new file mode 100644 index 00000000000..80471ed836a --- /dev/null +++ b/config/metrics/counts_7d/20210302105258_i_code_review_user_mr_discussion_unlocked_weekly.yml @@ -0,0 +1,20 @@ +--- +key_path: redis_hll_counters.code_review.i_code_review_user_mr_discussion_unlocked_weekly +description: Count of unique users per week who unlocked a MR +product_section: dev +product_stage: create +product_group: group::code review +product_category: code_review +value_type: number +status: implemented +milestone: "13.10" +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55069 +time_frame: 7d +data_source: redis_hll +distribution: +- ce +- ee +tier: +- free +- premium +- ultimate diff --git a/config/metrics/counts_7d/20210302105318_i_code_review_user_mr_discussion_locked_weekly.yml b/config/metrics/counts_7d/20210302105318_i_code_review_user_mr_discussion_locked_weekly.yml new file mode 100644 index 00000000000..2295fb75a48 --- /dev/null +++ b/config/metrics/counts_7d/20210302105318_i_code_review_user_mr_discussion_locked_weekly.yml @@ -0,0 +1,20 @@ +--- +key_path: redis_hll_counters.code_review.i_code_review_user_mr_discussion_locked_weekly +description: Count of unique users per week who locked a MR +product_section: dev +product_stage: create +product_group: group::code review +product_category: code_review +value_type: number +status: implemented +milestone: "13.10" +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55069 +time_frame: 7d +data_source: redis_hll +distribution: +- ce +- ee +tier: +- free +- premium +- ultimate diff --git a/doc/administration/pages/index.md b/doc/administration/pages/index.md index b552c2ba957..40e9d5f6780 100644 --- a/doc/administration/pages/index.md +++ b/doc/administration/pages/index.md @@ -862,7 +862,7 @@ In installations from source: remote_directory: "pages" # The bucket name connection: provider: AWS # Only AWS supported at the moment - aws_access_key_id: AWS_ACESS_KEY_ID + aws_access_key_id: AWS_ACCESS_KEY_ID aws_secret_access_key: AWS_SECRET_ACCESS_KEY region: eu-central-1 ``` @@ -969,14 +969,15 @@ to define the explicit address that the GitLab Pages daemon should listen on: gitlab_pages['listen_proxy'] = '127.0.0.1:8090' ``` -### 404 error after transferring project to a different group or user +### 404 error after transferring the project to a different group or user, or changing project path If you encounter a `404 Not Found` error a Pages site after transferring a project to -another group or user, you must trigger a domain configuration update for Pages. To do -so, write something in the `.update` file. The Pages daemon monitors for changes to this -file, and reloads the configuration when changes occur. +another group or user, or changing project path, you must trigger a domain configuration +update for Pages. To do so, write something in the `.update` file. The Pages daemon +monitors for changes to this file, and reloads the configuration when changes occur. -Use this example to fix a `404 Not Found` error after transferring a project with Pages: +Use this example to fix a `404 Not Found` error after transferring a project or changing +a project path with Pages: ```shell date > /var/opt/gitlab/gitlab-rails/shared/pages/.update diff --git a/doc/administration/pseudonymizer.md b/doc/administration/pseudonymizer.md index 5f1272b1f4a..4aa73212e43 100644 --- a/doc/administration/pseudonymizer.md +++ b/doc/administration/pseudonymizer.md @@ -24,7 +24,7 @@ be textually exported. This ensures that: ## Configuration -To configure the pseudonymizer, you need to: +To configure the Pseudonymizer, you need to: - Provide a manifest file that describes which fields should be included or pseudonymized ([example `manifest.yml` file](https://gitlab.com/gitlab-org/gitlab/tree/master/config/pseudonymizer.yml)). @@ -87,7 +87,7 @@ To configure the pseudonymizer, you need to: ## Usage -You can optionally run the pseudonymizer using the following environment variables: +You can optionally run the Pseudonymizer using the following environment variables: - `PSEUDONYMIZER_OUTPUT_DIR` - where to store the output CSV files (defaults to `/tmp`) - `PSEUDONYMIZER_BATCH` - the batch size when querying the DB (defaults to `100000`) diff --git a/doc/administration/terraform_state.md b/doc/administration/terraform_state.md index 6e5d6b274b6..58a18d49417 100644 --- a/doc/administration/terraform_state.md +++ b/doc/administration/terraform_state.md @@ -121,7 +121,7 @@ See [the available connection settings for different providers](object_storage.m remote_directory: "terraform" # The bucket name connection: provider: AWS # Only AWS supported at the moment - aws_access_key_id: AWS_ACESS_KEY_ID + aws_access_key_id: AWS_ACCESS_KEY_ID aws_secret_access_key: AWS_SECRET_ACCESS_KEY region: eu-central-1 ``` diff --git a/doc/administration/uploads.md b/doc/administration/uploads.md index d3a9777775f..949687cfa0a 100644 --- a/doc/administration/uploads.md +++ b/doc/administration/uploads.md @@ -146,7 +146,7 @@ _The uploads are stored by default in remote_directory: "uploads" # The bucket name connection: provider: AWS # Only AWS supported at the moment - aws_access_key_id: AWS_ACESS_KEY_ID + aws_access_key_id: AWS_ACCESS_KEY_ID aws_secret_access_key: AWS_SECRET_ACCESS_KEY region: eu-central-1 ``` diff --git a/doc/api/projects.md b/doc/api/projects.md index b8687314697..da883d376b0 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -179,6 +179,7 @@ When the user is authenticated and `simple` is not set this returns something li "packages_size": 0, "snippets_size": 0 }, + "container_registry_image_prefix": "registry.example.com/diaspora/diaspora-client", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -284,6 +285,7 @@ When the user is authenticated and `simple` is not set this returns something li "packages_size": 0, "snippets_size": 0 }, + "container_registry_image_prefix": "registry.example.com/brightbox/puppet", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -439,6 +441,7 @@ GET /users/:user_id/projects "packages_size": 0, "snippets_size": 0 }, + "container_registry_image_prefix": "registry.example.com/diaspora/diaspora-client", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -544,6 +547,7 @@ GET /users/:user_id/projects "packages_size": 0, "snippets_size": 0 }, + "container_registry_image_prefix": "registry.example.com/brightbox/puppet", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -658,6 +662,7 @@ Example response: "lfs_objects_size": 0, "job_artifacts_size": 0 }, + "container_registry_image_prefix": "registry.example.com/diaspora/diaspora-client", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -758,6 +763,7 @@ Example response: "lfs_objects_size": 0, "job_artifacts_size": 0 }, + "container_registry_image_prefix": "registry.example.com/brightbox/puppet", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -921,6 +927,7 @@ GET /projects/:id "packages_size": 0, "snippets_size": 0 }, + "container_registry_image_prefix": "registry.example.com/diaspora/diaspora-client", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -1373,6 +1380,7 @@ Example responses: "merge_method": "merge", "autoclose_referenced_issues": true, "suggestion_commit_message": null, + "container_registry_image_prefix": "registry.example.com/diaspora/diaspora-project-site", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -1467,6 +1475,7 @@ Example response: "merge_method": "merge", "autoclose_referenced_issues": true, "suggestion_commit_message": null, + "container_registry_image_prefix": "registry.example.com/diaspora/diaspora-project-site", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -1559,6 +1568,7 @@ Example response: "merge_method": "merge", "autoclose_referenced_issues": true, "suggestion_commit_message": null, + "container_registry_image_prefix": "registry.example.com/diaspora/diaspora-project-site", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -1745,6 +1755,7 @@ Example response: "merge_method": "merge", "autoclose_referenced_issues": true, "suggestion_commit_message": null, + "container_registry_image_prefix": "registry.example.com/diaspora/diaspora-project-site", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -1858,6 +1869,7 @@ Example response: "merge_method": "merge", "autoclose_referenced_issues": true, "suggestion_commit_message": null, + "container_registry_image_prefix": "registry.example.com/diaspora/diaspora-project-site", "_links": { "self": "http://example.com/api/v4/projects", "issues": "http://example.com/api/v4/projects/1/issues", @@ -2354,6 +2366,7 @@ Example response: "avatar_url": null, "web_url": "https://gitlab.example.com/groups/cute-cats" }, + "container_registry_image_prefix": "registry.example.com/cute-cats/hello-world", "_links": { "self": "https://gitlab.example.com/api/v4/projects/7", "issues": "https://gitlab.example.com/api/v4/projects/7/issues", diff --git a/doc/development/usage_ping/dictionary.md b/doc/development/usage_ping/dictionary.md index 512f31903c8..93ce26fceb5 100644 --- a/doc/development/usage_ping/dictionary.md +++ b/doc/development/usage_ping/dictionary.md @@ -13384,6 +13384,86 @@ Count of unique users per week|month who merged a MR | `tier` | | | `skip_validation` | true | +## `redis_hll_counters.code_review.i_code_review_user_mr_discussion_locked_monthly` + +Count of unique users per month who locked a MR + +| field | value | +| --- | --- | +| `key_path` | **`redis_hll_counters.code_review.i_code_review_user_mr_discussion_locked_monthly`** | +| `product_section` | dev | +| `product_stage` | create | +| `product_group` | `group::code review` | +| `product_category` | `code_review` | +| `value_type` | number | +| `status` | implemented | +| `milestone` | 13.10 | +| `introduced_by_url` | [Introduced by](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55069) | +| `time_frame` | 28d | +| `data_source` | Redis_hll | +| `distribution` | ce, ee | +| `tier` | free, premium, ultimate | + +## `redis_hll_counters.code_review.i_code_review_user_mr_discussion_locked_weekly` + +Count of unique users per week who locked a MR + +| field | value | +| --- | --- | +| `key_path` | **`redis_hll_counters.code_review.i_code_review_user_mr_discussion_locked_weekly`** | +| `product_section` | dev | +| `product_stage` | create | +| `product_group` | `group::code review` | +| `product_category` | `code_review` | +| `value_type` | number | +| `status` | implemented | +| `milestone` | 13.10 | +| `introduced_by_url` | [Introduced by](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55069) | +| `time_frame` | 7d | +| `data_source` | Redis_hll | +| `distribution` | ce, ee | +| `tier` | free, premium, ultimate | + +## `redis_hll_counters.code_review.i_code_review_user_mr_discussion_unlocked_monthly` + +Count of unique users per month who unlocked a MR + +| field | value | +| --- | --- | +| `key_path` | **`redis_hll_counters.code_review.i_code_review_user_mr_discussion_unlocked_monthly`** | +| `product_section` | dev | +| `product_stage` | create | +| `product_group` | `group::code review` | +| `product_category` | `code_review` | +| `value_type` | number | +| `status` | implemented | +| `milestone` | 13.10 | +| `introduced_by_url` | [Introduced by](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55069) | +| `time_frame` | 28d | +| `data_source` | Redis_hll | +| `distribution` | ce, ee | +| `tier` | free, premium, ultimate | + +## `redis_hll_counters.code_review.i_code_review_user_mr_discussion_unlocked_weekly` + +Count of unique users per week who unlocked a MR + +| field | value | +| --- | --- | +| `key_path` | **`redis_hll_counters.code_review.i_code_review_user_mr_discussion_unlocked_weekly`** | +| `product_section` | dev | +| `product_stage` | create | +| `product_group` | `group::code review` | +| `product_category` | `code_review` | +| `value_type` | number | +| `status` | implemented | +| `milestone` | 13.10 | +| `introduced_by_url` | [Introduced by](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55069) | +| `time_frame` | 7d | +| `data_source` | Redis_hll | +| `distribution` | ce, ee | +| `tier` | free, premium, ultimate | + ## `redis_hll_counters.code_review.i_code_review_user_publish_review_monthly` Missing description diff --git a/lib/api/entities/project.rb b/lib/api/entities/project.rb index 6ad6123a20e..e332e5e40fa 100644 --- a/lib/api/entities/project.rb +++ b/lib/api/entities/project.rb @@ -5,6 +5,8 @@ module API class Project < BasicProjectDetails include ::API::Helpers::RelatedResourcesHelpers + expose :container_registry_url, as: :container_registry_image_prefix, if: -> (_, _) { Gitlab.config.registry.enabled } + expose :_links do expose :self do |project| expose_url(api_v4_projects_path(id: project.id)) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 0abb21c9831..9db4a03c5b9 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -467,7 +467,7 @@ module API def handle_api_exception(exception) if report_exception?(exception) define_params_for_grape_middleware - Gitlab::ErrorTracking.with_context(current_user) do + Gitlab::ApplicationContext.with_context(user: current_user) do Gitlab::ErrorTracking.track_exception(exception) end end diff --git a/lib/gitlab/application_context.rb b/lib/gitlab/application_context.rb index cefe983848c..5d07610feb5 100644 --- a/lib/gitlab/application_context.rb +++ b/lib/gitlab/application_context.rb @@ -27,8 +27,12 @@ module Gitlab Labkit::Context.push(application_context.to_lazy_hash) end + def self.current + Labkit::Context.current.to_h + end + def self.current_context_include?(attribute_name) - Labkit::Context.current.to_h.include?(Labkit::Context.log_key(attribute_name)) + current.include?(Labkit::Context.log_key(attribute_name)) end def initialize(**args) diff --git a/lib/gitlab/error_tracking.rb b/lib/gitlab/error_tracking.rb index 1a8e5aaf07a..dfed8db8df0 100644 --- a/lib/gitlab/error_tracking.rb +++ b/lib/gitlab/error_tracking.rb @@ -27,33 +27,16 @@ module Gitlab config.sanitize_fields = Rails.application.config.filter_parameters.map(&:to_s) config.processors << ::Gitlab::ErrorTracking::Processor::SidekiqProcessor config.processors << ::Gitlab::ErrorTracking::Processor::GrpcErrorProcessor + config.processors << ::Gitlab::ErrorTracking::Processor::ContextPayloadProcessor # Sanitize authentication headers config.sanitize_http_headers = %w[Authorization Private-Token] - config.tags = extra_tags_from_env.merge(program: Gitlab.process_name) config.before_send = method(:before_send) yield config if block_given? end end - def with_context(current_user = nil) - last_user_context = Raven.context.user - - user_context = { - id: current_user&.id, - email: current_user&.email, - username: current_user&.username - }.compact - - Raven.tags_context(default_tags) - Raven.user_context(user_context) - - yield - ensure - Raven.user_context(last_user_context) - end - # This should be used when you want to passthrough exception handling: # rescue and raise to be catched in upper layers of the application. # @@ -118,37 +101,20 @@ module Gitlab end def process_exception(exception, sentry: false, logging: true, extra:) - exception.try(:sentry_extra_data)&.tap do |data| - extra = extra.merge(data) if data.is_a?(Hash) - end - - extra = sanitize_request_parameters(extra) + context_payload = Gitlab::ErrorTracking::ContextPayloadGenerator.generate(exception, extra) if sentry && Raven.configuration.server - Raven.capture_exception(exception, tags: default_tags, extra: extra) + Raven.capture_exception(exception, **context_payload) end if logging - # TODO: this logic could migrate into `Gitlab::ExceptionLogFormatter` - # and we could also flatten deep nested hashes if required for search - # (e.g. if `extra` includes hash of hashes). - # In the current implementation, we don't flatten multi-level folded hashes. - log_hash = {} - Raven.context.tags.each { |name, value| log_hash["tags.#{name}"] = value } - Raven.context.user.each { |name, value| log_hash["user.#{name}"] = value } - Raven.context.extra.merge(extra).each { |name, value| log_hash["extra.#{name}"] = value } - - Gitlab::ExceptionLogFormatter.format!(exception, log_hash) + formatter = Gitlab::ErrorTracking::LogFormatter.new + log_hash = formatter.generate_log(exception, context_payload) Gitlab::ErrorTracking::Logger.error(log_hash) end end - def sanitize_request_parameters(parameters) - filter = ActiveSupport::ParameterFilter.new(::Rails.application.config.filter_parameters) - filter.filter(parameters) - end - def sentry_dsn return unless Rails.env.production? || Rails.env.development? return unless Gitlab.config.sentry.enabled @@ -160,22 +126,6 @@ module Gitlab Rails.env.development? || Rails.env.test? end - def default_tags - { - Labkit::Correlation::CorrelationId::LOG_KEY.to_sym => Labkit::Correlation::CorrelationId.current_id, - locale: I18n.locale - } - end - - # Static tags that are set on application start - def extra_tags_from_env - Gitlab::Json.parse(ENV.fetch('GITLAB_SENTRY_EXTRA_TAGS', '{}')).to_hash - rescue => e - Gitlab::AppLogger.debug("GITLAB_SENTRY_EXTRA_TAGS could not be parsed as JSON: #{e.class.name}: #{e.message}") - - {} - end - # Group common, mostly non-actionable exceptions by type and message, # rather than cause def custom_fingerprinting(event, ex) diff --git a/lib/gitlab/error_tracking/context_payload_generator.rb b/lib/gitlab/error_tracking/context_payload_generator.rb new file mode 100644 index 00000000000..c99283b3d20 --- /dev/null +++ b/lib/gitlab/error_tracking/context_payload_generator.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +module Gitlab + module ErrorTracking + class ContextPayloadGenerator + def self.generate(exception, extra = {}) + new.generate(exception, extra) + end + + def generate(exception, extra = {}) + { + extra: extra_payload(exception, extra), + tags: tags_payload, + user: user_payload + } + end + + private + + def extra_payload(exception, extra) + inline_extra = exception.try(:sentry_extra_data) + if inline_extra.present? && inline_extra.is_a?(Hash) + extra = extra.merge(inline_extra) + end + + sanitize_request_parameters(extra) + end + + def sanitize_request_parameters(parameters) + filter = ActiveSupport::ParameterFilter.new(::Rails.application.config.filter_parameters) + filter.filter(parameters) + end + + def tags_payload + extra_tags_from_env.merge!( + program: Gitlab.process_name, + locale: I18n.locale, + feature_category: current_context['meta.feature_category'], + Labkit::Correlation::CorrelationId::LOG_KEY.to_sym => Labkit::Correlation::CorrelationId.current_id + ) + end + + def user_payload + { + username: current_context['meta.user'] + } + end + + # Static tags that are set on application start + def extra_tags_from_env + Gitlab::Json.parse(ENV.fetch('GITLAB_SENTRY_EXTRA_TAGS', '{}')).to_hash + rescue => e + Gitlab::AppLogger.debug("GITLAB_SENTRY_EXTRA_TAGS could not be parsed as JSON: #{e.class.name}: #{e.message}") + + {} + end + + def current_context + # In case Gitlab::ErrorTracking is used when the app starts + return {} unless defined?(::Gitlab::ApplicationContext) + + ::Gitlab::ApplicationContext.current.to_h + end + end + end +end diff --git a/lib/gitlab/error_tracking/log_formatter.rb b/lib/gitlab/error_tracking/log_formatter.rb new file mode 100644 index 00000000000..d004c4e20bb --- /dev/null +++ b/lib/gitlab/error_tracking/log_formatter.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module Gitlab + module ErrorTracking + class LogFormatter + # Note: all the accesses to Raven's contexts here are to keep the + # backward-compatibility to Sentry's built-in integrations. In future, + # they can be removed. + def generate_log(exception, context_payload) + payload = {} + + Gitlab::ExceptionLogFormatter.format!(exception, payload) + append_user_to_log!(payload, context_payload) + append_tags_to_log!(payload, context_payload) + append_extra_to_log!(payload, context_payload) + + payload + end + + private + + def append_user_to_log!(payload, context_payload) + user_context = Raven.context.user.merge(context_payload[:user]) + user_context.each do |key, value| + payload["user.#{key}"] = value + end + end + + def append_tags_to_log!(payload, context_payload) + tags_context = Raven.context.tags.merge(context_payload[:tags]) + tags_context.each do |key, value| + payload["tags.#{key}"] = value + end + end + + def append_extra_to_log!(payload, context_payload) + extra = Raven.context.extra.merge(context_payload[:extra]) + extra = extra.except(:server) + + # The extra value for sidekiq is a hash whose keys are strings. + if extra[:sidekiq].is_a?(Hash) && extra[:sidekiq].key?('args') + sidekiq_extra = extra.delete(:sidekiq) + sidekiq_extra['args'] = Gitlab::ErrorTracking::Processor::SidekiqProcessor.loggable_arguments( + sidekiq_extra['args'], sidekiq_extra['class'] + ) + payload["extra.sidekiq"] = sidekiq_extra + end + + extra.each do |key, value| + payload["extra.#{key}"] = value + end + end + end + end +end diff --git a/lib/gitlab/error_tracking/processor/context_payload_processor.rb b/lib/gitlab/error_tracking/processor/context_payload_processor.rb new file mode 100644 index 00000000000..5185205e94e --- /dev/null +++ b/lib/gitlab/error_tracking/processor/context_payload_processor.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Gitlab + module ErrorTracking + module Processor + class ContextPayloadProcessor < ::Raven::Processor + # This processor is added to inject application context into Sentry + # events generated by Sentry built-in integrations. When the + # integrations are re-implemented and use Gitlab::ErrorTracking, this + # processor should be removed. + def process(payload) + context_payload = Gitlab::ErrorTracking::ContextPayloadGenerator.generate(nil, {}) + payload.deep_merge!(context_payload) + end + end + end + end +end diff --git a/lib/gitlab/exception_log_formatter.rb b/lib/gitlab/exception_log_formatter.rb index 6aff8f909f3..9898651c9e3 100644 --- a/lib/gitlab/exception_log_formatter.rb +++ b/lib/gitlab/exception_log_formatter.rb @@ -12,16 +12,6 @@ module Gitlab 'exception.message' => exception.message ) - payload.delete('extra.server') - - payload['extra.sidekiq'].tap do |value| - if value.is_a?(Hash) && value.key?('args') - value = value.dup - payload['extra.sidekiq']['args'] = Gitlab::ErrorTracking::Processor::SidekiqProcessor - .loggable_arguments(value['args'], value['class']) - end - end - if exception.backtrace payload['exception.backtrace'] = Rails.backtrace_cleaner.clean(exception.backtrace) end diff --git a/lib/gitlab/usage_data_counters/aggregated_metrics/code_review.yml b/lib/gitlab/usage_data_counters/aggregated_metrics/code_review.yml index 4f86d500164..c856d0a5eee 100644 --- a/lib/gitlab/usage_data_counters/aggregated_metrics/code_review.yml +++ b/lib/gitlab/usage_data_counters/aggregated_metrics/code_review.yml @@ -42,7 +42,9 @@ 'i_code_review_user_approval_rule_edited', 'i_code_review_user_vs_code_api_request', 'i_code_review_user_toggled_task_item_status', - 'i_code_review_user_create_mr_from_issue' + 'i_code_review_user_create_mr_from_issue', + 'i_code_review_user_mr_discussion_locked', + 'i_code_review_user_mr_discussion_unlocked' ] - name: code_review_category_monthly_active_users operator: OR @@ -78,7 +80,9 @@ 'i_code_review_user_approval_rule_deleted', 'i_code_review_user_approval_rule_edited', 'i_code_review_user_toggled_task_item_status', - 'i_code_review_user_create_mr_from_issue' + 'i_code_review_user_create_mr_from_issue', + 'i_code_review_user_mr_discussion_locked', + 'i_code_review_user_mr_discussion_unlocked' ] - name: code_review_extension_category_monthly_active_users operator: OR diff --git a/lib/gitlab/usage_data_counters/known_events/code_review_events.yml b/lib/gitlab/usage_data_counters/known_events/code_review_events.yml index d657c5487d7..21613740142 100644 --- a/lib/gitlab/usage_data_counters/known_events/code_review_events.yml +++ b/lib/gitlab/usage_data_counters/known_events/code_review_events.yml @@ -164,3 +164,13 @@ category: code_review aggregation: weekly feature_flag: usage_data_i_code_review_user_create_mr_from_issue +- name: i_code_review_user_mr_discussion_locked + redis_slot: code_review + category: code_review + aggregation: weekly + feature_flag: usage_data_i_code_review_user_mr_discussion_locked +- name: i_code_review_user_mr_discussion_unlocked + redis_slot: code_review + category: code_review + aggregation: weekly + feature_flag: usage_data_i_code_review_user_mr_discussion_unlocked diff --git a/lib/gitlab/usage_data_counters/merge_request_activity_unique_counter.rb b/lib/gitlab/usage_data_counters/merge_request_activity_unique_counter.rb index b9856e1f74a..b94caa32bf7 100644 --- a/lib/gitlab/usage_data_counters/merge_request_activity_unique_counter.rb +++ b/lib/gitlab/usage_data_counters/merge_request_activity_unique_counter.rb @@ -35,6 +35,8 @@ module Gitlab MR_EDIT_MR_TITLE_ACTION = 'i_code_review_edit_mr_title' MR_EDIT_MR_DESC_ACTION = 'i_code_review_edit_mr_desc' MR_CREATE_FROM_ISSUE_ACTION = 'i_code_review_user_create_mr_from_issue' + MR_DISCUSSION_LOCKED_ACTION = 'i_code_review_user_mr_discussion_locked' + MR_DISCUSSION_UNLOCKED_ACTION = 'i_code_review_user_mr_discussion_unlocked' class << self def track_mr_diffs_action(merge_request:) @@ -153,6 +155,14 @@ module Gitlab track_unique_action_by_user(MR_CREATE_FROM_ISSUE_ACTION, user) end + def track_discussion_locked_action(user:) + track_unique_action_by_user(MR_DISCUSSION_LOCKED_ACTION, user) + end + + def track_discussion_unlocked_action(user:) + track_unique_action_by_user(MR_DISCUSSION_UNLOCKED_ACTION, user) + end + private def track_unique_action_by_merge_request(action, merge_request) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4c4f25cf7a4..d8e05fd9baf 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -21028,6 +21028,9 @@ msgstr "" msgid "OnCallSchedules|Restrict to time intervals" msgstr "" +msgid "OnCallSchedules|Rotation end date/time must come after start date/time" +msgstr "" + msgid "OnCallSchedules|Rotation length" msgstr "" diff --git a/package.json b/package.json index b03966214d3..4672866478f 100644 --- a/package.json +++ b/package.json @@ -58,7 +58,7 @@ "@gitlab/favicon-overlay": "2.0.0", "@gitlab/svgs": "1.184.0", "@gitlab/tributejs": "1.0.0", - "@gitlab/ui": "28.0.0", + "@gitlab/ui": "28.1.0", "@gitlab/visual-review-tools": "1.6.1", "@rails/actioncable": "^6.0.3-4", "@rails/ujs": "^6.0.3-4", diff --git a/scripts/review_apps/review-apps.sh b/scripts/review_apps/review-apps.sh index 59e7d183ae7..88be78c7615 100755 --- a/scripts/review_apps/review-apps.sh +++ b/scripts/review_apps/review-apps.sh @@ -314,7 +314,7 @@ function deploy() { if [[ "$(base_config_changed)" == "true" ]]; then base_config_file_ref="${CI_COMMIT_SHA}"; fi local base_config_file="https://gitlab.com/gitlab-org/gitlab/raw/${base_config_file_ref}/scripts/review_apps/base-config.yaml" - echoinfo "Deploying ${release}..." true + echoinfo "Deploying ${release} to ${CI_ENVIRONMENT_URL} ..." true IMAGE_REPOSITORY="registry.gitlab.com/gitlab-org/build/cng-mirror" gitlab_migrations_image_repository="${IMAGE_REPOSITORY}/gitlab-rails-ee" diff --git a/spec/frontend/environments/environments_app_spec.js b/spec/frontend/environments/environments_app_spec.js index 50d84b19ce8..542cf58b079 100644 --- a/spec/frontend/environments/environments_app_spec.js +++ b/spec/frontend/environments/environments_app_spec.js @@ -97,13 +97,21 @@ describe('Environment', () => { jest.spyOn(wrapper.vm, 'updateContent').mockImplementation(() => {}); wrapper.find('.gl-pagination li:nth-child(3) .page-link').trigger('click'); - expect(wrapper.vm.updateContent).toHaveBeenCalledWith({ scope: 'available', page: '2' }); + expect(wrapper.vm.updateContent).toHaveBeenCalledWith({ + scope: 'available', + page: '2', + nested: true, + }); }); it('should make an API request when using tabs', () => { jest.spyOn(wrapper.vm, 'updateContent').mockImplementation(() => {}); findEnvironmentsTabStopped().trigger('click'); - expect(wrapper.vm.updateContent).toHaveBeenCalledWith({ scope: 'stopped', page: '1' }); + expect(wrapper.vm.updateContent).toHaveBeenCalledWith({ + scope: 'stopped', + page: '1', + nested: true, + }); }); it('should not make the same API request when clicking on the current scope tab', () => { diff --git a/spec/frontend/environments/folder/environments_folder_view_spec.js b/spec/frontend/environments/folder/environments_folder_view_spec.js index 3943e89c6cf..d02ed8688c6 100644 --- a/spec/frontend/environments/folder/environments_folder_view_spec.js +++ b/spec/frontend/environments/folder/environments_folder_view_spec.js @@ -103,13 +103,18 @@ describe('Environments Folder View', () => { expect(wrapper.vm.updateContent).toHaveBeenCalledWith({ scope: wrapper.vm.scope, page: '10', + nested: true, }); }); it('should make an API request when using tabs', () => { jest.spyOn(wrapper.vm, 'updateContent').mockImplementation(() => {}); findEnvironmentsTabStopped().trigger('click'); - expect(wrapper.vm.updateContent).toHaveBeenCalledWith({ scope: 'stopped', page: '1' }); + expect(wrapper.vm.updateContent).toHaveBeenCalledWith({ + scope: 'stopped', + page: '1', + nested: true, + }); }); }); }); @@ -161,7 +166,11 @@ describe('Environments Folder View', () => { it('should set page to 1', () => { jest.spyOn(wrapper.vm, 'updateContent').mockImplementation(() => {}); wrapper.vm.onChangeTab('stopped'); - expect(wrapper.vm.updateContent).toHaveBeenCalledWith({ scope: 'stopped', page: '1' }); + expect(wrapper.vm.updateContent).toHaveBeenCalledWith({ + scope: 'stopped', + page: '1', + nested: true, + }); }); }); @@ -172,6 +181,7 @@ describe('Environments Folder View', () => { expect(wrapper.vm.updateContent).toHaveBeenCalledWith({ scope: wrapper.vm.scope, page: '4', + nested: true, }); }); }); diff --git a/spec/lib/gitlab/error_tracking/context_payload_generator_spec.rb b/spec/lib/gitlab/error_tracking/context_payload_generator_spec.rb new file mode 100644 index 00000000000..0e72dd7ec5e --- /dev/null +++ b/spec/lib/gitlab/error_tracking/context_payload_generator_spec.rb @@ -0,0 +1,176 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'rspec-parameterized' + +RSpec.describe Gitlab::ErrorTracking::ContextPayloadGenerator do + subject(:generator) { described_class.new } + + let(:extra) do + { + some_other_info: 'info', + issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/-/issues/1' + } + end + + let(:exception) { StandardError.new("Dummy exception") } + + before do + allow(Labkit::Correlation::CorrelationId).to receive(:current_id).and_return('cid') + allow(I18n).to receive(:locale).and_return('en') + end + + context 'user metadata' do + let(:user) { create(:user) } + + it 'appends user metadata to the payload' do + payload = {} + + Gitlab::ApplicationContext.with_context(user: user) do + payload = generator.generate(exception, extra) + end + + expect(payload[:user]).to eql( + username: user.username + ) + end + end + + context 'tags metadata' do + context 'when the GITLAB_SENTRY_EXTRA_TAGS env is not set' do + before do + stub_env('GITLAB_SENTRY_EXTRA_TAGS', nil) + end + + it 'does not log into AppLogger' do + expect(Gitlab::AppLogger).not_to receive(:debug) + + generator.generate(exception, extra) + end + + it 'does not send any extra tags' do + payload = {} + + Gitlab::ApplicationContext.with_context(feature_category: 'feature_a') do + payload = generator.generate(exception, extra) + end + + expect(payload[:tags]).to eql( + correlation_id: 'cid', + locale: 'en', + program: 'test', + feature_category: 'feature_a' + ) + end + end + + context 'when the GITLAB_SENTRY_EXTRA_TAGS env is a JSON hash' do + it 'includes those tags in all events' do + stub_env('GITLAB_SENTRY_EXTRA_TAGS', { foo: 'bar', baz: 'quux' }.to_json) + payload = {} + + Gitlab::ApplicationContext.with_context(feature_category: 'feature_a') do + payload = generator.generate(exception, extra) + end + + expect(payload[:tags]).to eql( + correlation_id: 'cid', + locale: 'en', + program: 'test', + feature_category: 'feature_a', + 'foo' => 'bar', + 'baz' => 'quux' + ) + end + + it 'does not log into AppLogger' do + expect(Gitlab::AppLogger).not_to receive(:debug) + + generator.generate(exception, extra) + end + end + + context 'when the GITLAB_SENTRY_EXTRA_TAGS env is not a JSON hash' do + using RSpec::Parameterized::TableSyntax + + where(:env_var, :error) do + { foo: 'bar', baz: 'quux' }.inspect | 'JSON::ParserError' + [].to_json | 'NoMethodError' + [%w[foo bar]].to_json | 'NoMethodError' + %w[foo bar].to_json | 'NoMethodError' + '"string"' | 'NoMethodError' + end + + with_them do + before do + stub_env('GITLAB_SENTRY_EXTRA_TAGS', env_var) + end + + it 'logs into AppLogger' do + expect(Gitlab::AppLogger).to receive(:debug).with(a_string_matching(error)) + + generator.generate({}) + end + + it 'does not include any extra tags' do + payload = {} + + Gitlab::ApplicationContext.with_context(feature_category: 'feature_a') do + payload = generator.generate(exception, extra) + end + + expect(payload[:tags]).to eql( + correlation_id: 'cid', + locale: 'en', + program: 'test', + feature_category: 'feature_a' + ) + end + end + end + end + + context 'extra metadata' do + it 'appends extra metadata to the payload' do + payload = generator.generate(exception, extra) + + expect(payload[:extra]).to eql( + some_other_info: 'info', + issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/-/issues/1' + ) + end + + it 'appends exception embedded extra metadata to the payload' do + allow(exception).to receive(:sentry_extra_data).and_return( + some_other_info: 'another_info', + mr_url: 'https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/1' + ) + + payload = generator.generate(exception, extra) + + expect(payload[:extra]).to eql( + some_other_info: 'another_info', + issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/-/issues/1', + mr_url: 'https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/1' + ) + end + + it 'filters sensitive extra info' do + extra[:my_token] = '456' + allow(exception).to receive(:sentry_extra_data).and_return( + mr_url: 'https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/1', + another_token: '1234' + ) + + payload = generator.generate(exception, extra) + + expect(payload[:extra]).to eql( + some_other_info: 'info', + issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/-/issues/1', + mr_url: 'https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/1', + my_token: '[FILTERED]', + another_token: '[FILTERED]' + ) + end + end +end diff --git a/spec/lib/gitlab/error_tracking/log_formatter_spec.rb b/spec/lib/gitlab/error_tracking/log_formatter_spec.rb new file mode 100644 index 00000000000..188ccd000a1 --- /dev/null +++ b/spec/lib/gitlab/error_tracking/log_formatter_spec.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::ErrorTracking::LogFormatter do + let(:exception) { StandardError.new('boom') } + let(:context_payload) do + { + server: 'local-hostname-of-the-server', + user: { + ip_address: '127.0.0.1', + username: 'root' + }, + tags: { + locale: 'en', + feature_category: 'category_a' + }, + extra: { + some_other_info: 'other_info', + sidekiq: { + 'class' => 'HelloWorker', + 'args' => ['senstive string', 1, 2], + 'another_field' => 'field' + } + } + } + end + + before do + Raven.context.user[:user_flag] = 'flag' + Raven.context.tags[:shard] = 'catchall' + Raven.context.extra[:some_info] = 'info' + + allow(exception).to receive(:backtrace).and_return( + [ + 'lib/gitlab/file_a.rb:1', + 'lib/gitlab/file_b.rb:2' + ] + ) + end + + after do + ::Raven::Context.clear! + end + + it 'appends error-related log fields and filters sensitive Sidekiq arguments' do + payload = described_class.new.generate_log(exception, context_payload) + + expect(payload).to eql( + 'exception.class' => 'StandardError', + 'exception.message' => 'boom', + 'exception.backtrace' => [ + 'lib/gitlab/file_a.rb:1', + 'lib/gitlab/file_b.rb:2' + ], + 'user.ip_address' => '127.0.0.1', + 'user.username' => 'root', + 'user.user_flag' => 'flag', + 'tags.locale' => 'en', + 'tags.feature_category' => 'category_a', + 'tags.shard' => 'catchall', + 'extra.some_other_info' => 'other_info', + 'extra.some_info' => 'info', + "extra.sidekiq" => { + "another_field" => "field", + "args" => ["[FILTERED]", "1", "2"], + "class" => "HelloWorker" + } + ) + end +end diff --git a/spec/lib/gitlab/error_tracking/processor/context_payload_processor_spec.rb b/spec/lib/gitlab/error_tracking/processor/context_payload_processor_spec.rb new file mode 100644 index 00000000000..0db40eca989 --- /dev/null +++ b/spec/lib/gitlab/error_tracking/processor/context_payload_processor_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::ErrorTracking::Processor::ContextPayloadProcessor do + subject(:processor) { described_class.new } + + before do + allow_next_instance_of(Gitlab::ErrorTracking::ContextPayloadGenerator) do |generator| + allow(generator).to receive(:generate).and_return( + user: { username: 'root' }, + tags: { locale: 'en', program: 'test', feature_category: 'feature_a', correlation_id: 'cid' }, + extra: { some_info: 'info' } + ) + end + end + + it 'merges the context payload into event payload' do + payload = { + user: { ip_address: '127.0.0.1' }, + tags: { priority: 'high' }, + extra: { sidekiq: { class: 'SomeWorker', args: ['[FILTERED]', 1, 2] } } + } + + processor.process(payload) + + expect(payload).to eql( + user: { + ip_address: '127.0.0.1', + username: 'root' + }, + tags: { + priority: 'high', + locale: 'en', + program: 'test', + feature_category: 'feature_a', + correlation_id: 'cid' + }, + extra: { + some_info: 'info', + sidekiq: { class: 'SomeWorker', args: ['[FILTERED]', 1, 2] } + } + ) + end +end diff --git a/spec/lib/gitlab/error_tracking_spec.rb b/spec/lib/gitlab/error_tracking_spec.rb index 764478ad1d7..a905b9f8d40 100644 --- a/spec/lib/gitlab/error_tracking_spec.rb +++ b/spec/lib/gitlab/error_tracking_spec.rb @@ -8,116 +8,55 @@ RSpec.describe Gitlab::ErrorTracking do let(:exception) { RuntimeError.new('boom') } let(:issue_url) { 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' } - let(:expected_payload_includes) do - [ - { 'exception.class' => 'RuntimeError' }, - { 'exception.message' => 'boom' }, - { 'tags.correlation_id' => 'cid' }, - { 'extra.some_other_info' => 'info' }, - { 'extra.issue_url' => 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' } - ] + let(:user) { create(:user) } + + let(:sentry_payload) do + { + tags: { + program: 'test', + locale: 'en', + feature_category: 'feature_a', + correlation_id: 'cid' + }, + user: { + username: user.username + }, + extra: { + some_other_info: 'info', + issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' + } + } end - let(:sentry_event) { Gitlab::Json.parse(Raven.client.transport.events.last[1]) } + let(:logger_payload) do + { + 'exception.class' => 'RuntimeError', + 'exception.message' => 'boom', + 'tags.program' => 'test', + 'tags.locale' => 'en', + 'tags.feature_category' => 'feature_a', + 'tags.correlation_id' => 'cid', + 'user.username' => user.username, + 'extra.some_other_info' => 'info', + 'extra.issue_url' => 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' + } + end before do stub_sentry_settings allow(described_class).to receive(:sentry_dsn).and_return(Gitlab.config.sentry.dsn) allow(Labkit::Correlation::CorrelationId).to receive(:current_id).and_return('cid') + allow(I18n).to receive(:locale).and_return('en') described_class.configure do |config| config.encoding = 'json' end end - describe '.configure' do - context 'default tags from GITLAB_SENTRY_EXTRA_TAGS' do - context 'when the value is a JSON hash' do - it 'includes those tags in all events' do - stub_env('GITLAB_SENTRY_EXTRA_TAGS', { foo: 'bar', baz: 'quux' }.to_json) - - described_class.configure do |config| - config.encoding = 'json' - end - - described_class.track_exception(StandardError.new) - - expect(sentry_event['tags'].except('correlation_id', 'locale', 'program')) - .to eq('foo' => 'bar', 'baz' => 'quux') - end - end - - context 'when the value is not set' do - before do - stub_env('GITLAB_SENTRY_EXTRA_TAGS', nil) - end - - it 'does not log an error' do - expect(Gitlab::AppLogger).not_to receive(:debug) - - described_class.configure do |config| - config.encoding = 'json' - end - end - - it 'does not send any extra tags' do - described_class.configure do |config| - config.encoding = 'json' - end - - described_class.track_exception(StandardError.new) - - expect(sentry_event['tags'].keys).to contain_exactly('correlation_id', 'locale', 'program') - end - end - - context 'when the value is not a JSON hash' do - using RSpec::Parameterized::TableSyntax - - where(:env_var, :error) do - { foo: 'bar', baz: 'quux' }.inspect | 'JSON::ParserError' - [].to_json | 'NoMethodError' - [%w[foo bar]].to_json | 'NoMethodError' - %w[foo bar].to_json | 'NoMethodError' - '"string"' | 'NoMethodError' - end - - with_them do - before do - stub_env('GITLAB_SENTRY_EXTRA_TAGS', env_var) - end - - it 'does not include any extra tags' do - described_class.configure do |config| - config.encoding = 'json' - end - - described_class.track_exception(StandardError.new) - - expect(sentry_event['tags'].except('correlation_id', 'locale', 'program')) - .to be_empty - end - - it 'logs the error class' do - expect(Gitlab::AppLogger).to receive(:debug).with(a_string_matching(error)) - - described_class.configure do |config| - config.encoding = 'json' - end - end - end - end - end - end - - describe '.with_context' do - it 'adds the expected tags' do - described_class.with_context {} - - expect(Raven.tags_context[:locale].to_s).to eq(I18n.locale.to_s) - expect(Raven.tags_context[Labkit::Correlation::CorrelationId::LOG_KEY.to_sym].to_s) - .to eq('cid') + around do |example| + Gitlab::ApplicationContext.with_context(user: user, feature_category: 'feature_a') do + example.run end end @@ -128,10 +67,15 @@ RSpec.describe Gitlab::ErrorTracking do end it 'raises the exception' do - expect(Raven).to receive(:capture_exception) - - expect { described_class.track_and_raise_for_dev_exception(exception) } - .to raise_error(RuntimeError) + expect(Raven).to receive(:capture_exception).with(exception, sentry_payload) + + expect do + described_class.track_and_raise_for_dev_exception( + exception, + issue_url: issue_url, + some_other_info: 'info' + ) + end.to raise_error(RuntimeError, /boom/) end end @@ -141,19 +85,7 @@ RSpec.describe Gitlab::ErrorTracking do end it 'logs the exception with all attributes passed' do - expected_extras = { - some_other_info: 'info', - issue_url: 'http://gitlab.com/gitlab-org/gitlab-foss/issues/1' - } - - expected_tags = { - correlation_id: 'cid' - } - - expect(Raven).to receive(:capture_exception) - .with(exception, - tags: a_hash_including(expected_tags), - extra: a_hash_including(expected_extras)) + expect(Raven).to receive(:capture_exception).with(exception, sentry_payload) described_class.track_and_raise_for_dev_exception( exception, @@ -163,8 +95,7 @@ RSpec.describe Gitlab::ErrorTracking do end it 'calls Gitlab::ErrorTracking::Logger.error with formatted payload' do - expect(Gitlab::ErrorTracking::Logger).to receive(:error) - .with(a_hash_including(*expected_payload_includes)) + expect(Gitlab::ErrorTracking::Logger).to receive(:error).with(logger_payload) described_class.track_and_raise_for_dev_exception( exception, @@ -177,15 +108,19 @@ RSpec.describe Gitlab::ErrorTracking do describe '.track_and_raise_exception' do it 'always raises the exception' do - expect(Raven).to receive(:capture_exception) + expect(Raven).to receive(:capture_exception).with(exception, sentry_payload) - expect { described_class.track_and_raise_exception(exception) } - .to raise_error(RuntimeError) + expect do + described_class.track_and_raise_for_dev_exception( + exception, + issue_url: issue_url, + some_other_info: 'info' + ) + end.to raise_error(RuntimeError, /boom/) end it 'calls Gitlab::ErrorTracking::Logger.error with formatted payload' do - expect(Gitlab::ErrorTracking::Logger).to receive(:error) - .with(a_hash_including(*expected_payload_includes)) + expect(Gitlab::ErrorTracking::Logger).to receive(:error).with(logger_payload) expect do described_class.track_and_raise_exception( @@ -210,17 +145,16 @@ RSpec.describe Gitlab::ErrorTracking do it 'calls Raven.capture_exception' do track_exception - expect(Raven).to have_received(:capture_exception) - .with(exception, - tags: a_hash_including(correlation_id: 'cid'), - extra: a_hash_including(some_other_info: 'info', issue_url: issue_url)) + expect(Raven).to have_received(:capture_exception).with( + exception, + sentry_payload + ) end it 'calls Gitlab::ErrorTracking::Logger.error with formatted payload' do track_exception - expect(Gitlab::ErrorTracking::Logger).to have_received(:error) - .with(a_hash_including(*expected_payload_includes)) + expect(Gitlab::ErrorTracking::Logger).to have_received(:error).with(logger_payload) end context 'with filterable parameters' do @@ -229,8 +163,9 @@ RSpec.describe Gitlab::ErrorTracking do it 'filters parameters' do track_exception - expect(Gitlab::ErrorTracking::Logger).to have_received(:error) - .with(hash_including({ 'extra.test' => 1, 'extra.my_token' => '[FILTERED]' })) + expect(Gitlab::ErrorTracking::Logger).to have_received(:error).with( + hash_including({ 'extra.test' => 1, 'extra.my_token' => '[FILTERED]' }) + ) end end @@ -241,8 +176,9 @@ RSpec.describe Gitlab::ErrorTracking do it 'includes the extra data from the exception in the tracking information' do track_exception - expect(Raven).to have_received(:capture_exception) - .with(exception, a_hash_including(extra: a_hash_including(extra_info))) + expect(Raven).to have_received(:capture_exception).with( + exception, a_hash_including(extra: a_hash_including(extra_info)) + ) end end @@ -253,8 +189,9 @@ RSpec.describe Gitlab::ErrorTracking do it 'just includes the other extra info' do track_exception - expect(Raven).to have_received(:capture_exception) - .with(exception, a_hash_including(extra: a_hash_including(extra))) + expect(Raven).to have_received(:capture_exception).with( + exception, a_hash_including(extra: a_hash_including(extra)) + ) end end @@ -266,7 +203,13 @@ RSpec.describe Gitlab::ErrorTracking do track_exception expect(Gitlab::ErrorTracking::Logger).to have_received(:error).with( - hash_including({ 'extra.sidekiq' => { 'class' => 'PostReceive', 'args' => ['1', '{"id"=>2, "name"=>"hello"}', 'some-value', 'another-value'] } })) + hash_including( + 'extra.sidekiq' => { + 'class' => 'PostReceive', + 'args' => ['1', '{"id"=>2, "name"=>"hello"}', 'some-value', 'another-value'] + } + ) + ) end end @@ -276,9 +219,17 @@ RSpec.describe Gitlab::ErrorTracking do it 'filters sensitive arguments before sending' do track_exception + sentry_event = Gitlab::Json.parse(Raven.client.transport.events.last[1]) + expect(sentry_event.dig('extra', 'sidekiq', 'args')).to eq(['[FILTERED]', 1, 2]) expect(Gitlab::ErrorTracking::Logger).to have_received(:error).with( - hash_including('extra.sidekiq' => { 'class' => 'UnknownWorker', 'args' => ['[FILTERED]', '1', '2'] })) + hash_including( + 'extra.sidekiq' => { + 'class' => 'UnknownWorker', + 'args' => ['[FILTERED]', '1', '2'] + } + ) + ) end end end diff --git a/spec/lib/gitlab/usage_data_counters/merge_request_activity_unique_counter_spec.rb b/spec/lib/gitlab/usage_data_counters/merge_request_activity_unique_counter_spec.rb index a604de4a61f..6bc42430889 100644 --- a/spec/lib/gitlab/usage_data_counters/merge_request_activity_unique_counter_spec.rb +++ b/spec/lib/gitlab/usage_data_counters/merge_request_activity_unique_counter_spec.rb @@ -284,4 +284,20 @@ RSpec.describe Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter, :cl let(:action) { described_class::MR_CREATE_FROM_ISSUE_ACTION } end end + + describe '.track_discussion_locked_action' do + subject { described_class.track_discussion_locked_action(user: user) } + + it_behaves_like 'a tracked merge request unique event' do + let(:action) { described_class::MR_DISCUSSION_LOCKED_ACTION } + end + end + + describe '.track_discussion_unlocked_action' do + subject { described_class.track_discussion_unlocked_action(user: user) } + + it_behaves_like 'a tracked merge request unique event' do + let(:action) { described_class::MR_DISCUSSION_UNLOCKED_ACTION } + end + end end diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 18388b4cd83..6bac5e31435 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -221,7 +221,7 @@ RSpec.describe Upload do it 'does not send a message to Sentry' do upload = described_class.new(path: "#{__FILE__}-nope", store: ObjectStorage::Store::LOCAL) - expect(Raven).not_to receive(:capture_message) + expect(Gitlab::ErrorTracking).not_to receive(:track_exception) upload.exist? end diff --git a/spec/requests/api/graphql/project/merge_requests_spec.rb b/spec/requests/api/graphql/project/merge_requests_spec.rb index d684be91dc9..12060eb51e9 100644 --- a/spec/requests/api/graphql/project/merge_requests_spec.rb +++ b/spec/requests/api/graphql/project/merge_requests_spec.rb @@ -7,13 +7,27 @@ RSpec.describe 'getting merge request listings nested in a project' do let_it_be(:project) { create(:project, :repository, :public) } let_it_be(:current_user) { create(:user) } - let_it_be(:label) { create(:label, project: project) } - let_it_be(:merge_request_a) { create(:labeled_merge_request, :unique_branches, source_project: project, labels: [label]) } - let_it_be(:merge_request_b) { create(:merge_request, :closed, :unique_branches, source_project: project) } - let_it_be(:merge_request_c) { create(:labeled_merge_request, :closed, :unique_branches, source_project: project, labels: [label]) } - let_it_be(:merge_request_d) { create(:merge_request, :locked, :unique_branches, source_project: project) } - let_it_be(:merge_request_e) { create(:merge_request, :unique_branches, source_project: project) } + + let_it_be(:merge_request_a) do + create(:labeled_merge_request, :unique_branches, source_project: project, labels: [label]) + end + + let_it_be(:merge_request_b) do + create(:merge_request, :closed, :unique_branches, source_project: project) + end + + let_it_be(:merge_request_c) do + create(:labeled_merge_request, :closed, :unique_branches, source_project: project, labels: [label]) + end + + let_it_be(:merge_request_d) do + create(:merge_request, :locked, :unique_branches, source_project: project) + end + + let_it_be(:merge_request_e) do + create(:merge_request, :unique_branches, source_project: project) + end let(:results) { graphql_data.dig('project', 'mergeRequests', 'nodes') } @@ -27,32 +41,38 @@ RSpec.describe 'getting merge request listings nested in a project' do ) end - let(:query) do - query_merge_requests(all_graphql_fields_for('MergeRequest', max_depth: 1)) - end - it_behaves_like 'a working graphql query' do + let(:query) do + query_merge_requests(all_graphql_fields_for('MergeRequest', max_depth: 2)) + end + before do - post_graphql(query, current_user: current_user) + # We cannot call the whitelist here, since the transaction does not + # begin until we enter the controller. + headers = { + 'X-GITLAB-QUERY-WHITELIST-ISSUE' => 'https://gitlab.com/gitlab-org/gitlab/-/issues/322979' + } + + post_graphql(query, current_user: current_user, headers: headers) end end # The following tests are needed to guarantee that we have correctly annotated # all the gitaly calls. Selecting combinations of fields may mask this due to # memoization. - context 'requesting a single field' do + context 'when requesting a single field' do let_it_be(:fresh_mr) { create(:merge_request, :unique_branches, source_project: project) } + let(:search_params) { { iids: [fresh_mr.iid.to_s] } } + let(:graphql_data) do + GitlabSchema.execute(query, context: { current_user: current_user }).to_h['data'] + end before do project.repository.expire_branches_cache end - let(:graphql_data) do - GitlabSchema.execute(query, context: { current_user: current_user }).to_h['data'] - end - - context 'selecting any single scalar field' do + context 'when selecting any single scalar field' do where(:field) do scalar_fields_of('MergeRequest').map { |name| [name] } end @@ -68,7 +88,7 @@ RSpec.describe 'getting merge request listings nested in a project' do end end - context 'selecting any single nested field' do + context 'when selecting any single nested field' do where(:field, :subfield, :is_connection) do nested_fields_of('MergeRequest').flat_map do |name, field| type = field_type(field) @@ -95,7 +115,11 @@ RSpec.describe 'getting merge request listings nested in a project' do end end - shared_examples 'searching with parameters' do + shared_examples 'when searching with parameters' do + let(:query) do + query_merge_requests('iid title') + end + let(:expected) do mrs.map { |mr| a_hash_including('iid' => mr.iid.to_s, 'title' => mr.title) } end @@ -107,60 +131,60 @@ RSpec.describe 'getting merge request listings nested in a project' do end end - context 'there are no search params' do + context 'when there are no search params' do let(:search_params) { nil } let(:mrs) { [merge_request_a, merge_request_b, merge_request_c, merge_request_d, merge_request_e] } - it_behaves_like 'searching with parameters' + it_behaves_like 'when searching with parameters' end - context 'the search params do not match anything' do - let(:search_params) { { iids: %w(foo bar baz) } } + context 'when the search params do not match anything' do + let(:search_params) { { iids: %w[foo bar baz] } } let(:mrs) { [] } - it_behaves_like 'searching with parameters' + it_behaves_like 'when searching with parameters' end - context 'searching by iids' do + context 'when searching by iids' do let(:search_params) { { iids: mrs.map(&:iid).map(&:to_s) } } let(:mrs) { [merge_request_a, merge_request_c] } - it_behaves_like 'searching with parameters' + it_behaves_like 'when searching with parameters' end - context 'searching by state' do + context 'when searching by state' do let(:search_params) { { state: :closed } } let(:mrs) { [merge_request_b, merge_request_c] } - it_behaves_like 'searching with parameters' + it_behaves_like 'when searching with parameters' end - context 'searching by source_branch' do + context 'when searching by source_branch' do let(:search_params) { { source_branches: mrs.map(&:source_branch) } } let(:mrs) { [merge_request_b, merge_request_c] } - it_behaves_like 'searching with parameters' + it_behaves_like 'when searching with parameters' end - context 'searching by target_branch' do + context 'when searching by target_branch' do let(:search_params) { { target_branches: mrs.map(&:target_branch) } } let(:mrs) { [merge_request_a, merge_request_d] } - it_behaves_like 'searching with parameters' + it_behaves_like 'when searching with parameters' end - context 'searching by label' do + context 'when searching by label' do let(:search_params) { { labels: [label.title] } } let(:mrs) { [merge_request_a, merge_request_c] } - it_behaves_like 'searching with parameters' + it_behaves_like 'when searching with parameters' end - context 'searching by combination' do + context 'when searching by combination' do let(:search_params) { { state: :closed, labels: [label.title] } } let(:mrs) { [merge_request_c] } - it_behaves_like 'searching with parameters' + it_behaves_like 'when searching with parameters' end context 'when requesting `approved_by`' do @@ -203,10 +227,10 @@ RSpec.describe 'getting merge request listings nested in a project' do it 'exposes `commit_count`' do execute_query - expect(results).to match_array([ + expect(results).to match_array [ { "iid" => merge_request_a.iid.to_s, "commitCount" => 0 }, { "iid" => merge_request_with_commits.iid.to_s, "commitCount" => 29 } - ]) + ] end end @@ -216,8 +240,8 @@ RSpec.describe 'getting merge request listings nested in a project' do before do # make the MRs "merged" [merge_request_a, merge_request_b, merge_request_c].each do |mr| - mr.update_column(:state_id, MergeRequest.available_states[:merged]) - mr.metrics.update_column(:merged_at, Time.now) + mr.update!(state_id: MergeRequest.available_states[:merged]) + mr.metrics.update!(merged_at: Time.now) end end @@ -256,13 +280,12 @@ RSpec.describe 'getting merge request listings nested in a project' do end it 'returns the reviewers' do + nodes = merge_request_a.reviewers.map { |r| { 'username' => r.username } } + reviewers = { 'nodes' => match_array(nodes) } + execute_query - expect(results).to include a_hash_including('reviewers' => { - 'nodes' => match_array(merge_request_a.reviewers.map do |r| - a_hash_including('username' => r.username) - end) - }) + expect(results).to include a_hash_including('reviewers' => match(reviewers)) end include_examples 'N+1 query check' @@ -309,12 +332,14 @@ RSpec.describe 'getting merge request listings nested in a project' do allow(Gitlab::Database).to receive(:read_only?).and_return(false) end + def query_context + { current_user: current_user } + end + def run_query(number) # Ensure that we have a fresh request store and batch-context between runs - result = run_with_clean_state(query, - context: { current_user: current_user }, - variables: { first: number } - ) + vars = { first: number } + result = run_with_clean_state(query, context: query_context, variables: vars) graphql_dig_at(result.to_h, :data, :project, :merge_requests, :nodes) end @@ -348,13 +373,11 @@ RSpec.describe 'getting merge request listings nested in a project' do let(:data_path) { [:project, :mergeRequests] } def pagination_query(params) - graphql_query_for(:project, { full_path: project.full_path }, - <<~QUERY + graphql_query_for(:project, { full_path: project.full_path }, <<~QUERY) mergeRequests(#{params}) { #{page_info} nodes { id } } - QUERY - ) + QUERY end context 'when sorting by merged_at DESC' do @@ -396,14 +419,12 @@ RSpec.describe 'getting merge request listings nested in a project' do let(:query) do # Note: __typename meta field is always requested by the FE - graphql_query_for(:project, { full_path: project.full_path }, - <<~QUERY + graphql_query_for(:project, { full_path: project.full_path }, <<~QUERY) mergeRequests(mergedAfter: "2020-01-01", mergedBefore: "2020-01-05", first: 0, sourceBranches: null, labels: null) { count __typename } QUERY - ) end shared_examples 'count examples' do @@ -430,14 +451,12 @@ RSpec.describe 'getting merge request listings nested in a project' do context 'when total_time_to_merge and count is queried' do let(:query) do - graphql_query_for(:project, { full_path: project.full_path }, - <<~QUERY + graphql_query_for(:project, { full_path: project.full_path }, <<~QUERY) mergeRequests(mergedAfter: "2020-01-01", mergedBefore: "2020-01-05", first: 0) { totalTimeToMerge count } QUERY - ) end it 'does not query the merge requests table for the total_time_to_merge' do diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb index 91d10791541..8160a94aef2 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -314,14 +314,13 @@ RSpec.describe API::Helpers do expect(Gitlab::ErrorTracking).to receive(:sentry_dsn).and_return(Gitlab.config.sentry.dsn) Gitlab::ErrorTracking.configure - Raven.client.configuration.encoding = 'json' end it 'does not report a MethodNotAllowed exception to Sentry' do exception = Grape::Exceptions::MethodNotAllowed.new({ 'X-GitLab-Test' => '1' }) allow(exception).to receive(:backtrace).and_return(caller) - expect(Raven).not_to receive(:capture_exception).with(exception) + expect(Gitlab::ErrorTracking).not_to receive(:track_exception).with(exception) handle_api_exception(exception) end @@ -330,8 +329,7 @@ RSpec.describe API::Helpers do exception = RuntimeError.new('test error') allow(exception).to receive(:backtrace).and_return(caller) - expect(Raven).to receive(:capture_exception).with(exception, tags: - a_hash_including(correlation_id: 'new-correlation-id'), extra: {}) + expect(Gitlab::ErrorTracking).to receive(:track_exception).with(exception) Labkit::Correlation::CorrelationId.use_id('new-correlation-id') do handle_api_exception(exception) @@ -357,20 +355,6 @@ RSpec.describe API::Helpers do expect(json_response['message']).to start_with("\nRuntimeError (Runtime Error!):") end end - - context 'extra information' do - # Sentry events are an array of the form [auth_header, data, options] - let(:event_data) { Raven.client.transport.events.first[1] } - - it 'sends the params, excluding confidential values' do - expect(ProjectsFinder).to receive(:new).and_raise('Runtime Error!') - - get api('/projects', user), params: { password: 'dont_send_this', other_param: 'send_this' } - - expect(event_data).to include('other_param=send_this') - expect(event_data).to include('password=********') - end - end end describe '.authenticate_non_get!' do diff --git a/spec/requests/api/project_attributes.yml b/spec/requests/api/project_attributes.yml index 181fcafd577..104918810f8 100644 --- a/spec/requests/api/project_attributes.yml +++ b/spec/requests/api/project_attributes.yml @@ -56,6 +56,7 @@ itself: # project - can_create_merge_request_in - compliance_frameworks - container_expiration_policy + - container_registry_image_prefix - default_branch - empty_repo - forks_count diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 8a68db6ae4b..19cab51ef34 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1540,6 +1540,10 @@ RSpec.describe API::Projects do end context 'when authenticated as an admin' do + before do + stub_container_registry_config(enabled: true, host_port: 'registry.example.org:5000') + end + let(:project_attributes_file) { 'spec/requests/api/project_attributes.yml' } let(:project_attributes) { YAML.load_file(project_attributes_file) } @@ -1569,7 +1573,7 @@ RSpec.describe API::Projects do keys end - it 'returns a project by id' do + it 'returns a project by id', :aggregate_failures do project project_member group = create(:group) @@ -1587,6 +1591,7 @@ RSpec.describe API::Projects do expect(json_response['ssh_url_to_repo']).to be_present expect(json_response['http_url_to_repo']).to be_present expect(json_response['web_url']).to be_present + expect(json_response['container_registry_image_prefix']).to eq("registry.example.org:5000/#{project.full_path}") expect(json_response['owner']).to be_a Hash expect(json_response['name']).to eq(project.name) expect(json_response['path']).to be_present @@ -1644,9 +1649,10 @@ RSpec.describe API::Projects do before do project project_member + stub_container_registry_config(enabled: true, host_port: 'registry.example.org:5000') end - it 'returns a project by id' do + it 'returns a project by id', :aggregate_failures do group = create(:group) link = create(:project_group_link, project: project, group: group) @@ -1662,6 +1668,7 @@ RSpec.describe API::Projects do expect(json_response['ssh_url_to_repo']).to be_present expect(json_response['http_url_to_repo']).to be_present expect(json_response['web_url']).to be_present + expect(json_response['container_registry_image_prefix']).to eq("registry.example.org:5000/#{project.full_path}") expect(json_response['owner']).to be_a Hash expect(json_response['name']).to eq(project.name) expect(json_response['path']).to be_present diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index a6bd8416e58..e9ec3bccda3 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -48,6 +48,8 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end context 'valid params' do + let(:locked) { true } + let(:opts) do { title: 'New title', @@ -58,7 +60,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do label_ids: [label.id], target_branch: 'target', force_remove_source_branch: '1', - discussion_locked: true + discussion_locked: locked } end @@ -117,6 +119,56 @@ RSpec.describe MergeRequests::UpdateService, :mailer do MergeRequests::UpdateService.new(project, user, opts).execute(draft_merge_request) end + + context 'when MR is locked' do + context 'when locked again' do + it 'does not track discussion locking' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_discussion_locked_action) + + opts[:discussion_locked] = true + + MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + end + end + + context 'when unlocked' do + it 'tracks dicussion unlocking' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_discussion_unlocked_action).once.with(user: user) + + opts[:discussion_locked] = false + + MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + end + end + end + + context 'when MR is unlocked' do + let(:locked) { false } + + context 'when unlocked again' do + it 'does not track discussion unlocking' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .not_to receive(:track_discussion_unlocked_action) + + opts[:discussion_locked] = false + + MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + end + end + + context 'when locked' do + it 'tracks dicussion locking' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_discussion_locked_action).once.with(user: user) + + opts[:discussion_locked] = true + + MergeRequests::UpdateService.new(project, user, opts).execute(merge_request) + end + end + end end context 'updating milestone' do diff --git a/yarn.lock b/yarn.lock index 08717ee05c6..768b29ab5ba 100644 --- a/yarn.lock +++ b/yarn.lock @@ -907,10 +907,10 @@ resolved "https://registry.yarnpkg.com/@gitlab/tributejs/-/tributejs-1.0.0.tgz#672befa222aeffc83e7d799b0500a7a4418e59b8" integrity sha512-nmKw1+hB6MHvlmPz63yPwVs1qQkycHwsKgxpEbzmky16Y6mL4EJMk3w1b8QlOAF/AIAzjCERPhe/R4MJiohbZw== -"@gitlab/ui@28.0.0": - version "28.0.0" - resolved "https://registry.yarnpkg.com/@gitlab/ui/-/ui-28.0.0.tgz#712a24ba970e69eed595209194d69574851034c6" - integrity sha512-6gJKUEnqYAYxyWta/CJcSSPNu07iVrQ4m6Sxoh/Sjo7VpPE4VjLgTax3CdqET4KokUbIKGSWyH3s+IvtzDzFqA== +"@gitlab/ui@28.1.0": + version "28.1.0" + resolved "https://registry.yarnpkg.com/@gitlab/ui/-/ui-28.1.0.tgz#aaec227c03c345632bcb2bc6222ed49a004b2dbd" + integrity sha512-V4AH41sRVPp4gOSNt/safVbmsF8RS7H49JuaMWtOkNqfc90ePW+OGyv5ioCYDMYLvk8BaGlmH8f01BuCgOrplA== dependencies: "@babel/standalone" "^7.0.0" "@gitlab/vue-toasted" "^1.3.0" |