From 25cc54cacd4eb8f997dba2290e9d09818bd6a269 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 15 Jul 2019 14:55:59 +0200 Subject: Deploy serverless apps with `gitlabktl` --- lib/gitlab/ci/templates/Serverless.gitlab-ci.yml | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/gitlab/ci/templates/Serverless.gitlab-ci.yml b/lib/gitlab/ci/templates/Serverless.gitlab-ci.yml index a3db2705bf6..280e75d46f5 100644 --- a/lib/gitlab/ci/templates/Serverless.gitlab-ci.yml +++ b/lib/gitlab/ci/templates/Serverless.gitlab-ci.yml @@ -8,26 +8,23 @@ stages: - deploy .serverless:build:image: - stage: build image: registry.gitlab.com/gitlab-org/gitlabktl:latest + stage: build script: /usr/bin/gitlabktl app build .serverless:deploy:image: + image: registry.gitlab.com/gitlab-org/gitlabktl:latest stage: deploy - image: gcr.io/triggermesh/tm@sha256:3cfdd470a66b741004fb02354319d79f1598c70117ce79978d2e07e192bfb336 # v0.0.11 environment: development - script: - - echo "$CI_REGISTRY_IMAGE" - - tm -n "$KUBE_NAMESPACE" --config "$KUBECONFIG" deploy service "$CI_PROJECT_NAME" --from-image "$CI_REGISTRY_IMAGE" --wait + script: /usr/bin/gitlabktl app deploy .serverless:build:functions: - stage: build - environment: development image: registry.gitlab.com/gitlab-org/gitlabktl:latest + stage: build script: /usr/bin/gitlabktl serverless build .serverless:deploy:functions: + image: registry.gitlab.com/gitlab-org/gitlabktl:latest stage: deploy environment: development - image: registry.gitlab.com/gitlab-org/gitlabktl:latest script: /usr/bin/gitlabktl serverless deploy -- cgit v1.2.1 From beaa63530669d10c7244d187fa386144bc5da7eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Thu, 4 Jul 2019 16:42:31 +0200 Subject: Add class for group level analytics Add specs for group level Update entities Update base classes Add groups-centric changes Update plan and review stage Add summary classes Add summary spec Update specs files Add to specs test cases for group Add changelog entry Add group serializer Fix typo Fix typo Add fetching namespace in sql query Update specs Add rubocop fix Add rubocop fix Modify method to be in sync with code review Add counting deploys from subgroup To group summary stage Add subgroups handling In group stage summary Add additional spec Add additional specs Add more precise inheritance Add attr reader to group level Fix rubocop offence Fix problems with specs Add cr remarks Renaming median method and a lot of calls in specs Move spec setup Rename method in specs Add code review remarks regarding module Add proper module name --- app/models/cycle_analytics/base.rb | 27 ------- app/models/cycle_analytics/base_methods.rb | 27 +++++++ app/models/cycle_analytics/group_level.rb | 30 +++++++ app/models/cycle_analytics/project_level.rb | 5 +- app/serializers/analytics_issue_entity.rb | 6 +- app/serializers/analytics_merge_request_entity.rb | 2 +- app/serializers/analytics_stage_entity.rb | 4 +- app/serializers/group_analytics_stage_entity.rb | 16 ++++ .../group_analytics_stage_serializer.rb | 5 ++ .../adjust-cycle-analytics-to-group-level.yml | 5 ++ lib/gitlab/cycle_analytics/base_event_fetcher.rb | 21 +++-- lib/gitlab/cycle_analytics/base_query.rb | 14 +++- lib/gitlab/cycle_analytics/base_stage.rb | 24 ++++-- lib/gitlab/cycle_analytics/code_event_fetcher.rb | 6 +- lib/gitlab/cycle_analytics/group_stage_summary.rb | 24 ++++++ lib/gitlab/cycle_analytics/issue_event_fetcher.rb | 6 +- lib/gitlab/cycle_analytics/issue_helper.rb | 3 + lib/gitlab/cycle_analytics/metrics_tables.rb | 8 ++ lib/gitlab/cycle_analytics/permissions.rb | 2 +- lib/gitlab/cycle_analytics/plan_event_fetcher.rb | 6 +- lib/gitlab/cycle_analytics/plan_helper.rb | 13 ++- .../cycle_analytics/production_event_fetcher.rb | 6 +- lib/gitlab/cycle_analytics/review_event_fetcher.rb | 6 +- lib/gitlab/cycle_analytics/summary/group/base.rb | 24 ++++++ lib/gitlab/cycle_analytics/summary/group/deploy.rb | 29 +++++++ lib/gitlab/cycle_analytics/summary/group/issue.rb | 25 ++++++ .../cycle_analytics/base_event_fetcher_spec.rb | 6 +- spec/lib/gitlab/cycle_analytics/code_stage_spec.rb | 94 +++++++++++++++++++--- .../cycle_analytics/group_stage_summary_spec.rb | 65 +++++++++++++++ .../lib/gitlab/cycle_analytics/issue_stage_spec.rb | 75 +++++++++++++++-- spec/lib/gitlab/cycle_analytics/plan_stage_spec.rb | 74 ++++++++++++++++- .../gitlab/cycle_analytics/review_stage_spec.rb | 61 +++++++++++--- .../gitlab/cycle_analytics/shared_event_spec.rb | 2 +- .../gitlab/cycle_analytics/shared_stage_spec.rb | 4 +- .../gitlab/cycle_analytics/staging_stage_spec.rb | 64 ++++++++++++--- spec/lib/gitlab/cycle_analytics/usage_data_spec.rb | 2 +- spec/models/cycle_analytics/code_spec.rb | 4 +- spec/models/cycle_analytics/group_level_spec.rb | 46 +++++++++++ spec/models/cycle_analytics/issue_spec.rb | 2 +- spec/models/cycle_analytics/plan_spec.rb | 2 +- spec/models/cycle_analytics/production_spec.rb | 4 +- spec/models/cycle_analytics/project_level_spec.rb | 2 +- spec/models/cycle_analytics/review_spec.rb | 2 +- spec/models/cycle_analytics/staging_spec.rb | 4 +- spec/models/cycle_analytics/test_spec.rb | 8 +- spec/serializers/analytics_issue_entity_spec.rb | 6 +- .../serializers/analytics_issue_serializer_spec.rb | 6 +- .../analytics_merge_request_serializer_spec.rb | 6 +- .../serializers/analytics_stage_serializer_spec.rb | 10 +-- .../cycle_analytics_helpers/test_generation.rb | 12 +-- 50 files changed, 768 insertions(+), 137 deletions(-) delete mode 100644 app/models/cycle_analytics/base.rb create mode 100644 app/models/cycle_analytics/base_methods.rb create mode 100644 app/models/cycle_analytics/group_level.rb create mode 100644 app/serializers/group_analytics_stage_entity.rb create mode 100644 app/serializers/group_analytics_stage_serializer.rb create mode 100644 changelogs/unreleased/adjust-cycle-analytics-to-group-level.yml create mode 100644 lib/gitlab/cycle_analytics/group_stage_summary.rb create mode 100644 lib/gitlab/cycle_analytics/summary/group/base.rb create mode 100644 lib/gitlab/cycle_analytics/summary/group/deploy.rb create mode 100644 lib/gitlab/cycle_analytics/summary/group/issue.rb create mode 100644 spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb create mode 100644 spec/models/cycle_analytics/group_level_spec.rb diff --git a/app/models/cycle_analytics/base.rb b/app/models/cycle_analytics/base.rb deleted file mode 100644 index d7b28cd1b67..00000000000 --- a/app/models/cycle_analytics/base.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -module CycleAnalytics - class Base - STAGES = %i[issue plan code test review staging production].freeze - - def all_medians_by_stage - STAGES.each_with_object({}) do |stage_name, medians_per_stage| - medians_per_stage[stage_name] = self[stage_name].median - end - end - - def stats - @stats ||= STAGES.map do |stage_name| - self[stage_name].as_json - end - end - - def no_stats? - stats.all? { |hash| hash[:value].nil? } - end - - def [](stage_name) - Gitlab::CycleAnalytics::Stage[stage_name].new(project: @project, options: @options) - end - end -end diff --git a/app/models/cycle_analytics/base_methods.rb b/app/models/cycle_analytics/base_methods.rb new file mode 100644 index 00000000000..1c2a0854769 --- /dev/null +++ b/app/models/cycle_analytics/base_methods.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module CycleAnalytics + module BaseMethods + STAGES = %i[issue plan code test review staging production].freeze + + def all_medians_by_stage + STAGES.each_with_object({}) do |stage_name, medians_per_stage| + medians_per_stage[stage_name] = self[stage_name].project_median + end + end + + def stats + @stats ||= STAGES.map do |stage_name| + self[stage_name].as_json + end + end + + def no_stats? + stats.all? { |hash| hash[:value].nil? } + end + + def [](stage_name) + Gitlab::CycleAnalytics::Stage[stage_name].new(options: options) + end + end +end diff --git a/app/models/cycle_analytics/group_level.rb b/app/models/cycle_analytics/group_level.rb new file mode 100644 index 00000000000..58bd2eb0d9a --- /dev/null +++ b/app/models/cycle_analytics/group_level.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module CycleAnalytics + class GroupLevel + include BaseMethods + attr_reader :options + + def initialize(options:) + @options = options + end + + def summary + @summary ||= ::Gitlab::CycleAnalytics::GroupStageSummary.new(options[:group], + from: options[:from], + current_user: options[:current_user]).data + end + + def permissions(user: nil) + STAGES.each_with_object({}) do |stage, obj| + obj[stage] = true + end + end + + def stats + @stats ||= STAGES.map do |stage_name| + self[stage_name].as_json(serializer: GroupAnalyticsStageSerializer) + end + end + end +end diff --git a/app/models/cycle_analytics/project_level.rb b/app/models/cycle_analytics/project_level.rb index 22631cc7d41..abe1be11eae 100644 --- a/app/models/cycle_analytics/project_level.rb +++ b/app/models/cycle_analytics/project_level.rb @@ -1,12 +1,13 @@ # frozen_string_literal: true module CycleAnalytics - class ProjectLevel < Base + class ProjectLevel + include BaseMethods attr_reader :project, :options def initialize(project, options:) @project = project - @options = options + @options = options.merge(project: project) end def summary diff --git a/app/serializers/analytics_issue_entity.rb b/app/serializers/analytics_issue_entity.rb index ab15bd0ac7a..29d4a6ae1d0 100644 --- a/app/serializers/analytics_issue_entity.rb +++ b/app/serializers/analytics_issue_entity.rb @@ -20,12 +20,12 @@ class AnalyticsIssueEntity < Grape::Entity end expose :url do |object| - url_to(:namespace_project_issue, id: object[:iid].to_s) + url_to(:namespace_project_issue, object) end private - def url_to(route, id) - public_send("#{route}_url", request.project.namespace, request.project, id) # rubocop:disable GitlabSecurity/PublicSend + def url_to(route, object) + public_send("#{route}_url", object[:path], object[:name], object[:iid].to_s) # rubocop:disable GitlabSecurity/PublicSend end end diff --git a/app/serializers/analytics_merge_request_entity.rb b/app/serializers/analytics_merge_request_entity.rb index b7134da9461..21d7eeb81b0 100644 --- a/app/serializers/analytics_merge_request_entity.rb +++ b/app/serializers/analytics_merge_request_entity.rb @@ -4,6 +4,6 @@ class AnalyticsMergeRequestEntity < AnalyticsIssueEntity expose :state expose :url do |object| - url_to(:namespace_project_merge_request, id: object[:iid].to_s) + url_to(:namespace_project_merge_request, object) end end diff --git a/app/serializers/analytics_stage_entity.rb b/app/serializers/analytics_stage_entity.rb index 8bc6da5aeeb..eb38b90fb18 100644 --- a/app/serializers/analytics_stage_entity.rb +++ b/app/serializers/analytics_stage_entity.rb @@ -8,9 +8,9 @@ class AnalyticsStageEntity < Grape::Entity expose :legend expose :description - expose :median, as: :value do |stage| + expose :project_median, as: :value do |stage| # median returns a BatchLoader instance which we first have to unwrap by using to_f # we use to_f to make sure results below 1 are presented to the end-user - stage.median.to_f.nonzero? ? distance_of_time_in_words(stage.median) : nil + stage.project_median.to_f.nonzero? ? distance_of_time_in_words(stage.project_median) : nil end end diff --git a/app/serializers/group_analytics_stage_entity.rb b/app/serializers/group_analytics_stage_entity.rb new file mode 100644 index 00000000000..019a3086f68 --- /dev/null +++ b/app/serializers/group_analytics_stage_entity.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class GroupAnalyticsStageEntity < Grape::Entity + include EntityDateHelper + + expose :title + expose :name + expose :legend + expose :description + + expose :group_median, as: :value do |stage| + # median returns a BatchLoader instance which we first have to unwrap by using to_f + # we use to_f to make sure results below 1 are presented to the end-user + stage.group_median.to_f.nonzero? ? distance_of_time_in_words(stage.group_median) : nil + end +end diff --git a/app/serializers/group_analytics_stage_serializer.rb b/app/serializers/group_analytics_stage_serializer.rb new file mode 100644 index 00000000000..ec448dea602 --- /dev/null +++ b/app/serializers/group_analytics_stage_serializer.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class GroupAnalyticsStageSerializer < BaseSerializer + entity GroupAnalyticsStageEntity +end diff --git a/changelogs/unreleased/adjust-cycle-analytics-to-group-level.yml b/changelogs/unreleased/adjust-cycle-analytics-to-group-level.yml new file mode 100644 index 00000000000..49f558a6c9c --- /dev/null +++ b/changelogs/unreleased/adjust-cycle-analytics-to-group-level.yml @@ -0,0 +1,5 @@ +--- +title: Adjust cycle analytics to group level +merge_request: 30391 +author: +type: added diff --git a/lib/gitlab/cycle_analytics/base_event_fetcher.rb b/lib/gitlab/cycle_analytics/base_event_fetcher.rb index 0cacef5b278..96aa799e864 100644 --- a/lib/gitlab/cycle_analytics/base_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/base_event_fetcher.rb @@ -5,12 +5,11 @@ module Gitlab class BaseEventFetcher include BaseQuery - attr_reader :projections, :query, :stage, :order, :project, :options + attr_reader :projections, :query, :stage, :order, :options MAX_EVENTS = 50 - def initialize(project: nil, stage:, options:) - @project = project + def initialize(stage:, options:) @stage = stage @options = options end @@ -68,11 +67,23 @@ module Gitlab end def allowed_ids_source - { project_id: project.id } + group ? { group_id: group.id, include_subgroups: true } : { project_id: project.id } + end + + def serialization_context + {} end def projects - [project] + group ? Project.inside_path(group.full_path) : [project] + end + + def group + @group ||= options.fetch(:group, nil) + end + + def project + @project ||= options.fetch(:project, nil) end end end diff --git a/lib/gitlab/cycle_analytics/base_query.rb b/lib/gitlab/cycle_analytics/base_query.rb index 39fc1759cfc..9c98c0bfbf2 100644 --- a/lib/gitlab/cycle_analytics/base_query.rb +++ b/lib/gitlab/cycle_analytics/base_query.rb @@ -16,17 +16,25 @@ module Gitlab def stage_query(project_ids) query = mr_closing_issues_table.join(issue_table).on(issue_table[:id].eq(mr_closing_issues_table[:issue_id])) .join(issue_metrics_table).on(issue_table[:id].eq(issue_metrics_table[:issue_id])) + .join(projects_table).on(issue_table[:project_id].eq(projects_table[:id])) + .join(routes_table).on(projects_table[:namespace_id].eq(routes_table[:source_id])) .project(issue_table[:project_id].as("project_id")) .where(issue_table[:project_id].in(project_ids)) + .where(routes_table[:source_type].eq('Namespace')) .where(issue_table[:created_at].gteq(options[:from])) # Load merge_requests - query = query.join(mr_table, Arel::Nodes::OuterJoin) + + query = load_merge_requests(query) + + query + end + + def load_merge_requests(query) + query.join(mr_table, Arel::Nodes::OuterJoin) .on(mr_table[:id].eq(mr_closing_issues_table[:merge_request_id])) .join(mr_metrics_table) .on(mr_table[:id].eq(mr_metrics_table[:merge_request_id])) - - query end end end diff --git a/lib/gitlab/cycle_analytics/base_stage.rb b/lib/gitlab/cycle_analytics/base_stage.rb index 98b86e54340..678a891e941 100644 --- a/lib/gitlab/cycle_analytics/base_stage.rb +++ b/lib/gitlab/cycle_analytics/base_stage.rb @@ -5,10 +5,9 @@ module Gitlab class BaseStage include BaseQuery - attr_reader :project, :options + attr_reader :options - def initialize(project: nil, options:) - @project = project + def initialize(options:) @options = options end @@ -24,7 +23,7 @@ module Gitlab raise NotImplementedError.new("Expected #{self.name} to implement title") end - def median + def project_median return if project.nil? BatchLoader.for(project.id).batch(key: name) do |project_ids, loader| @@ -42,6 +41,10 @@ module Gitlab end end + def group_median + median_query(projects.map(&:id)) + end + def median_query(project_ids) # Build a `SELECT` query. We find the first of the `end_time_attrs` that isn't `NULL` (call this end_time). # Next, we find the first of the start_time_attrs that isn't `NULL` (call this start_time). @@ -67,8 +70,7 @@ module Gitlab private def event_fetcher - @event_fetcher ||= Gitlab::CycleAnalytics::EventFetcher[name].new(project: project, - stage: name, + @event_fetcher ||= Gitlab::CycleAnalytics::EventFetcher[name].new(stage: name, options: event_options) end @@ -77,7 +79,15 @@ module Gitlab end def projects - [project] + group ? Project.inside_path(group.full_path) : [project] + end + + def group + @group ||= options.fetch(:group, nil) + end + + def project + @project ||= options.fetch(:project, nil) end end end diff --git a/lib/gitlab/cycle_analytics/code_event_fetcher.rb b/lib/gitlab/cycle_analytics/code_event_fetcher.rb index 9e7ca529579..1e4e9b9e02c 100644 --- a/lib/gitlab/cycle_analytics/code_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/code_event_fetcher.rb @@ -11,7 +11,9 @@ module Gitlab mr_table[:id], mr_table[:created_at], mr_table[:state], - mr_table[:author_id]] + mr_table[:author_id], + projects_table[:name], + routes_table[:path]] @order = mr_table[:created_at] super(*args) @@ -20,7 +22,7 @@ module Gitlab private def serialize(event) - AnalyticsMergeRequestSerializer.new(project: project).represent(event) + AnalyticsMergeRequestSerializer.new(serialization_context).represent(event) end def allowed_ids_finder_class diff --git a/lib/gitlab/cycle_analytics/group_stage_summary.rb b/lib/gitlab/cycle_analytics/group_stage_summary.rb new file mode 100644 index 00000000000..7b5c74e1a1b --- /dev/null +++ b/lib/gitlab/cycle_analytics/group_stage_summary.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Gitlab + module CycleAnalytics + class GroupStageSummary + def initialize(group, from:, current_user:) + @group = group + @from = from + @current_user = current_user + end + + def data + [serialize(Summary::Group::Issue.new(group: @group, from: @from, current_user: @current_user)), + serialize(Summary::Group::Deploy.new(group: @group, from: @from))] + end + + private + + def serialize(summary_object) + AnalyticsSummarySerializer.new.represent(summary_object) + end + end + end +end diff --git a/lib/gitlab/cycle_analytics/issue_event_fetcher.rb b/lib/gitlab/cycle_analytics/issue_event_fetcher.rb index bb3520ae920..2d03e425a6a 100644 --- a/lib/gitlab/cycle_analytics/issue_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/issue_event_fetcher.rb @@ -10,7 +10,9 @@ module Gitlab issue_table[:iid], issue_table[:id], issue_table[:created_at], - issue_table[:author_id]] + issue_table[:author_id], + projects_table[:name], + routes_table[:path]] super(*args) end @@ -18,7 +20,7 @@ module Gitlab private def serialize(event) - AnalyticsIssueSerializer.new(project: project).represent(event) + AnalyticsIssueSerializer.new(serialization_context).represent(event) end def allowed_ids_finder_class diff --git a/lib/gitlab/cycle_analytics/issue_helper.rb b/lib/gitlab/cycle_analytics/issue_helper.rb index ac836b8bf0f..0fc4f1dd41a 100644 --- a/lib/gitlab/cycle_analytics/issue_helper.rb +++ b/lib/gitlab/cycle_analytics/issue_helper.rb @@ -5,8 +5,11 @@ module Gitlab module IssueHelper def stage_query(project_ids) query = issue_table.join(issue_metrics_table).on(issue_table[:id].eq(issue_metrics_table[:issue_id])) + .join(projects_table).on(issue_table[:project_id].eq(projects_table[:id])) + .join(routes_table).on(projects_table[:namespace_id].eq(routes_table[:source_id])) .project(issue_table[:project_id].as("project_id")) .where(issue_table[:project_id].in(project_ids)) + .where(routes_table[:source_type].eq('Namespace')) .where(issue_table[:created_at].gteq(options[:from])) .where(issue_metrics_table[:first_added_to_board_at].not_eq(nil).or(issue_metrics_table[:first_associated_with_milestone_at].not_eq(nil))) diff --git a/lib/gitlab/cycle_analytics/metrics_tables.rb b/lib/gitlab/cycle_analytics/metrics_tables.rb index 3e0302d308d..015f7bfde24 100644 --- a/lib/gitlab/cycle_analytics/metrics_tables.rb +++ b/lib/gitlab/cycle_analytics/metrics_tables.rb @@ -35,6 +35,14 @@ module Gitlab User.arel_table end + def projects_table + Project.arel_table + end + + def routes_table + Route.arel_table + end + def build_table ::CommitStatus.arel_table end diff --git a/lib/gitlab/cycle_analytics/permissions.rb b/lib/gitlab/cycle_analytics/permissions.rb index 03ba98b4dfb..0da041f8950 100644 --- a/lib/gitlab/cycle_analytics/permissions.rb +++ b/lib/gitlab/cycle_analytics/permissions.rb @@ -23,7 +23,7 @@ module Gitlab end def get - ::CycleAnalytics::Base::STAGES.each do |stage| + ::CycleAnalytics::BaseMethods::STAGES.each do |stage| @stage_permission_hash[stage] = authorized_stage?(stage) end diff --git a/lib/gitlab/cycle_analytics/plan_event_fetcher.rb b/lib/gitlab/cycle_analytics/plan_event_fetcher.rb index 49a6b099f34..77cc358daa9 100644 --- a/lib/gitlab/cycle_analytics/plan_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/plan_event_fetcher.rb @@ -10,7 +10,9 @@ module Gitlab issue_table[:iid], issue_table[:id], issue_table[:created_at], - issue_table[:author_id]] + issue_table[:author_id], + projects_table[:name], + routes_table[:path]] super(*args) end @@ -18,7 +20,7 @@ module Gitlab private def serialize(event) - AnalyticsIssueSerializer.new(project: project).represent(event) + AnalyticsIssueSerializer.new(serialization_context).represent(event) end def allowed_ids_finder_class diff --git a/lib/gitlab/cycle_analytics/plan_helper.rb b/lib/gitlab/cycle_analytics/plan_helper.rb index ae578d45ad5..c3f742503a9 100644 --- a/lib/gitlab/cycle_analytics/plan_helper.rb +++ b/lib/gitlab/cycle_analytics/plan_helper.rb @@ -5,14 +5,21 @@ module Gitlab module PlanHelper def stage_query(project_ids) query = issue_table.join(issue_metrics_table).on(issue_table[:id].eq(issue_metrics_table[:issue_id])) + .join(projects_table).on(issue_table[:project_id].eq(projects_table[:id])) + .join(routes_table).on(projects_table[:namespace_id].eq(routes_table[:source_id])) .project(issue_table[:project_id].as("project_id")) .where(issue_table[:project_id].in(project_ids)) - .where(issue_table[:created_at].gteq(options[:from])) - .where(issue_metrics_table[:first_added_to_board_at].not_eq(nil).or(issue_metrics_table[:first_associated_with_milestone_at].not_eq(nil))) - .where(issue_metrics_table[:first_mentioned_in_commit_at].not_eq(nil)) + .where(routes_table[:source_type].eq('Namespace')) + query = add_conditions_to_query(query) query end + + def add_conditions_to_query(query) + query.where(issue_table[:created_at].gteq(options[:from])) + .where(issue_metrics_table[:first_added_to_board_at].not_eq(nil).or(issue_metrics_table[:first_associated_with_milestone_at].not_eq(nil))) + .where(issue_metrics_table[:first_mentioned_in_commit_at].not_eq(nil)) + end end end end diff --git a/lib/gitlab/cycle_analytics/production_event_fetcher.rb b/lib/gitlab/cycle_analytics/production_event_fetcher.rb index 949119d69a0..404b2460814 100644 --- a/lib/gitlab/cycle_analytics/production_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/production_event_fetcher.rb @@ -10,7 +10,9 @@ module Gitlab issue_table[:iid], issue_table[:id], issue_table[:created_at], - issue_table[:author_id]] + issue_table[:author_id], + projects_table[:name], + routes_table[:path]] super(*args) end @@ -18,7 +20,7 @@ module Gitlab private def serialize(event) - AnalyticsIssueSerializer.new(project: project).represent(event) + AnalyticsIssueSerializer.new(serialization_context).represent(event) end def allowed_ids_finder_class diff --git a/lib/gitlab/cycle_analytics/review_event_fetcher.rb b/lib/gitlab/cycle_analytics/review_event_fetcher.rb index d31736e755d..6acd12517fa 100644 --- a/lib/gitlab/cycle_analytics/review_event_fetcher.rb +++ b/lib/gitlab/cycle_analytics/review_event_fetcher.rb @@ -11,7 +11,9 @@ module Gitlab mr_table[:id], mr_table[:created_at], mr_table[:state], - mr_table[:author_id]] + mr_table[:author_id], + projects_table[:name], + routes_table[:path]] super(*args) end @@ -19,7 +21,7 @@ module Gitlab private def serialize(event) - AnalyticsMergeRequestSerializer.new(project: project).represent(event) + AnalyticsMergeRequestSerializer.new(serialization_context).represent(event) end def allowed_ids_finder_class diff --git a/lib/gitlab/cycle_analytics/summary/group/base.rb b/lib/gitlab/cycle_analytics/summary/group/base.rb new file mode 100644 index 00000000000..7f18b61d309 --- /dev/null +++ b/lib/gitlab/cycle_analytics/summary/group/base.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Gitlab + module CycleAnalytics + module Summary + module Group + class Base + def initialize(group:, from:) + @group = group + @from = from + end + + def title + raise NotImplementedError.new("Expected #{self.name} to implement title") + end + + def value + raise NotImplementedError.new("Expected #{self.name} to implement value") + end + end + end + end + end +end diff --git a/lib/gitlab/cycle_analytics/summary/group/deploy.rb b/lib/gitlab/cycle_analytics/summary/group/deploy.rb new file mode 100644 index 00000000000..d8fcd8f2ce4 --- /dev/null +++ b/lib/gitlab/cycle_analytics/summary/group/deploy.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Gitlab + module CycleAnalytics + module Summary + module Group + class Deploy < Group::Base + def title + n_('Deploy', 'Deploys', value) + end + + def value + @value ||= Deployment.joins(:project) + .where(projects: { id: projects }) + .where("deployments.created_at > ?", @from) + .success + .count + end + + private + + def projects + Project.inside_path(@group.full_path).ids + end + end + end + end + end +end diff --git a/lib/gitlab/cycle_analytics/summary/group/issue.rb b/lib/gitlab/cycle_analytics/summary/group/issue.rb new file mode 100644 index 00000000000..a5188056cb7 --- /dev/null +++ b/lib/gitlab/cycle_analytics/summary/group/issue.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Gitlab + module CycleAnalytics + module Summary + module Group + class Issue < Group::Base + def initialize(group:, from:, current_user:) + @group = group + @from = from + @current_user = current_user + end + + def title + n_('New Issue', 'New Issues', value) + end + + def value + @value ||= IssuesFinder.new(@current_user, group_id: @group.id, include_subgroups: true).execute.created_after(@from).count + end + end + end + end + end +end diff --git a/spec/lib/gitlab/cycle_analytics/base_event_fetcher_spec.rb b/spec/lib/gitlab/cycle_analytics/base_event_fetcher_spec.rb index 8b07da11c5d..b7a64adc2ff 100644 --- a/spec/lib/gitlab/cycle_analytics/base_event_fetcher_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/base_event_fetcher_spec.rb @@ -9,12 +9,12 @@ describe Gitlab::CycleAnalytics::BaseEventFetcher do let(:options) do { start_time_attrs: start_time_attrs, end_time_attrs: end_time_attrs, - from: 30.days.ago } + from: 30.days.ago, + project: project } end subject do - described_class.new(project: project, - stage: :issue, + described_class.new(stage: :issue, options: options).fetch end diff --git a/spec/lib/gitlab/cycle_analytics/code_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/code_stage_spec.rb index c738cc49c1f..68f73ba3c93 100644 --- a/spec/lib/gitlab/cycle_analytics/code_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/code_stage_spec.rb @@ -5,31 +5,31 @@ describe Gitlab::CycleAnalytics::CodeStage do let(:stage_name) { :code } let(:project) { create(:project) } - let!(:issue_1) { create(:issue, project: project, created_at: 90.minutes.ago) } - let!(:issue_2) { create(:issue, project: project, created_at: 60.minutes.ago) } - let!(:issue_3) { create(:issue, project: project, created_at: 60.minutes.ago) } - let!(:mr_1) { create(:merge_request, source_project: project, created_at: 15.minutes.ago) } - let!(:mr_2) { create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'A') } - let!(:mr_3) { create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'B') } - let(:stage) { described_class.new(project: project, options: { from: 2.days.ago, current_user: project.creator }) } + let(:issue_1) { create(:issue, project: project, created_at: 90.minutes.ago) } + let(:issue_2) { create(:issue, project: project, created_at: 60.minutes.ago) } + let(:issue_3) { create(:issue, project: project, created_at: 60.minutes.ago) } + let(:mr_1) { create(:merge_request, source_project: project, created_at: 15.minutes.ago) } + let(:mr_2) { create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'A') } + let(:stage) { described_class.new(options: { from: 2.days.ago, current_user: project.creator, project: project }) } before do issue_1.metrics.update!(first_associated_with_milestone_at: 60.minutes.ago, first_mentioned_in_commit_at: 45.minutes.ago) issue_2.metrics.update!(first_added_to_board_at: 60.minutes.ago, first_mentioned_in_commit_at: 40.minutes.ago) issue_3.metrics.update!(first_added_to_board_at: 60.minutes.ago, first_mentioned_in_commit_at: 40.minutes.ago) + create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'B') create(:merge_requests_closing_issues, merge_request: mr_1, issue: issue_1) create(:merge_requests_closing_issues, merge_request: mr_2, issue: issue_2) end it_behaves_like 'base stage' - describe '#median' do + describe '#project_median' do around do |example| Timecop.freeze { example.run } end it 'counts median from issues with metrics' do - expect(stage.median).to eq(ISSUES_MEDIAN) + expect(stage.project_median).to eq(ISSUES_MEDIAN) end end @@ -41,4 +41,80 @@ describe Gitlab::CycleAnalytics::CodeStage do expect(result.map { |event| event[:title] }).to contain_exactly(mr_1.title, mr_2.title) end end + + context 'when group is given' do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:project_2) { create(:project, group: group) } + let(:project_3) { create(:project, group: group) } + let(:issue_2_1) { create(:issue, project: project_2, created_at: 90.minutes.ago) } + let(:issue_2_2) { create(:issue, project: project_3, created_at: 60.minutes.ago) } + let(:issue_2_3) { create(:issue, project: project_2, created_at: 60.minutes.ago) } + let(:mr_2_1) { create(:merge_request, source_project: project_2, created_at: 15.minutes.ago) } + let(:mr_2_2) { create(:merge_request, source_project: project_3, created_at: 10.minutes.ago, source_branch: 'A') } + let(:stage) { described_class.new(options: { from: 2.days.ago, current_user: user, group: group }) } + + before do + group.add_owner(user) + issue_2_1.metrics.update!(first_associated_with_milestone_at: 60.minutes.ago, first_mentioned_in_commit_at: 45.minutes.ago) + issue_2_2.metrics.update!(first_added_to_board_at: 60.minutes.ago, first_mentioned_in_commit_at: 40.minutes.ago) + issue_2_3.metrics.update!(first_added_to_board_at: 60.minutes.ago, first_mentioned_in_commit_at: 40.minutes.ago) + create(:merge_requests_closing_issues, merge_request: mr_2_1, issue: issue_2_1) + create(:merge_requests_closing_issues, merge_request: mr_2_2, issue: issue_2_2) + end + + describe '#group_median' do + around do |example| + Timecop.freeze { example.run } + end + + it 'counts median from issues with metrics' do + expect(stage.group_median).to eq(ISSUES_MEDIAN) + end + end + + describe '#events' do + it 'exposes merge requests that close issues' do + result = stage.events + + expect(result.count).to eq(2) + expect(result.map { |event| event[:title] }).to contain_exactly(mr_2_1.title, mr_2_2.title) + end + end + + context 'when subgroup is given' do + let(:subgroup) { create(:group, parent: group) } + let(:project_4) { create(:project, group: subgroup) } + let(:project_5) { create(:project, group: subgroup) } + let(:issue_3_1) { create(:issue, project: project_4, created_at: 90.minutes.ago) } + let(:issue_3_2) { create(:issue, project: project_5, created_at: 60.minutes.ago) } + let(:issue_3_3) { create(:issue, project: project_5, created_at: 60.minutes.ago) } + let(:mr_3_1) { create(:merge_request, source_project: project_4, created_at: 15.minutes.ago) } + let(:mr_3_2) { create(:merge_request, source_project: project_5, created_at: 10.minutes.ago, source_branch: 'A') } + + before do + issue_3_1.metrics.update!(first_associated_with_milestone_at: 60.minutes.ago, first_mentioned_in_commit_at: 45.minutes.ago) + issue_3_2.metrics.update!(first_added_to_board_at: 60.minutes.ago, first_mentioned_in_commit_at: 40.minutes.ago) + issue_3_3.metrics.update!(first_added_to_board_at: 60.minutes.ago, first_mentioned_in_commit_at: 40.minutes.ago) + create(:merge_requests_closing_issues, merge_request: mr_3_1, issue: issue_3_1) + create(:merge_requests_closing_issues, merge_request: mr_3_2, issue: issue_3_2) + end + + describe '#events' do + it 'exposes merge requests that close issues' do + result = stage.events + + expect(result.count).to eq(4) + expect(result.map { |event| event[:title] }).to contain_exactly(mr_2_1.title, mr_2_2.title, mr_3_1.title, mr_3_2.title) + end + + it 'exposes merge requests that close issues with full path for subgroup' do + result = stage.events + + expect(result.count).to eq(4) + expect(result.find { |event| event[:title] == mr_3_1.title }[:url]).to include("#{subgroup.full_path}") + end + end + end + end end diff --git a/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb b/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb new file mode 100644 index 00000000000..8e552b23283 --- /dev/null +++ b/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe Gitlab::CycleAnalytics::GroupStageSummary do + let(:group) { create(:group) } + let(:project) { create(:project, :repository, namespace: group) } + let(:project_2) { create(:project, :repository, namespace: group) } + let(:from) { 1.day.ago } + let(:user) { create(:user, :admin) } + subject { described_class.new(group, from: Time.now, current_user: user).data } + + describe "#new_issues" do + it "finds the number of issues created after the 'from date'" do + Timecop.freeze(5.days.ago) { create(:issue, project: project) } + Timecop.freeze(5.days.ago) { create(:issue, project: project_2) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project_2) } + + expect(subject.first[:value]).to eq(2) + end + + it "doesn't find issues from other projects" do + Timecop.freeze(5.days.from_now) { create(:issue, project: create(:project, namespace: create(:group))) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project_2) } + + expect(subject.first[:value]).to eq(2) + end + + it "finds issues from subgroups" do + Timecop.freeze(5.days.from_now) { create(:issue, project: create(:project, namespace: create(:group, parent: group))) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project_2) } + + expect(subject.first[:value]).to eq(3) + end + end + + describe "#deploys" do + it "finds the number of deploys made created after the 'from date'" do + Timecop.freeze(5.days.ago) { create(:deployment, :success, project: project) } + Timecop.freeze(5.days.from_now) { create(:deployment, :success, project: project) } + Timecop.freeze(5.days.ago) { create(:deployment, :success, project: project_2) } + Timecop.freeze(5.days.from_now) { create(:deployment, :success, project: project_2) } + + expect(subject.second[:value]).to eq(2) + end + + it "doesn't find deploys from other projects" do + Timecop.freeze(5.days.from_now) do + create(:deployment, :success, project: create(:project, :repository, namespace: create(:group))) + end + + expect(subject.second[:value]).to eq(0) + end + + it "finds deploys from subgroups" do + Timecop.freeze(5.days.from_now) do + create(:deployment, :success, project: create(:project, :repository, namespace: create(:group, parent: group))) + end + + expect(subject.second[:value]).to eq(1) + end + end +end diff --git a/spec/lib/gitlab/cycle_analytics/issue_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/issue_stage_spec.rb index 3b6af9cbaed..64ac9df52b2 100644 --- a/spec/lib/gitlab/cycle_analytics/issue_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/issue_stage_spec.rb @@ -4,11 +4,11 @@ require 'lib/gitlab/cycle_analytics/shared_stage_spec' describe Gitlab::CycleAnalytics::IssueStage do let(:stage_name) { :issue } let(:project) { create(:project) } - let!(:issue_1) { create(:issue, project: project, created_at: 90.minutes.ago) } - let!(:issue_2) { create(:issue, project: project, created_at: 60.minutes.ago) } - let!(:issue_3) { create(:issue, project: project, created_at: 30.minutes.ago) } + let(:issue_1) { create(:issue, project: project, created_at: 90.minutes.ago) } + let(:issue_2) { create(:issue, project: project, created_at: 60.minutes.ago) } + let(:issue_3) { create(:issue, project: project, created_at: 30.minutes.ago) } let!(:issue_without_milestone) { create(:issue, project: project, created_at: 1.minute.ago) } - let(:stage) { described_class.new(project: project, options: { from: 2.days.ago, current_user: project.creator }) } + let(:stage) { described_class.new(options: { from: 2.days.ago, current_user: project.creator, project: project }) } before do issue_1.metrics.update!(first_associated_with_milestone_at: 60.minutes.ago ) @@ -24,7 +24,7 @@ describe Gitlab::CycleAnalytics::IssueStage do end it 'counts median from issues with metrics' do - expect(stage.median).to eq(ISSUES_MEDIAN) + expect(stage.project_median).to eq(ISSUES_MEDIAN) end end @@ -36,4 +36,69 @@ describe Gitlab::CycleAnalytics::IssueStage do expect(result.map { |event| event[:title] }).to contain_exactly(issue_1.title, issue_2.title, issue_3.title) end end + context 'when group is given' do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:project_2) { create(:project, group: group) } + let(:project_3) { create(:project, group: group) } + let(:issue_2_1) { create(:issue, project: project_2, created_at: 90.minutes.ago) } + let(:issue_2_2) { create(:issue, project: project_3, created_at: 60.minutes.ago) } + let(:issue_2_3) { create(:issue, project: project_2, created_at: 60.minutes.ago) } + let(:stage) { described_class.new(options: { from: 2.days.ago, current_user: user, group: group }) } + + before do + group.add_owner(user) + issue_2_1.metrics.update!(first_associated_with_milestone_at: 60.minutes.ago) + issue_2_2.metrics.update!(first_added_to_board_at: 30.minutes.ago) + end + + describe '#group_median' do + around do |example| + Timecop.freeze { example.run } + end + + it 'counts median from issues with metrics' do + expect(stage.group_median).to eq(ISSUES_MEDIAN) + end + end + + describe '#events' do + it 'exposes merge requests that close issues' do + result = stage.events + + expect(result.count).to eq(2) + expect(result.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title) + end + end + + context 'when subgroup is given' do + let(:subgroup) { create(:group, parent: group) } + let(:project_4) { create(:project, group: subgroup) } + let(:project_5) { create(:project, group: subgroup) } + let(:issue_3_1) { create(:issue, project: project_4, created_at: 90.minutes.ago) } + let(:issue_3_2) { create(:issue, project: project_5, created_at: 60.minutes.ago) } + let(:issue_3_3) { create(:issue, project: project_5, created_at: 60.minutes.ago) } + + before do + issue_3_1.metrics.update!(first_associated_with_milestone_at: 60.minutes.ago) + issue_3_2.metrics.update!(first_added_to_board_at: 30.minutes.ago) + end + + describe '#events' do + it 'exposes merge requests that close issues' do + result = stage.events + + expect(result.count).to eq(4) + expect(result.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title, issue_3_1.title, issue_3_2.title) + end + + it 'exposes merge requests that close issues with full path for subgroup' do + result = stage.events + + expect(result.count).to eq(4) + expect(result.find { |event| event[:title] == issue_3_1.title }[:url]).to include("#{subgroup.full_path}") + end + end + end + end end diff --git a/spec/lib/gitlab/cycle_analytics/plan_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/plan_stage_spec.rb index 506a8160412..55de6192af1 100644 --- a/spec/lib/gitlab/cycle_analytics/plan_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/plan_stage_spec.rb @@ -8,7 +8,7 @@ describe Gitlab::CycleAnalytics::PlanStage do let!(:issue_2) { create(:issue, project: project, created_at: 60.minutes.ago) } let!(:issue_3) { create(:issue, project: project, created_at: 30.minutes.ago) } let!(:issue_without_milestone) { create(:issue, project: project, created_at: 1.minute.ago) } - let(:stage) { described_class.new(project: project, options: { from: 2.days.ago, current_user: project.creator }) } + let(:stage) { described_class.new(options: { from: 2.days.ago, current_user: project.creator, project: project }) } before do issue_1.metrics.update!(first_associated_with_milestone_at: 60.minutes.ago, first_mentioned_in_commit_at: 10.minutes.ago) @@ -18,13 +18,13 @@ describe Gitlab::CycleAnalytics::PlanStage do it_behaves_like 'base stage' - describe '#median' do + describe '#project_median' do around do |example| Timecop.freeze { example.run } end it 'counts median from issues with metrics' do - expect(stage.median).to eq(ISSUES_MEDIAN) + expect(stage.project_median).to eq(ISSUES_MEDIAN) end end @@ -36,4 +36,72 @@ describe Gitlab::CycleAnalytics::PlanStage do expect(result.map { |event| event[:title] }).to contain_exactly(issue_1.title, issue_2.title) end end + + context 'when group is given' do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:project_2) { create(:project, group: group) } + let(:project_3) { create(:project, group: group) } + let(:issue_2_1) { create(:issue, project: project_2, created_at: 90.minutes.ago) } + let(:issue_2_2) { create(:issue, project: project_3, created_at: 60.minutes.ago) } + let(:issue_2_3) { create(:issue, project: project_2, created_at: 60.minutes.ago) } + let(:stage) { described_class.new(options: { from: 2.days.ago, current_user: user, group: group }) } + + before do + group.add_owner(user) + issue_2_1.metrics.update!(first_associated_with_milestone_at: 60.minutes.ago, first_mentioned_in_commit_at: 10.minutes.ago) + issue_2_2.metrics.update!(first_added_to_board_at: 30.minutes.ago, first_mentioned_in_commit_at: 20.minutes.ago) + issue_2_3.metrics.update!(first_added_to_board_at: 15.minutes.ago) + end + + describe '#group_median' do + around do |example| + Timecop.freeze { example.run } + end + + it 'counts median from issues with metrics' do + expect(stage.group_median).to eq(ISSUES_MEDIAN) + end + end + + describe '#events' do + it 'exposes merge requests that close issues' do + result = stage.events + + expect(result.count).to eq(2) + expect(result.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title) + end + end + + context 'when subgroup is given' do + let(:subgroup) { create(:group, parent: group) } + let(:project_4) { create(:project, group: subgroup) } + let(:project_5) { create(:project, group: subgroup) } + let(:issue_3_1) { create(:issue, project: project_4, created_at: 90.minutes.ago) } + let(:issue_3_2) { create(:issue, project: project_5, created_at: 60.minutes.ago) } + let(:issue_3_3) { create(:issue, project: project_5, created_at: 60.minutes.ago) } + + before do + issue_3_1.metrics.update!(first_associated_with_milestone_at: 60.minutes.ago, first_mentioned_in_commit_at: 10.minutes.ago) + issue_3_2.metrics.update!(first_added_to_board_at: 30.minutes.ago, first_mentioned_in_commit_at: 20.minutes.ago) + issue_3_3.metrics.update!(first_added_to_board_at: 15.minutes.ago) + end + + describe '#events' do + it 'exposes merge requests that close issues' do + result = stage.events + + expect(result.count).to eq(4) + expect(result.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title, issue_3_1.title, issue_3_2.title) + end + + it 'exposes merge requests that close issues with full path for subgroup' do + result = stage.events + + expect(result.count).to eq(4) + expect(result.find { |event| event[:title] == issue_3_1.title }[:url]).to include("#{subgroup.full_path}") + end + end + end + end end diff --git a/spec/lib/gitlab/cycle_analytics/review_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/review_stage_spec.rb index f072a9644e8..9c7cb5811d0 100644 --- a/spec/lib/gitlab/cycle_analytics/review_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/review_stage_spec.rb @@ -4,14 +4,14 @@ require 'lib/gitlab/cycle_analytics/shared_stage_spec' describe Gitlab::CycleAnalytics::ReviewStage do let(:stage_name) { :review } let(:project) { create(:project) } - let!(:issue_1) { create(:issue, project: project, created_at: 90.minutes.ago) } - let!(:issue_2) { create(:issue, project: project, created_at: 60.minutes.ago) } - let!(:issue_3) { create(:issue, project: project, created_at: 60.minutes.ago) } - let!(:mr_1) { create(:merge_request, :closed, source_project: project, created_at: 60.minutes.ago) } - let!(:mr_2) { create(:merge_request, :closed, source_project: project, created_at: 40.minutes.ago, source_branch: 'A') } - let!(:mr_3) { create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'B') } + let(:issue_1) { create(:issue, project: project, created_at: 90.minutes.ago) } + let(:issue_2) { create(:issue, project: project, created_at: 60.minutes.ago) } + let(:issue_3) { create(:issue, project: project, created_at: 60.minutes.ago) } + let(:mr_1) { create(:merge_request, :closed, source_project: project, created_at: 60.minutes.ago) } + let(:mr_2) { create(:merge_request, :closed, source_project: project, created_at: 40.minutes.ago, source_branch: 'A') } + let(:mr_3) { create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'B') } let!(:mr_4) { create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'C') } - let(:stage) { described_class.new(project: project, options: { from: 2.days.ago, current_user: project.creator }) } + let(:stage) { described_class.new(options: { from: 2.days.ago, current_user: project.creator, project: project }) } before do mr_1.metrics.update!(merged_at: 30.minutes.ago) @@ -24,13 +24,13 @@ describe Gitlab::CycleAnalytics::ReviewStage do it_behaves_like 'base stage' - describe '#median' do + describe '#project_median' do around do |example| Timecop.freeze { example.run } end it 'counts median from issues with metrics' do - expect(stage.median).to eq(ISSUES_MEDIAN) + expect(stage.project_median).to eq(ISSUES_MEDIAN) end end @@ -42,4 +42,47 @@ describe Gitlab::CycleAnalytics::ReviewStage do expect(result.map { |event| event[:title] }).to contain_exactly(mr_1.title, mr_2.title) end end + context 'when group is given' do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:project_2) { create(:project, group: group) } + let(:project_3) { create(:project, group: group) } + let(:issue_2_1) { create(:issue, project: project_2, created_at: 90.minutes.ago) } + let(:issue_2_2) { create(:issue, project: project_3, created_at: 60.minutes.ago) } + let(:issue_2_3) { create(:issue, project: project_2, created_at: 60.minutes.ago) } + let(:mr_2_1) { create(:merge_request, :closed, source_project: project_2, created_at: 60.minutes.ago) } + let(:mr_2_2) { create(:merge_request, :closed, source_project: project_3, created_at: 40.minutes.ago, source_branch: 'A') } + let(:mr_2_3) { create(:merge_request, source_project: project_2, created_at: 10.minutes.ago, source_branch: 'B') } + let!(:mr_2_4) { create(:merge_request, source_project: project_3, created_at: 10.minutes.ago, source_branch: 'C') } + let(:stage) { described_class.new(options: { from: 2.days.ago, current_user: user, group: group }) } + + before do + group.add_owner(user) + mr_2_1.metrics.update!(merged_at: 30.minutes.ago) + mr_2_2.metrics.update!(merged_at: 10.minutes.ago) + + create(:merge_requests_closing_issues, merge_request: mr_2_1, issue: issue_2_1) + create(:merge_requests_closing_issues, merge_request: mr_2_2, issue: issue_2_2) + create(:merge_requests_closing_issues, merge_request: mr_2_3, issue: issue_2_3) + end + + describe '#group_median' do + around do |example| + Timecop.freeze { example.run } + end + + it 'counts median from issues with metrics' do + expect(stage.group_median).to eq(ISSUES_MEDIAN) + end + end + + describe '#events' do + it 'exposes merge requests that close issues' do + result = stage.events + + expect(result.count).to eq(2) + expect(result.map { |event| event[:title] }).to contain_exactly(mr_2_1.title, mr_2_2.title) + end + end + end end diff --git a/spec/lib/gitlab/cycle_analytics/shared_event_spec.rb b/spec/lib/gitlab/cycle_analytics/shared_event_spec.rb index c22d27f60d6..b001a46001e 100644 --- a/spec/lib/gitlab/cycle_analytics/shared_event_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/shared_event_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' shared_examples 'default query config' do let(:project) { create(:project) } - let(:event) { described_class.new(project: project, stage: stage_name, options: { from: 1.day.ago }) } + let(:event) { described_class.new(stage: stage_name, options: { from: 1.day.ago, project: project }) } it 'has the stage attribute' do expect(event.stage).not_to be_nil diff --git a/spec/lib/gitlab/cycle_analytics/shared_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/shared_stage_spec.rb index 1a4b572cc11..c146146723f 100644 --- a/spec/lib/gitlab/cycle_analytics/shared_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/shared_stage_spec.rb @@ -3,10 +3,10 @@ require 'spec_helper' shared_examples 'base stage' do ISSUES_MEDIAN = 30.minutes.to_i - let(:stage) { described_class.new(project: double, options: {}) } + let(:stage) { described_class.new(options: { project: double }) } before do - allow(stage).to receive(:median).and_return(1.12) + allow(stage).to receive(:project_median).and_return(1.12) allow_any_instance_of(Gitlab::CycleAnalytics::BaseEventFetcher).to receive(:event_result).and_return({}) end diff --git a/spec/lib/gitlab/cycle_analytics/staging_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/staging_stage_spec.rb index 17d5fbb9733..3e2d748396f 100644 --- a/spec/lib/gitlab/cycle_analytics/staging_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/staging_stage_spec.rb @@ -5,16 +5,16 @@ describe Gitlab::CycleAnalytics::StagingStage do let(:stage_name) { :staging } let(:project) { create(:project) } - let!(:issue_1) { create(:issue, project: project, created_at: 90.minutes.ago) } - let!(:issue_2) { create(:issue, project: project, created_at: 60.minutes.ago) } - let!(:issue_3) { create(:issue, project: project, created_at: 60.minutes.ago) } - let!(:mr_1) { create(:merge_request, :closed, source_project: project, created_at: 60.minutes.ago) } - let!(:mr_2) { create(:merge_request, :closed, source_project: project, created_at: 40.minutes.ago, source_branch: 'A') } - let!(:mr_3) { create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'B') } + let(:issue_1) { create(:issue, project: project, created_at: 90.minutes.ago) } + let(:issue_2) { create(:issue, project: project, created_at: 60.minutes.ago) } + let(:issue_3) { create(:issue, project: project, created_at: 60.minutes.ago) } + let(:mr_1) { create(:merge_request, :closed, source_project: project, created_at: 60.minutes.ago) } + let(:mr_2) { create(:merge_request, :closed, source_project: project, created_at: 40.minutes.ago, source_branch: 'A') } + let(:mr_3) { create(:merge_request, source_project: project, created_at: 10.minutes.ago, source_branch: 'B') } let(:build_1) { create(:ci_build, project: project) } let(:build_2) { create(:ci_build, project: project) } - let(:stage) { described_class.new(project: project, options: { from: 2.days.ago, current_user: project.creator }) } + let(:stage) { described_class.new(options: { from: 2.days.ago, current_user: project.creator, project: project }) } before do mr_1.metrics.update!(merged_at: 80.minutes.ago, first_deployed_to_production_at: 50.minutes.ago, pipeline_id: build_1.commit_id) @@ -28,13 +28,13 @@ describe Gitlab::CycleAnalytics::StagingStage do it_behaves_like 'base stage' - describe '#median' do + describe '#project_median' do around do |example| Timecop.freeze { example.run } end it 'counts median from issues with metrics' do - expect(stage.median).to eq(ISSUES_MEDIAN) + expect(stage.project_median).to eq(ISSUES_MEDIAN) end end @@ -46,4 +46,50 @@ describe Gitlab::CycleAnalytics::StagingStage do expect(result.map { |event| event[:name] }).to contain_exactly(build_1.name, build_2.name) end end + + context 'when group is given' do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:project_2) { create(:project, group: group) } + let(:project_3) { create(:project, group: group) } + let(:issue_2_1) { create(:issue, project: project_2, created_at: 90.minutes.ago) } + let(:issue_2_2) { create(:issue, project: project_3, created_at: 60.minutes.ago) } + let(:issue_2_3) { create(:issue, project: project_2, created_at: 60.minutes.ago) } + let(:mr_1) { create(:merge_request, :closed, source_project: project_2, created_at: 60.minutes.ago) } + let(:mr_2) { create(:merge_request, :closed, source_project: project_3, created_at: 40.minutes.ago, source_branch: 'A') } + let(:mr_3) { create(:merge_request, source_project: project_2, created_at: 10.minutes.ago, source_branch: 'B') } + let(:build_1) { create(:ci_build, project: project_2) } + let(:build_2) { create(:ci_build, project: project_3) } + let(:stage) { described_class.new(options: { from: 2.days.ago, current_user: user, group: group }) } + + before do + group.add_owner(user) + mr_1.metrics.update!(merged_at: 80.minutes.ago, first_deployed_to_production_at: 50.minutes.ago, pipeline_id: build_1.commit_id) + mr_2.metrics.update!(merged_at: 60.minutes.ago, first_deployed_to_production_at: 30.minutes.ago, pipeline_id: build_2.commit_id) + mr_3.metrics.update!(merged_at: 10.minutes.ago, first_deployed_to_production_at: 3.days.ago, pipeline_id: create(:ci_build, project: project_2).commit_id) + + create(:merge_requests_closing_issues, merge_request: mr_1, issue: issue_2_1) + create(:merge_requests_closing_issues, merge_request: mr_2, issue: issue_2_2) + create(:merge_requests_closing_issues, merge_request: mr_3, issue: issue_2_3) + end + + describe '#group_median' do + around do |example| + Timecop.freeze { example.run } + end + + it 'counts median from issues with metrics' do + expect(stage.group_median).to eq(ISSUES_MEDIAN) + end + end + + describe '#events' do + it 'exposes merge requests that close issues' do + result = stage.events + + expect(result.count).to eq(2) + expect(result.map { |event| event[:name] }).to contain_exactly(build_1.name, build_2.name) + end + end + end end diff --git a/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb b/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb index 8122e85a981..4ef33ff6e2b 100644 --- a/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb @@ -34,7 +34,7 @@ describe Gitlab::CycleAnalytics::UsageData do expect(result).to have_key(:avg_cycle_analytics) - CycleAnalytics::Base::STAGES.each do |stage| + CycleAnalytics::BaseMethods::STAGES.each do |stage| expect(result[:avg_cycle_analytics]).to have_key(stage) stage_values = result[:avg_cycle_analytics][stage] diff --git a/spec/models/cycle_analytics/code_spec.rb b/spec/models/cycle_analytics/code_spec.rb index db6e70973ae..808659552ff 100644 --- a/spec/models/cycle_analytics/code_spec.rb +++ b/spec/models/cycle_analytics/code_spec.rb @@ -38,7 +38,7 @@ describe 'CycleAnalytics#code' do merge_merge_requests_closing_issue(user, project, issue) deploy_master(user, project) - expect(subject[:code].median).to be_nil + expect(subject[:code].project_median).to be_nil end end end @@ -68,7 +68,7 @@ describe 'CycleAnalytics#code' do merge_merge_requests_closing_issue(user, project, issue) - expect(subject[:code].median).to be_nil + expect(subject[:code].project_median).to be_nil end end end diff --git a/spec/models/cycle_analytics/group_level_spec.rb b/spec/models/cycle_analytics/group_level_spec.rb new file mode 100644 index 00000000000..70c370bc39d --- /dev/null +++ b/spec/models/cycle_analytics/group_level_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe CycleAnalytics::GroupLevel do + let(:group) { create(:group)} + let(:project) { create(:project, :repository, namespace: group) } + let(:from_date) { 10.days.ago } + let(:user) { create(:user, :admin) } + let(:issue) { create(:issue, project: project, created_at: 2.days.ago) } + let(:milestone) { create(:milestone, project: project) } + let(:mr) { create_merge_request_closing_issue(user, project, issue, commit_message: "References #{issue.to_reference}") } + let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha, head_pipeline_of: mr) } + + subject { described_class.new(options: { from: from_date, group: group, current_user: user }) } + + describe '#permissions' do + it 'returns permissions' do + expect(subject.permissions.values.uniq).to eq([true]) + end + end + + describe '#stats' do + before do + allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) + + create_cycle(user, project, issue, mr, milestone, pipeline) + deploy_master(user, project) + end + + it 'returns medians for each stage for a specific group' do + expect(subject.no_stats?).to eq(false) + end + end + + describe '#summary' do + before do + create_cycle(user, project, issue, mr, milestone, pipeline) + deploy_master(user, project) + end + + it 'returns medians for each stage for a specific group' do + expect(subject.summary.map { |summary| summary[:value] }).to contain_exactly(1, 1) + end + end +end diff --git a/spec/models/cycle_analytics/issue_spec.rb b/spec/models/cycle_analytics/issue_spec.rb index 4ccbdf29df6..8cdf83b1292 100644 --- a/spec/models/cycle_analytics/issue_spec.rb +++ b/spec/models/cycle_analytics/issue_spec.rb @@ -43,7 +43,7 @@ describe 'CycleAnalytics#issue' do create_merge_request_closing_issue(user, project, issue) merge_merge_requests_closing_issue(user, project, issue) - expect(subject[:issue].median).to be_nil + expect(subject[:issue].project_median).to be_nil end end end diff --git a/spec/models/cycle_analytics/plan_spec.rb b/spec/models/cycle_analytics/plan_spec.rb index c99c38e9cf3..28ad9bd194d 100644 --- a/spec/models/cycle_analytics/plan_spec.rb +++ b/spec/models/cycle_analytics/plan_spec.rb @@ -47,7 +47,7 @@ describe 'CycleAnalytics#plan' do create_merge_request_closing_issue(user, project, issue, source_branch: branch_name) merge_merge_requests_closing_issue(user, project, issue) - expect(subject[:issue].median).to be_nil + expect(subject[:issue].project_median).to be_nil end end end diff --git a/spec/models/cycle_analytics/production_spec.rb b/spec/models/cycle_analytics/production_spec.rb index ddd199362d1..613c1786540 100644 --- a/spec/models/cycle_analytics/production_spec.rb +++ b/spec/models/cycle_analytics/production_spec.rb @@ -41,7 +41,7 @@ describe 'CycleAnalytics#production' do MergeRequests::MergeService.new(project, user).execute(merge_request) deploy_master(user, project) - expect(subject[:production].median).to be_nil + expect(subject[:production].project_median).to be_nil end end @@ -52,7 +52,7 @@ describe 'CycleAnalytics#production' do MergeRequests::MergeService.new(project, user).execute(merge_request) deploy_master(user, project, environment: 'staging') - expect(subject[:production].median).to be_nil + expect(subject[:production].project_median).to be_nil end end end diff --git a/spec/models/cycle_analytics/project_level_spec.rb b/spec/models/cycle_analytics/project_level_spec.rb index 77bd0bfeb9c..4de01b1c679 100644 --- a/spec/models/cycle_analytics/project_level_spec.rb +++ b/spec/models/cycle_analytics/project_level_spec.rb @@ -23,7 +23,7 @@ describe CycleAnalytics::ProjectLevel do it 'returns every median for each stage for a specific project' do values = described_class::STAGES.each_with_object({}) do |stage_name, hsh| - hsh[stage_name] = subject[stage_name].median.presence + hsh[stage_name] = subject[stage_name].project_median.presence end expect(subject.all_medians_by_stage).to eq(values) diff --git a/spec/models/cycle_analytics/review_spec.rb b/spec/models/cycle_analytics/review_spec.rb index 63c481ed465..ef88fd86340 100644 --- a/spec/models/cycle_analytics/review_spec.rb +++ b/spec/models/cycle_analytics/review_spec.rb @@ -28,7 +28,7 @@ describe 'CycleAnalytics#review' do it "returns nil" do MergeRequests::MergeService.new(project, user).execute(create(:merge_request)) - expect(subject[:review].median).to be_nil + expect(subject[:review].project_median).to be_nil end end end diff --git a/spec/models/cycle_analytics/staging_spec.rb b/spec/models/cycle_analytics/staging_spec.rb index c134b97553f..571792559d8 100644 --- a/spec/models/cycle_analytics/staging_spec.rb +++ b/spec/models/cycle_analytics/staging_spec.rb @@ -45,7 +45,7 @@ describe 'CycleAnalytics#staging' do MergeRequests::MergeService.new(project, user).execute(merge_request) deploy_master(user, project) - expect(subject[:staging].median).to be_nil + expect(subject[:staging].project_median).to be_nil end end @@ -56,7 +56,7 @@ describe 'CycleAnalytics#staging' do MergeRequests::MergeService.new(project, user).execute(merge_request) deploy_master(user, project, environment: 'staging') - expect(subject[:staging].median).to be_nil + expect(subject[:staging].project_median).to be_nil end end end diff --git a/spec/models/cycle_analytics/test_spec.rb b/spec/models/cycle_analytics/test_spec.rb index a6ea73b2699..7b3001d2bd8 100644 --- a/spec/models/cycle_analytics/test_spec.rb +++ b/spec/models/cycle_analytics/test_spec.rb @@ -36,7 +36,7 @@ describe 'CycleAnalytics#test' do merge_merge_requests_closing_issue(user, project, issue) - expect(subject[:test].median).to be_nil + expect(subject[:test].project_median).to be_nil end end @@ -47,7 +47,7 @@ describe 'CycleAnalytics#test' do pipeline.run! pipeline.succeed! - expect(subject[:test].median).to be_nil + expect(subject[:test].project_median).to be_nil end end @@ -62,7 +62,7 @@ describe 'CycleAnalytics#test' do merge_merge_requests_closing_issue(user, project, issue) - expect(subject[:test].median).to be_nil + expect(subject[:test].project_median).to be_nil end end @@ -77,7 +77,7 @@ describe 'CycleAnalytics#test' do merge_merge_requests_closing_issue(user, project, issue) - expect(subject[:test].median).to be_nil + expect(subject[:test].project_median).to be_nil end end end diff --git a/spec/serializers/analytics_issue_entity_spec.rb b/spec/serializers/analytics_issue_entity_spec.rb index 89588b4df2b..dd5e43a4b62 100644 --- a/spec/serializers/analytics_issue_entity_spec.rb +++ b/spec/serializers/analytics_issue_entity_spec.rb @@ -9,12 +9,14 @@ describe AnalyticsIssueEntity do iid: "1", id: "1", created_at: "2016-11-12 15:04:02.948604", - author: user + author: user, + name: project.name, + path: project.namespace } end let(:project) { create(:project) } - let(:request) { EntityRequest.new(project: project, entity: :merge_request) } + let(:request) { EntityRequest.new(entity: :merge_request) } let(:entity) do described_class.new(entity_hash, request: request, project: project) diff --git a/spec/serializers/analytics_issue_serializer_spec.rb b/spec/serializers/analytics_issue_serializer_spec.rb index 5befc28f4fa..c9ffe1c5dad 100644 --- a/spec/serializers/analytics_issue_serializer_spec.rb +++ b/spec/serializers/analytics_issue_serializer_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe AnalyticsIssueSerializer do subject do described_class - .new(project: project, entity: :merge_request) + .new(entity: :merge_request) .represent(resource) end @@ -16,7 +16,9 @@ describe AnalyticsIssueSerializer do iid: "1", id: "1", created_at: "2016-11-12 15:04:02.948604", - author: user + author: user, + name: project.name, + path: project.namespace } end diff --git a/spec/serializers/analytics_merge_request_serializer_spec.rb b/spec/serializers/analytics_merge_request_serializer_spec.rb index 62067cc0ef2..123d7d795ce 100644 --- a/spec/serializers/analytics_merge_request_serializer_spec.rb +++ b/spec/serializers/analytics_merge_request_serializer_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe AnalyticsMergeRequestSerializer do subject do described_class - .new(project: project, entity: :merge_request) + .new(entity: :merge_request) .represent(resource) end @@ -17,7 +17,9 @@ describe AnalyticsMergeRequestSerializer do id: "1", state: 'open', created_at: "2016-11-12 15:04:02.948604", - author: user + author: user, + name: project.name, + path: project.namespace } end diff --git a/spec/serializers/analytics_stage_serializer_spec.rb b/spec/serializers/analytics_stage_serializer_spec.rb index 5b05c2f2ef3..86a796a2d94 100644 --- a/spec/serializers/analytics_stage_serializer_spec.rb +++ b/spec/serializers/analytics_stage_serializer_spec.rb @@ -6,11 +6,11 @@ describe AnalyticsStageSerializer do end let(:resource) do - Gitlab::CycleAnalytics::CodeStage.new(project: double, options: {}) + Gitlab::CycleAnalytics::CodeStage.new(options: { project: double }) end before do - allow_any_instance_of(Gitlab::CycleAnalytics::BaseStage).to receive(:median).and_return(1.12) + allow_any_instance_of(Gitlab::CycleAnalytics::BaseStage).to receive(:project_median).and_return(1.12) allow_any_instance_of(Gitlab::CycleAnalytics::BaseEventFetcher).to receive(:event_result).and_return({}) end @@ -24,7 +24,7 @@ describe AnalyticsStageSerializer do context 'when median is equal 0' do before do - allow_any_instance_of(Gitlab::CycleAnalytics::BaseStage).to receive(:median).and_return(0) + allow_any_instance_of(Gitlab::CycleAnalytics::BaseStage).to receive(:project_median).and_return(0) end it 'sets the value to nil' do @@ -34,7 +34,7 @@ describe AnalyticsStageSerializer do context 'when median is below 1' do before do - allow_any_instance_of(Gitlab::CycleAnalytics::BaseStage).to receive(:median).and_return(0.12) + allow_any_instance_of(Gitlab::CycleAnalytics::BaseStage).to receive(:project_median).and_return(0.12) end it 'sets the value to equal to median' do @@ -44,7 +44,7 @@ describe AnalyticsStageSerializer do context 'when median is above 1' do before do - allow_any_instance_of(Gitlab::CycleAnalytics::BaseStage).to receive(:median).and_return(60.12) + allow_any_instance_of(Gitlab::CycleAnalytics::BaseStage).to receive(:project_median).and_return(60.12) end it 'sets the value to equal to median' do diff --git a/spec/support/cycle_analytics_helpers/test_generation.rb b/spec/support/cycle_analytics_helpers/test_generation.rb index 19b32c84d81..be1c2bc3046 100644 --- a/spec/support/cycle_analytics_helpers/test_generation.rb +++ b/spec/support/cycle_analytics_helpers/test_generation.rb @@ -50,7 +50,7 @@ module CycleAnalyticsHelpers end median_time_difference = time_differences.sort[2] - expect(subject[phase].median).to be_within(5).of(median_time_difference) + expect(subject[phase].project_median).to be_within(5).of(median_time_difference) end context "when the data belongs to another project" do @@ -80,7 +80,7 @@ module CycleAnalyticsHelpers # Turn off the stub before checking assertions allow(self).to receive(:project).and_call_original - expect(subject[phase].median).to be_nil + expect(subject[phase].project_median).to be_nil end end @@ -103,7 +103,7 @@ module CycleAnalyticsHelpers Timecop.freeze(end_time + 1.day) { post_fn[self, data] } if post_fn - expect(subject[phase].median).to be_nil + expect(subject[phase].project_median).to be_nil end end end @@ -121,7 +121,7 @@ module CycleAnalyticsHelpers Timecop.freeze(end_time + 1.day) { post_fn[self, data] } if post_fn - expect(subject[phase].median).to be_nil + expect(subject[phase].project_median).to be_nil end end end @@ -138,7 +138,7 @@ module CycleAnalyticsHelpers post_fn[self, data] if post_fn - expect(subject[phase].median).to be_nil + expect(subject[phase].project_median).to be_nil end end end @@ -146,7 +146,7 @@ module CycleAnalyticsHelpers context "when none of the start / end conditions are matched" do it "returns nil" do - expect(subject[phase].median).to be_nil + expect(subject[phase].project_median).to be_nil end end end -- cgit v1.2.1 From 70d74f4730d69d61b07362fbe953c37494a732ca Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 15 Jul 2019 15:18:50 +0200 Subject: Update docs on serverless apps deployments --- doc/user/project/clusters/serverless/index.md | 37 ++++++++++++--------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/doc/user/project/clusters/serverless/index.md b/doc/user/project/clusters/serverless/index.md index 14ee6303bf9..a32759c7bdc 100644 --- a/doc/user/project/clusters/serverless/index.md +++ b/doc/user/project/clusters/serverless/index.md @@ -41,8 +41,7 @@ To run Knative on Gitlab, you will need: external IP address or hostname for that domain. 1. **`.gitlab-ci.yml`:** GitLab uses [Kaniko](https://github.com/GoogleContainerTools/kaniko) to build the application. We also use [gitlabktl](https://gitlab.com/gitlab-org/gitlabktl) - and [TriggerMesh CLI](https://github.com/triggermesh/tm) CLIs to simplify the - deployment of services and functions to Knative. + CLI to simplify the deployment of services and functions to Knative. 1. **`serverless.yml`** (for [functions only](#deploying-functions)): When using serverless to deploy functions, the `serverless.yml` file will contain the information for all the functions being hosted in the repository as well as a reference to the runtime being used. @@ -249,7 +248,7 @@ Explanation of the fields used above: | Parameter | Description | |-----------|-------------| -| `name` | Indicates which provider is used to execute the `serverless.yml` file. In this case, the TriggerMesh `tm` CLI. | +| `name` | Indicates which provider is used to execute the `serverless.yml` file. In this case, the TriggerMesh middleware. | | `environment` | Includes the environment variables to be passed as part of function execution for **all** functions in the file, where `FOO` is the variable name and `BAR` are he variable contents. You may replace this with you own variables. | ### `functions` @@ -343,27 +342,23 @@ Go to the **CI/CD > Pipelines** and click on the pipeline that deployed your app The output will look like this: ```bash -Running with gitlab-runner 11.5.0~beta.844.g96d88322 (96d88322) - on docker-auto-scale 72989761 -Using Docker executor with image gcr.io/triggermesh/tm@sha256:e3ee74db94d215bd297738d93577481f3e4db38013326c90d57f873df7ab41d5 ... -Pulling docker image gcr.io/triggermesh/tm@sha256:e3ee74db94d215bd297738d93577481f3e4db38013326c90d57f873df7ab41d5 ... -Using docker image sha256:6b3f6590a9b30bd7aafb9573f047d930c70066e43955b4beb18a1eee175f6de1 for gcr.io/triggermesh/tm@sha256:e3ee74db94d215bd297738d93577481f3e4db38013326c90d57f873df7ab41d5 ... -Running on runner-72989761-project-4342902-concurrent-0 via runner-72989761-stg-srm-1541795796-27929c96... -Cloning repository... -Cloning into '/builds/danielgruesso/knative'... -Checking out 8671ad20 as master... -Skipping Git submodules setup -$ echo "$CI_REGISTRY_IMAGE" -registry.staging.gitlab.com/danielgruesso/knative -$ tm -n "$KUBE_NAMESPACE" --config "$KUBECONFIG" deploy service "$CI_PROJECT_NAME" --from-image "$CI_REGISTRY_IMAGE" --wait -Deployment started. Run "tm -n knative-4342902 describe service knative" to see the details -Waiting for ready state....... -Service domain: knative.knative-4342902.example.com +Running with gitlab-runner 12.1.0-rc1 (6da35412) + on prm-com-gitlab-org ae3bfce3 +Using Docker executor with image registry.gitlab.com/gitlab-org/gitlabktl:latest ... +Running on runner-ae3bfc-concurrent-0 via runner-ae3bfc ... +Fetching changes... +Authenticating with credentials from job payload (GitLab Registry) +$ /usr/bin/gitlabktl application deploy +Welcome to gitlabktl tool +time="2019-07-15T10:51:07Z" level=info msg="deploying registry credentials" +Creating app-hello function +Waiting for app-hello ready state +Service app-hello URL: http://app-hello.serverless.example.com Job succeeded ``` -The second to last line, labeled **Service domain** contains the URL for the deployment. Copy and paste the domain into your -browser to see the app live. +The second to last line, labeled **Service domain** contains the URL for the +deployment. Copy and paste the domain into your browser to see the app live. ![knative app](img/knative-app.png) -- cgit v1.2.1 From d2400a990822dc291382761753e3fc08250fc7f2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 15 Jul 2019 15:20:10 +0200 Subject: Add changelog entry for serverless apps deployment --- .../unreleased/feature-gb-serverless-app-deployment-template.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/feature-gb-serverless-app-deployment-template.yml diff --git a/changelogs/unreleased/feature-gb-serverless-app-deployment-template.yml b/changelogs/unreleased/feature-gb-serverless-app-deployment-template.yml new file mode 100644 index 00000000000..bd9001bd671 --- /dev/null +++ b/changelogs/unreleased/feature-gb-serverless-app-deployment-template.yml @@ -0,0 +1,5 @@ +--- +title: Deploy serverless apps with gitlabktl +merge_request: 30740 +author: +type: added -- cgit v1.2.1 From 72cddf5a1f7e851864661ad97a611b1ef8d5563a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Mon, 15 Jul 2019 16:22:15 +0200 Subject: Fix test stage specs --- spec/lib/gitlab/cycle_analytics/test_stage_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/lib/gitlab/cycle_analytics/test_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/test_stage_spec.rb index 8633a63849f..41028c44a00 100644 --- a/spec/lib/gitlab/cycle_analytics/test_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/test_stage_spec.rb @@ -4,7 +4,7 @@ require 'lib/gitlab/cycle_analytics/shared_stage_spec' describe Gitlab::CycleAnalytics::TestStage do let(:stage_name) { :test } let(:project) { create(:project) } - let(:stage) { described_class.new(project: project, options: { from: 2.days.ago, current_user: project.creator }) } + let(:stage) { described_class.new(options: { from: 2.days.ago, current_user: project.creator, project: project }) } it_behaves_like 'base stage' @@ -36,7 +36,7 @@ describe Gitlab::CycleAnalytics::TestStage do end it 'counts median from issues with metrics' do - expect(stage.median).to eq(ISSUES_MEDIAN) + expect(stage.project_median).to eq(ISSUES_MEDIAN) end end end -- cgit v1.2.1 From 1dde18f39424f6eaebe1777d0bfa35c5805178cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Tue, 16 Jul 2019 11:38:19 +0200 Subject: Add code review remarks Make specs more readable --- app/models/cycle_analytics/base_methods.rb | 27 ------------------- app/models/cycle_analytics/group_level.rb | 2 +- app/models/cycle_analytics/level_base.rb | 27 +++++++++++++++++++ app/models/cycle_analytics/project_level.rb | 2 +- lib/gitlab/cycle_analytics/permissions.rb | 2 +- spec/lib/gitlab/cycle_analytics/code_stage_spec.rb | 30 ++++++++++------------ .../cycle_analytics/group_stage_summary_spec.rb | 1 + .../lib/gitlab/cycle_analytics/issue_stage_spec.rb | 22 ++++++++-------- spec/lib/gitlab/cycle_analytics/plan_stage_spec.rb | 30 ++++++++++------------ .../gitlab/cycle_analytics/review_stage_spec.rb | 17 ++++++------ .../gitlab/cycle_analytics/staging_stage_spec.rb | 16 ++++++------ spec/lib/gitlab/cycle_analytics/usage_data_spec.rb | 2 +- 12 files changed, 87 insertions(+), 91 deletions(-) delete mode 100644 app/models/cycle_analytics/base_methods.rb create mode 100644 app/models/cycle_analytics/level_base.rb diff --git a/app/models/cycle_analytics/base_methods.rb b/app/models/cycle_analytics/base_methods.rb deleted file mode 100644 index 1c2a0854769..00000000000 --- a/app/models/cycle_analytics/base_methods.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -module CycleAnalytics - module BaseMethods - STAGES = %i[issue plan code test review staging production].freeze - - def all_medians_by_stage - STAGES.each_with_object({}) do |stage_name, medians_per_stage| - medians_per_stage[stage_name] = self[stage_name].project_median - end - end - - def stats - @stats ||= STAGES.map do |stage_name| - self[stage_name].as_json - end - end - - def no_stats? - stats.all? { |hash| hash[:value].nil? } - end - - def [](stage_name) - Gitlab::CycleAnalytics::Stage[stage_name].new(options: options) - end - end -end diff --git a/app/models/cycle_analytics/group_level.rb b/app/models/cycle_analytics/group_level.rb index 58bd2eb0d9a..35efd8b8809 100644 --- a/app/models/cycle_analytics/group_level.rb +++ b/app/models/cycle_analytics/group_level.rb @@ -2,7 +2,7 @@ module CycleAnalytics class GroupLevel - include BaseMethods + include LevelBase attr_reader :options def initialize(options:) diff --git a/app/models/cycle_analytics/level_base.rb b/app/models/cycle_analytics/level_base.rb new file mode 100644 index 00000000000..543349ebf8f --- /dev/null +++ b/app/models/cycle_analytics/level_base.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module CycleAnalytics + module LevelBase + STAGES = %i[issue plan code test review staging production].freeze + + def all_medians_by_stage + STAGES.each_with_object({}) do |stage_name, medians_per_stage| + medians_per_stage[stage_name] = self[stage_name].project_median + end + end + + def stats + @stats ||= STAGES.map do |stage_name| + self[stage_name].as_json + end + end + + def no_stats? + stats.all? { |hash| hash[:value].nil? } + end + + def [](stage_name) + Gitlab::CycleAnalytics::Stage[stage_name].new(options: options) + end + end +end diff --git a/app/models/cycle_analytics/project_level.rb b/app/models/cycle_analytics/project_level.rb index abe1be11eae..4aa426c58a1 100644 --- a/app/models/cycle_analytics/project_level.rb +++ b/app/models/cycle_analytics/project_level.rb @@ -2,7 +2,7 @@ module CycleAnalytics class ProjectLevel - include BaseMethods + include LevelBase attr_reader :project, :options def initialize(project, options:) diff --git a/lib/gitlab/cycle_analytics/permissions.rb b/lib/gitlab/cycle_analytics/permissions.rb index 0da041f8950..55214e6b896 100644 --- a/lib/gitlab/cycle_analytics/permissions.rb +++ b/lib/gitlab/cycle_analytics/permissions.rb @@ -23,7 +23,7 @@ module Gitlab end def get - ::CycleAnalytics::BaseMethods::STAGES.each do |stage| + ::CycleAnalytics::LevelBase::STAGES.each do |stage| @stage_permission_hash[stage] = authorized_stage?(stage) end diff --git a/spec/lib/gitlab/cycle_analytics/code_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/code_stage_spec.rb index 68f73ba3c93..933f3c7896e 100644 --- a/spec/lib/gitlab/cycle_analytics/code_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/code_stage_spec.rb @@ -34,11 +34,11 @@ describe Gitlab::CycleAnalytics::CodeStage do end describe '#events' do - it 'exposes merge requests that closes issues' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(2) - expect(result.map { |event| event[:title] }).to contain_exactly(mr_1.title, mr_2.title) + it 'exposes merge requests that closes issues' do + expect(subject.count).to eq(2) + expect(subject.map { |event| event[:title] }).to contain_exactly(mr_1.title, mr_2.title) end end @@ -74,11 +74,11 @@ describe Gitlab::CycleAnalytics::CodeStage do end describe '#events' do - it 'exposes merge requests that close issues' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(2) - expect(result.map { |event| event[:title] }).to contain_exactly(mr_2_1.title, mr_2_2.title) + it 'exposes merge requests that close issues' do + expect(subject.count).to eq(2) + expect(subject.map { |event| event[:title] }).to contain_exactly(mr_2_1.title, mr_2_2.title) end end @@ -101,18 +101,16 @@ describe Gitlab::CycleAnalytics::CodeStage do end describe '#events' do - it 'exposes merge requests that close issues' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(4) - expect(result.map { |event| event[:title] }).to contain_exactly(mr_2_1.title, mr_2_2.title, mr_3_1.title, mr_3_2.title) + it 'exposes merge requests that close issues' do + expect(subject.count).to eq(4) + expect(subject.map { |event| event[:title] }).to contain_exactly(mr_2_1.title, mr_2_2.title, mr_3_1.title, mr_3_2.title) end it 'exposes merge requests that close issues with full path for subgroup' do - result = stage.events - - expect(result.count).to eq(4) - expect(result.find { |event| event[:title] == mr_3_1.title }[:url]).to include("#{subgroup.full_path}") + expect(subject.count).to eq(4) + expect(subject.find { |event| event[:title] == mr_3_1.title }[:url]).to include("#{subgroup.full_path}") end end end diff --git a/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb b/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb index 8e552b23283..7e79c3f939c 100644 --- a/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb @@ -7,6 +7,7 @@ describe Gitlab::CycleAnalytics::GroupStageSummary do let(:project_2) { create(:project, :repository, namespace: group) } let(:from) { 1.day.ago } let(:user) { create(:user, :admin) } + subject { described_class.new(group, from: Time.now, current_user: user).data } describe "#new_issues" do diff --git a/spec/lib/gitlab/cycle_analytics/issue_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/issue_stage_spec.rb index 64ac9df52b2..ffd0b84cb57 100644 --- a/spec/lib/gitlab/cycle_analytics/issue_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/issue_stage_spec.rb @@ -63,11 +63,11 @@ describe Gitlab::CycleAnalytics::IssueStage do end describe '#events' do - it 'exposes merge requests that close issues' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(2) - expect(result.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title) + it 'exposes merge requests that close issues' do + expect(subject.count).to eq(2) + expect(subject.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title) end end @@ -85,18 +85,16 @@ describe Gitlab::CycleAnalytics::IssueStage do end describe '#events' do - it 'exposes merge requests that close issues' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(4) - expect(result.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title, issue_3_1.title, issue_3_2.title) + it 'exposes merge requests that close issues' do + expect(subject.count).to eq(4) + expect(subject.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title, issue_3_1.title, issue_3_2.title) end it 'exposes merge requests that close issues with full path for subgroup' do - result = stage.events - - expect(result.count).to eq(4) - expect(result.find { |event| event[:title] == issue_3_1.title }[:url]).to include("#{subgroup.full_path}") + expect(subject.count).to eq(4) + expect(subject.find { |event| event[:title] == issue_3_1.title }[:url]).to include("#{subgroup.full_path}") end end end diff --git a/spec/lib/gitlab/cycle_analytics/plan_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/plan_stage_spec.rb index 55de6192af1..3cd1320ca9c 100644 --- a/spec/lib/gitlab/cycle_analytics/plan_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/plan_stage_spec.rb @@ -29,11 +29,11 @@ describe Gitlab::CycleAnalytics::PlanStage do end describe '#events' do - it 'exposes issues with metrics' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(2) - expect(result.map { |event| event[:title] }).to contain_exactly(issue_1.title, issue_2.title) + it 'exposes issues with metrics' do + expect(subject.count).to eq(2) + expect(subject.map { |event| event[:title] }).to contain_exactly(issue_1.title, issue_2.title) end end @@ -65,11 +65,11 @@ describe Gitlab::CycleAnalytics::PlanStage do end describe '#events' do - it 'exposes merge requests that close issues' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(2) - expect(result.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title) + it 'exposes merge requests that close issues' do + expect(subject.count).to eq(2) + expect(subject.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title) end end @@ -88,18 +88,16 @@ describe Gitlab::CycleAnalytics::PlanStage do end describe '#events' do - it 'exposes merge requests that close issues' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(4) - expect(result.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title, issue_3_1.title, issue_3_2.title) + it 'exposes merge requests that close issues' do + expect(subject.count).to eq(4) + expect(subject.map { |event| event[:title] }).to contain_exactly(issue_2_1.title, issue_2_2.title, issue_3_1.title, issue_3_2.title) end it 'exposes merge requests that close issues with full path for subgroup' do - result = stage.events - - expect(result.count).to eq(4) - expect(result.find { |event| event[:title] == issue_3_1.title }[:url]).to include("#{subgroup.full_path}") + expect(subject.count).to eq(4) + expect(subject.find { |event| event[:title] == issue_3_1.title }[:url]).to include("#{subgroup.full_path}") end end end diff --git a/spec/lib/gitlab/cycle_analytics/review_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/review_stage_spec.rb index 9c7cb5811d0..6d14973c711 100644 --- a/spec/lib/gitlab/cycle_analytics/review_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/review_stage_spec.rb @@ -35,13 +35,14 @@ describe Gitlab::CycleAnalytics::ReviewStage do end describe '#events' do - it 'exposes merge requests that close issues' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(2) - expect(result.map { |event| event[:title] }).to contain_exactly(mr_1.title, mr_2.title) + it 'exposes merge requests that close issues' do + expect(subject.count).to eq(2) + expect(subject.map { |event| event[:title] }).to contain_exactly(mr_1.title, mr_2.title) end end + context 'when group is given' do let(:user) { create(:user) } let(:group) { create(:group) } @@ -77,11 +78,11 @@ describe Gitlab::CycleAnalytics::ReviewStage do end describe '#events' do - it 'exposes merge requests that close issues' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(2) - expect(result.map { |event| event[:title] }).to contain_exactly(mr_2_1.title, mr_2_2.title) + it 'exposes merge requests that close issues' do + expect(subject.count).to eq(2) + expect(subject.map { |event| event[:title] }).to contain_exactly(mr_2_1.title, mr_2_2.title) end end end diff --git a/spec/lib/gitlab/cycle_analytics/staging_stage_spec.rb b/spec/lib/gitlab/cycle_analytics/staging_stage_spec.rb index 3e2d748396f..9ca12cc448c 100644 --- a/spec/lib/gitlab/cycle_analytics/staging_stage_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/staging_stage_spec.rb @@ -39,11 +39,11 @@ describe Gitlab::CycleAnalytics::StagingStage do end describe '#events' do - it 'exposes builds connected to merge request' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(2) - expect(result.map { |event| event[:name] }).to contain_exactly(build_1.name, build_2.name) + it 'exposes builds connected to merge request' do + expect(subject.count).to eq(2) + expect(subject.map { |event| event[:name] }).to contain_exactly(build_1.name, build_2.name) end end @@ -84,11 +84,11 @@ describe Gitlab::CycleAnalytics::StagingStage do end describe '#events' do - it 'exposes merge requests that close issues' do - result = stage.events + subject { stage.events } - expect(result.count).to eq(2) - expect(result.map { |event| event[:name] }).to contain_exactly(build_1.name, build_2.name) + it 'exposes merge requests that close issues' do + expect(subject.count).to eq(2) + expect(subject.map { |event| event[:name] }).to contain_exactly(build_1.name, build_2.name) end end end diff --git a/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb b/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb index 4ef33ff6e2b..ad61bdeace7 100644 --- a/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/usage_data_spec.rb @@ -34,7 +34,7 @@ describe Gitlab::CycleAnalytics::UsageData do expect(result).to have_key(:avg_cycle_analytics) - CycleAnalytics::BaseMethods::STAGES.each do |stage| + CycleAnalytics::LevelBase::STAGES.each do |stage| expect(result[:avg_cycle_analytics]).to have_key(stage) stage_values = result[:avg_cycle_analytics][stage] -- cgit v1.2.1 From e6c61c89527eb1a8f0f8affff5a76c81af656d8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Tue, 16 Jul 2019 12:28:08 +0200 Subject: Add timecop remarks --- .../cycle_analytics/group_stage_summary_spec.rb | 84 ++++++++++++++-------- 1 file changed, 54 insertions(+), 30 deletions(-) diff --git a/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb b/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb index 7e79c3f939c..6505fc714c4 100644 --- a/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb @@ -11,56 +11,80 @@ describe Gitlab::CycleAnalytics::GroupStageSummary do subject { described_class.new(group, from: Time.now, current_user: user).data } describe "#new_issues" do - it "finds the number of issues created after the 'from date'" do - Timecop.freeze(5.days.ago) { create(:issue, project: project) } - Timecop.freeze(5.days.ago) { create(:issue, project: project_2) } - Timecop.freeze(5.days.from_now) { create(:issue, project: project) } - Timecop.freeze(5.days.from_now) { create(:issue, project: project_2) } + context 'with from date' do + before do + Timecop.freeze(5.days.ago) { create(:issue, project: project) } + Timecop.freeze(5.days.ago) { create(:issue, project: project_2) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project_2) } + end - expect(subject.first[:value]).to eq(2) + it "finds the number of issues created after it" do + expect(subject.first[:value]).to eq(2) + end end - it "doesn't find issues from other projects" do - Timecop.freeze(5.days.from_now) { create(:issue, project: create(:project, namespace: create(:group))) } - Timecop.freeze(5.days.from_now) { create(:issue, project: project) } - Timecop.freeze(5.days.from_now) { create(:issue, project: project_2) } + context 'with other projects' do + before do + Timecop.freeze(5.days.from_now) { create(:issue, project: create(:project, namespace: create(:group))) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project_2) } + end - expect(subject.first[:value]).to eq(2) + it "doesn't find issues from them" do + expect(subject.first[:value]).to eq(2) + end end - it "finds issues from subgroups" do - Timecop.freeze(5.days.from_now) { create(:issue, project: create(:project, namespace: create(:group, parent: group))) } - Timecop.freeze(5.days.from_now) { create(:issue, project: project) } - Timecop.freeze(5.days.from_now) { create(:issue, project: project_2) } + context 'with subgroups' do + before do + Timecop.freeze(5.days.from_now) { create(:issue, project: create(:project, namespace: create(:group, parent: group))) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project) } + Timecop.freeze(5.days.from_now) { create(:issue, project: project_2) } + end - expect(subject.first[:value]).to eq(3) + it "finds issues from them" do + expect(subject.first[:value]).to eq(3) + end end end describe "#deploys" do - it "finds the number of deploys made created after the 'from date'" do - Timecop.freeze(5.days.ago) { create(:deployment, :success, project: project) } - Timecop.freeze(5.days.from_now) { create(:deployment, :success, project: project) } - Timecop.freeze(5.days.ago) { create(:deployment, :success, project: project_2) } - Timecop.freeze(5.days.from_now) { create(:deployment, :success, project: project_2) } + context 'with from date' do + before do + Timecop.freeze(5.days.ago) { create(:deployment, :success, project: project) } + Timecop.freeze(5.days.from_now) { create(:deployment, :success, project: project) } + Timecop.freeze(5.days.ago) { create(:deployment, :success, project: project_2) } + Timecop.freeze(5.days.from_now) { create(:deployment, :success, project: project_2) } + end - expect(subject.second[:value]).to eq(2) + it "finds the number of deploys made created after it" do + expect(subject.second[:value]).to eq(2) + end end - it "doesn't find deploys from other projects" do - Timecop.freeze(5.days.from_now) do - create(:deployment, :success, project: create(:project, :repository, namespace: create(:group))) + context 'with other projects' do + before do + Timecop.freeze(5.days.from_now) do + create(:deployment, :success, project: create(:project, :repository, namespace: create(:group))) + end end - expect(subject.second[:value]).to eq(0) + it "doesn't find deploys from them" do + expect(subject.second[:value]).to eq(0) + end end - it "finds deploys from subgroups" do - Timecop.freeze(5.days.from_now) do - create(:deployment, :success, project: create(:project, :repository, namespace: create(:group, parent: group))) + context 'with subgroups' do + before do + Timecop.freeze(5.days.from_now) do + create(:deployment, :success, project: create(:project, :repository, namespace: create(:group, parent: group))) + end end - expect(subject.second[:value]).to eq(1) + it "finds deploys from them" do + expect(subject.second[:value]).to eq(1) + end end end end -- cgit v1.2.1 From 18535eb411887ac8283cc24b8ed1e384d3aabcc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Tue, 16 Jul 2019 17:17:21 +0200 Subject: Add code review remarks Change small things for better readability --- app/models/cycle_analytics/group_level.rb | 11 +++--- app/serializers/group_analytics_stage_entity.rb | 2 +- lib/gitlab/cycle_analytics/summary/group/issue.rb | 2 +- .../cycle_analytics/group_stage_summary_spec.rb | 46 +++++++++++----------- spec/models/cycle_analytics/group_level_spec.rb | 4 +- 5 files changed, 32 insertions(+), 33 deletions(-) diff --git a/app/models/cycle_analytics/group_level.rb b/app/models/cycle_analytics/group_level.rb index 35efd8b8809..508cde0ca00 100644 --- a/app/models/cycle_analytics/group_level.rb +++ b/app/models/cycle_analytics/group_level.rb @@ -3,19 +3,20 @@ module CycleAnalytics class GroupLevel include LevelBase - attr_reader :options + attr_reader :options, :group - def initialize(options:) - @options = options + def initialize(group:, options:) + @group = group + @options = options.merge(group: group) end def summary - @summary ||= ::Gitlab::CycleAnalytics::GroupStageSummary.new(options[:group], + @summary ||= ::Gitlab::CycleAnalytics::GroupStageSummary.new(group, from: options[:from], current_user: options[:current_user]).data end - def permissions(user: nil) + def permissions(*) STAGES.each_with_object({}) do |stage, obj| obj[stage] = true end diff --git a/app/serializers/group_analytics_stage_entity.rb b/app/serializers/group_analytics_stage_entity.rb index 019a3086f68..81be20e7dd8 100644 --- a/app/serializers/group_analytics_stage_entity.rb +++ b/app/serializers/group_analytics_stage_entity.rb @@ -9,7 +9,7 @@ class GroupAnalyticsStageEntity < Grape::Entity expose :description expose :group_median, as: :value do |stage| - # median returns a BatchLoader instance which we first have to unwrap by using to_f + # group_median returns a BatchLoader instance which we first have to unwrap by using to_f # we use to_f to make sure results below 1 are presented to the end-user stage.group_median.to_f.nonzero? ? distance_of_time_in_words(stage.group_median) : nil end diff --git a/lib/gitlab/cycle_analytics/summary/group/issue.rb b/lib/gitlab/cycle_analytics/summary/group/issue.rb index a5188056cb7..70073e6d843 100644 --- a/lib/gitlab/cycle_analytics/summary/group/issue.rb +++ b/lib/gitlab/cycle_analytics/summary/group/issue.rb @@ -16,7 +16,7 @@ module Gitlab end def value - @value ||= IssuesFinder.new(@current_user, group_id: @group.id, include_subgroups: true).execute.created_after(@from).count + @value ||= IssuesFinder.new(@current_user, group_id: @group.id, include_subgroups: true, created_after: @from).execute.count end end end diff --git a/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb b/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb index 6505fc714c4..eea4f33ccb8 100644 --- a/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/group_stage_summary_spec.rb @@ -22,6 +22,16 @@ describe Gitlab::CycleAnalytics::GroupStageSummary do it "finds the number of issues created after it" do expect(subject.first[:value]).to eq(2) end + + context 'with subgroups' do + before do + Timecop.freeze(5.days.from_now) { create(:issue, project: create(:project, namespace: create(:group, parent: group))) } + end + + it "finds issues from them" do + expect(subject.first[:value]).to eq(3) + end + end end context 'with other projects' do @@ -35,18 +45,6 @@ describe Gitlab::CycleAnalytics::GroupStageSummary do expect(subject.first[:value]).to eq(2) end end - - context 'with subgroups' do - before do - Timecop.freeze(5.days.from_now) { create(:issue, project: create(:project, namespace: create(:group, parent: group))) } - Timecop.freeze(5.days.from_now) { create(:issue, project: project) } - Timecop.freeze(5.days.from_now) { create(:issue, project: project_2) } - end - - it "finds issues from them" do - expect(subject.first[:value]).to eq(3) - end - end end describe "#deploys" do @@ -61,29 +59,29 @@ describe Gitlab::CycleAnalytics::GroupStageSummary do it "finds the number of deploys made created after it" do expect(subject.second[:value]).to eq(2) end - end - context 'with other projects' do - before do - Timecop.freeze(5.days.from_now) do - create(:deployment, :success, project: create(:project, :repository, namespace: create(:group))) + context 'with subgroups' do + before do + Timecop.freeze(5.days.from_now) do + create(:deployment, :success, project: create(:project, :repository, namespace: create(:group, parent: group))) + end end - end - it "doesn't find deploys from them" do - expect(subject.second[:value]).to eq(0) + it "finds deploys from them" do + expect(subject.second[:value]).to eq(3) + end end end - context 'with subgroups' do + context 'with other projects' do before do Timecop.freeze(5.days.from_now) do - create(:deployment, :success, project: create(:project, :repository, namespace: create(:group, parent: group))) + create(:deployment, :success, project: create(:project, :repository, namespace: create(:group))) end end - it "finds deploys from them" do - expect(subject.second[:value]).to eq(1) + it "doesn't find deploys from them" do + expect(subject.second[:value]).to eq(0) end end end diff --git a/spec/models/cycle_analytics/group_level_spec.rb b/spec/models/cycle_analytics/group_level_spec.rb index 70c370bc39d..154c1b9c0f8 100644 --- a/spec/models/cycle_analytics/group_level_spec.rb +++ b/spec/models/cycle_analytics/group_level_spec.rb @@ -12,10 +12,10 @@ describe CycleAnalytics::GroupLevel do let(:mr) { create_merge_request_closing_issue(user, project, issue, commit_message: "References #{issue.to_reference}") } let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha, head_pipeline_of: mr) } - subject { described_class.new(options: { from: from_date, group: group, current_user: user }) } + subject { described_class.new(group: group, options: { from: from_date, current_user: user }) } describe '#permissions' do - it 'returns permissions' do + it 'returns true for all stages' do expect(subject.permissions.values.uniq).to eq([true]) end end -- cgit v1.2.1 From 196dfd1243d5b5cf9786ff5019049104f7c22c7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Tue, 16 Jul 2019 18:03:13 +0200 Subject: Remove changelog entry --- changelogs/unreleased/adjust-cycle-analytics-to-group-level.yml | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 changelogs/unreleased/adjust-cycle-analytics-to-group-level.yml diff --git a/changelogs/unreleased/adjust-cycle-analytics-to-group-level.yml b/changelogs/unreleased/adjust-cycle-analytics-to-group-level.yml deleted file mode 100644 index 49f558a6c9c..00000000000 --- a/changelogs/unreleased/adjust-cycle-analytics-to-group-level.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Adjust cycle analytics to group level -merge_request: 30391 -author: -type: added -- cgit v1.2.1 From 355330dd1ea6f13f1049b1be044ec1ecf51ba658 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 16 Jul 2019 11:42:48 -0500 Subject: Add EE-only class instrumentation --- config/initializers/zz_metrics.rb | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/config/initializers/zz_metrics.rb b/config/initializers/zz_metrics.rb index 5aa6f73c5c5..5a062559c29 100644 --- a/config/initializers/zz_metrics.rb +++ b/config/initializers/zz_metrics.rb @@ -6,6 +6,7 @@ # that we can stub it for testing, as it is only called when metrics are # enabled. # +# rubocop:disable Metrics/AbcSize def instrument_classes(instrumentation) instrumentation.instrument_instance_methods(Gitlab::Shell) @@ -86,12 +87,42 @@ def instrument_classes(instrumentation) instrumentation.instrument_methods(Gitlab::Highlight) instrumentation.instrument_instance_methods(Gitlab::Highlight) + Gitlab.ee do + instrumentation.instrument_methods(Elasticsearch::Git::Repository) + instrumentation.instrument_instance_methods(Elasticsearch::Git::Repository) + + instrumentation.instrument_instance_methods(Search::GlobalService) + instrumentation.instrument_instance_methods(Search::ProjectService) + + instrumentation.instrument_instance_methods(Gitlab::Elastic::SearchResults) + instrumentation.instrument_instance_methods(Gitlab::Elastic::ProjectSearchResults) + instrumentation.instrument_instance_methods(Gitlab::Elastic::Indexer) + instrumentation.instrument_instance_methods(Gitlab::Elastic::SnippetSearchResults) + instrumentation.instrument_methods(Gitlab::Elastic::Helper) + + instrumentation.instrument_instance_methods(Elastic::ApplicationSearch) + instrumentation.instrument_instance_methods(Elastic::IssuesSearch) + instrumentation.instrument_instance_methods(Elastic::MergeRequestsSearch) + instrumentation.instrument_instance_methods(Elastic::MilestonesSearch) + instrumentation.instrument_instance_methods(Elastic::NotesSearch) + instrumentation.instrument_instance_methods(Elastic::ProjectsSearch) + instrumentation.instrument_instance_methods(Elastic::RepositoriesSearch) + instrumentation.instrument_instance_methods(Elastic::SnippetsSearch) + instrumentation.instrument_instance_methods(Elastic::WikiRepositoriesSearch) + + instrumentation.instrument_instance_methods(Gitlab::BitbucketImport::Importer) + instrumentation.instrument_instance_methods(Bitbucket::Connection) + + instrumentation.instrument_instance_methods(Geo::RepositorySyncWorker) + end + # This is a Rails scope so we have to instrument it manually. instrumentation.instrument_method(Project, :visible_to_user) # Needed for https://gitlab.com/gitlab-org/gitlab-ce/issues/30224#note_32306159 instrumentation.instrument_instance_method(MergeRequestDiff, :load_commits) end +# rubocop:enable Metrics/AbcSize # With prometheus enabled by default this breaks all specs # that stubs methods using `any_instance_of` for the models reloaded here. -- cgit v1.2.1 From 8765d537373679ccbb219ee400c277384972c742 Mon Sep 17 00:00:00 2001 From: John Cai Date: Wed, 10 Jul 2019 14:36:49 -0700 Subject: Wrap rugged calls with access disk block Whenever we use the rugged implementation, we are going straight to disk so we want to bypass the disk access check. --- lib/gitlab/git/rugged_impl/blob.rb | 2 +- lib/gitlab/git/rugged_impl/commit.rb | 6 +++--- lib/gitlab/git/rugged_impl/repository.rb | 2 +- lib/gitlab/git/rugged_impl/tree.rb | 2 +- lib/gitlab/git/rugged_impl/use_rugged.rb | 6 ++++++ lib/gitlab/gitaly_client.rb | 25 +++++++++++----------- .../cache/ci/project_pipeline_status_spec.rb | 1 - spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb | 3 +-- spec/spec_helper.rb | 2 ++ 9 files changed, 27 insertions(+), 22 deletions(-) diff --git a/lib/gitlab/git/rugged_impl/blob.rb b/lib/gitlab/git/rugged_impl/blob.rb index 86c9f33d82a..9aea736527b 100644 --- a/lib/gitlab/git/rugged_impl/blob.rb +++ b/lib/gitlab/git/rugged_impl/blob.rb @@ -16,7 +16,7 @@ module Gitlab override :tree_entry def tree_entry(repository, sha, path, limit) if use_rugged?(repository, :rugged_tree_entry) - rugged_tree_entry(repository, sha, path, limit) + wrap_rugged_call { rugged_tree_entry(repository, sha, path, limit) } else super end diff --git a/lib/gitlab/git/rugged_impl/commit.rb b/lib/gitlab/git/rugged_impl/commit.rb index 971a33b2e99..29ae9bdd851 100644 --- a/lib/gitlab/git/rugged_impl/commit.rb +++ b/lib/gitlab/git/rugged_impl/commit.rb @@ -36,7 +36,7 @@ module Gitlab override :find_commit def find_commit(repo, commit_id) if use_rugged?(repo, :rugged_find_commit) - rugged_find(repo, commit_id) + wrap_rugged_call { rugged_find(repo, commit_id) } else super end @@ -45,7 +45,7 @@ module Gitlab override :batch_by_oid def batch_by_oid(repo, oids) if use_rugged?(repo, :rugged_list_commits_by_oid) - rugged_batch_by_oid(repo, oids) + wrap_rugged_call { rugged_batch_by_oid(repo, oids) } else super end @@ -68,7 +68,7 @@ module Gitlab override :commit_tree_entry def commit_tree_entry(path) if use_rugged?(@repository, :rugged_commit_tree_entry) - rugged_tree_entry(path) + wrap_rugged_call { rugged_tree_entry(path) } else super end diff --git a/lib/gitlab/git/rugged_impl/repository.rb b/lib/gitlab/git/rugged_impl/repository.rb index 9268abdfed9..7bed553393c 100644 --- a/lib/gitlab/git/rugged_impl/repository.rb +++ b/lib/gitlab/git/rugged_impl/repository.rb @@ -48,7 +48,7 @@ module Gitlab override :ancestor? def ancestor?(from, to) if use_rugged?(self, :rugged_commit_is_ancestor) - rugged_is_ancestor?(from, to) + wrap_rugged_call { rugged_is_ancestor?(from, to) } else super end diff --git a/lib/gitlab/git/rugged_impl/tree.rb b/lib/gitlab/git/rugged_impl/tree.rb index f3721a3f1b7..479c5f9d8b7 100644 --- a/lib/gitlab/git/rugged_impl/tree.rb +++ b/lib/gitlab/git/rugged_impl/tree.rb @@ -16,7 +16,7 @@ module Gitlab override :tree_entries def tree_entries(repository, sha, path, recursive) if use_rugged?(repository, :rugged_tree_entries) - tree_entries_with_flat_path_from_rugged(repository, sha, path, recursive) + wrap_rugged_call { tree_entries_with_flat_path_from_rugged(repository, sha, path, recursive) } else super end diff --git a/lib/gitlab/git/rugged_impl/use_rugged.rb b/lib/gitlab/git/rugged_impl/use_rugged.rb index 99091b03cd1..902fa3c7822 100644 --- a/lib/gitlab/git/rugged_impl/use_rugged.rb +++ b/lib/gitlab/git/rugged_impl/use_rugged.rb @@ -10,6 +10,12 @@ module Gitlab Gitlab::GitalyClient.can_use_disk?(repo.storage) end + + def wrap_rugged_call(&block) + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + yield + end + end end end end diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index 091351e5cb2..3cafa49ec51 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -388,21 +388,20 @@ module Gitlab end def self.can_use_disk?(storage) - false - # cached_value = MUTEX.synchronize do - # @can_use_disk ||= {} - # @can_use_disk[storage] - # end + cached_value = MUTEX.synchronize do + @can_use_disk ||= {} + @can_use_disk[storage] + end - # return cached_value unless cached_value.nil? + return cached_value if cached_value.present? - # gitaly_filesystem_id = filesystem_id(storage) - # direct_filesystem_id = filesystem_id_from_disk(storage) + gitaly_filesystem_id = filesystem_id(storage) + direct_filesystem_id = filesystem_id_from_disk(storage) - # MUTEX.synchronize do - # @can_use_disk[storage] = gitaly_filesystem_id.present? && - # gitaly_filesystem_id == direct_filesystem_id - # end + MUTEX.synchronize do + @can_use_disk[storage] = gitaly_filesystem_id.present? && + gitaly_filesystem_id == direct_filesystem_id + end end def self.filesystem_id(storage) @@ -415,7 +414,7 @@ module Gitlab metadata_file = File.read(storage_metadata_file_path(storage)) metadata_hash = JSON.parse(metadata_file) metadata_hash['gitaly_filesystem_id'] - rescue Errno::ENOENT, JSON::ParserError + rescue Errno::ENOENT, Errno::ACCESS, JSON::ParserError nil end diff --git a/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb b/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb index 972dd7e0d2b..483c5ea9cff 100644 --- a/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb +++ b/spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb @@ -26,7 +26,6 @@ describe Gitlab::Cache::Ci::ProjectPipelineStatus, :clean_gitlab_redis_cache do end it 'loads 10 projects without hitting Gitaly call limit', :request_store do - allow(Gitlab::GitalyClient).to receive(:can_access_disk?).and_return(false) projects = Gitlab::GitalyClient.allow_n_plus_1_calls do (1..10).map { create(:project, :repository) } end diff --git a/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb b/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb index e7ef9d08f80..e437647c258 100644 --- a/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb +++ b/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb @@ -21,6 +21,7 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do end before do + allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_call_original Gitlab::GitalyClient.instance_variable_set(:@can_use_disk, {}) end @@ -30,7 +31,6 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do end it 'returns true when gitaly matches disk' do - pending('temporary disabled because of https://gitlab.com/gitlab-org/gitlab-ce/issues/64338') expect(subject.use_rugged?(repository, feature_flag_name)).to be true end @@ -49,7 +49,6 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do end it "doesn't lead to a second rpc call because gitaly client should use the cached value" do - pending('temporary disabled because of https://gitlab.com/gitlab-org/gitlab-ce/issues/64338') expect(subject.use_rugged?(repository, feature_flag_name)).to be true expect(Gitlab::GitalyClient).not_to receive(:filesystem_id) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 95e0d8858b9..978158192d9 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -134,6 +134,8 @@ RSpec.configure do |config| allow(Feature).to receive(:enabled?).with(flag).and_return(enabled) end + allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(enabled) + # The following can be removed when we remove the staged rollout strategy # and we can just enable it using instance wide settings # (ie. ApplicationSetting#auto_devops_enabled) -- cgit v1.2.1 From 34c6f2723c3bc44aba1d9d886522fdfe8db6a9f6 Mon Sep 17 00:00:00 2001 From: Guillaume Grossetie Date: Wed, 10 Jul 2019 10:30:10 +0200 Subject: Preserve footnote link ids --- ...4645-asciidoctor-preserve-footnote-link-ids.yml | 5 +++ lib/banzai/filter/ascii_doc_sanitization_filter.rb | 33 +++++++++++--- spec/lib/gitlab/asciidoc_spec.rb | 50 ++++++++++++++++++++++ 3 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/64645-asciidoctor-preserve-footnote-link-ids.yml diff --git a/changelogs/unreleased/64645-asciidoctor-preserve-footnote-link-ids.yml b/changelogs/unreleased/64645-asciidoctor-preserve-footnote-link-ids.yml new file mode 100644 index 00000000000..5427a035478 --- /dev/null +++ b/changelogs/unreleased/64645-asciidoctor-preserve-footnote-link-ids.yml @@ -0,0 +1,5 @@ +--- +title: "Preserve footnote link ids in Asciidoctor" +merge_request: 30790 +author: Guillaume Grossetie +type: added \ No newline at end of file diff --git a/lib/banzai/filter/ascii_doc_sanitization_filter.rb b/lib/banzai/filter/ascii_doc_sanitization_filter.rb index d8d63075752..9105e86ad04 100644 --- a/lib/banzai/filter/ascii_doc_sanitization_filter.rb +++ b/lib/banzai/filter/ascii_doc_sanitization_filter.rb @@ -8,12 +8,18 @@ module Banzai class AsciiDocSanitizationFilter < Banzai::Filter::BaseSanitizationFilter # Section anchor link pattern SECTION_LINK_REF_PATTERN = /\A#{Gitlab::Asciidoc::DEFAULT_ADOC_ATTRS['idprefix']}(:?[[:alnum:]]|-|_)+\z/.freeze + SECTION_HEADINGS = %w(h2 h3 h4 h5 h6).freeze + + # Footnote link patterns + FOOTNOTE_LINK_ID_PATTERNS = { + a: /\A_footnoteref_\d+\z/, + div: /\A_footnotedef_\d+\z/ + }.freeze # Classes used by Asciidoctor to style components ADMONITION_CLASSES = %w(fa icon-note icon-tip icon-warning icon-caution icon-important).freeze CALLOUT_CLASSES = ['conum'].freeze CHECKLIST_CLASSES = %w(fa fa-check-square-o fa-square-o).freeze - LIST_CLASSES = %w(checklist none no-bullet unnumbered unstyled).freeze ELEMENT_CLASSES_WHITELIST = { @@ -26,8 +32,6 @@ module Banzai a: ['anchor'].freeze }.freeze - ALLOWED_HEADERS = %w(h2 h3 h4 h5 h6).freeze - def customize_whitelist(whitelist) # Allow marks whitelist[:elements].push('mark') @@ -44,20 +48,39 @@ module Banzai whitelist[:transformers].push(self.class.remove_element_classes) # Allow `id` in heading elements for section anchors - ALLOWED_HEADERS.each do |header| + SECTION_HEADINGS.each do |header| whitelist[:attributes][header] = %w(id) end whitelist[:transformers].push(self.class.remove_non_heading_ids) + # Allow `id` in footnote elements + FOOTNOTE_LINK_ID_PATTERNS.keys.each do |element| + whitelist[:attributes][element.to_s].push('id') + end + whitelist[:transformers].push(self.class.remove_non_footnote_ids) + whitelist end class << self + def remove_non_footnote_ids + lambda do |env| + node = env[:node] + + return unless (pattern = FOOTNOTE_LINK_ID_PATTERNS[node.name.to_sym]) + return unless node.has_attribute?('id') + + return if node['id'] =~ pattern + + node.remove_attribute('id') + end + end + def remove_non_heading_ids lambda do |env| node = env[:node] - return unless ALLOWED_HEADERS.any?(node.name) + return unless SECTION_HEADINGS.any?(node.name) return unless node.has_attribute?('id') return if node['id'] =~ SECTION_LINK_REF_PATTERN diff --git a/spec/lib/gitlab/asciidoc_spec.rb b/spec/lib/gitlab/asciidoc_spec.rb index 5293732ead1..cbd4a509a55 100644 --- a/spec/lib/gitlab/asciidoc_spec.rb +++ b/spec/lib/gitlab/asciidoc_spec.rb @@ -110,6 +110,56 @@ module Gitlab expect(render(input, context)).to include(output.strip) end + + it 'removes non footnote def ids' do + input = <<~ADOC + ++++ +
Footnote definition
+ ++++ + ADOC + + output = <<~HTML +
Footnote definition
+ HTML + + expect(render(input, context)).to include(output.strip) + end + + it 'removes non footnote ref ids' do + input = <<~ADOC + ++++ + Footnote reference + ++++ + ADOC + + output = <<~HTML + Footnote reference + HTML + + expect(render(input, context)).to include(output.strip) + end + end + + context 'with footnotes' do + it 'preserves ids and links' do + input = <<~ADOC + This paragraph has a footnote.footnote:[This is the text of the footnote.] + ADOC + + output = <<~HTML +
+

This paragraph has a footnote.[1]

+
+
+
+
+ 1. This is the text of the footnote. +
+
+ HTML + + expect(render(input, context)).to include(output.strip) + end end context 'with section anchors' do -- cgit v1.2.1 From a1bee1a09d529007c4ff11ab55d355b51f352ed8 Mon Sep 17 00:00:00 2001 From: Desiree Chevalier Date: Fri, 7 Jun 2019 08:46:48 -0400 Subject: Generate parallel spec reports Creates reports for parallel specs and collates them into a single HTML report and displays results on the merge request. --- .gitlab/ci/review.gitlab-ci.yml | 33 +++++++++++++++- qa/Gemfile | 1 + qa/Gemfile.lock | 3 ++ scripts/merge-html-reports | 84 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 1 deletion(-) create mode 100755 scripts/merge-html-reports diff --git a/.gitlab/ci/review.gitlab-ci.yml b/.gitlab/ci/review.gitlab-ci.yml index ce019de213b..41d52c4e095 100644 --- a/.gitlab/ci/review.gitlab-ci.yml +++ b/.gitlab/ci/review.gitlab-ci.yml @@ -174,7 +174,38 @@ review-qa-all: script: - export KNAPSACK_REPORT_PATH=knapsack/${CI_PROJECT_NAME}/review-qa-all_master_report.json - export KNAPSACK_TEST_FILE_PATTERN=qa/specs/features/**/*_spec.rb - - gitlab-qa Test::Instance::Any "${QA_IMAGE}" "${CI_ENVIRONMENT_URL}" + - gitlab-qa Test::Instance::Any "${QA_IMAGE}" "${CI_ENVIRONMENT_URL}" -- --format RspecJunitFormatter --out tmp/rspec-${CI_JOB_ID}.xml --format html --out tmp/rspec.htm --color --format documentation + +parallel-spec-reports: + extends: .dedicated-runner + dependencies: + - review-qa-all + image: ruby:2.6-alpine + services: [] + before_script: [] + variables: + SETUP_DB: "false" + NEW_PARALLEL_SPECS_REPORT: qa/report-new.html + BASE_ARTIFACT_URL: "${CI_PROJECT_URL}/-/jobs/${CI_JOB_ID}/artifacts/file/qa/" + stage: post-test + allow_failure: true + when: manual + retry: 0 + artifacts: + when: always + paths: + - qa/report-new.html + - qa/gitlab-qa-run-* + reports: + junit: qa/gitlab-qa-run-*/**/rspec-*.xml + script: + - apk add --update build-base libxml2-dev libxslt-dev && rm -rf /var/cache/apk/* + - gem install nokogiri + - cd qa/gitlab-qa-run-*/gitlab-* + - ARTIFACT_DIRS=$(pwd |rev| awk -F / '{print $1,$2}' | rev | sed s_\ _/_) + - cd ../../.. + - '[[ -f $NEW_PARALLEL_SPECS_REPORT ]] || echo "{}" > ${NEW_PARALLEL_SPECS_REPORT}' + - scripts/merge-html-reports ${NEW_PARALLEL_SPECS_REPORT} ${BASE_ARTIFACT_URL}${ARTIFACT_DIRS} qa/gitlab-qa-run-*/**/rspec.htm .review-performance-base: &review-performance-base <<: *review-qa-base diff --git a/qa/Gemfile b/qa/Gemfile index c46be8a0362..53e7cc497e2 100644 --- a/qa/Gemfile +++ b/qa/Gemfile @@ -10,6 +10,7 @@ gem 'selenium-webdriver', '~> 3.12' gem 'airborne', '~> 0.2.13' gem 'nokogiri', '~> 1.10.3' gem 'rspec-retry', '~> 0.6.1' +gem 'rspec_junit_formatter', '~> 0.4.1' gem 'faker', '~> 1.6', '>= 1.6.6' gem 'knapsack', '~> 1.17' gem 'parallel_tests', '~> 2.29' diff --git a/qa/Gemfile.lock b/qa/Gemfile.lock index 73aabf2c6ad..7d19366f83b 100644 --- a/qa/Gemfile.lock +++ b/qa/Gemfile.lock @@ -87,6 +87,8 @@ GEM rspec-retry (0.6.1) rspec-core (> 3.3) rspec-support (3.7.0) + rspec_junit_formatter (0.4.1) + rspec-core (>= 2, < 4, != 2.12.0) rubyzip (1.2.2) selenium-webdriver (3.141.0) childprocess (~> 0.5) @@ -116,6 +118,7 @@ DEPENDENCIES rake (~> 12.3.0) rspec (~> 3.7) rspec-retry (~> 0.6.1) + rspec_junit_formatter (~> 0.4.1) selenium-webdriver (~> 3.12) BUNDLED WITH diff --git a/scripts/merge-html-reports b/scripts/merge-html-reports new file mode 100755 index 00000000000..7d1e15186c8 --- /dev/null +++ b/scripts/merge-html-reports @@ -0,0 +1,84 @@ +#!/usr/bin/env ruby + +require 'nokogiri' + +main_report_file = ARGV.shift +unless main_report_file + puts 'usage: merge-html-reports [parallel reports...]' + exit 1 +end + +base_artifact_url = ARGV.shift +unless base_artifact_url + puts 'usage: merge-html-reports [parallel reports...]' + exit 1 +end + +# Create the base report with empty body tag +new_report = Nokogiri::HTML.parse(File.read(ARGV[0])) +new_report.at_css('body').remove +empty_body = Nokogiri::XML::Node.new('body', new_report) +new_report.at_css('head').add_next_sibling(empty_body) + +ARGV.each do |report_file| + report = Nokogiri::HTML.parse(File.read(report_file)) + + report.css('a').each do |link| + link_suffix = link['href'].slice(19..-1) + link['href'] = base_artifact_url + link_suffix + end + + header = report.css('div #rspec-header') + tests = report.css('dt[id^="example_group_"]') + + tests.each do |test| + title = test.parent + group = title.parent + script = title.css('script') + + if script.inner_html.include? 'makeYellow' + test.remove_class('passed') + test.add_class('not_implemented') + + group.remove_class('passed') + group.add_class('not_implemented') + header.add_class('not_implemented') + + script.remove + test.next_sibling.remove + test.next_sibling.remove + + elsif script.inner_html.include? 'makeRed' + test.remove_class('passed') + test.add_class('failed') + + group.remove_class('passed') + group.add_class('failed') + header.add_class('failed') + + script.remove + test.next_sibling.remove + test.next_sibling.remove + end + end + + duration = report.at_css('p#duration') + totals = report.at_css('p#totals') + + duration_script = report.css('div.results script')[-2] + totals_script = report.css('div.results script')[-1] + + duration_text = duration_script.text.slice(49..-3) + totals_text = totals_script.text.slice(47..-3) + + duration.inner_html = duration_text + totals.inner_html = totals_text + + duration_script.remove + totals_script.remove + + # Add the new result after the last one to keep the test order + new_report.css('body')[-1].add_next_sibling(report.at_css('body')) +end + +File.write(main_report_file, new_report) -- cgit v1.2.1 From 926e16e5d16ff5bfc52c21a98f0266e5939274c5 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Wed, 17 Jul 2019 07:15:01 -0700 Subject: Document how to make Oracle Cloud S3 work Related issues: * https://github.com/fog/fog-aws/issues/523 * https://gitlab.com/gitlab-org/gitlab-ce/issues/63041 --- doc/administration/job_artifacts.md | 1 + doc/administration/merge_request_diffs.md | 1 + doc/administration/uploads.md | 17 +++++++++++++++++ doc/workflow/lfs/lfs_administration.md | 1 + 4 files changed, 20 insertions(+) diff --git a/doc/administration/job_artifacts.md b/doc/administration/job_artifacts.md index 5490a59ceac..2b624f8de77 100644 --- a/doc/administration/job_artifacts.md +++ b/doc/administration/job_artifacts.md @@ -115,6 +115,7 @@ The connection settings match those provided by [Fog](https://github.com/fog), a | `aws_access_key_id` | AWS credentials, or compatible | | | `aws_secret_access_key` | AWS credentials, or compatible | | | `aws_signature_version` | AWS signature version to use. 2 or 4 are valid options. Digital Ocean Spaces and other providers may need 2. | 4 | +| `enable_signature_v4_streaming` | Set to true to enable HTTP chunked transfers with AWS v4 signatures (https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-streaming.html). Oracle Cloud S3 needs this to be false | true | `region` | AWS region | us-east-1 | | `host` | S3 compatible host for when not using AWS, e.g. `localhost` or `storage.example.com` | s3.amazonaws.com | | `endpoint` | Can be used when configuring an S3 compatible service such as [Minio](https://www.minio.io), by entering a URL such as `http://127.0.0.1:9000` | (optional) | diff --git a/doc/administration/merge_request_diffs.md b/doc/administration/merge_request_diffs.md index 99cd9051778..0b065082ded 100644 --- a/doc/administration/merge_request_diffs.md +++ b/doc/administration/merge_request_diffs.md @@ -90,6 +90,7 @@ The connection settings match those provided by [Fog](https://github.com/fog), a | `aws_access_key_id` | AWS credentials, or compatible | | | `aws_secret_access_key` | AWS credentials, or compatible | | | `aws_signature_version` | AWS signature version to use. 2 or 4 are valid options. Digital Ocean Spaces and other providers may need 2. | 4 | +| `enable_signature_v4_streaming` | Set to true to enable HTTP chunked transfers with AWS v4 signatures (https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-streaming.html). Oracle Cloud S3 needs this to be false | true | `region` | AWS region | us-east-1 | | `host` | S3 compatible host for when not using AWS, e.g. `localhost` or `storage.example.com` | s3.amazonaws.com | | `endpoint` | Can be used when configuring an S3 compatible service such as [Minio](https://www.minio.io), by entering a URL such as `http://127.0.0.1:9000` | (optional) | diff --git a/doc/administration/uploads.md b/doc/administration/uploads.md index 51e267c865e..99f1c386183 100644 --- a/doc/administration/uploads.md +++ b/doc/administration/uploads.md @@ -78,6 +78,7 @@ The connection settings match those provided by [Fog](https://github.com/fog), a | `aws_access_key_id` | AWS credentials, or compatible | | | `aws_secret_access_key` | AWS credentials, or compatible | | | `aws_signature_version` | AWS signature version to use. 2 or 4 are valid options. Digital Ocean Spaces and other providers may need 2. | 4 | +| `enable_signature_v4_streaming` | Set to true to enable HTTP chunked transfers with AWS v4 signatures (https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-streaming.html). Oracle Cloud S3 needs this to be false | true | `region` | AWS region | us-east-1 | | `host` | S3 compatible host for when not using AWS, e.g. `localhost` or `storage.example.com` | s3.amazonaws.com | | `endpoint` | Can be used when configuring an S3 compatible service such as [Minio](https://www.minio.io), by entering a URL such as `http://127.0.0.1:9000` | (optional) | @@ -140,6 +141,22 @@ _The uploads are stored by default in 1. Save the file and [restart GitLab][] for the changes to take effect. 1. Migrate any existing local uploads to the object storage using [`gitlab:uploads:migrate` rake task](raketasks/uploads/migrate.md). +### Oracle Cloud S3 connection settings + +Note that Oracle Cloud S3 must be sure to use the following settings: + +| Setting | Value | +|---------|-------| +| `enable_signature_v4_streaming` | false | +| `path_style` | true | + +If `enable_signature_v4_streaming` is set to `true`, you may see the +following error: + +``` +STREAMING-AWS4-HMAC-SHA256-PAYLOAD is not supported +``` + ### OpenStack compatible connection settings The connection settings match those provided by [Fog](https://github.com/fog), and are as follows: diff --git a/doc/workflow/lfs/lfs_administration.md b/doc/workflow/lfs/lfs_administration.md index 55183408a10..3160b0c4deb 100644 --- a/doc/workflow/lfs/lfs_administration.md +++ b/doc/workflow/lfs/lfs_administration.md @@ -91,6 +91,7 @@ Here is a configuration example with S3. | `aws_access_key_id` | AWS credentials, or compatible | `ABC123DEF456` | | `aws_secret_access_key` | AWS credentials, or compatible | `ABC123DEF456ABC123DEF456ABC123DEF456` | | `aws_signature_version` | AWS signature version to use. 2 or 4 are valid options. Digital Ocean Spaces and other providers may need 2. | 4 | +| `enable_signature_v4_streaming` | Set to true to enable HTTP chunked transfers with AWS v4 signatures (https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-streaming.html). Oracle Cloud S3 needs this to be false | true | `region` | AWS region | us-east-1 | | `host` | S3 compatible host for when not using AWS, e.g. `localhost` or `storage.example.com` | s3.amazonaws.com | | `endpoint` | Can be used when configuring an S3 compatible service such as [Minio](https://www.minio.io), by entering a URL such as `http://127.0.0.1:9000` | (optional) | -- cgit v1.2.1 From 7b18f3a022e34b8599cf509a0cc9263c1b97c0f7 Mon Sep 17 00:00:00 2001 From: mtmail Date: Wed, 17 Jul 2019 16:57:56 +0000 Subject: typo --- doc/ci/caching/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/ci/caching/index.md b/doc/ci/caching/index.md index 9a5a3624c73..5b2c3a8765c 100644 --- a/doc/ci/caching/index.md +++ b/doc/ci/caching/index.md @@ -8,7 +8,7 @@ GitLab CI/CD provides a caching mechanism that can be used to save time when your jobs are running. Caching is about speeding the time a job is executed by reusing the same -content of a previous job. It can be particularly useful when your are +content of a previous job. It can be particularly useful when you are developing software that depends on other libraries which are fetched via the internet during build time. -- cgit v1.2.1 From 1b9dbef3fd2e90cb094b1228a00e8e8d6ee800da Mon Sep 17 00:00:00 2001 From: Lucas Charles Date: Wed, 17 Jul 2019 17:43:35 +0000 Subject: Add rule_type to approval_project_rules Adds migration to introduce non-regular rule_types to approval_project_rules --- .../9928ee-add-rule_type-to-approval-project-rules.yml | 5 +++++ ...709204413_add_rule_type_to_approval_project_rules.rb | 17 +++++++++++++++++ ...229_add_index_to_approval_project_rules_rule_type.rb | 17 +++++++++++++++++ db/schema.rb | 2 ++ 4 files changed, 41 insertions(+) create mode 100644 changelogs/unreleased/9928ee-add-rule_type-to-approval-project-rules.yml create mode 100644 db/migrate/20190709204413_add_rule_type_to_approval_project_rules.rb create mode 100644 db/migrate/20190710151229_add_index_to_approval_project_rules_rule_type.rb diff --git a/changelogs/unreleased/9928ee-add-rule_type-to-approval-project-rules.yml b/changelogs/unreleased/9928ee-add-rule_type-to-approval-project-rules.yml new file mode 100644 index 00000000000..698ecebb971 --- /dev/null +++ b/changelogs/unreleased/9928ee-add-rule_type-to-approval-project-rules.yml @@ -0,0 +1,5 @@ +--- +title: Add migration for adding rule_type to approval_project_rules +merge_request: 30575 +author: +type: added diff --git a/db/migrate/20190709204413_add_rule_type_to_approval_project_rules.rb b/db/migrate/20190709204413_add_rule_type_to_approval_project_rules.rb new file mode 100644 index 00000000000..87a228c9bf9 --- /dev/null +++ b/db/migrate/20190709204413_add_rule_type_to_approval_project_rules.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddRuleTypeToApprovalProjectRules < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default :approval_project_rules, :rule_type, :integer, limit: 2, default: 0, allow_null: false + end + + def down + remove_column :approval_project_rules, :rule_type + end +end diff --git a/db/migrate/20190710151229_add_index_to_approval_project_rules_rule_type.rb b/db/migrate/20190710151229_add_index_to_approval_project_rules_rule_type.rb new file mode 100644 index 00000000000..64123c53c4a --- /dev/null +++ b/db/migrate/20190710151229_add_index_to_approval_project_rules_rule_type.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddIndexToApprovalProjectRulesRuleType < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :approval_project_rules, :rule_type + end + + def down + remove_concurrent_index :approval_project_rules, :rule_type + end +end diff --git a/db/schema.rb b/db/schema.rb index f752211f2ec..a5079d3a5bc 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -282,7 +282,9 @@ ActiveRecord::Schema.define(version: 2019_07_15_114644) do t.integer "project_id", null: false t.integer "approvals_required", limit: 2, default: 0, null: false t.string "name", null: false + t.integer "rule_type", limit: 2, default: 0, null: false t.index ["project_id"], name: "index_approval_project_rules_on_project_id", using: :btree + t.index ["rule_type"], name: "index_approval_project_rules_on_rule_type", using: :btree end create_table "approval_project_rules_groups", force: :cascade do |t| -- cgit v1.2.1 From e424fcedcc5cbe9cd96b74b057f243161db6e2e9 Mon Sep 17 00:00:00 2001 From: Martin Hanzel Date: Wed, 17 Jul 2019 17:47:19 +0000 Subject: Add docs about auto-injected Jest mocks --- doc/development/testing_guide/frontend_testing.md | 28 +++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/doc/development/testing_guide/frontend_testing.md b/doc/development/testing_guide/frontend_testing.md index bb44cc595e9..c909745b1ab 100644 --- a/doc/development/testing_guide/frontend_testing.md +++ b/doc/development/testing_guide/frontend_testing.md @@ -79,6 +79,34 @@ describe('Component', () => { Remember that the performance of each test depends on the environment. +### Manual module mocks +Jest supports [manual module mocks](https://jestjs.io/docs/en/manual-mocks) by placing a mock in a `__mocks__/` directory next to the source module. **Don't do this.** We want to keep all of our test-related code in one place (the `spec/` folder), and the logic that Jest uses to apply mocks from `__mocks__/` is rather inconsistent. + +Instead, our test runner detects manual mocks from `spec/frontend/mocks/`. Any mock placed here is automatically picked up and injected whenever you import its source module. + +- Files in `spec/frontend/mocks/ce` will mock the corresponding CE module from `app/assets/javascripts`, mirroring the source module's path. + - Example: `spec/frontend/mocks/ce/lib/utils/axios_utils` will mock the module `~/lib/utils/axios_utils`. +- Files in `spec/frontend/mocks/node` will mock NPM packages of the same name or path. +- We don't support mocking EE modules yet. + +If a mock is found for which a source module doesn't exist, the test suite will fail. 'Virtual' mocks, or mocks that don't have a 1-to-1 association with a source module, are not supported yet. + +#### Writing a mock +Create a JS module in the appropriate place in `spec/frontend/mocks/`. That's it. It will automatically mock its source package in all tests. + +Make sure that your mock's export has the same format as the mocked module. So, if you're mocking a CommonJS module, you'll need to use `module.exports` instead of the ES6 `export`. + +It might be useful for a mock to expose a property that indicates if the mock was loaded. This way, tests can assert the presence of a mock without calling any logic and causing side-effects. The `~/lib/utils/axios_utils` module mock has such a property, `isMock`, that is `true` in the mock and undefined in the original class. Jest's mock functions also have a `mock` property that you can test. + +#### Bypassing mocks +If you ever need to import the original module in your tests, use [`jest.requireActual()`](https://jestjs.io/docs/en/jest-object#jestrequireactualmodulename) (or `jest.requireActual().default` for the default export). The `jest.mock()` and `jest.unmock()` won't have an effect on modules that have a manual mock, because mocks are imported and cached before any tests are run. + +#### Keep mocks light +Global mocks introduce magic and can affect how modules are imported in your tests. Try to keep them as light as possible and dependency-free. A global mock should be useful for any unit test. For example, the `axios_utils` and `jquery` module mocks throw an error when an HTTP request is attempted, since this is useful behaviour in >99% of tests. + +When in doubt, construct mocks in your test file using [`jest.mock()`](https://jestjs.io/docs/en/jest-object#jestmockmodulename-factory-options), [`jest.spyOn()`](https://jestjs.io/docs/en/jest-object#jestspyonobject-methodname), etc. + + ## Karma test suite GitLab uses the [Karma][karma] test runner with [Jasmine] as its test -- cgit v1.2.1 From eb45cb8c99f4232da90c26b307eff35c926d1975 Mon Sep 17 00:00:00 2001 From: Martin Hanzel Date: Wed, 17 Jul 2019 17:47:39 +0000 Subject: Mockify jquery and axios packages Moved the block that fails tests on unmocked axios requests from test_setup to its own mock, and added a jQuery mock that fails tests if they use unmocked $.ajax(). --- package.json | 1 + spec/frontend/mocks/ce/lib/utils/axios_utils.js | 15 +++ spec/frontend/mocks/mocks_helper.js | 60 +++++++++ spec/frontend/mocks/mocks_helper_spec.js | 147 +++++++++++++++++++++ spec/frontend/mocks/node/jquery.js | 13 ++ spec/frontend/mocks_spec.js | 13 ++ .../components/external_dashboard_spec.js | 5 +- spec/frontend/test_setup.js | 16 +-- yarn.lock | 13 ++ 9 files changed, 269 insertions(+), 14 deletions(-) create mode 100644 spec/frontend/mocks/ce/lib/utils/axios_utils.js create mode 100644 spec/frontend/mocks/mocks_helper.js create mode 100644 spec/frontend/mocks/mocks_helper_spec.js create mode 100644 spec/frontend/mocks/node/jquery.js create mode 100644 spec/frontend/mocks_spec.js diff --git a/package.json b/package.json index 44aa850860e..4ba9a0d9a1b 100644 --- a/package.json +++ b/package.json @@ -191,6 +191,7 @@ "pixelmatch": "^4.0.2", "postcss": "^7.0.14", "prettier": "1.18.2", + "readdir-enhanced": "^2.2.4", "stylelint": "^9.10.1", "stylelint-config-recommended": "^2.1.0", "stylelint-scss": "^3.5.4", diff --git a/spec/frontend/mocks/ce/lib/utils/axios_utils.js b/spec/frontend/mocks/ce/lib/utils/axios_utils.js new file mode 100644 index 00000000000..b4065626b09 --- /dev/null +++ b/spec/frontend/mocks/ce/lib/utils/axios_utils.js @@ -0,0 +1,15 @@ +const axios = jest.requireActual('~/lib/utils/axios_utils').default; + +axios.isMock = true; + +// Fail tests for unmocked requests +axios.defaults.adapter = config => { + const message = + `Unexpected unmocked request: ${JSON.stringify(config, null, 2)}\n` + + 'Consider using the `axios-mock-adapter` in tests.'; + const error = new Error(message); + error.config = config; + throw error; +}; + +export default axios; diff --git a/spec/frontend/mocks/mocks_helper.js b/spec/frontend/mocks/mocks_helper.js new file mode 100644 index 00000000000..21c032cd3c9 --- /dev/null +++ b/spec/frontend/mocks/mocks_helper.js @@ -0,0 +1,60 @@ +/** + * @module + * + * This module implements auto-injected manual mocks that are cleaner than Jest's approach. + * + * See https://docs.gitlab.com/ee/development/testing_guide/frontend_testing.html + */ + +import fs from 'fs'; +import path from 'path'; + +import readdir from 'readdir-enhanced'; + +const MAX_DEPTH = 20; +const prefixMap = [ + // E.g. the mock ce/foo/bar maps to require path ~/foo/bar + { mocksRoot: 'ce', requirePrefix: '~' }, + // { mocksRoot: 'ee', requirePrefix: 'ee' }, // We'll deal with EE-specific mocks later + { mocksRoot: 'node', requirePrefix: '' }, + // { mocksRoot: 'virtual', requirePrefix: '' }, // We'll deal with virtual mocks later +]; + +const mockFileFilter = stats => stats.isFile() && stats.path.endsWith('.js'); + +const getMockFiles = root => readdir.sync(root, { deep: MAX_DEPTH, filter: mockFileFilter }); + +// Function that performs setting a mock. This has to be overridden by the unit test, because +// jest.setMock can't be overwritten across files. +// Use require() because jest.setMock expects the CommonJS exports object +const defaultSetMock = (srcPath, mockPath) => + jest.mock(srcPath, () => jest.requireActual(mockPath)); + +// eslint-disable-next-line import/prefer-default-export +export const setupManualMocks = function setupManualMocks(setMock = defaultSetMock) { + prefixMap.forEach(({ mocksRoot, requirePrefix }) => { + const mocksRootAbsolute = path.join(__dirname, mocksRoot); + if (!fs.existsSync(mocksRootAbsolute)) { + return; + } + + getMockFiles(path.join(__dirname, mocksRoot)).forEach(mockPath => { + const mockPathNoExt = mockPath.substring(0, mockPath.length - path.extname(mockPath).length); + const sourcePath = path.join(requirePrefix, mockPathNoExt); + const mockPathRelative = `./${path.join(mocksRoot, mockPathNoExt)}`; + + try { + setMock(sourcePath, mockPathRelative); + } catch (e) { + if (e.message.includes('Could not locate module')) { + // The corresponding mocked module doesn't exist. Raise a better error. + // Eventualy, we may support virtual mocks (mocks whose path doesn't directly correspond + // to a module, like with the `ee_else_ce` prefix). + throw new Error( + `A manual mock was defined for module ${sourcePath}, but the module doesn't exist!`, + ); + } + } + }); + }); +}; diff --git a/spec/frontend/mocks/mocks_helper_spec.js b/spec/frontend/mocks/mocks_helper_spec.js new file mode 100644 index 00000000000..34be110a7e3 --- /dev/null +++ b/spec/frontend/mocks/mocks_helper_spec.js @@ -0,0 +1,147 @@ +/* eslint-disable global-require, promise/catch-or-return */ + +import path from 'path'; + +import axios from '~/lib/utils/axios_utils'; + +const absPath = path.join.bind(null, __dirname); + +jest.mock('fs'); +jest.mock('readdir-enhanced'); + +describe('mocks_helper.js', () => { + let setupManualMocks; + const setMock = jest.fn().mockName('setMock'); + let fs; + let readdir; + + beforeAll(() => { + jest.resetModules(); + jest.setMock = jest.fn().mockName('jest.setMock'); + fs = require('fs'); + readdir = require('readdir-enhanced'); + + // We need to provide setupManualMocks with a mock function that pretends to do the setup of + // the mock. This is because we can't mock jest.setMock across files. + setupManualMocks = () => require('./mocks_helper').setupManualMocks(setMock); + }); + + afterEach(() => { + fs.existsSync.mockReset(); + readdir.sync.mockReset(); + setMock.mockReset(); + }); + + it('enumerates through mock file roots', () => { + setupManualMocks(); + expect(fs.existsSync).toHaveBeenCalledTimes(2); + expect(fs.existsSync).toHaveBeenNthCalledWith(1, absPath('ce')); + expect(fs.existsSync).toHaveBeenNthCalledWith(2, absPath('node')); + + expect(readdir.sync).toHaveBeenCalledTimes(0); + }); + + it("doesn't traverse the directory tree infinitely", () => { + fs.existsSync.mockReturnValue(true); + readdir.sync.mockReturnValue([]); + setupManualMocks(); + + readdir.mock.calls.forEach(call => { + expect(call[1].deep).toBeLessThan(100); + }); + }); + + it('sets up mocks for CE (the ~/ prefix)', () => { + fs.existsSync.mockImplementation(root => root.endsWith('ce')); + readdir.sync.mockReturnValue(['root.js', 'lib/utils/util.js']); + setupManualMocks(); + + expect(readdir.sync).toHaveBeenCalledTimes(1); + expect(readdir.sync.mock.calls[0][0]).toBe(absPath('ce')); + + expect(setMock).toHaveBeenCalledTimes(2); + expect(setMock).toHaveBeenNthCalledWith(1, '~/root', './ce/root'); + expect(setMock).toHaveBeenNthCalledWith(2, '~/lib/utils/util', './ce/lib/utils/util'); + }); + + it('sets up mocks for node_modules', () => { + fs.existsSync.mockImplementation(root => root.endsWith('node')); + readdir.sync.mockReturnValue(['jquery', '@babel/core']); + setupManualMocks(); + + expect(readdir.sync).toHaveBeenCalledTimes(1); + expect(readdir.sync.mock.calls[0][0]).toBe(absPath('node')); + + expect(setMock).toHaveBeenCalledTimes(2); + expect(setMock).toHaveBeenNthCalledWith(1, 'jquery', './node/jquery'); + expect(setMock).toHaveBeenNthCalledWith(2, '@babel/core', './node/@babel/core'); + }); + + it('sets up mocks for all roots', () => { + const files = { + [absPath('ce')]: ['root', 'lib/utils/util'], + [absPath('node')]: ['jquery', '@babel/core'], + }; + + fs.existsSync.mockReturnValue(true); + readdir.sync.mockImplementation(root => files[root]); + setupManualMocks(); + + expect(readdir.sync).toHaveBeenCalledTimes(2); + expect(readdir.sync.mock.calls[0][0]).toBe(absPath('ce')); + expect(readdir.sync.mock.calls[1][0]).toBe(absPath('node')); + + expect(setMock).toHaveBeenCalledTimes(4); + expect(setMock).toHaveBeenNthCalledWith(1, '~/root', './ce/root'); + expect(setMock).toHaveBeenNthCalledWith(2, '~/lib/utils/util', './ce/lib/utils/util'); + expect(setMock).toHaveBeenNthCalledWith(3, 'jquery', './node/jquery'); + expect(setMock).toHaveBeenNthCalledWith(4, '@babel/core', './node/@babel/core'); + }); + + it('fails when given a virtual mock', () => { + fs.existsSync.mockImplementation(p => p.endsWith('ce')); + readdir.sync.mockReturnValue(['virtual', 'shouldntBeImported']); + setMock.mockImplementation(() => { + throw new Error('Could not locate module'); + }); + + expect(setupManualMocks).toThrow( + new Error("A manual mock was defined for module ~/virtual, but the module doesn't exist!"), + ); + + expect(readdir.sync).toHaveBeenCalledTimes(1); + expect(readdir.sync.mock.calls[0][0]).toBe(absPath('ce')); + }); + + describe('auto-injection', () => { + it('handles ambiguous paths', () => { + jest.isolateModules(() => { + const axios2 = require('../../../app/assets/javascripts/lib/utils/axios_utils').default; + expect(axios2.isMock).toBe(true); + }); + }); + + it('survives jest.isolateModules()', done => { + jest.isolateModules(() => { + const axios2 = require('~/lib/utils/axios_utils').default; + expect(axios2.get('http://gitlab.com')) + .rejects.toThrow('Unexpected unmocked request') + .then(done); + }); + }); + + it('can be unmocked and remocked', () => { + jest.dontMock('~/lib/utils/axios_utils'); + jest.resetModules(); + const axios2 = require('~/lib/utils/axios_utils').default; + expect(axios2).not.toBe(axios); + expect(axios2.isMock).toBeUndefined(); + + jest.doMock('~/lib/utils/axios_utils'); + jest.resetModules(); + const axios3 = require('~/lib/utils/axios_utils').default; + expect(axios3).not.toBe(axios2); + expect(axios3.isMock).toBe(true); + }); + }); +}); diff --git a/spec/frontend/mocks/node/jquery.js b/spec/frontend/mocks/node/jquery.js new file mode 100644 index 00000000000..34a25772f67 --- /dev/null +++ b/spec/frontend/mocks/node/jquery.js @@ -0,0 +1,13 @@ +/* eslint-disable import/no-commonjs */ + +const $ = jest.requireActual('jquery'); + +// Fail tests for unmocked requests +$.ajax = () => { + throw new Error( + 'Unexpected unmocked jQuery.ajax() call! Make sure to mock jQuery.ajax() in tests.', + ); +}; + +// jquery is not an ES6 module +module.exports = $; diff --git a/spec/frontend/mocks_spec.js b/spec/frontend/mocks_spec.js new file mode 100644 index 00000000000..2d2324120fd --- /dev/null +++ b/spec/frontend/mocks_spec.js @@ -0,0 +1,13 @@ +import $ from 'jquery'; +import axios from '~/lib/utils/axios_utils'; + +describe('Mock auto-injection', () => { + describe('mocks', () => { + it('~/lib/utils/axios_utils', () => + expect(axios.get('http://gitlab.com')).rejects.toThrow('Unexpected unmocked request')); + + it('jQuery.ajax()', () => { + expect($.ajax).toThrow('Unexpected unmocked'); + }); + }); +}); diff --git a/spec/frontend/operation_settings/components/external_dashboard_spec.js b/spec/frontend/operation_settings/components/external_dashboard_spec.js index a881de8fbfe..39d7c19e731 100644 --- a/spec/frontend/operation_settings/components/external_dashboard_spec.js +++ b/spec/frontend/operation_settings/components/external_dashboard_spec.js @@ -7,7 +7,6 @@ import { refreshCurrentPage } from '~/lib/utils/url_utility'; import createFlash from '~/flash'; import { TEST_HOST } from 'helpers/test_constants'; -jest.mock('~/lib/utils/axios_utils'); jest.mock('~/lib/utils/url_utility'); jest.mock('~/flash'); @@ -32,6 +31,10 @@ describe('operation settings external dashboard component', () => { wrapper = shallow ? shallowMount(...config) : mount(...config); }; + beforeEach(() => { + jest.spyOn(axios, 'patch').mockImplementation(); + }); + afterEach(() => { if (wrapper.destroy) { wrapper.destroy(); diff --git a/spec/frontend/test_setup.js b/spec/frontend/test_setup.js index 15cf18700ed..634c78ec029 100644 --- a/spec/frontend/test_setup.js +++ b/spec/frontend/test_setup.js @@ -2,10 +2,10 @@ import Vue from 'vue'; import * as jqueryMatchers from 'custom-jquery-matchers'; import $ from 'jquery'; import Translate from '~/vue_shared/translate'; -import axios from '~/lib/utils/axios_utils'; import { config as testUtilsConfig } from '@vue/test-utils'; import { initializeTestTimeout } from './helpers/timeout'; import { loadHTMLFixture, setHTMLFixture } from './helpers/fixtures'; +import { setupManualMocks } from './mocks/mocks_helper'; // Expose jQuery so specs using jQuery plugins can be imported nicely. // Here is an issue to explore better alternatives: @@ -14,6 +14,8 @@ window.jQuery = $; process.on('unhandledRejection', global.promiseRejectionHandler); +setupManualMocks(); + afterEach(() => // give Promises a bit more time so they fail the right test new Promise(setImmediate).then(() => { @@ -24,18 +26,6 @@ afterEach(() => initializeTestTimeout(process.env.CI ? 5000 : 500); -// fail tests for unmocked requests -beforeEach(done => { - axios.defaults.adapter = config => { - const error = new Error(`Unexpected unmocked request: ${JSON.stringify(config, null, 2)}`); - error.config = config; - done.fail(error); - return Promise.reject(error); - }; - - done(); -}); - Vue.config.devtools = false; Vue.config.productionTip = false; diff --git a/yarn.lock b/yarn.lock index eaa029281d6..a0fbdfd4541 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4919,6 +4919,11 @@ glob-to-regexp@^0.3.0: resolved "https://registry.yarnpkg.com/glob-to-regexp/-/glob-to-regexp-0.3.0.tgz#8c5a1494d2066c570cc3bfe4496175acc4d502ab" integrity sha1-jFoUlNIGbFcMw7/kSWF1rMTVAqs= +glob-to-regexp@^0.4.0: + version "0.4.1" + resolved "https://registry.yarnpkg.com/glob-to-regexp/-/glob-to-regexp-0.4.1.tgz#c75297087c851b9a578bd217dd59a92f59fe546e" + integrity sha512-lkX1HJXwyMcprw/5YUZc2s7DrpAiHB21/V+E1rHUrVNokkvB6bqMzT0VfV6/86ZNabt1k14YOIaT7nDvOX3Iiw== + "glob@5 - 7", glob@^7.0.0, glob@^7.0.3, glob@^7.1.1, glob@^7.1.2, glob@^7.1.3, glob@~7.1.1: version "7.1.4" resolved "https://registry.yarnpkg.com/glob/-/glob-7.1.4.tgz#aa608a2f6c577ad357e1ae5a5c26d9a8d1969255" @@ -9099,6 +9104,14 @@ readable-stream@~2.0.6: string_decoder "~0.10.x" util-deprecate "~1.0.1" +readdir-enhanced@^2.2.4: + version "2.2.4" + resolved "https://registry.yarnpkg.com/readdir-enhanced/-/readdir-enhanced-2.2.4.tgz#773fb8a8de5f645fb13d9403746d490d4facb3e6" + integrity sha512-JQD83C9gAs5B5j2j40qLn/K83HhR8po3bUonebNeuJQUZbbn7q1HxL9kQuPBtxoXkaUpbtEmpFBw5kzyYnnJDA== + dependencies: + call-me-maybe "^1.0.1" + glob-to-regexp "^0.4.0" + readdirp@^2.0.0: version "2.1.0" resolved "https://registry.yarnpkg.com/readdirp/-/readdirp-2.1.0.tgz#4ed0ad060df3073300c48440373f72d1cc642d78" -- cgit v1.2.1 From a7e16ee20893bbc7f31fb0eb3cdd3018ace9acda Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Wed, 17 Jul 2019 19:30:00 +0000 Subject: Add documentation surrounding [data-qa-selector] Documentation was lacking for the [data-qa-selector] method of defining methods vs .qa-selector method. --- .../testing_guide/end_to_end/page_objects.md | 38 +++++++++++++++++----- .../testing_guide/end_to_end/quick_start_guide.md | 33 +++++++++---------- 2 files changed, 45 insertions(+), 26 deletions(-) diff --git a/doc/development/testing_guide/end_to_end/page_objects.md b/doc/development/testing_guide/end_to_end/page_objects.md index 05cb03eb4bd..29ad49403fe 100644 --- a/doc/development/testing_guide/end_to_end/page_objects.md +++ b/doc/development/testing_guide/end_to_end/page_objects.md @@ -92,20 +92,25 @@ end The `view` DSL method will correspond to the rails View, partial, or vue component that renders the elements. The `element` DSL method in turn declares an element for which a corresponding -`qa-element-name-dasherized` CSS class will need to be added to the view file. +`data-qa-selector=element_name_snaked` data attribute will need to be added to the view file. You can also define a value (String or Regexp) to match to the actual view code but **this is deprecated** in favor of the above method for two reasons: - Consistency: there is only one way to define an element -- Separation of concerns: QA uses dedicated CSS classes instead of reusing code +- Separation of concerns: QA uses dedicated `data-qa-*` attributes instead of reusing code or classes used by other components (e.g. `js-*` classes etc.) ```ruby view 'app/views/my/view.html.haml' do - # Implicitly require `.qa-logout-button` CSS class to be present in the view + + ### Good ### + + # Implicitly require the CSS selector `[data-qa-selector="logout_button"]` to be present in the view element :logout_button + ### Bad ### + ## This is deprecated and forbidden by the `QA/ElementWithPattern` RuboCop cop. # Require `f.submit "Sign in"` to be present in `my/view.html.haml element :my_button, 'f.submit "Sign in"' # rubocop:disable QA/ElementWithPattern @@ -129,24 +134,39 @@ view 'app/views/my/view.html.haml' do end ``` -To add these elements to the view, you must change the rails View, partial, or vue component by adding a `qa-element-descriptor` class +To add these elements to the view, you must change the rails View, partial, or vue component by adding a `data-qa-selector` attribute for each element defined. -In our case, `qa-login-field`, `qa-password-field` and `qa-sign-in-button` +In our case, `data-qa-selector="login_field"`, `data-qa-selector="password_field"` and `data-qa-selector="sign_in_button"` **app/views/my/view.html.haml** ```haml -= f.text_field :login, class: "form-control top qa-login-field", autofocus: "autofocus", autocapitalize: "off", autocorrect: "off", required: true, title: "This field is required." -= f.password_field :password, class: "form-control bottom qa-password-field", required: true, title: "This field is required." -= f.submit "Sign in", class: "btn btn-success qa-sign-in-button" += f.text_field :login, class: "form-control top", autofocus: "autofocus", autocapitalize: "off", autocorrect: "off", required: true, title: "This field is required.", data: { qa_selector: 'login_field' } += f.password_field :password, class: "form-control bottom", required: true, title: "This field is required.", data: { qa_selector: 'password_field' } += f.submit "Sign in", class: "btn btn-success", data: { qa_selector: 'sign_in_button' } ``` Things to note: -- The CSS class must be `kebab-cased` (separated with hyphens "`-`") +- The name of the element and the qa_selector must match and be snake_cased - If the element appears on the page unconditionally, add `required: true` to the element. See [Dynamic element validation](dynamic_element_validation.md) +- You may see `.qa-selector` classes in existing Page Objects. We should prefer the [`data-qa-selector`](#data-qa-selector-vs-qa-selector) + method of definition over the `.qa-selector` CSS class + + +### `data-qa-selector` vs `.qa-selector` + +> Introduced in GitLab 12.1 + +There are two supported methods of defining elements within a view. + +1. `data-qa-selector` attribute +1. `.qa-selector` class + +Any existing `.qa-selector` class should be considered deprecated +and we should prefer the `data-qa-selector` method of definition. ## Running the test locally diff --git a/doc/development/testing_guide/end_to_end/quick_start_guide.md b/doc/development/testing_guide/end_to_end/quick_start_guide.md index efcfd44bc22..3bbf8feab39 100644 --- a/doc/development/testing_guide/end_to_end/quick_start_guide.md +++ b/doc/development/testing_guide/end_to_end/quick_start_guide.md @@ -101,7 +101,7 @@ it 'replaces an existing label if it has the same key' do page.find('#content-body').click page.refresh - labels_block = page.find('.qa-labels-block') + labels_block = page.find(%q([data-qa-selector="labels_block"])) expect(labels_block).to have_content('animal::dolphin') expect(labels_block).not_to have_content('animal::fox') @@ -130,7 +130,7 @@ it 'keeps both scoped labels when adding a label with a different key' do page.find('#content-body').click page.refresh - labels_block = page.find('.qa-labels-block') + labels_block = page.find(%q([data-qa-selector="labels_block"])) expect(labels_block).to have_content('animal::fox') expect(labels_block).to have_content('plant::orchid') @@ -139,7 +139,7 @@ it 'keeps both scoped labels when adding a label with a different key' do end ``` -> Note that elements are always located using CSS selectors, and a good practice is to add test-specific selectors (this is called adding testability to the application and we will talk more about it later.) For example, the `labels_block` element uses the selector `.qa-labels-block`, which was added specifically for testing purposes. +> Note that elements are always located using CSS selectors, and a good practice is to add test-specific selectors (this is called "testability"). For example, the `labels_block` element uses the CSS selector [`data-qa-selector="labels_block"`](page_objects.md#data-qa-selector-vs-qa-selector), which was added specifically for testing purposes. Below are the steps that the test covers: @@ -168,7 +168,7 @@ end it 'replaces an existing label if it has the same key' do select_label_and_refresh @new_label_same_scope - labels_block = page.find('.qa-labels-block') + labels_block = page.find(%q([data-qa-selector="labels_block"])) expect(labels_block).to have_content(@new_label_same_scope) expect(labels_block).not_to have_content(@initial_label) @@ -179,7 +179,7 @@ end it 'keeps both scoped label when adding a label with a different key' do select_label_and_refresh @new_label_different_scope - labels_block = page.find('.qa-labels-block') + labels_block = page.find(%q([data-qa-selector="labels_block"])) expect(labels_blocks).to have_content(@new_label_different_scope) expect(labels_blocks).to have_content(@initial_label) @@ -305,7 +305,7 @@ module QA it 'correctly applies scoped labels depending on if they are from the same or a different scope' do select_labels_and_refresh [@new_label_same_scope, @new_label_different_scope] - labels_block = page.all('.qa-labels-block') + labels_block = page.all(%q([data-qa-selector="labels_block"])) expect(labels_block).to have_content(@new_label_same_scope) expect(labels_block).to have_content(@new_label_different_scope) @@ -552,37 +552,36 @@ The `text_of_labels_block` method is a simple method that returns the `:labels_b #### Updates in the view (*.html.haml) and `dropdowns_helper.rb` files -Now let's change the view and the `dropdowns_helper` files to add the selectors that relate to the Page Object. +Now let's change the view and the `dropdowns_helper` files to add the selectors that relate to the [Page Objects]. -In the [app/views/shared/issuable/_sidebar.html.haml](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/views/shared/issuable/_sidebar.html.haml) file, on [line 105 ](https://gitlab.com/gitlab-org/gitlab-ee/blob/84043fa72ca7f83ae9cde48ad670e6d5d16501a3/app/views/shared/issuable/_sidebar.html.haml#L105), add an extra class `qa-edit-link-labels`. +In [`app/views/shared/issuable/_sidebar.html.haml:105`](https://gitlab.com/gitlab-org/gitlab-ee/blob/7ca12defc7a965987b162a6ebef302f95dc8867f/app/views/shared/issuable/_sidebar.html.haml#L105), add a `data: { qa_selector: 'edit_link_labels' }` data attribute. The code should look like this: ```haml -= link_to _('Edit'), '#', class: 'js-sidebar-dropdown-toggle edit-link float-right qa-edit-link-labels' += link_to _('Edit'), '#', class: 'js-sidebar-dropdown-toggle edit-link float-right', data: { qa_selector: 'edit_link_labels' } ``` -In the same file, on [line 121](https://gitlab.com/gitlab-org/gitlab-ee/blob/84043fa72ca7f83ae9cde48ad670e6d5d16501a3/app/views/shared/issuable/_sidebar.html.haml#L121), add an extra class `.qa-dropdown-menu-labels`. +In the same file, on [line 121](https://gitlab.com/gitlab-org/gitlab-ee/blob/7ca12defc7a965987b162a6ebef302f95dc8867f/app/views/shared/issuable/_sidebar.html.haml#L121), add a `data: { qa_selector: 'dropdown_menu_labels' }` data attribute. The code should look like this: ```haml -.dropdown-menu.dropdown-select.dropdown-menu-paging.dropdown-menu-labels.dropdown-menu-selectable.qa-dropdown-menu-labels +.dropdown-menu.dropdown-select.dropdown-menu-paging.dropdown-menu-labels.dropdown-menu-selectable.dropdown-extended-height{ data: { qa_selector: 'dropdown_menu_labels' } } ``` -In the [`dropdowns_helper.rb`](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/helpers/dropdowns_helper.rb) file, on [line 94](https://gitlab.com/gitlab-org/gitlab-ee/blob/99e51a374f2c20bee0989cac802e4b5621f72714/app/helpers/dropdowns_helper.rb#L94), add an extra class `qa-dropdown-input-field`. +In [`app/helpers/dropdowns_helper.rb:94`](https://gitlab.com/gitlab-org/gitlab-ee/blob/7ca12defc7a965987b162a6ebef302f95dc8867f/app/helpers/dropdowns_helper.rb#L94), add a `data: { qa_selector: 'dropdown_input_field' }` data attribute. The code should look like this: ```ruby -filter_output = search_field_tag search_id, nil, class: "dropdown-input-field qa-dropdown-input-field", placeholder: placeholder, autocomplete: 'off' +filter_output = search_field_tag search_id, nil, class: "dropdown-input-field", placeholder: placeholder, autocomplete: 'off', data: { qa_selector: 'dropdown_input_field' } ``` -> Classes starting with `qa-` are used for testing purposes only, and by defining such classes in the elements we add **testability** in the application. +> `data-qa-*` data attributes and CSS classes starting with `qa-` are used solely for the purpose of QA and testing. +> By defining these, we add **testability** to the application. -> When defining a class like `qa-labels-block`, it is transformed into `:labels_block` for usage in the Page Objects. So, `qa-edit-link-labels` is transformed into `:edit_link_labels`, `qa-dropdown-menu-labels` is transformed into `:dropdown_menu_labels`, and `qa-dropdown-input-field` is transformed into `:dropdown_input_field`. Also, we use a [sanity test](https://gitlab.com/gitlab-org/gitlab-ce/tree/master/qa/qa/page#how-did-we-solve-fragile-tests-problem) to check that defined elements have their respective `qa-` selectors in the specified views. - -> We did not define the `qa-labels-block` class in the `app/views/shared/issuable/_sidebar.html.haml` file because it was already there to be used. +> When defining a data attribute like: `qa_selector: 'labels_block'`, it should match the element definition: `element :labels_block`. We use a [sanity test](https://gitlab.com/gitlab-org/gitlab-ce/tree/master/qa/qa/page#how-did-we-solve-fragile-tests-problem) to check that defined elements have their respective selectors in the specified views. #### Updates in the `QA::Page::Base` class -- cgit v1.2.1 From 0e62440722b14db14f54498d1fb3c389cbfe367f Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Wed, 17 Jul 2019 19:52:03 +0000 Subject: Fix factory default for pages_access_level Pages access level currently depends on project visibilty which is ignored by factory, this commit fixes that --- spec/factories/projects.rb | 4 +++- spec/services/projects/update_service_spec.rb | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 7d7738a30c8..0e8810b73a1 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -25,7 +25,9 @@ FactoryBot.define do issues_access_level ProjectFeature::ENABLED merge_requests_access_level ProjectFeature::ENABLED repository_access_level ProjectFeature::ENABLED - pages_access_level ProjectFeature::ENABLED + pages_access_level do + visibility_level == Gitlab::VisibilityLevel::PUBLIC ? ProjectFeature::ENABLED : ProjectFeature::PRIVATE + end # we can't assign the delegated `#ci_cd_settings` attributes directly, as the # `#ci_cd_settings` relation needs to be created first diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 1dcfb739eb6..6bbaa410d56 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -347,13 +347,13 @@ describe Projects::UpdateService do context 'when updating #pages_access_level' do subject(:call_service) do - update_project(project, admin, project_feature_attributes: { pages_access_level: ProjectFeature::PRIVATE }) + update_project(project, admin, project_feature_attributes: { pages_access_level: ProjectFeature::ENABLED }) end it 'updates the attribute' do expect { call_service } .to change { project.project_feature.pages_access_level } - .to(ProjectFeature::PRIVATE) + .to(ProjectFeature::ENABLED) end it 'calls Projects::UpdatePagesConfigurationService' do -- cgit v1.2.1 From 67b0c419be85639f246154c96f131a2c1b0622c5 Mon Sep 17 00:00:00 2001 From: Krasimir Angelov Date: Wed, 17 Jul 2019 20:08:58 +0000 Subject: Add tests for when deploy token usernames are not unique Ensure correct behaviour when deploy tokens have the same username or deploy token and user have the same username. --- spec/lib/gitlab/auth_spec.rb | 64 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index d9c73cff01e..0403830f700 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -297,6 +297,70 @@ describe Gitlab::Auth do let(:project) { create(:project) } let(:auth_failure) { Gitlab::Auth::Result.new(nil, nil) } + context 'when deploy token and user have the same username' do + let(:username) { 'normal_user' } + let(:user) { create(:user, username: username, password: 'my-secret') } + let(:deploy_token) { create(:deploy_token, username: username, read_registry: false, projects: [project]) } + + before do + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: username) + end + + it 'succeeds for the token' do + auth_success = Gitlab::Auth::Result.new(deploy_token, project, :deploy_token, [:download_code]) + + expect(gl_auth.find_for_git_client(username, deploy_token.token, project: project, ip: 'ip')) + .to eq(auth_success) + end + + it 'succeeds for the user' do + auth_success = Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities) + + expect(gl_auth.find_for_git_client(username, 'my-secret', project: project, ip: 'ip')) + .to eq(auth_success) + end + end + + context 'when deploy tokens have the same username' do + context 'and belong to the same project' do + let!(:read_registry) { create(:deploy_token, username: 'deployer', read_repository: false, projects: [project]) } + let!(:read_repository) { create(:deploy_token, username: read_registry.username, read_registry: false, projects: [project]) } + + it 'succeeds for the right token' do + auth_success = Gitlab::Auth::Result.new(read_repository, project, :deploy_token, [:download_code]) + + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: 'deployer') + expect(gl_auth.find_for_git_client('deployer', read_repository.token, project: project, ip: 'ip')) + .to eq(auth_success) + end + + it 'fails for the wrong token' do + expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: 'deployer') + expect(gl_auth.find_for_git_client('deployer', read_registry.token, project: project, ip: 'ip')) + .to eq(auth_failure) + end + end + + context 'and belong to different projects' do + let!(:read_registry) { create(:deploy_token, username: 'deployer', read_repository: false, projects: [create(:project)]) } + let!(:read_repository) { create(:deploy_token, username: read_registry.username, read_registry: false, projects: [project]) } + + it 'succeeds for the right token' do + auth_success = Gitlab::Auth::Result.new(read_repository, project, :deploy_token, [:download_code]) + + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: 'deployer') + expect(gl_auth.find_for_git_client('deployer', read_repository.token, project: project, ip: 'ip')) + .to eq(auth_success) + end + + it 'fails for the wrong token' do + expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: 'deployer') + expect(gl_auth.find_for_git_client('deployer', read_registry.token, project: project, ip: 'ip')) + .to eq(auth_failure) + end + end + end + context 'when the deploy token has read_repository as scope' do let(:deploy_token) { create(:deploy_token, read_registry: false, projects: [project]) } let(:login) { deploy_token.username } -- cgit v1.2.1 From 7e87990ecd8d1c1ac0a7a32661552a44c95b89d0 Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Wed, 17 Jul 2019 20:42:33 +0000 Subject: Move boards switcher partial (cherry picked from commit a82e4d57a6fbba840a8a944e372b80866a1e48cc) --- .../javascripts/boards/components/board_form.vue | 216 +++++++++++++ .../boards/components/boards_selector.vue | 334 +++++++++++++++++++++ app/assets/javascripts/boards/index.js | 2 +- .../boards/mount_multiple_boards_switcher.js | 37 ++- app/views/shared/boards/_switcher.html.haml | 16 + app/views/shared/issuable/_search_bar.html.haml | 2 +- .../unreleased/winh-multiple-issueboards-core.yml | 5 + locale/gitlab.pot | 18 ++ .../boards/components/board_form_spec.js | 56 ++++ .../boards/components/boards_selector_spec.js | 205 +++++++++++++ 10 files changed, 887 insertions(+), 4 deletions(-) create mode 100644 app/assets/javascripts/boards/components/board_form.vue create mode 100644 app/assets/javascripts/boards/components/boards_selector.vue create mode 100644 app/views/shared/boards/_switcher.html.haml create mode 100644 changelogs/unreleased/winh-multiple-issueboards-core.yml create mode 100644 spec/javascripts/boards/components/board_form_spec.js create mode 100644 spec/javascripts/boards/components/boards_selector_spec.js diff --git a/app/assets/javascripts/boards/components/board_form.vue b/app/assets/javascripts/boards/components/board_form.vue new file mode 100644 index 00000000000..6754abf4019 --- /dev/null +++ b/app/assets/javascripts/boards/components/board_form.vue @@ -0,0 +1,216 @@ + + + diff --git a/app/assets/javascripts/boards/components/boards_selector.vue b/app/assets/javascripts/boards/components/boards_selector.vue new file mode 100644 index 00000000000..b05de4538f2 --- /dev/null +++ b/app/assets/javascripts/boards/components/boards_selector.vue @@ -0,0 +1,334 @@ + + + diff --git a/app/assets/javascripts/boards/index.js b/app/assets/javascripts/boards/index.js index d6a5372b22d..f5a617a85ad 100644 --- a/app/assets/javascripts/boards/index.js +++ b/app/assets/javascripts/boards/index.js @@ -1,7 +1,6 @@ import $ from 'jquery'; import Vue from 'vue'; -import mountMultipleBoardsSwitcher from 'ee_else_ce/boards/mount_multiple_boards_switcher'; import Flash from '~/flash'; import { __ } from '~/locale'; import './models/label'; @@ -31,6 +30,7 @@ import { } from '~/lib/utils/common_utils'; import boardConfigToggle from 'ee_else_ce/boards/config_toggle'; import toggleFocusMode from 'ee_else_ce/boards/toggle_focus'; +import mountMultipleBoardsSwitcher from './mount_multiple_boards_switcher'; let issueBoardsApp; diff --git a/app/assets/javascripts/boards/mount_multiple_boards_switcher.js b/app/assets/javascripts/boards/mount_multiple_boards_switcher.js index bdb14a7f2f2..8d22f009784 100644 --- a/app/assets/javascripts/boards/mount_multiple_boards_switcher.js +++ b/app/assets/javascripts/boards/mount_multiple_boards_switcher.js @@ -1,2 +1,35 @@ -// this will be moved from EE to CE as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/53811 -export default () => {}; +import Vue from 'vue'; +import { parseBoolean } from '~/lib/utils/common_utils'; +import BoardsSelector from '~/boards/components/boards_selector.vue'; + +export default () => { + const boardsSwitcherElement = document.getElementById('js-multiple-boards-switcher'); + return new Vue({ + el: boardsSwitcherElement, + components: { + BoardsSelector, + }, + data() { + const { dataset } = boardsSwitcherElement; + + const boardsSelectorProps = { + ...dataset, + currentBoard: JSON.parse(dataset.currentBoard), + hasMissingBoards: parseBoolean(dataset.hasMissingBoards), + canAdminBoard: parseBoolean(dataset.canAdminBoard), + multipleIssueBoardsAvailable: parseBoolean(dataset.multipleIssueBoardsAvailable), + projectId: Number(dataset.projectId), + groupId: Number(dataset.groupId), + scopedIssueBoardFeatureEnabled: parseBoolean(dataset.scopedIssueBoardFeatureEnabled), + weights: JSON.parse(dataset.weights), + }; + + return { boardsSelectorProps }; + }, + render(createElement) { + return createElement(BoardsSelector, { + props: this.boardsSelectorProps, + }); + }, + }); +}; diff --git a/app/views/shared/boards/_switcher.html.haml b/app/views/shared/boards/_switcher.html.haml new file mode 100644 index 00000000000..79118630762 --- /dev/null +++ b/app/views/shared/boards/_switcher.html.haml @@ -0,0 +1,16 @@ +- parent = board.parent +- milestone_filter_opts = { format: :json } +- milestone_filter_opts = milestone_filter_opts.merge(only_group_milestones: true) if board.group_board? +- weights = Gitlab.ee? ? ([Issue::WEIGHT_ANY] + Issue.weight_options) : [] + +#js-multiple-boards-switcher.inline.boards-switcher{ data: { current_board: current_board_json.to_json, + milestone_path: milestones_filter_path(milestone_filter_opts), + board_base_url: board_base_url, + has_missing_boards: (!multiple_boards_available? && current_board_parent.boards.size > 1).to_s, + can_admin_board: can?(current_user, :admin_board, parent).to_s, + multiple_issue_boards_available: parent.multiple_issue_boards_available?.to_s, + labels_path: labels_filter_path_with_defaults(only_group_labels: true, include_descendant_groups: true), + project_id: @project&.id, + group_id: @group&.id, + scoped_issue_board_feature_enabled: Gitlab.ee? && parent.feature_available?(:scoped_issue_board) ? 'true' : 'false', + weights: weights.to_json } } diff --git a/app/views/shared/issuable/_search_bar.html.haml b/app/views/shared/issuable/_search_bar.html.haml index a97ac5e2a2d..e253413929a 100644 --- a/app/views/shared/issuable/_search_bar.html.haml +++ b/app/views/shared/issuable/_search_bar.html.haml @@ -6,7 +6,7 @@ .issues-filters{ class: ("w-100" if type == :boards_modal) } .issues-details-filters.filtered-search-block.d-flex.flex-column.flex-md-row{ class: block_css_class, "v-pre" => type == :boards_modal } - if type == :boards - = render_if_exists "shared/boards/switcher", board: board + = render "shared/boards/switcher", board: board = form_tag page_filter_path, method: :get, class: 'filter-form js-filter-form w-100' do - if params[:search].present? = hidden_field_tag :search, params[:search] diff --git a/changelogs/unreleased/winh-multiple-issueboards-core.yml b/changelogs/unreleased/winh-multiple-issueboards-core.yml new file mode 100644 index 00000000000..c45e420c133 --- /dev/null +++ b/changelogs/unreleased/winh-multiple-issueboards-core.yml @@ -0,0 +1,5 @@ +--- +title: Move multiple issue boards to core +merge_request: 30503 +author: +type: changed diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5e8d1ac206a..7b742660a4c 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5717,6 +5717,21 @@ msgstr "" msgid "IssueBoards|Boards" msgstr "" +msgid "IssueBoards|Create new board" +msgstr "" + +msgid "IssueBoards|Delete board" +msgstr "" + +msgid "IssueBoards|No matching boards found" +msgstr "" + +msgid "IssueBoards|Some of your boards are hidden, activate a license to see them again." +msgstr "" + +msgid "IssueBoards|Switch board" +msgstr "" + msgid "IssueTracker|Bugzilla issue tracker" msgstr "" @@ -8698,6 +8713,9 @@ msgstr "" msgid "Receive notifications about your own activity" msgstr "" +msgid "Recent" +msgstr "" + msgid "Recent Project Activity" msgstr "" diff --git a/spec/javascripts/boards/components/board_form_spec.js b/spec/javascripts/boards/components/board_form_spec.js new file mode 100644 index 00000000000..e9014156a98 --- /dev/null +++ b/spec/javascripts/boards/components/board_form_spec.js @@ -0,0 +1,56 @@ +import $ from 'jquery'; +import Vue from 'vue'; +import boardsStore from '~/boards/stores/boards_store'; +import boardForm from '~/boards/components/board_form.vue'; +import mountComponent from 'spec/helpers/vue_mount_component_helper'; + +describe('board_form.vue', () => { + const props = { + canAdminBoard: false, + labelsPath: `${gl.TEST_HOST}/labels/path`, + milestonePath: `${gl.TEST_HOST}/milestone/path`, + }; + let vm; + + beforeEach(() => { + spyOn($, 'ajax'); + boardsStore.state.currentPage = 'edit'; + const Component = Vue.extend(boardForm); + vm = mountComponent(Component, props); + }); + + afterEach(() => { + vm.$destroy(); + }); + + describe('methods', () => { + describe('cancel', () => { + it('resets currentPage', done => { + vm.cancel(); + + Vue.nextTick() + .then(() => { + expect(boardsStore.state.currentPage).toBe(''); + }) + .then(done) + .catch(done.fail); + }); + }); + }); + + describe('buttons', () => { + it('cancel button triggers cancel()', done => { + spyOn(vm, 'cancel'); + + Vue.nextTick() + .then(() => { + const cancelButton = vm.$el.querySelector('button[data-dismiss="modal"]'); + cancelButton.click(); + + expect(vm.cancel).toHaveBeenCalled(); + }) + .then(done) + .catch(done.fail); + }); + }); +}); diff --git a/spec/javascripts/boards/components/boards_selector_spec.js b/spec/javascripts/boards/components/boards_selector_spec.js new file mode 100644 index 00000000000..504bc51778c --- /dev/null +++ b/spec/javascripts/boards/components/boards_selector_spec.js @@ -0,0 +1,205 @@ +import Vue from 'vue'; +import BoardService from '~/boards/services/board_service'; +import BoardsSelector from '~/boards/components/boards_selector.vue'; +import mountComponent from 'spec/helpers/vue_mount_component_helper'; +import { TEST_HOST } from 'spec/test_constants'; +import boardsStore from '~/boards/stores/boards_store'; + +const throttleDuration = 1; + +function boardGenerator(n) { + return new Array(n).fill().map((board, id) => { + const name = `board${id}`; + + return { + id, + name, + }; + }); +} + +describe('BoardsSelector', () => { + let vm; + let allBoardsResponse; + let recentBoardsResponse; + let fillSearchBox; + const boards = boardGenerator(20); + const recentBoards = boardGenerator(5); + + beforeEach(done => { + setFixtures('
'); + window.gl = window.gl || {}; + + boardsStore.setEndpoints({ + boardsEndpoint: '', + recentBoardsEndpoint: '', + listsEndpoint: '', + bulkUpdatePath: '', + boardId: '', + }); + window.gl.boardService = new BoardService(); + + allBoardsResponse = Promise.resolve({ + data: boards, + }); + recentBoardsResponse = Promise.resolve({ + data: recentBoards, + }); + + spyOn(BoardService.prototype, 'allBoards').and.returnValue(allBoardsResponse); + spyOn(BoardService.prototype, 'recentBoards').and.returnValue(recentBoardsResponse); + + const Component = Vue.extend(BoardsSelector); + vm = mountComponent( + Component, + { + throttleDuration, + currentBoard: { + id: 1, + name: 'Development', + milestone_id: null, + weight: null, + assignee_id: null, + labels: [], + }, + milestonePath: `${TEST_HOST}/milestone/path`, + boardBaseUrl: `${TEST_HOST}/board/base/url`, + hasMissingBoards: false, + canAdminBoard: true, + multipleIssueBoardsAvailable: true, + labelsPath: `${TEST_HOST}/labels/path`, + projectId: 42, + groupId: 19, + scopedIssueBoardFeatureEnabled: true, + weights: [], + }, + document.querySelector('.js-boards-selector'), + ); + + vm.$el.querySelector('.js-dropdown-toggle').click(); + + Promise.all([allBoardsResponse, recentBoardsResponse]) + .then(() => vm.$nextTick()) + .then(done) + .catch(done.fail); + + fillSearchBox = filterTerm => { + const { searchBox } = vm.$refs; + const searchBoxInput = searchBox.$el.querySelector('input'); + searchBoxInput.value = filterTerm; + searchBoxInput.dispatchEvent(new Event('input')); + }; + }); + + afterEach(() => { + vm.$destroy(); + window.gl.boardService = undefined; + }); + + describe('filtering', () => { + it('shows all boards without filtering', done => { + vm.$nextTick() + .then(() => { + const dropdownItem = vm.$el.querySelectorAll('.js-dropdown-item'); + + expect(dropdownItem.length).toBe(boards.length + recentBoards.length); + }) + .then(done) + .catch(done.fail); + }); + + it('shows only matching boards when filtering', done => { + const filterTerm = 'board1'; + const expectedCount = boards.filter(board => board.name.includes(filterTerm)).length; + + fillSearchBox(filterTerm); + + vm.$nextTick() + .then(() => { + const dropdownItems = vm.$el.querySelectorAll('.js-dropdown-item'); + + expect(dropdownItems.length).toBe(expectedCount); + }) + .then(done) + .catch(done.fail); + }); + + it('shows message if there are no matching boards', done => { + fillSearchBox('does not exist'); + + vm.$nextTick() + .then(() => { + const dropdownItems = vm.$el.querySelectorAll('.js-dropdown-item'); + + expect(dropdownItems.length).toBe(0); + expect(vm.$el).toContainText('No matching boards found'); + }) + .then(done) + .catch(done.fail); + }); + }); + + describe('recent boards section', () => { + it('shows only when boards are greater than 10', done => { + vm.$nextTick() + .then(() => { + const headerEls = vm.$el.querySelectorAll('.dropdown-bold-header'); + + const expectedCount = 2; // Recent + All + + expect(expectedCount).toBe(headerEls.length); + }) + .then(done) + .catch(done.fail); + }); + + it('does not show when boards are less than 10', done => { + spyOn(vm, 'initScrollFade'); + spyOn(vm, 'setScrollFade'); + + vm.$nextTick() + .then(() => { + vm.boards = vm.boards.slice(0, 5); + }) + .then(vm.$nextTick) + .then(() => { + const headerEls = vm.$el.querySelectorAll('.dropdown-bold-header'); + const expectedCount = 0; + + expect(expectedCount).toBe(headerEls.length); + }) + .then(done) + .catch(done.fail); + }); + + it('does not show when recentBoards api returns empty array', done => { + vm.$nextTick() + .then(() => { + vm.recentBoards = []; + }) + .then(vm.$nextTick) + .then(() => { + const headerEls = vm.$el.querySelectorAll('.dropdown-bold-header'); + const expectedCount = 0; + + expect(expectedCount).toBe(headerEls.length); + }) + .then(done) + .catch(done.fail); + }); + + it('does not show when search is active', done => { + fillSearchBox('Random string'); + + vm.$nextTick() + .then(() => { + const headerEls = vm.$el.querySelectorAll('.dropdown-bold-header'); + const expectedCount = 0; + + expect(expectedCount).toBe(headerEls.length); + }) + .then(done) + .catch(done.fail); + }); + }); +}); -- cgit v1.2.1 From bcd2458076512ad80c6e470d9434618f27dfec3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Wed, 17 Jul 2019 23:45:35 +0000 Subject: Refactor RedisCounter and WebIdeCommitsCounter This MR refactor RedisCounter and WebIdeCommitsCounter to be reused by other components. --- lib/api/commits.rb | 2 +- lib/gitlab/usage_data.rb | 2 +- lib/gitlab/usage_data_counters/redis_counter.rb | 8 +--- .../usage_data_counters/web_ide_commits_counter.rb | 13 ------ lib/gitlab/usage_data_counters/web_ide_counter.rb | 21 +++++++++ .../usage_data_counters/redis_counter_spec.rb | 54 ---------------------- .../usage_data_counters/web_ide_counter_spec.rb | 21 +++++++++ spec/requests/api/commits_spec.rb | 2 +- 8 files changed, 47 insertions(+), 76 deletions(-) delete mode 100644 lib/gitlab/usage_data_counters/web_ide_commits_counter.rb create mode 100644 lib/gitlab/usage_data_counters/web_ide_counter.rb delete mode 100644 spec/lib/gitlab/usage_data_counters/redis_counter_spec.rb create mode 100644 spec/lib/gitlab/usage_data_counters/web_ide_counter_spec.rb diff --git a/lib/api/commits.rb b/lib/api/commits.rb index c414ad75d9d..fe910d46f6c 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -126,7 +126,7 @@ module API if result[:status] == :success commit_detail = user_project.repository.commit(result[:result]) - Gitlab::UsageDataCounters::WebIdeCommitsCounter.increment if find_user_from_warden + Gitlab::UsageDataCounters::WebIdeCounter.increment_commits_count if find_user_from_warden present commit_detail, with: Entities::CommitDetail, stats: params[:stats] else diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb index 055e01a9399..7572c0bdbfd 100644 --- a/lib/gitlab/usage_data.rb +++ b/lib/gitlab/usage_data.rb @@ -130,7 +130,7 @@ module Gitlab def usage_counters { - web_ide_commits: Gitlab::UsageDataCounters::WebIdeCommitsCounter.total_count + web_ide_commits: Gitlab::UsageDataCounters::WebIdeCounter.total_commits_count } end diff --git a/lib/gitlab/usage_data_counters/redis_counter.rb b/lib/gitlab/usage_data_counters/redis_counter.rb index 123b8e1bef1..d10871f704c 100644 --- a/lib/gitlab/usage_data_counters/redis_counter.rb +++ b/lib/gitlab/usage_data_counters/redis_counter.rb @@ -3,17 +3,13 @@ module Gitlab module UsageDataCounters module RedisCounter - def increment + def increment(redis_counter_key) Gitlab::Redis::SharedState.with { |redis| redis.incr(redis_counter_key) } end - def total_count + def total_count(redis_counter_key) Gitlab::Redis::SharedState.with { |redis| redis.get(redis_counter_key).to_i } end - - def redis_counter_key - raise NotImplementedError - end end end end diff --git a/lib/gitlab/usage_data_counters/web_ide_commits_counter.rb b/lib/gitlab/usage_data_counters/web_ide_commits_counter.rb deleted file mode 100644 index 62236fa07a3..00000000000 --- a/lib/gitlab/usage_data_counters/web_ide_commits_counter.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module UsageDataCounters - class WebIdeCommitsCounter - extend RedisCounter - - def self.redis_counter_key - 'WEB_IDE_COMMITS_COUNT' - end - end - end -end diff --git a/lib/gitlab/usage_data_counters/web_ide_counter.rb b/lib/gitlab/usage_data_counters/web_ide_counter.rb new file mode 100644 index 00000000000..6fbffb94c58 --- /dev/null +++ b/lib/gitlab/usage_data_counters/web_ide_counter.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Gitlab + module UsageDataCounters + class WebIdeCounter + extend RedisCounter + + COMMITS_COUNT_KEY = 'WEB_IDE_COMMITS_COUNT' + + class << self + def increment_commits_count + increment(COMMITS_COUNT_KEY) + end + + def total_commits_count + total_count(COMMITS_COUNT_KEY) + end + end + end + end +end diff --git a/spec/lib/gitlab/usage_data_counters/redis_counter_spec.rb b/spec/lib/gitlab/usage_data_counters/redis_counter_spec.rb deleted file mode 100644 index 38b4c22e186..00000000000 --- a/spec/lib/gitlab/usage_data_counters/redis_counter_spec.rb +++ /dev/null @@ -1,54 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::UsageDataCounters::RedisCounter, :clean_gitlab_redis_shared_state do - context 'when redis_key is not defined' do - subject do - Class.new.extend(described_class) - end - - describe '.increment' do - it 'raises a NotImplementedError exception' do - expect { subject.increment}.to raise_error(NotImplementedError) - end - end - - describe '.total_count' do - it 'raises a NotImplementedError exception' do - expect { subject.total_count}.to raise_error(NotImplementedError) - end - end - end - - context 'when redis_key is defined' do - subject do - counter_module = described_class - - Class.new do - extend counter_module - - def self.redis_counter_key - 'foo_redis_key' - end - end - end - - describe '.increment' do - it 'increments the web ide commits counter by 1' do - expect do - subject.increment - end.to change { subject.total_count }.from(0).to(1) - end - end - - describe '.total_count' do - it 'returns the total amount of web ide commits' do - subject.increment - subject.increment - - expect(subject.total_count).to eq(2) - end - end - end -end diff --git a/spec/lib/gitlab/usage_data_counters/web_ide_counter_spec.rb b/spec/lib/gitlab/usage_data_counters/web_ide_counter_spec.rb new file mode 100644 index 00000000000..fa0cf15e1b2 --- /dev/null +++ b/spec/lib/gitlab/usage_data_counters/web_ide_counter_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::UsageDataCounters::WebIdeCounter, :clean_gitlab_redis_shared_state do + describe '.increment_commits_count' do + it 'increments the web ide commits counter by 1' do + expect do + described_class.increment_commits_count + end.to change { described_class.total_commits_count }.by(1) + end + end + + describe '.total_commits_count' do + it 'returns the total amount of web ide commits' do + 2.times { described_class.increment_commits_count } + + expect(described_class.total_commits_count).to eq(2) + end + end +end diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 204e378f7be..45da5391bc8 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -281,7 +281,7 @@ describe API::Commits do end it 'does not increment the usage counters using access token authentication' do - expect(::Gitlab::UsageDataCounters::WebIdeCommitsCounter).not_to receive(:increment) + expect(::Gitlab::UsageDataCounters::WebIdeCounter).not_to receive(:increment_commits_count) post api(url, user), params: valid_c_params end -- cgit v1.2.1 From 746f54787799ee5ea8595a8730d363bfd250ffab Mon Sep 17 00:00:00 2001 From: Marcel Amirault Date: Thu, 18 Jul 2019 01:15:58 +0000 Subject: Fix unordered list spacing Correct the spacing of unordered markdown lists in docs, to maintain standards of documentation. --- doc/administration/repository_checks.md | 4 +- doc/development/code_review.md | 32 +++--- doc/development/database_debugging.md | 34 +++---- doc/development/diffs.md | 2 +- .../documentation/feature-change-workflow.md | 30 +++--- doc/development/fe_guide/icons.md | 16 +-- doc/development/file_storage.md | 4 +- doc/development/geo.md | 26 ++--- doc/development/git_object_deduplication.md | 110 ++++++++++----------- doc/development/new_fe_guide/style/javascript.md | 5 +- doc/development/testing_guide/review_apps.md | 8 +- doc/gitlab-basics/create-project.md | 32 +++--- doc/university/training/topics/getting_started.md | 19 ++-- 13 files changed, 161 insertions(+), 161 deletions(-) diff --git a/doc/administration/repository_checks.md b/doc/administration/repository_checks.md index 7cf8f20a9dc..05a7cb006a3 100644 --- a/doc/administration/repository_checks.md +++ b/doc/administration/repository_checks.md @@ -33,8 +33,8 @@ in `repocheck.log`: - in the [admin panel](logs.md#repochecklog) - or on disk, see: - - `/var/log/gitlab/gitlab-rails` for Omnibus installations - - `/home/git/gitlab/log` for installations from source + - `/var/log/gitlab/gitlab-rails` for Omnibus installations + - `/home/git/gitlab/log` for installations from source If for some reason the periodic repository check caused a lot of false alarms you can choose to clear *all* repository check states by diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 35ce6b8cbe4..e9c8b193309 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -45,9 +45,9 @@ page, with these behaviours: 1. It will not pick people whose [GitLab status](../user/profile/index.md#current-status) contains the string 'OOO'. -2. [Trainee maintainers](https://about.gitlab.com/handbook/engineering/workflow/code-review/#trainee-maintainer) +1. [Trainee maintainers](https://about.gitlab.com/handbook/engineering/workflow/code-review/#trainee-maintainer) are three times as likely to be picked as other reviewers. -3. It always picks the same reviewers and maintainers for the same +1. It always picks the same reviewers and maintainers for the same branch name (unless their OOO status changes, as in point 1). It removes leading `ce-` and `ee-`, and trailing `-ce` and `-ee`, so that it can be stable for backport branches. @@ -58,20 +58,20 @@ As described in the section on the responsibility of the maintainer below, you are recommended to get your merge request approved and merged by maintainer(s) from teams other than your own. - 1. If your merge request includes backend changes [^1], it must be - **approved by a [backend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_backend)**. - 1. If your merge request includes database migrations or changes to expensive queries [^2], it must be - **approved by a [database maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_database)**. - 1. If your merge request includes frontend changes [^1], it must be - **approved by a [frontend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_frontend)**. - 1. If your merge request includes UX changes [^1], it must be - **approved by a [UX team member][team]**. - 1. If your merge request includes adding a new JavaScript library [^1], it must be - **approved by a [frontend lead][team]**. - 1. If your merge request includes adding a new UI/UX paradigm [^1], it must be - **approved by a [UX lead][team]**. - 1. If your merge request includes a new dependency or a filesystem change, it must be - **approved by a [Distribution team member][team]**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) for more details. +1. If your merge request includes backend changes [^1], it must be + **approved by a [backend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_backend)**. +1. If your merge request includes database migrations or changes to expensive queries [^2], it must be + **approved by a [database maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_database)**. +1. If your merge request includes frontend changes [^1], it must be + **approved by a [frontend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_frontend)**. +1. If your merge request includes UX changes [^1], it must be + **approved by a [UX team member][team]**. +1. If your merge request includes adding a new JavaScript library [^1], it must be + **approved by a [frontend lead][team]**. +1. If your merge request includes adding a new UI/UX paradigm [^1], it must be + **approved by a [UX lead][team]**. +1. If your merge request includes a new dependency or a filesystem change, it must be + **approved by a [Distribution team member][team]**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) for more details. #### Security requirements diff --git a/doc/development/database_debugging.md b/doc/development/database_debugging.md index 0311eda1ff1..eb3b227473b 100644 --- a/doc/development/database_debugging.md +++ b/doc/development/database_debugging.md @@ -9,31 +9,31 @@ An easy first step is to search for your error in Slack or google "GitLab (my er Available `RAILS_ENV` - - `production` (generally not for your main GDK db, but you may need this for e.g. omnibus) - - `development` (this is your main GDK db) - - `test` (used for tests like rspec) +- `production` (generally not for your main GDK db, but you may need this for e.g. omnibus) +- `development` (this is your main GDK db) +- `test` (used for tests like rspec) ## Nuke everything and start over If you just want to delete everything and start over with an empty DB (~1 minute): - - `bundle exec rake db:reset RAILS_ENV=development` +- `bundle exec rake db:reset RAILS_ENV=development` If you just want to delete everything and start over with dummy data (~40 minutes). This also does `db:reset` and runs DB-specific migrations: - - `bundle exec rake dev:setup RAILS_ENV=development` +- `bundle exec rake dev:setup RAILS_ENV=development` If your test DB is giving you problems, it is safe to nuke it because it doesn't contain important data: - - `bundle exec rake db:reset RAILS_ENV=test` +- `bundle exec rake db:reset RAILS_ENV=test` ## Migration wrangling - - `bundle exec rake db:migrate RAILS_ENV=development`: Execute any pending migrations that you may have picked up from a MR - - `bundle exec rake db:migrate:status RAILS_ENV=development`: Check if all migrations are `up` or `down` - - `bundle exec rake db:migrate:down VERSION=20170926203418 RAILS_ENV=development`: Tear down a migration - - `bundle exec rake db:migrate:up VERSION=20170926203418 RAILS_ENV=development`: Set up a migration - - `bundle exec rake db:migrate:redo VERSION=20170926203418 RAILS_ENV=development`: Re-run a specific migration +- `bundle exec rake db:migrate RAILS_ENV=development`: Execute any pending migrations that you may have picked up from a MR +- `bundle exec rake db:migrate:status RAILS_ENV=development`: Check if all migrations are `up` or `down` +- `bundle exec rake db:migrate:down VERSION=20170926203418 RAILS_ENV=development`: Tear down a migration +- `bundle exec rake db:migrate:up VERSION=20170926203418 RAILS_ENV=development`: Set up a migration +- `bundle exec rake db:migrate:redo VERSION=20170926203418 RAILS_ENV=development`: Re-run a specific migration ## Manually access the database @@ -45,12 +45,12 @@ bundle exec rails dbconsole RAILS_ENV=development bundle exec rails db RAILS_ENV=development ``` - - `\q`: Quit/exit - - `\dt`: List all tables - - `\d+ issues`: List columns for `issues` table - - `CREATE TABLE board_labels();`: Create a table called `board_labels` - - `SELECT * FROM schema_migrations WHERE version = '20170926203418';`: Check if a migration was run - - `DELETE FROM schema_migrations WHERE version = '20170926203418';`: Manually remove a migration +- `\q`: Quit/exit +- `\dt`: List all tables +- `\d+ issues`: List columns for `issues` table +- `CREATE TABLE board_labels();`: Create a table called `board_labels` +- `SELECT * FROM schema_migrations WHERE version = '20170926203418';`: Check if a migration was run +- `DELETE FROM schema_migrations WHERE version = '20170926203418';`: Manually remove a migration ## FAQ diff --git a/doc/development/diffs.md b/doc/development/diffs.md index 5655398c886..ac0b8555360 100644 --- a/doc/development/diffs.md +++ b/doc/development/diffs.md @@ -133,4 +133,4 @@ File diff will be suppressed (technically different from collapsed, but behaves Diff Viewers, which can be found on `models/diff_viewer/*` are classes used to map metadata about each type of Diff File. It has information whether it's a binary, which partial should be used to render it or which File extensions this class accounts for. -`DiffViewer::Base` validates _blobs_ (old and new versions) content, extension and file type in order to check if it can be rendered. \ No newline at end of file +`DiffViewer::Base` validates _blobs_ (old and new versions) content, extension and file type in order to check if it can be rendered. diff --git a/doc/development/documentation/feature-change-workflow.md b/doc/development/documentation/feature-change-workflow.md index ca29353ecbe..ac93ada5a4b 100644 --- a/doc/development/documentation/feature-change-workflow.md +++ b/doc/development/documentation/feature-change-workflow.md @@ -121,27 +121,27 @@ All reviewers can help ensure accuracy, clarity, completeness, and adherence to - **Prior to merging**, documentation changes committed by the developer must be reviewed by: - 1. **The code reviewer** for the MR, to confirm accuracy, clarity, and completeness. - 1. Optionally: Others involved in the work, such as other devs or the PM. - 1. Optionally: The technical writer for the DevOps stage. If not prior to merging, the technical writer will review after the merge. - This helps us ensure that the developer has time to merge good content by the freeze, and that it can be further refined by the release, if needed. - - To decide whether to request this review before the merge, consider the amount of time left before the code freeze, the size of the change, - and your degree of confidence in having users of an RC use your docs as written. - - Pre-merge tech writer reviews should be most common when the code is complete well in advance of the freeze and/or for larger documentation changes. - - You can request a review and if there is not sufficient time to complete it prior to the freeze, - the maintainer can merge the current doc changes (if complete) and create a follow-up doc review issue. - - The technical writer can also help decide what docs to merge before the freeze and whether to work on further changes in a follow up MR. - - **To request a pre-merge technical writer review**, assign the writer listed for the applicable [DevOps stage](https://about.gitlab.com/handbook/product/categories/#devops-stages). - - **To request a post-merge technical writer review**, [create an issue for one using the Doc Review template](https://gitlab.com/gitlab-org/gitlab-ce/issues/new?issuable_template=Doc%20Review) and link it from the MR that makes the doc change. - 1. **The maintainer** who is assigned to merge the MR, to verify clarity, completeness, and quality, to the best of their ability. + 1. **The code reviewer** for the MR, to confirm accuracy, clarity, and completeness. + 1. Optionally: Others involved in the work, such as other devs or the PM. + 1. Optionally: The technical writer for the DevOps stage. If not prior to merging, the technical writer will review after the merge. + This helps us ensure that the developer has time to merge good content by the freeze, and that it can be further refined by the release, if needed. + - To decide whether to request this review before the merge, consider the amount of time left before the code freeze, the size of the change, + and your degree of confidence in having users of an RC use your docs as written. + - Pre-merge tech writer reviews should be most common when the code is complete well in advance of the freeze and/or for larger documentation changes. + - You can request a review and if there is not sufficient time to complete it prior to the freeze, + the maintainer can merge the current doc changes (if complete) and create a follow-up doc review issue. + - The technical writer can also help decide what docs to merge before the freeze and whether to work on further changes in a follow up MR. + - **To request a pre-merge technical writer review**, assign the writer listed for the applicable [DevOps stage](https://about.gitlab.com/handbook/product/categories/#devops-stages). + - **To request a post-merge technical writer review**, [create an issue for one using the Doc Review template](https://gitlab.com/gitlab-org/gitlab-ce/issues/new?issuable_template=Doc%20Review) and link it from the MR that makes the doc change. + 1. **The maintainer** who is assigned to merge the MR, to verify clarity, completeness, and quality, to the best of their ability. - Upon merging, if a technical writer review has not been performed and there is not yet a linked issue for a follow-up review, the maintainer should [create an issue using the Doc Review template](https://gitlab.com/gitlab-org/gitlab-ce/issues/new?issuable_template=Doc%20Review), link it from the MR, and mention the original MR author in the new issue. Alternatively, the maintainer can ask the MR author to create and link this issue before the MR is merged. - After merging, documentation changes are reviewed by: - 1. The technical writer--**if** their review was not performed prior to the merge. - 2. Optionally: by the PM (for accuracy and to ensure it's consistent with the vision for how the product will be used). + 1. The technical writer -- **if** their review was not performed prior to the merge. + 1. Optionally: by the PM (for accuracy and to ensure it's consistent with the vision for how the product will be used). Any party can raise the item to the PM for review at any point: the dev, the technical writer, or the PM, who can request/plan a review at the outset. ### Technical Writer role diff --git a/doc/development/fe_guide/icons.md b/doc/development/fe_guide/icons.md index 533e2001300..4f687d8642e 100644 --- a/doc/development/fe_guide/icons.md +++ b/doc/development/fe_guide/icons.md @@ -21,10 +21,10 @@ To use a sprite Icon in HAML or Rails we use a specific helper function : sprite_icon(icon_name, size: nil, css_class: '') ``` -- **icon_name** Use the icon_name that you can find in the SVG Sprite - ([Overview is available here][svg-preview]). -- **size (optional)** Use one of the following sizes : 16, 24, 32, 48, 72 (this will be translated into a `s16` class) -- **css_class (optional)** If you want to add additional css classes +- **icon_name** Use the icon_name that you can find in the SVG Sprite + ([Overview is available here][svg-preview]). +- **size (optional)** Use one of the following sizes : 16, 24, 32, 48, 72 (this will be translated into a `s16` class) +- **css_class (optional)** If you want to add additional css classes **Example** @@ -65,10 +65,10 @@ export default { ``` -- **name** Name of the Icon in the SVG Sprite ([Overview is available here][svg-preview]). -- **size (optional)** Number value for the size which is then mapped to a specific CSS class - (Available Sizes: 8, 12, 16, 18, 24, 32, 48, 72 are mapped to `sXX` css classes) -- **css-classes (optional)** Additional CSS Classes to add to the svg tag. +- **name** Name of the Icon in the SVG Sprite ([Overview is available here][svg-preview]). +- **size (optional)** Number value for the size which is then mapped to a specific CSS class + (Available Sizes: 8, 12, 16, 18, 24, 32, 48, 72 are mapped to `sXX` css classes) +- **css-classes (optional)** Additional CSS Classes to add to the svg tag. ### Usage in HTML/JS diff --git a/doc/development/file_storage.md b/doc/development/file_storage.md index 02874d18a30..475d1c1611e 100644 --- a/doc/development/file_storage.md +++ b/doc/development/file_storage.md @@ -92,8 +92,8 @@ in your uploader, you need to either 1) include `RecordsUpload::Concern` and pre The `CarrierWave::Uploader#store_dir` is overridden to - - `GitlabUploader.base_dir` + `GitlabUploader.dynamic_segment` when the store is LOCAL - - `GitlabUploader.dynamic_segment` when the store is REMOTE (the bucket name is used to namespace) +- `GitlabUploader.base_dir` + `GitlabUploader.dynamic_segment` when the store is LOCAL +- `GitlabUploader.dynamic_segment` when the store is REMOTE (the bucket name is used to namespace) ### Using `ObjectStorage::Extension::RecordsUploads` diff --git a/doc/development/geo.md b/doc/development/geo.md index 685d4e44ad3..24f16eae9fa 100644 --- a/doc/development/geo.md +++ b/doc/development/geo.md @@ -341,9 +341,9 @@ not used, so sessions etc. aren't shared between nodes. GitLab can optionally use Object Storage to store data it would otherwise store on disk. These things can be: - - LFS Objects - - CI Job Artifacts - - Uploads +- LFS Objects +- CI Job Artifacts +- Uploads Objects that are stored in object storage, are not handled by Geo. Geo ignores items in object storage. Either: @@ -412,15 +412,15 @@ The Geo **primary** stores events in the `geo_event_log` table. Each entry in the log contains a specific type of event. These type of events include: - - Repository Deleted event - - Repository Renamed event - - Repositories Changed event - - Repository Created event - - Hashed Storage Migrated event - - Lfs Object Deleted event - - Hashed Storage Attachments event - - Job Artifact Deleted event - - Upload Deleted event +- Repository Deleted event +- Repository Renamed event +- Repositories Changed event +- Repository Created event +- Hashed Storage Migrated event +- Lfs Object Deleted event +- Hashed Storage Attachments event +- Job Artifact Deleted event +- Upload Deleted event ### Geo Log Cursor @@ -526,4 +526,4 @@ old method: - Replication is synchronous and we preserve the order of events. - Replication of the events happen at the same time as the changes in the - database. + database. diff --git a/doc/development/git_object_deduplication.md b/doc/development/git_object_deduplication.md index c103a4527ff..5ce59891afa 100644 --- a/doc/development/git_object_deduplication.md +++ b/doc/development/git_object_deduplication.md @@ -79,11 +79,11 @@ at the Rails application level in SQL. In conclusion, we need three things for effective object deduplication across a collection of GitLab project repositories at the Git level: -1. A pool repository must exist. -2. The participating project repositories must be linked to the pool - repository via their respective `objects/info/alternates` files. -3. The pool repository must contain Git object data common to the - participating project repositories. +1. A pool repository must exist. +1. The participating project repositories must be linked to the pool + repository via their respective `objects/info/alternates` files. +1. The pool repository must contain Git object data common to the + participating project repositories. ### Deduplication factor @@ -105,71 +105,71 @@ With pool repositories we made a fresh start. These live in their own `pool_repositories` SQL table. The relations between these two tables are as follows: -- a `Project` belongs to at most one `PoolRepository` - (`project.pool_repository`) -- as an automatic consequence of the above, a `PoolRepository` has - many `Project`s -- a `PoolRepository` has exactly one "source `Project`" - (`pool.source_project`) +- a `Project` belongs to at most one `PoolRepository` + (`project.pool_repository`) +- as an automatic consequence of the above, a `PoolRepository` has + many `Project`s +- a `PoolRepository` has exactly one "source `Project`" + (`pool.source_project`) > TODO Fix invalid SQL data for pools created prior to GitLab 11.11 > . ### Assumptions -- All repositories in a pool must use [hashed - storage](../administration/repository_storage_types.md). This is so - that we don't have to ever worry about updating paths in - `object/info/alternates` files. -- All repositories in a pool must be on the same Gitaly storage shard. - The Git alternates mechanism relies on direct disk access across - multiple repositories, and we can only assume direct disk access to - be possible within a Gitaly storage shard. -- The only two ways to remove a member project from a pool are (1) to - delete the project or (2) to move the project to another Gitaly - storage shard. +- All repositories in a pool must use [hashed + storage](../administration/repository_storage_types.md). This is so + that we don't have to ever worry about updating paths in + `object/info/alternates` files. +- All repositories in a pool must be on the same Gitaly storage shard. + The Git alternates mechanism relies on direct disk access across + multiple repositories, and we can only assume direct disk access to + be possible within a Gitaly storage shard. +- The only two ways to remove a member project from a pool are (1) to + delete the project or (2) to move the project to another Gitaly + storage shard. ### Creating pools and pool memberships -- When a pool gets created, it must have a source project. The initial - contents of the pool repository are a Git clone of the source - project repository. -- The occasion for creating a pool is when an existing eligible - (public, hashed storage, non-forked) GitLab project gets forked and - this project does not belong to a pool repository yet. The fork - parent project becomes the source project of the new pool, and both - the fork parent and the fork child project become members of the new - pool. -- Once project A has become the source project of a pool, all future - eligible forks of A will become pool members. -- If the fork source is itself a fork, the resulting repository will - neither join the repository nor will a new pool repository be - seeded. - - eg: - - Suppose fork A is part of a pool repository, any forks created off - of fork A *will not* be a part of the pool repository that fork A is - a part of. - - Suppose B is a fork of A, and A does not belong to an object pool. - Now C gets created as a fork of B. C will not be part of a pool - repository. +- When a pool gets created, it must have a source project. The initial + contents of the pool repository are a Git clone of the source + project repository. +- The occasion for creating a pool is when an existing eligible + (public, hashed storage, non-forked) GitLab project gets forked and + this project does not belong to a pool repository yet. The fork + parent project becomes the source project of the new pool, and both + the fork parent and the fork child project become members of the new + pool. +- Once project A has become the source project of a pool, all future + eligible forks of A will become pool members. +- If the fork source is itself a fork, the resulting repository will + neither join the repository nor will a new pool repository be + seeded. + + eg: + + Suppose fork A is part of a pool repository, any forks created off + of fork A *will not* be a part of the pool repository that fork A is + a part of. + + Suppose B is a fork of A, and A does not belong to an object pool. + Now C gets created as a fork of B. C will not be part of a pool + repository. > TODO should forks of forks be deduplicated? > ### Consequences -- If a normal Project participating in a pool gets moved to another - Gitaly storage shard, its "belongs to PoolRepository" relation will - be broken. Because of the way moving repositories between shard is - implemented, we will automatically get a fresh self-contained copy - of the project's repository on the new storage shard. -- If the source project of a pool gets moved to another Gitaly storage - shard or is deleted the "source project" relation is not broken. - However, as of GitLab 12.0 a pool will not fetch from a source - unless the source is on the same Gitaly shard. +- If a normal Project participating in a pool gets moved to another + Gitaly storage shard, its "belongs to PoolRepository" relation will + be broken. Because of the way moving repositories between shard is + implemented, we will automatically get a fresh self-contained copy + of the project's repository on the new storage shard. +- If the source project of a pool gets moved to another Gitaly storage + shard or is deleted the "source project" relation is not broken. + However, as of GitLab 12.0 a pool will not fetch from a source + unless the source is on the same Gitaly shard. ## Consistency between the SQL pool relation and Gitaly diff --git a/doc/development/new_fe_guide/style/javascript.md b/doc/development/new_fe_guide/style/javascript.md index 3019eaa089c..802ebd12d92 100644 --- a/doc/development/new_fe_guide/style/javascript.md +++ b/doc/development/new_fe_guide/style/javascript.md @@ -71,7 +71,6 @@ class myClass { } const instance = new myClass(); instance.makeRequest(); - ``` ## Avoid classes to handle DOM events @@ -189,8 +188,8 @@ disabled due to legacy compatibility reasons but they are in the process of bein Do not disable specific ESLint rules. Due to technical debt, you may disable the following rules only if you are invoking/instantiating existing code modules. - - [no-new](http://eslint.org/docs/rules/no-new) - - [class-method-use-this](http://eslint.org/docs/rules/class-methods-use-this) +- [no-new](http://eslint.org/docs/rules/no-new) +- [class-method-use-this](http://eslint.org/docs/rules/class-methods-use-this) > Note: Disable these rules on a per line basis. This makes it easier to refactor in the future. E.g. use `eslint-disable-next-line` or `eslint-disable-line`. diff --git a/doc/development/testing_guide/review_apps.md b/doc/development/testing_guide/review_apps.md index ae40d628717..96761622cfe 100644 --- a/doc/development/testing_guide/review_apps.md +++ b/doc/development/testing_guide/review_apps.md @@ -137,8 +137,8 @@ secure note named **gitlab-{ce,ee} Review App's root password**. ### Run a Rails console -1. [Filter Workloads by your Review App slug](https://console.cloud.google.com/kubernetes/workload?project=gitlab-review-apps) - , e.g. `review-qa-raise-e-12chm0`. +1. [Filter Workloads by your Review App slug](https://console.cloud.google.com/kubernetes/workload?project=gitlab-review-apps), + e.g. `review-qa-raise-e-12chm0`. 1. Find and open the `task-runner` Deployment, e.g. `review-qa-raise-e-12chm0-task-runner`. 1. Click on the Pod in the "Managed pods" section, e.g. `review-qa-raise-e-12chm0-task-runner-d5455cc8-2lsvz`. 1. Click on the `KUBECTL` dropdown, then `Exec` -> `task-runner`. @@ -196,7 +196,7 @@ For the record, the debugging steps to find out this issue were: 1. `kubectl describe pod ` & confirm exact error message 1. Web search for exact error message, following rabbit hole to [a relevant kubernetes bug report](https://github.com/kubernetes/kubernetes/issues/57345) 1. Access the node over SSH via the GCP console (**Computer Engine > VM - instances** then click the "SSH" button for the node where the `dns-gitlab-review-app-external-dns` pod runs) + instances** then click the "SSH" button for the node where the `dns-gitlab-review-app-external-dns` pod runs) 1. In the node: `systemctl --version` => systemd 232 1. Gather some more information: - `mount | grep kube | wc -l` => e.g. 290 @@ -211,7 +211,7 @@ For the record, the debugging steps to find out this issue were: To resolve the problem, we needed to (forcibly) drain some nodes: 1. Try a normal drain on the node where the `dns-gitlab-review-app-external-dns` - pod runs so that Kubernetes automatically move it to another node: `kubectl drain NODE_NAME` + pod runs so that Kubernetes automatically move it to another node: `kubectl drain NODE_NAME` 1. If that doesn't work, you can also perform a forcible "drain" the node by removing all pods: `kubectl delete pods --field-selector=spec.nodeName=NODE_NAME` 1. In the node: - Perform `systemctl daemon-reload` to remove the dead/inactive units diff --git a/doc/gitlab-basics/create-project.md b/doc/gitlab-basics/create-project.md index ccba72f0ef8..2caf7dbbc7a 100644 --- a/doc/gitlab-basics/create-project.md +++ b/doc/gitlab-basics/create-project.md @@ -23,18 +23,18 @@ To create a project in GitLab: To create a new blank project on the **New project** page: 1. On the **Blank project** tab, provide the following information: - - The name of your project in the **Project name** field. You can't use - special characters, but you can use spaces, hyphens, underscores or even - emoji. - - The **Project description (optional)** field enables you to enter a - description for your project's dashboard, which will help others - understand what your project is about. Though it's not required, it's a good - idea to fill this in. - - Changing the **Visibility Level** modifies the project's - [viewing and access rights](../public_access/public_access.md) for users. - - Selecting the **Initialize repository with a README** option creates a - README file so that the Git repository is initialized, has a default branch, and - can be cloned. + - The name of your project in the **Project name** field. You can't use + special characters, but you can use spaces, hyphens, underscores or even + emoji. + - The **Project description (optional)** field enables you to enter a + description for your project's dashboard, which will help others + understand what your project is about. Though it's not required, it's a good + idea to fill this in. + - Changing the **Visibility Level** modifies the project's + [viewing and access rights](../public_access/public_access.md) for users. + - Selecting the **Initialize repository with a README** option creates a + README file so that the Git repository is initialized, has a default branch, and + can be cloned. 1. Click **Create project**. ## Project templates @@ -60,8 +60,8 @@ To use a built-in template on the **New project** page: 1. On the **Create from template** tab, select the **Built-in** tab. 1. From the list of available built-in templates, click the: - - **Preview** button to look at the template source itself. - - **Use template** button to start creating the project. + - **Preview** button to look at the template source itself. + - **Use template** button to start creating the project. 1. Finish creating the project by filling out the project's details. The process is the same as for using a [blank project](#blank-projects). @@ -83,8 +83,8 @@ To use a custom project template on the **New project** page: 1. On the **Create from template** tab, select the **Instance** tab or the **Group** tab. 1. From the list of available custom templates, click the: - - **Preview** button to look at the template source itself. - - **Use template** button to start creating the project. + - **Preview** button to look at the template source itself. + - **Use template** button to start creating the project. 1. Finish creating the project by filling out the project's details. The process is the same as for using a [blank project](#blank-projects). diff --git a/doc/university/training/topics/getting_started.md b/doc/university/training/topics/getting_started.md index 08027c5d15b..e8ff7916590 100644 --- a/doc/university/training/topics/getting_started.md +++ b/doc/university/training/topics/getting_started.md @@ -8,14 +8,15 @@ comments: false - Create a new repository by instantiating it through: - ```bash - git init - ``` + ```bash + git init + ``` + - Copy an existing project by cloning the repository through: - ```bash - git clone - ``` + ```bash + git clone + ``` ## Central Repos @@ -23,9 +24,9 @@ comments: false - Bare repositories don't allow file editing or committing changes. - Create a bare repo with: - ```bash - git init --bare project-name.git - ``` + ```bash + git init --bare project-name.git + ``` ## Instantiate workflow with clone -- cgit v1.2.1 From feb571a1ddf41826b339bf5dd1984561622998bb Mon Sep 17 00:00:00 2001 From: Imre Farkas Date: Thu, 18 Jul 2019 01:19:46 +0000 Subject: Doc for "Move external authorization service API management to EE" --- doc/api/projects.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/projects.md b/doc/api/projects.md index 781192fb92e..ba7e28c279b 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -799,7 +799,7 @@ POST /projects/user/:user_id | `auto_devops_deploy_strategy` | string | no | Auto Deploy strategy (`continuous`, `manual` or `timed_incremental`) | | `repository_storage` | string | no | Which storage shard the repository is on. Available only to admins | | `approvals_before_merge` | integer | no | **(STARTER)** How many approvers should approve merge requests by default | -| `external_authorization_classification_label` | string | no | **(CORE ONLY)** The classification label for the project | +| `external_authorization_classification_label` | string | no | **(PREMIUM)** The classification label for the project | | `mirror` | boolean | no | **(STARTER)** Enables pull mirroring in a project | | `mirror_trigger_builds` | boolean | no | **(STARTER)** Pull mirroring triggers builds | @@ -856,7 +856,7 @@ PUT /projects/:id | `auto_devops_deploy_strategy` | string | no | Auto Deploy strategy (`continuous`, `manual` or `timed_incremental`) | | `repository_storage` | string | no | Which storage shard the repository is on. Available only to admins | | `approvals_before_merge` | integer | no | **(STARTER)** How many approvers should approve merge request by default | -| `external_authorization_classification_label` | string | no | **(CORE ONLY)** The classification label for the project | +| `external_authorization_classification_label` | string | no | **(PREMIUM)** The classification label for the project | | `mirror` | boolean | no | **(STARTER)** Enables pull mirroring in a project | | `mirror_user_id` | integer | no | **(STARTER)** User responsible for all the activity surrounding a pull mirror event | | `mirror_trigger_builds` | boolean | no | **(STARTER)** Pull mirroring triggers builds | -- cgit v1.2.1 From 739f50c3b571d3566f9f21769357010f0989b19c Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Thu, 18 Jul 2019 01:46:46 +0000 Subject: Go guide: be more explicit on testing frameworks + diffing test results --- doc/development/go_guide/index.md | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/doc/development/go_guide/index.md b/doc/development/go_guide/index.md index f09339eb3a4..f827d240bf6 100644 --- a/doc/development/go_guide/index.md +++ b/doc/development/go_guide/index.md @@ -129,17 +129,50 @@ deploy a new pod, migrating the data automatically. ## Testing +### Testing frameworks + We should not use any specific library or framework for testing, as the [standard library](https://golang.org/pkg/) provides already everything to get -started. For example, some external dependencies might be worth considering in -case we decide to use a specific library or framework: +started. If there is a need for more sophisticated testing tools, the following +external dependencies might be worth considering in case we decide to use a specific +library or framework: - [Testify](https://github.com/stretchr/testify) - [httpexpect](https://github.com/gavv/httpexpect) +### Subtests + Use [subtests](https://blog.golang.org/subtests) whenever possible to improve code readability and test output. +### Better output in tests + +When comparing expected and actual values in tests, use +[testify/require.Equal](https://godoc.org/github.com/stretchr/testify/require#Equal), +[testify/require.EqualError](https://godoc.org/github.com/stretchr/testify/require#EqualError), +[testify/require.EqualValues](https://godoc.org/github.com/stretchr/testify/require#EqualValues), +and others to improve readability when comparing structs, errors, +large portions of text, or JSON documents: + +```go +type TestData struct { + // ... +} + +func FuncUnderTest() TestData { + // ... +} + +func Test(t *testing.T) { + t.Run("FuncUnderTest", func(t *testing.T) { + want := TestData{} + got := FuncUnderTest() + + require.Equal(t, want, got) // note that expected value comes first, then comes the actual one ("diff" semantics) + }) +} +``` + ### Benchmarks Programs handling a lot of IO or complex operations should always include -- cgit v1.2.1 From 97076881516edd0523841e679f8252928dedc167 Mon Sep 17 00:00:00 2001 From: Mark Lapierre Date: Thu, 18 Jul 2019 04:20:54 +0000 Subject: Unquarantine object storage test --compat mode in Minio is now enabled so this test should pass See https://gitlab.com/gitlab-org/gitlab-qa/merge_requests/260 --- qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb b/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb index 1eea3efec7f..dc04e7c22a9 100644 --- a/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb +++ b/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb @@ -13,8 +13,7 @@ module QA expect(page).to have_content(issue_title) end - # Failure issue: https://gitlab.com/gitlab-org/quality/nightly/issues/101 - context 'when using attachments in comments', :object_storage, :quarantine do + context 'when using attachments in comments', :object_storage do let(:file_to_attach) do File.absolute_path(File.join('spec', 'fixtures', 'banana_sample.gif')) end -- cgit v1.2.1 From 9599936cb2b62724668d3c708d346c7c44894442 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Thu, 18 Jul 2019 05:25:07 +0000 Subject: Refactor all existing usages of .act Replace any occurance of .act with the preferred method .perform --- qa/qa/page/main/login.rb | 6 +++--- .../browser_ui/1_manage/group/transfer_project_spec.rb | 2 +- .../specs/features/browser_ui/1_manage/login/log_in_spec.rb | 2 +- .../1_manage/login/log_into_gitlab_via_ldap_spec.rb | 2 +- .../1_manage/login/login_via_instance_wide_saml_sso_spec.rb | 4 ++-- .../browser_ui/1_manage/project/create_project_spec.rb | 2 +- .../browser_ui/1_manage/project/import_github_repo_spec.rb | 12 ++++++------ .../features/browser_ui/2_plan/issue/comment_issue_spec.rb | 2 +- .../features/browser_ui/2_plan/issue/create_issue_spec.rb | 4 ++-- .../browser_ui/2_plan/issue/filter_issue_comments_spec.rb | 2 +- .../browser_ui/3_create/repository/add_ssh_key_spec.rb | 6 +++--- .../repository/create_edit_delete_file_via_web_spec.rb | 4 ++-- .../3_create/repository/push_protected_branch_spec.rb | 2 +- .../browser_ui/4_verify/runner/register_runner_spec.rb | 2 +- .../6_release/deploy_token/add_deploy_token_spec.rb | 2 +- .../mattermost/create_group_with_mattermost_team_spec.rb | 4 ++-- 16 files changed, 29 insertions(+), 29 deletions(-) diff --git a/qa/qa/page/main/login.rb b/qa/qa/page/main/login.rb index 8970eeb6678..ffce3b72275 100644 --- a/qa/qa/page/main/login.rb +++ b/qa/qa/page/main/login.rb @@ -44,7 +44,7 @@ module QA def sign_in_using_credentials(user = nil) # Don't try to log-in if we're already logged-in - return if Page::Main::Menu.act { has_personal_area?(wait: 0) } + return if Page::Main::Menu.perform { |menu| menu.has_personal_area?(wait: 0) } using_wait_time 0 do set_initial_password_if_present @@ -58,7 +58,7 @@ module QA end end - Page::Main::Menu.act { has_personal_area? } + Page::Main::Menu.perform(&:has_personal_area?) end def sign_in_using_admin_credentials @@ -73,7 +73,7 @@ module QA sign_in_using_gitlab_credentials(admin) end - Page::Main::Menu.act { has_personal_area? } + Page::Main::Menu.perform(&:has_personal_area?) end def self.path diff --git a/qa/qa/specs/features/browser_ui/1_manage/group/transfer_project_spec.rb b/qa/qa/specs/features/browser_ui/1_manage/group/transfer_project_spec.rb index 2363836d5e3..c9acd7df776 100644 --- a/qa/qa/specs/features/browser_ui/1_manage/group/transfer_project_spec.rb +++ b/qa/qa/specs/features/browser_ui/1_manage/group/transfer_project_spec.rb @@ -5,7 +5,7 @@ module QA describe 'Project transfer between groups' do it 'user transfers a project between groups' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) source_group = Resource::Group.fabricate_via_api! do |group| group.path = 'source-group' diff --git a/qa/qa/specs/features/browser_ui/1_manage/login/log_in_spec.rb b/qa/qa/specs/features/browser_ui/1_manage/login/log_in_spec.rb index 09d1d3fe76e..6556c28ccab 100644 --- a/qa/qa/specs/features/browser_ui/1_manage/login/log_in_spec.rb +++ b/qa/qa/specs/features/browser_ui/1_manage/login/log_in_spec.rb @@ -5,7 +5,7 @@ module QA describe 'basic user login' do it 'user logs in using basic credentials and logs out' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) Page::Main::Menu.perform do |menu| expect(menu).to have_personal_area diff --git a/qa/qa/specs/features/browser_ui/1_manage/login/log_into_gitlab_via_ldap_spec.rb b/qa/qa/specs/features/browser_ui/1_manage/login/log_into_gitlab_via_ldap_spec.rb index 72dde4e5bd8..10cd8470a8f 100644 --- a/qa/qa/specs/features/browser_ui/1_manage/login/log_into_gitlab_via_ldap_spec.rb +++ b/qa/qa/specs/features/browser_ui/1_manage/login/log_into_gitlab_via_ldap_spec.rb @@ -5,7 +5,7 @@ module QA describe 'LDAP login' do it 'user logs into GitLab using LDAP credentials' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) Page::Main::Menu.perform do |menu| expect(menu).to have_personal_area diff --git a/qa/qa/specs/features/browser_ui/1_manage/login/login_via_instance_wide_saml_sso_spec.rb b/qa/qa/specs/features/browser_ui/1_manage/login/login_via_instance_wide_saml_sso_spec.rb index 87f0e9030d2..101143399f6 100644 --- a/qa/qa/specs/features/browser_ui/1_manage/login/login_via_instance_wide_saml_sso_spec.rb +++ b/qa/qa/specs/features/browser_ui/1_manage/login/login_via_instance_wide_saml_sso_spec.rb @@ -6,9 +6,9 @@ module QA it 'User logs in to gitlab with SAML SSO' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_with_saml } + Page::Main::Login.perform(&:sign_in_with_saml) - Vendor::SAMLIdp::Page::Login.act { login } + Vendor::SAMLIdp::Page::Login.perform(&:login) expect(page).to have_content('Welcome to GitLab') end diff --git a/qa/qa/specs/features/browser_ui/1_manage/project/create_project_spec.rb b/qa/qa/specs/features/browser_ui/1_manage/project/create_project_spec.rb index 6632c2977ef..fbe857dc2a5 100644 --- a/qa/qa/specs/features/browser_ui/1_manage/project/create_project_spec.rb +++ b/qa/qa/specs/features/browser_ui/1_manage/project/create_project_spec.rb @@ -5,7 +5,7 @@ module QA describe 'Project creation' do it 'user creates a new project' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) created_project = Resource::Project.fabricate_via_browser_ui! do |project| project.name = 'awesome-project' diff --git a/qa/qa/specs/features/browser_ui/1_manage/project/import_github_repo_spec.rb b/qa/qa/specs/features/browser_ui/1_manage/project/import_github_repo_spec.rb index a9eafd61a91..4f8c46cbd5f 100644 --- a/qa/qa/specs/features/browser_ui/1_manage/project/import_github_repo_spec.rb +++ b/qa/qa/specs/features/browser_ui/1_manage/project/import_github_repo_spec.rb @@ -24,16 +24,16 @@ module QA it 'user imports a GitHub repo' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) imported_project # import the project - Page::Main::Menu.act { go_to_projects } + Page::Main::Menu.perform(&:go_to_projects) Page::Dashboard::Projects.perform do |dashboard| dashboard.go_to_project(imported_project.name) end - Page::Project::Show.act { wait_for_import } + Page::Project::Show.perform(&:wait_for_import) verify_repository_import verify_issues_import @@ -50,7 +50,7 @@ module QA def verify_issues_import QA::Support::Retrier.retry_on_exception do - Page::Project::Menu.act { click_issues } + Page::Project::Menu.perform(&:click_issues) expect(page).to have_content('This is a sample issue') click_link 'This is a sample issue' @@ -73,7 +73,7 @@ module QA end def verify_merge_requests_import - Page::Project::Menu.act { click_merge_requests } + Page::Project::Menu.perform(&:click_merge_requests) expect(page).to have_content('Improve README.md') click_link 'Improve README.md' @@ -108,7 +108,7 @@ module QA end def verify_wiki_import - Page::Project::Menu.act { click_wiki } + Page::Project::Menu.perform(&:click_wiki) expect(page).to have_content('Welcome to the test-project wiki!') end diff --git a/qa/qa/specs/features/browser_ui/2_plan/issue/comment_issue_spec.rb b/qa/qa/specs/features/browser_ui/2_plan/issue/comment_issue_spec.rb index a62a51b11f4..2e71ecb5718 100644 --- a/qa/qa/specs/features/browser_ui/2_plan/issue/comment_issue_spec.rb +++ b/qa/qa/specs/features/browser_ui/2_plan/issue/comment_issue_spec.rb @@ -5,7 +5,7 @@ module QA describe 'Issue comments' do it 'user comments on an issue and edits the comment' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) issue = Resource::Issue.fabricate_via_api! do |issue| issue.title = 'issue title' diff --git a/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb b/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb index dc04e7c22a9..342f65593f6 100644 --- a/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb +++ b/qa/qa/specs/features/browser_ui/2_plan/issue/create_issue_spec.rb @@ -8,7 +8,7 @@ module QA it 'user creates an issue' do create_issue - Page::Project::Menu.act { click_issues } + Page::Project::Menu.perform(&:click_issues) expect(page).to have_content(issue_title) end @@ -39,7 +39,7 @@ module QA def create_issue Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) Resource::Issue.fabricate_via_browser_ui! do |issue| issue.title = issue_title diff --git a/qa/qa/specs/features/browser_ui/2_plan/issue/filter_issue_comments_spec.rb b/qa/qa/specs/features/browser_ui/2_plan/issue/filter_issue_comments_spec.rb index 301836f5ce8..af9514008f9 100644 --- a/qa/qa/specs/features/browser_ui/2_plan/issue/filter_issue_comments_spec.rb +++ b/qa/qa/specs/features/browser_ui/2_plan/issue/filter_issue_comments_spec.rb @@ -7,7 +7,7 @@ module QA it 'user filters comments and activities in an issue' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) issue = Resource::Issue.fabricate_via_api! do |issue| issue.title = issue_title diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/add_ssh_key_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/add_ssh_key_spec.rb index f41240b7605..56a7a04e840 100644 --- a/qa/qa/specs/features/browser_ui/3_create/repository/add_ssh_key_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/repository/add_ssh_key_spec.rb @@ -7,7 +7,7 @@ module QA it 'user adds and then removes an SSH key', :smoke do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) key = Resource::SSHKey.fabricate! do |resource| resource.title = key_title @@ -16,8 +16,8 @@ module QA expect(page).to have_content("Title: #{key_title}") expect(page).to have_content(key.fingerprint) - Page::Main::Menu.act { click_settings_link } - Page::Profile::Menu.act { click_ssh_keys } + Page::Main::Menu.perform(&:click_settings_link) + Page::Profile::Menu.perform(&:click_ssh_keys) Page::Profile::SSHKeys.perform do |ssh_keys| ssh_keys.remove_key(key_title) diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/create_edit_delete_file_via_web_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/create_edit_delete_file_via_web_spec.rb index d345fbfe995..51a1c19f0f7 100644 --- a/qa/qa/specs/features/browser_ui/3_create/repository/create_edit_delete_file_via_web_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/repository/create_edit_delete_file_via_web_spec.rb @@ -5,7 +5,7 @@ module QA describe 'Files management' do it 'user creates, edits and deletes a file via the Web' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) # Create file_name = 'QA Test - File name' @@ -27,7 +27,7 @@ module QA updated_file_content = 'QA Test - Updated file content' commit_message_for_update = 'QA Test - Update file' - Page::File::Show.act { click_edit } + Page::File::Show.perform(&:click_edit) Page::File::Form.act do remove_content diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/push_protected_branch_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/push_protected_branch_spec.rb index 6aebd04af03..e159e517cbb 100644 --- a/qa/qa/specs/features/browser_ui/3_create/repository/push_protected_branch_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/repository/push_protected_branch_spec.rb @@ -13,7 +13,7 @@ module QA before do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) end after do diff --git a/qa/qa/specs/features/browser_ui/4_verify/runner/register_runner_spec.rb b/qa/qa/specs/features/browser_ui/4_verify/runner/register_runner_spec.rb index 3af7db751e7..33744236dd4 100644 --- a/qa/qa/specs/features/browser_ui/4_verify/runner/register_runner_spec.rb +++ b/qa/qa/specs/features/browser_ui/4_verify/runner/register_runner_spec.rb @@ -11,7 +11,7 @@ module QA it 'user registers a new specific runner' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) Resource::Runner.fabricate! do |runner| runner.name = executor diff --git a/qa/qa/specs/features/browser_ui/6_release/deploy_token/add_deploy_token_spec.rb b/qa/qa/specs/features/browser_ui/6_release/deploy_token/add_deploy_token_spec.rb index 791dc62e32f..ec0c45652fd 100644 --- a/qa/qa/specs/features/browser_ui/6_release/deploy_token/add_deploy_token_spec.rb +++ b/qa/qa/specs/features/browser_ui/6_release/deploy_token/add_deploy_token_spec.rb @@ -5,7 +5,7 @@ module QA describe 'Deploy token creation' do it 'user adds a deploy token' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } + Page::Main::Login.perform(&:sign_in_using_credentials) deploy_token_name = 'deploy token name' one_week_from_now = Date.today + 7 diff --git a/qa/qa/specs/features/browser_ui/7_configure/mattermost/create_group_with_mattermost_team_spec.rb b/qa/qa/specs/features/browser_ui/7_configure/mattermost/create_group_with_mattermost_team_spec.rb index 8383dcdb983..94d20106de4 100644 --- a/qa/qa/specs/features/browser_ui/7_configure/mattermost/create_group_with_mattermost_team_spec.rb +++ b/qa/qa/specs/features/browser_ui/7_configure/mattermost/create_group_with_mattermost_team_spec.rb @@ -5,8 +5,8 @@ module QA describe 'Mattermost support' do it 'user creates a group with a mattermost team' do Runtime::Browser.visit(:gitlab, Page::Main::Login) - Page::Main::Login.act { sign_in_using_credentials } - Page::Main::Menu.act { go_to_groups } + Page::Main::Login.perform(&:sign_in_using_credentials) + Page::Main::Menu.perform(&:go_to_groups) Page::Dashboard::Groups.perform do |page| page.click_new_group -- cgit v1.2.1 From 7c91a466db66725ae1d1facee3954f9424deea27 Mon Sep 17 00:00:00 2001 From: Dietrich Stein Date: Thu, 18 Jul 2019 06:03:29 +0000 Subject: Perform more redactions in Redis performance bar traces HMSET and AUTH commands were not properly redacted. This commit does that and adds a test. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/64309 --- app/views/projects/pipelines/charts.html.haml | 7 ++----- app/views/projects/pipelines/charts/_overall.haml | 21 ++++++--------------- .../pipelines/charts/_pipeline_statistics.haml | 14 ++++++++++++++ app/views/projects/pipelines/charts/_pipelines.haml | 2 +- changelogs/unreleased/ds-charts-whitespace.yml | 5 +++++ 5 files changed, 28 insertions(+), 21 deletions(-) create mode 100644 app/views/projects/pipelines/charts/_pipeline_statistics.haml create mode 100644 changelogs/unreleased/ds-charts-whitespace.yml diff --git a/app/views/projects/pipelines/charts.html.haml b/app/views/projects/pipelines/charts.html.haml index 4d1d078661d..6b4110e07d2 100644 --- a/app/views/projects/pipelines/charts.html.haml +++ b/app/views/projects/pipelines/charts.html.haml @@ -2,12 +2,9 @@ - page_title _('CI / CD Charts') %div{ class: container_class } + #charts.ci-charts - .row - .col-md-6 - = render 'projects/pipelines/charts/overall' - .col-md-6 - = render 'projects/pipelines/charts/pipeline_times' + = render 'projects/pipelines/charts/overall' %hr = render 'projects/pipelines/charts/pipelines' diff --git a/app/views/projects/pipelines/charts/_overall.haml b/app/views/projects/pipelines/charts/_overall.haml index 66786c7ff59..651f9217455 100644 --- a/app/views/projects/pipelines/charts/_overall.haml +++ b/app/views/projects/pipelines/charts/_overall.haml @@ -1,15 +1,6 @@ -%h4= s_("PipelineCharts|Overall statistics") -%ul - %li - = s_("PipelineCharts|Total:") - %strong= n_("1 pipeline", "%d pipelines", @counts[:total]) % @counts[:total] - %li - = s_("PipelineCharts|Successful:") - %strong= n_("1 pipeline", "%d pipelines", @counts[:success]) % @counts[:success] - %li - = s_("PipelineCharts|Failed:") - %strong= n_("1 pipeline", "%d pipelines", @counts[:failed]) % @counts[:failed] - %li - = s_("PipelineCharts|Success ratio:") - %strong - #{success_ratio(@counts)}% +%h4.mt-4.mb-4= s_("PipelineCharts|Overall statistics") +.row + .col-md-6 + = render 'projects/pipelines/charts/pipeline_statistics' + .col-md-6 + = render 'projects/pipelines/charts/pipeline_times' diff --git a/app/views/projects/pipelines/charts/_pipeline_statistics.haml b/app/views/projects/pipelines/charts/_pipeline_statistics.haml new file mode 100644 index 00000000000..b323e290ed4 --- /dev/null +++ b/app/views/projects/pipelines/charts/_pipeline_statistics.haml @@ -0,0 +1,14 @@ +%ul + %li + = s_("PipelineCharts|Total:") + %strong= n_("1 pipeline", "%d pipelines", @counts[:total]) % @counts[:total] + %li + = s_("PipelineCharts|Successful:") + %strong= n_("1 pipeline", "%d pipelines", @counts[:success]) % @counts[:success] + %li + = s_("PipelineCharts|Failed:") + %strong= n_("1 pipeline", "%d pipelines", @counts[:failed]) % @counts[:failed] + %li + = s_("PipelineCharts|Success ratio:") + %strong + #{success_ratio(@counts)}% diff --git a/app/views/projects/pipelines/charts/_pipelines.haml b/app/views/projects/pipelines/charts/_pipelines.haml index 47f1f074210..afff9e82e45 100644 --- a/app/views/projects/pipelines/charts/_pipelines.haml +++ b/app/views/projects/pipelines/charts/_pipelines.haml @@ -1,4 +1,4 @@ -%h4= _("Pipelines charts") +%h4.mt-4.mb-4= _("Pipelines charts") %p   %span.legend-success diff --git a/changelogs/unreleased/ds-charts-whitespace.yml b/changelogs/unreleased/ds-charts-whitespace.yml new file mode 100644 index 00000000000..210261764a2 --- /dev/null +++ b/changelogs/unreleased/ds-charts-whitespace.yml @@ -0,0 +1,5 @@ +--- +title: Improves section header whitespace on the CI/CD Charts page +merge_request: 30531 +author: +type: fixed -- cgit v1.2.1 From 06eb3d091d6f7fdba6559dc323098a1882a4acb4 Mon Sep 17 00:00:00 2001 From: Clement Ho <408677-ClemMakesApps@users.noreply.gitlab.com> Date: Thu, 18 Jul 2019 06:04:12 +0000 Subject: Localize updated text on projects list page --- app/views/shared/projects/_project.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index 71bd9320593..6cdd5026d52 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/app/views/shared/projects/_project.html.haml @@ -89,4 +89,4 @@ %span.icon-wrapper.pipeline-status = render 'ci/status/icon', status: project.commit.last_pipeline.detailed_status(current_user), type: 'commit', tooltip_placement: 'top', path: pipeline_path .updated-note - %span Updated #{updated_tooltip} + %span _('Updated') #{updated_tooltip} -- cgit v1.2.1 From f8cecafb07792bcaf9d7ffa85766c3b33c1dd252 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Thu, 13 Jun 2019 12:44:41 +0200 Subject: Add start_sha to commits API When passing start_branch on committing from the WebIDE, it's possible that the branch has changed since editing started, which results in the change being applied on top of the latest commit in the branch and overwriting the new changes. By passing the start_sha instead we can make sure that the change is applied on top of the commit which the user started editing from. --- app/services/commits/create_service.rb | 18 +- app/services/files/multi_service.rb | 1 + .../add-support-for-start-sha-to-commits-api.yml | 5 + doc/api/commits.md | 9 +- lib/api/commits.rb | 14 +- lib/gitlab/git.rb | 5 + lib/gitlab/git/repository.rb | 4 +- lib/gitlab/gitaly_client/operation_service.rb | 9 +- spec/lib/gitlab/git_spec.rb | 20 ++ spec/requests/api/commits_spec.rb | 203 ++++++++++++++++----- spec/services/submodules/update_service_spec.rb | 2 +- spec/spec_helper.rb | 1 + spec/support/helpers/expect_request_with_status.rb | 11 ++ 13 files changed, 240 insertions(+), 62 deletions(-) create mode 100644 changelogs/unreleased/add-support-for-start-sha-to-commits-api.yml create mode 100644 spec/support/helpers/expect_request_with_status.rb diff --git a/app/services/commits/create_service.rb b/app/services/commits/create_service.rb index bb34a3d3352..f3be68f9602 100644 --- a/app/services/commits/create_service.rb +++ b/app/services/commits/create_service.rb @@ -10,6 +10,7 @@ module Commits @start_project = params[:start_project] || @project @start_branch = params[:start_branch] + @start_sha = params[:start_sha] @branch_name = params[:branch_name] @force = params[:force] || false end @@ -40,7 +41,7 @@ module Commits end def different_branch? - @start_branch != @branch_name || @start_project != @project + @start_project != @project || @start_branch != @branch_name || @start_sha.present? end def force? @@ -49,6 +50,7 @@ module Commits def validate! validate_permissions! + validate_start_sha! validate_on_branch! validate_branch_existence! @@ -63,7 +65,21 @@ module Commits end end + def validate_start_sha! + return unless @start_sha + + if @start_branch + raise_error("You can't pass both start_branch and start_sha") + elsif !Gitlab::Git.commit_id?(@start_sha) + raise_error("Invalid start_sha '#{@start_sha}'") + elsif !@start_project.repository.commit(@start_sha) + raise_error("Cannot find start_sha '#{@start_sha}'") + end + end + def validate_on_branch! + return unless @start_branch + if !@start_project.empty_repo? && !@start_project.repository.branch_exists?(@start_branch) raise_error('You can only create or edit files when you are on a branch') end diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb index d8c4e5bc5e8..65af4dd5a28 100644 --- a/app/services/files/multi_service.rb +++ b/app/services/files/multi_service.rb @@ -47,6 +47,7 @@ module Files author_name: @author_name, start_project: @start_project, start_branch_name: @start_branch, + start_sha: @start_sha, force: force? ) rescue ArgumentError => e diff --git a/changelogs/unreleased/add-support-for-start-sha-to-commits-api.yml b/changelogs/unreleased/add-support-for-start-sha-to-commits-api.yml new file mode 100644 index 00000000000..f810c2c5ada --- /dev/null +++ b/changelogs/unreleased/add-support-for-start-sha-to-commits-api.yml @@ -0,0 +1,5 @@ +--- +title: Add support for start_sha to commits API +merge_request: 29598 +author: +type: changed diff --git a/doc/api/commits.md b/doc/api/commits.md index 6eb4c47415f..1a835c0a872 100644 --- a/doc/api/commits.md +++ b/doc/api/commits.md @@ -72,15 +72,16 @@ POST /projects/:id/repository/commits | Attribute | Type | Required | Description | | --------- | ---- | -------- | ----------- | | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) | -| `branch` | string | yes | Name of the branch to commit into. To create a new branch, also provide `start_branch`. | +| `branch` | string | yes | Name of the branch to commit into. To create a new branch, also provide either `start_branch` or `start_sha`, and optionally `start_project`. | | `commit_message` | string | yes | Commit message | -| `start_branch` | string | no | Name of the branch to start the new commit from | -| `start_project` | integer/string | no | The project ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) to start the commit from. Defaults to the value of `id`. | +| `start_branch` | string | no | Name of the branch to start the new branch from | +| `start_sha` | string | no | SHA of the commit to start the new branch from | +| `start_project` | integer/string | no | The project ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) to start the new branch from. Defaults to the value of `id`. | | `actions[]` | array | yes | An array of action hashes to commit as a batch. See the next table for what attributes it can take. | | `author_email` | string | no | Specify the commit author's email address | | `author_name` | string | no | Specify the commit author's name | | `stats` | boolean | no | Include commit stats. Default is true | -| `force` | boolean | no | When `true` overwrites the target branch with a new commit based on the `start_branch` | +| `force` | boolean | no | When `true` overwrites the target branch with a new commit based on the `start_branch` or `start_sha` | | `actions[]` Attribute | Type | Required | Description | | --------------------- | ---- | -------- | ----------- | diff --git a/lib/api/commits.rb b/lib/api/commits.rb index c414ad75d9d..0aeb9584641 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -76,7 +76,7 @@ module API detail 'This feature was introduced in GitLab 8.13' end params do - requires :branch, type: String, desc: 'Name of the branch to commit into. To create a new branch, also provide `start_branch`.', allow_blank: false + requires :branch, type: String, desc: 'Name of the branch to commit into. To create a new branch, also provide either `start_branch` or `start_sha`, and optionally `start_project`.', allow_blank: false requires :commit_message, type: String, desc: 'Commit message' requires :actions, type: Array, desc: 'Actions to perform in commit' do requires :action, type: String, desc: 'The action to perform, `create`, `delete`, `move`, `update`, `chmod`', values: %w[create update move delete chmod].freeze @@ -98,12 +98,16 @@ module API requires :execute_filemode, type: Boolean, desc: 'When `true/false` enables/disables the execute flag on the file.' end end - optional :start_branch, type: String, desc: 'Name of the branch to start the new commit from' - optional :start_project, types: [Integer, String], desc: 'The ID or path of the project to start the commit from' + + optional :start_branch, type: String, desc: 'Name of the branch to start the new branch from' + optional :start_sha, type: String, desc: 'SHA of the commit to start the new branch from' + mutually_exclusive :start_branch, :start_sha + + optional :start_project, types: [Integer, String], desc: 'The ID or path of the project to start the new branch from' optional :author_email, type: String, desc: 'Author email for commit' optional :author_name, type: String, desc: 'Author name for commit' optional :stats, type: Boolean, default: true, desc: 'Include commit stats' - optional :force, type: Boolean, default: false, desc: 'When `true` overwrites the target branch with a new commit based on the `start_branch`' + optional :force, type: Boolean, default: false, desc: 'When `true` overwrites the target branch with a new commit based on the `start_branch` or `start_sha`' end post ':id/repository/commits' do if params[:start_project] @@ -118,7 +122,7 @@ module API attrs = declared_params attrs[:branch_name] = attrs.delete(:branch) - attrs[:start_branch] ||= attrs[:branch_name] + attrs[:start_branch] ||= attrs[:branch_name] unless attrs[:start_sha] attrs[:start_project] = start_project if start_project result = ::Files::MultiService.new(user_project, current_user, attrs).execute diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index 44a62586a23..df9f33baec2 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -9,6 +9,7 @@ module Gitlab # https://github.com/git/git/blob/3ad8b5bf26362ac67c9020bf8c30eee54a84f56d/cache.h#L1011-L1012 EMPTY_TREE_ID = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'.freeze BLANK_SHA = ('0' * 40).freeze + COMMIT_ID = /\A[0-9a-f]{40}\z/.freeze TAG_REF_PREFIX = "refs/tags/".freeze BRANCH_REF_PREFIX = "refs/heads/".freeze @@ -65,6 +66,10 @@ module Gitlab ref == BLANK_SHA end + def commit_id?(ref) + COMMIT_ID.match?(ref) + end + def version Gitlab::Git::Version.git_version end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index a7d9ba51277..6e8aa5d578e 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -873,13 +873,13 @@ module Gitlab def multi_action( user, branch_name:, message:, actions:, author_email: nil, author_name: nil, - start_branch_name: nil, start_repository: self, + start_branch_name: nil, start_sha: nil, start_repository: self, force: false) wrapped_gitaly_errors do gitaly_operation_client.user_commit_files(user, branch_name, message, actions, author_email, author_name, - start_branch_name, start_repository, force) + start_branch_name, start_repository, force, start_sha) end end # rubocop:enable Metrics/ParameterLists diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index 783c2ff0915..33ca428a942 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -325,11 +325,11 @@ module Gitlab # rubocop:disable Metrics/ParameterLists def user_commit_files( user, branch_name, commit_message, actions, author_email, author_name, - start_branch_name, start_repository, force = false) + start_branch_name, start_repository, force = false, start_sha = nil) req_enum = Enumerator.new do |y| header = user_commit_files_request_header(user, branch_name, commit_message, actions, author_email, author_name, - start_branch_name, start_repository, force) + start_branch_name, start_repository, force, start_sha) y.yield Gitaly::UserCommitFilesRequest.new(header: header) @@ -445,7 +445,7 @@ module Gitlab # rubocop:disable Metrics/ParameterLists def user_commit_files_request_header( user, branch_name, commit_message, actions, author_email, author_name, - start_branch_name, start_repository, force) + start_branch_name, start_repository, force, start_sha) Gitaly::UserCommitFilesRequestHeader.new( repository: @gitaly_repo, @@ -456,7 +456,8 @@ module Gitlab commit_author_email: encode_binary(author_email), start_branch_name: encode_binary(start_branch_name), start_repository: start_repository.gitaly_repository, - force: force + force: force, + start_sha: encode_binary(start_sha) ) end # rubocop:enable Metrics/ParameterLists diff --git a/spec/lib/gitlab/git_spec.rb b/spec/lib/gitlab/git_spec.rb index ce15057dd7d..6515be85ae3 100644 --- a/spec/lib/gitlab/git_spec.rb +++ b/spec/lib/gitlab/git_spec.rb @@ -39,6 +39,26 @@ describe Gitlab::Git do end end + describe '.commit_id?' do + using RSpec::Parameterized::TableSyntax + + where(:sha, :result) do + '' | false + 'foobar' | false + '4b825dc' | false + 'zzz25dc642cb6eb9a060e54bf8d69288fbee4904' | false + + '4b825dc642cb6eb9a060e54bf8d69288fbee4904' | true + Gitlab::Git::BLANK_SHA | true + end + + with_them do + it 'returns the expected result' do + expect(described_class.commit_id?(sha)).to eq(result) + end + end + end + describe '.shas_eql?' do using RSpec::Parameterized::TableSyntax diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 204e378f7be..311ef3ee99a 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -320,67 +320,132 @@ describe API::Commits do end end - context 'when the API user is a guest' do + context 'when committing to a new branch' do def last_commit_id(project, branch_name) project.repository.find_branch(branch_name)&.dereferenced_target&.id end - let(:public_project) { create(:project, :public, :repository) } - let!(:url) { "/projects/#{public_project.id}/repository/commits" } - let(:guest) { create(:user).tap { |u| public_project.add_guest(u) } } + before do + valid_c_params[:start_branch] = 'master' + valid_c_params[:branch] = 'patch' + end - it 'returns a 403' do - post api(url, guest), params: valid_c_params + context 'when the API user is a guest' do + let(:public_project) { create(:project, :public, :repository) } + let(:url) { "/projects/#{public_project.id}/repository/commits" } + let(:guest) { create(:user).tap { |u| public_project.add_guest(u) } } - expect(response).to have_gitlab_http_status(403) - end + it 'returns a 403' do + post api(url, guest), params: valid_c_params - context 'when start_project is provided' do - context 'when posting to a forked project the user owns' do - let!(:forked_project) { fork_project(public_project, guest, namespace: guest.namespace, repository: true) } - let!(:url) { "/projects/#{forked_project.id}/repository/commits" } + expect(response).to have_gitlab_http_status(403) + end - before do - valid_c_params[:start_branch] = "master" - valid_c_params[:branch] = "patch" - end + context 'when start_project is provided' do + context 'when posting to a forked project the user owns' do + let(:forked_project) { fork_project(public_project, guest, namespace: guest.namespace, repository: true) } + let(:url) { "/projects/#{forked_project.id}/repository/commits" } + + context 'identified by Integer (id)' do + before do + valid_c_params[:start_project] = public_project.id + end + + it 'adds a new commit to forked_project and returns a 201' do + expect_request_with_status(201) { post api(url, guest), params: valid_c_params } + .to change { last_commit_id(forked_project, valid_c_params[:branch]) } + .and not_change { last_commit_id(public_project, valid_c_params[:start_branch]) } + end + end - context 'identified by Integer (id)' do - before do - valid_c_params[:start_project] = public_project.id + context 'identified by String (full_path)' do + before do + valid_c_params[:start_project] = public_project.full_path + end + + it 'adds a new commit to forked_project and returns a 201' do + expect_request_with_status(201) { post api(url, guest), params: valid_c_params } + .to change { last_commit_id(forked_project, valid_c_params[:branch]) } + .and not_change { last_commit_id(public_project, valid_c_params[:start_branch]) } + end end - it 'adds a new commit to forked_project and returns a 201' do - expect { post api(url, guest), params: valid_c_params } - .to change { last_commit_id(forked_project, valid_c_params[:branch]) } - .and not_change { last_commit_id(public_project, valid_c_params[:start_branch]) } + context 'when branch already exists' do + before do + valid_c_params.delete(:start_branch) + valid_c_params[:branch] = 'master' + valid_c_params[:start_project] = public_project.id + end + + it 'returns a 400' do + post api(url, guest), params: valid_c_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']).to eq("A branch called 'master' already exists. Switch to that branch in order to make changes") + end + + context 'when force is set to true' do + before do + valid_c_params[:force] = true + end + + it 'adds a new commit to forked_project and returns a 201' do + expect_request_with_status(201) { post api(url, guest), params: valid_c_params } + .to change { last_commit_id(forked_project, valid_c_params[:branch]) } + .and not_change { last_commit_id(public_project, valid_c_params[:branch]) } + end + end + end + + context 'when start_sha is also provided' do + let(:forked_project) { fork_project(public_project, guest, namespace: guest.namespace, repository: false) } + let(:start_sha) { public_project.repository.commit.parent.sha } + + before do + # initialize an empty repository to force fetching from the original project + forked_project.repository.create_if_not_exists - expect(response).to have_gitlab_http_status(201) + valid_c_params[:start_project] = public_project.id + valid_c_params[:start_sha] = start_sha + valid_c_params.delete(:start_branch) + end + + it 'fetches the start_sha from the original project to use as parent commit and returns a 201' do + expect_request_with_status(201) { post api(url, guest), params: valid_c_params } + .to change { last_commit_id(forked_project, valid_c_params[:branch]) } + .and not_change { last_commit_id(forked_project, 'master') } + + last_commit = forked_project.repository.find_branch(valid_c_params[:branch]).dereferenced_target + expect(last_commit.parent_id).to eq(start_sha) + end end end - context 'identified by String (full_path)' do + context 'when the target project is not part of the fork network of start_project' do + let(:unrelated_project) { create(:project, :public, :repository, creator: guest) } + let(:url) { "/projects/#{unrelated_project.id}/repository/commits" } + before do - valid_c_params[:start_project] = public_project.full_path + valid_c_params[:start_branch] = 'master' + valid_c_params[:branch] = 'patch' + valid_c_params[:start_project] = public_project.id end - it 'adds a new commit to forked_project and returns a 201' do - expect { post api(url, guest), params: valid_c_params } - .to change { last_commit_id(forked_project, valid_c_params[:branch]) } - .and not_change { last_commit_id(public_project, valid_c_params[:start_branch]) } + it 'returns a 403' do + post api(url, guest), params: valid_c_params - expect(response).to have_gitlab_http_status(201) + expect(response).to have_gitlab_http_status(403) end end end - context 'when the target project is not part of the fork network of start_project' do - let(:unrelated_project) { create(:project, :public, :repository, creator: guest) } - let!(:url) { "/projects/#{unrelated_project.id}/repository/commits" } + context 'when posting to a forked project the user does not have write access' do + let(:forked_project) { fork_project(public_project, user, namespace: user.namespace, repository: true) } + let(:url) { "/projects/#{forked_project.id}/repository/commits" } before do - valid_c_params[:start_branch] = "master" - valid_c_params[:branch] = "patch" + valid_c_params[:start_branch] = 'master' + valid_c_params[:branch] = 'patch' valid_c_params[:start_project] = public_project.id end @@ -392,20 +457,68 @@ describe API::Commits do end end - context 'when posting to a forked project the user does not have write access' do - let!(:forked_project) { fork_project(public_project, user, namespace: user.namespace, repository: true) } - let!(:url) { "/projects/#{forked_project.id}/repository/commits" } + context 'when start_sha is provided' do + let(:start_sha) { project.repository.commit.parent.sha } before do - valid_c_params[:start_branch] = "master" - valid_c_params[:branch] = "patch" - valid_c_params[:start_project] = public_project.id + valid_c_params[:start_sha] = start_sha + valid_c_params.delete(:start_branch) end - it 'returns a 403' do - post api(url, guest), params: valid_c_params + it 'returns a 400 if start_branch is also provided' do + valid_c_params[:start_branch] = 'master' + post api(url, user), params: valid_c_params - expect(response).to have_gitlab_http_status(403) + expect(response).to have_gitlab_http_status(400) + expect(json_response['error']).to eq('start_branch, start_sha are mutually exclusive') + end + + it 'returns a 400 if branch already exists' do + valid_c_params[:branch] = 'master' + post api(url, user), params: valid_c_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']).to eq("A branch called 'master' already exists. Switch to that branch in order to make changes") + end + + it 'returns a 400 if start_sha does not exist' do + valid_c_params[:start_sha] = '1' * 40 + post api(url, user), params: valid_c_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']).to eq("Cannot find start_sha '#{valid_c_params[:start_sha]}'") + end + + it 'returns a 400 if start_sha is not a full SHA' do + valid_c_params[:start_sha] = start_sha.slice(0, 7) + post api(url, user), params: valid_c_params + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']).to eq("Invalid start_sha '#{valid_c_params[:start_sha]}'") + end + + it 'uses the start_sha as parent commit and returns a 201' do + expect_request_with_status(201) { post api(url, user), params: valid_c_params } + .to change { last_commit_id(project, valid_c_params[:branch]) } + .and not_change { last_commit_id(project, 'master') } + + last_commit = project.repository.find_branch(valid_c_params[:branch]).dereferenced_target + expect(last_commit.parent_id).to eq(start_sha) + end + + context 'when force is set to true and branch already exists' do + before do + valid_c_params[:force] = true + valid_c_params[:branch] = 'master' + end + + it 'uses the start_sha as parent commit and returns a 201' do + expect_request_with_status(201) { post api(url, user), params: valid_c_params } + .to change { last_commit_id(project, valid_c_params[:branch]) } + + last_commit = project.repository.find_branch(valid_c_params[:branch]).dereferenced_target + expect(last_commit.parent_id).to eq(start_sha) + end end end end diff --git a/spec/services/submodules/update_service_spec.rb b/spec/services/submodules/update_service_spec.rb index cf92350c1b2..47b31d4bcbf 100644 --- a/spec/services/submodules/update_service_spec.rb +++ b/spec/services/submodules/update_service_spec.rb @@ -142,7 +142,7 @@ describe Submodules::UpdateService do let(:branch_name) { nil } it_behaves_like 'returns error result' do - let(:error_message) { 'You can only create or edit files when you are on a branch' } + let(:error_message) { 'Invalid parameters' } end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 95e0d8858b9..fb9e3b295f8 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -104,6 +104,7 @@ RSpec.configure do |config| config.include Rails.application.routes.url_helpers, type: :routing config.include PolicyHelpers, type: :policy config.include MemoryUsageHelper + config.include ExpectRequestWithStatus, type: :request if ENV['CI'] # This includes the first try, i.e. tests will be run 4 times before failing. diff --git a/spec/support/helpers/expect_request_with_status.rb b/spec/support/helpers/expect_request_with_status.rb new file mode 100644 index 00000000000..0469a94e336 --- /dev/null +++ b/spec/support/helpers/expect_request_with_status.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module ExpectRequestWithStatus + def expect_request_with_status(status) + expect do + yield + + expect(response).to have_gitlab_http_status(status) + end + end +end -- cgit v1.2.1 From d4cc92db09a0c765556882943b7508075a0ab0e2 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Wed, 26 Jun 2019 09:07:56 -0500 Subject: FE remove create branch call in IDE commit Previously `start_sha` was intercepted on the frontend to create the correct branch in a separate API call. Now that the commits API supports the `start_sha` parameter directly this workaround is not needed anymore. --- app/assets/javascripts/ide/services/index.js | 8 +----- app/assets/javascripts/ide/stores/utils.js | 2 +- spec/frontend/ide/services/index_spec.js | 32 +++------------------- .../ide/stores/modules/commit/actions_spec.js | 2 +- 4 files changed, 7 insertions(+), 37 deletions(-) diff --git a/app/assets/javascripts/ide/services/index.js b/app/assets/javascripts/ide/services/index.js index 840761f68db..ba33b6826d6 100644 --- a/app/assets/javascripts/ide/services/index.js +++ b/app/assets/javascripts/ide/services/index.js @@ -56,13 +56,7 @@ export default { return Api.branchSingle(projectId, currentBranchId); }, commit(projectId, payload) { - // Currently the `commit` endpoint does not support `start_sha` so we - // have to make the request in the FE. This is not ideal and will be - // resolved soon. https://gitlab.com/gitlab-org/gitlab-ce/issues/59023 - const { branch, start_sha: ref } = payload; - const branchPromise = ref ? Api.createBranch(projectId, { ref, branch }) : Promise.resolve(); - - return branchPromise.then(() => Api.commitMultiple(projectId, payload)); + return Api.commitMultiple(projectId, payload); }, getFiles(projectUrl, branchId) { const url = `${projectUrl}/files/${branchId}`; diff --git a/app/assets/javascripts/ide/stores/utils.js b/app/assets/javascripts/ide/stores/utils.js index 01f78a29cf6..04470064c1f 100644 --- a/app/assets/javascripts/ide/stores/utils.js +++ b/app/assets/javascripts/ide/stores/utils.js @@ -155,7 +155,7 @@ export const createCommitPayload = ({ last_commit_id: newBranch || f.deleted || f.prevPath || f.replaces ? undefined : f.lastCommitSha, })), - start_sha: newBranch ? rootGetters.lastCommit.short_id : undefined, + start_sha: newBranch ? rootGetters.lastCommit.id : undefined, }); export const createNewMergeRequestUrl = (projectUrl, source, target) => diff --git a/spec/frontend/ide/services/index_spec.js b/spec/frontend/ide/services/index_spec.js index 499fa8fc012..3d5ed4b5c0c 100644 --- a/spec/frontend/ide/services/index_spec.js +++ b/spec/frontend/ide/services/index_spec.js @@ -16,40 +16,16 @@ describe('IDE services', () => { branch: TEST_BRANCH, commit_message: 'Hello world', actions: [], - start_sha: undefined, + start_sha: TEST_COMMIT_SHA, }; - Api.createBranch.mockReturnValue(Promise.resolve()); Api.commitMultiple.mockReturnValue(Promise.resolve()); }); - describe.each` - startSha | shouldCreateBranch - ${undefined} | ${false} - ${TEST_COMMIT_SHA} | ${true} - `('when start_sha is $startSha', ({ startSha, shouldCreateBranch }) => { - beforeEach(() => { - payload.start_sha = startSha; + it('should commit', () => { + services.commit(TEST_PROJECT_ID, payload); - return services.commit(TEST_PROJECT_ID, payload); - }); - - if (shouldCreateBranch) { - it('should create branch', () => { - expect(Api.createBranch).toHaveBeenCalledWith(TEST_PROJECT_ID, { - ref: TEST_COMMIT_SHA, - branch: TEST_BRANCH, - }); - }); - } else { - it('should not create branch', () => { - expect(Api.createBranch).not.toHaveBeenCalled(); - }); - } - - it('should commit', () => { - expect(Api.commitMultiple).toHaveBeenCalledWith(TEST_PROJECT_ID, payload); - }); + expect(Api.commitMultiple).toHaveBeenCalledWith(TEST_PROJECT_ID, payload); }); }); }); diff --git a/spec/javascripts/ide/stores/modules/commit/actions_spec.js b/spec/javascripts/ide/stores/modules/commit/actions_spec.js index 8a3c132972e..590af53b3f2 100644 --- a/spec/javascripts/ide/stores/modules/commit/actions_spec.js +++ b/spec/javascripts/ide/stores/modules/commit/actions_spec.js @@ -245,7 +245,7 @@ describe('IDE commit module actions', () => { master: { workingReference: '1', commit: { - short_id: TEST_COMMIT_SHA, + id: TEST_COMMIT_SHA, }, }, }, -- cgit v1.2.1 From 40fd2977c51f2f4f51d63ba790fb5fa2acfd866d Mon Sep 17 00:00:00 2001 From: Dennis Tang <750946-dennis@users.noreply.gitlab.com> Date: Thu, 18 Jul 2019 08:55:04 +0000 Subject: Ensure visibility icons in group/project listings are grey - Project listing icons now use the `text-secondary` class - Group listing icons now use the `text-secondary` class - Unnecessary CSS was removed from groups.scss as a result --- app/assets/javascripts/groups/components/group_item.vue | 2 +- app/assets/stylesheets/pages/groups.scss | 4 ---- app/views/shared/projects/_project.html.haml | 2 +- .../64700-fix-the-color-of-the-visibility-icon-on-project-lists.yml | 5 +++++ 4 files changed, 7 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/64700-fix-the-color-of-the-visibility-icon-on-project-lists.yml diff --git a/app/assets/javascripts/groups/components/group_item.vue b/app/assets/javascripts/groups/components/group_item.vue index 9909f437fc8..830385941d8 100644 --- a/app/assets/javascripts/groups/components/group_item.vue +++ b/app/assets/javascripts/groups/components/group_item.vue @@ -129,7 +129,7 @@ export default { {{ group.permission }} diff --git a/app/assets/stylesheets/pages/groups.scss b/app/assets/stylesheets/pages/groups.scss index cff2e274390..1502cf18440 100644 --- a/app/assets/stylesheets/pages/groups.scss +++ b/app/assets/stylesheets/pages/groups.scss @@ -412,10 +412,6 @@ table.pipeline-project-metrics tr td { font-size: $gl-font-size-large; } - .item-visibility { - color: $gl-text-color-secondary; - } - @include media-breakpoint-down(md) { .title { font-size: $gl-font-size; diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index 6cdd5026d52..04f72420fe1 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/app/views/shared/projects/_project.html.haml @@ -37,7 +37,7 @@ %span.project-name< = project.name - %span.metadata-info.visibility-icon.append-right-10.prepend-top-8.has-tooltip{ data: { container: 'body', placement: 'top' }, title: visibility_icon_description(project) } + %span.metadata-info.visibility-icon.append-right-10.prepend-top-8.text-secondary.has-tooltip{ data: { container: 'body', placement: 'top' }, title: visibility_icon_description(project) } = visibility_level_icon(project.visibility_level, fw: true) - if explore_projects_tab? && project.repository.license diff --git a/changelogs/unreleased/64700-fix-the-color-of-the-visibility-icon-on-project-lists.yml b/changelogs/unreleased/64700-fix-the-color-of-the-visibility-icon-on-project-lists.yml new file mode 100644 index 00000000000..0d2fbaf01ed --- /dev/null +++ b/changelogs/unreleased/64700-fix-the-color-of-the-visibility-icon-on-project-lists.yml @@ -0,0 +1,5 @@ +--- +title: Ensure visibility icons in group/project listings are grey +merge_request: 30858 +author: +type: fixed -- cgit v1.2.1 From 9af90e8162aaaff5af313a4c5b951c847bf00bf9 Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Thu, 18 Jul 2019 09:11:37 +0000 Subject: Set correct file mode for autocomplete images --- .../project/img/autocomplete_characters_example1_v12_0.png | Bin .../project/img/autocomplete_characters_example2_v12_0.png | Bin 2 files changed, 0 insertions(+), 0 deletions(-) mode change 100755 => 100644 doc/user/project/img/autocomplete_characters_example1_v12_0.png mode change 100755 => 100644 doc/user/project/img/autocomplete_characters_example2_v12_0.png diff --git a/doc/user/project/img/autocomplete_characters_example1_v12_0.png b/doc/user/project/img/autocomplete_characters_example1_v12_0.png old mode 100755 new mode 100644 diff --git a/doc/user/project/img/autocomplete_characters_example2_v12_0.png b/doc/user/project/img/autocomplete_characters_example2_v12_0.png old mode 100755 new mode 100644 -- cgit v1.2.1