diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-01-27 18:09:08 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-01-27 18:09:08 +0000 |
commit | 874d5e8019150bfb2428cac3089c1dac1d6f9ed9 (patch) | |
tree | 3a1d9b1828222fe16a5ca9071da34598c7280d50 | |
parent | 0cbb4a75699e1ab6a0cb704b551e672e09192377 (diff) | |
download | gitlab-ce-874d5e8019150bfb2428cac3089c1dac1d6f9ed9.tar.gz |
Add latest changes from gitlab-org/gitlab@master
38 files changed, 588 insertions, 356 deletions
diff --git a/.gitlab/ci/rails.gitlab-ci.yml b/.gitlab/ci/rails.gitlab-ci.yml index 6de8cdfd9b0..bc21a93f23a 100644 --- a/.gitlab/ci/rails.gitlab-ci.yml +++ b/.gitlab/ci/rails.gitlab-ci.yml @@ -376,6 +376,17 @@ db:rollback: - bundle exec rake db:migrate VERSION=20181228175414 - bundle exec rake db:migrate SKIP_SCHEMA_VERSION_CHECK=true +db:gitlabcom-database-testing: + extends: .rails:rules:db:gitlabcom-database-testing + stage: test + image: ruby:2.7-alpine + needs: [] + allow_failure: true + script: + - source scripts/utils.sh + - install_gitlab_gem + - ./scripts/trigger-build gitlab-com-database-testing + gitlab:setup: extends: .db-job-base variables: diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index 5e8cdf0daaf..58fa1101834 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -523,6 +523,13 @@ changes: *db-patterns - <<: *if-merge-request-title-run-all-rspec +.rails:rules:db:gitlabcom-database-testing: + rules: + - if: '$GITLABCOM_DATABASE_TESTING_TRIGGER_TOKEN == null' + when: never + - <<: *if-merge-request + changes: *db-patterns + .rails:rules:ee-and-foss-unit: rules: - changes: *backend-patterns diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 262de3bbaa1..a5a1ec362aa 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -c38393255d22bf3533ca8b8714f614411f10fc30 +86d933c0a993eabbff4c725da3cff067f371e641 @@ -477,7 +477,7 @@ gem 'flipper', '~> 0.17.1' gem 'flipper-active_record', '~> 0.17.1' gem 'flipper-active_support_cache_store', '~> 0.17.1' gem 'unleash', '~> 0.1.5' -gem 'gitlab-experiment', '~> 0.4.8' +gem 'gitlab-experiment', '~> 0.4.5' # Structured logging gem 'lograge', '~> 0.5' diff --git a/Gemfile.lock b/Gemfile.lock index 61dfa7937b3..5e52a732906 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -424,7 +424,7 @@ GEM github-markup (1.7.0) gitlab-chronic (0.10.5) numerizer (~> 0.2) - gitlab-experiment (0.4.8) + gitlab-experiment (0.4.5) activesupport (>= 3.0) scientist (~> 1.5, >= 1.5.0) gitlab-fog-azure-rm (1.0.0) @@ -1364,7 +1364,7 @@ DEPENDENCIES gitaly (~> 13.8.0.pre.rc3) github-markup (~> 1.7.0) gitlab-chronic (~> 0.10.5) - gitlab-experiment (~> 0.4.8) + gitlab-experiment (~> 0.4.5) gitlab-fog-azure-rm (~> 1.0) gitlab-labkit (= 0.14.0) gitlab-license (~> 1.0) diff --git a/app/assets/javascripts/jobs/components/artifacts_block.vue b/app/assets/javascripts/jobs/components/artifacts_block.vue index 2850a8e86fd..0f34926f689 100644 --- a/app/assets/javascripts/jobs/components/artifacts_block.vue +++ b/app/assets/javascripts/jobs/components/artifacts_block.vue @@ -1,13 +1,15 @@ <script> -import { GlIcon, GlLink } from '@gitlab/ui'; +import { GlButton, GlButtonGroup, GlIcon, GlLink } from '@gitlab/ui'; import TimeagoTooltip from '~/vue_shared/components/time_ago_tooltip.vue'; import timeagoMixin from '~/vue_shared/mixins/timeago'; export default { components: { - TimeagoTooltip, + GlButton, + GlButtonGroup, GlIcon, GlLink, + TimeagoTooltip, }, mixins: [timeagoMixin], props: { @@ -36,7 +38,7 @@ export default { </script> <template> <div class="block"> - <div class="title font-weight-bold">{{ s__('Job|Job artifacts') }}</div> + <div class="title gl-font-weight-bold">{{ s__('Job|Job artifacts') }}</div> <p v-if="isExpired || willExpire" class="build-detail-row" @@ -61,32 +63,29 @@ export default { ) }}</span> </p> - <div class="btn-group d-flex gl-mt-3" role="group"> - <gl-link + <gl-button-group class="gl-display-flex gl-mt-3"> + <gl-button v-if="artifact.keep_path" :href="artifact.keep_path" - class="btn btn-sm btn-default" data-method="post" data-testid="keep-artifacts" - >{{ s__('Job|Keep') }}</gl-link + >{{ s__('Job|Keep') }}</gl-button > - <gl-link + <gl-button v-if="artifact.download_path" :href="artifact.download_path" - class="btn btn-sm btn-default" - download rel="nofollow" data-testid="download-artifacts" - >{{ s__('Job|Download') }}</gl-link + download + >{{ s__('Job|Download') }}</gl-button > - <gl-link + <gl-button v-if="artifact.browse_path" :href="artifact.browse_path" - class="btn btn-sm btn-default" data-testid="browse-artifacts" data-qa-selector="browse_artifacts_button" - >{{ s__('Job|Browse') }}</gl-link + >{{ s__('Job|Browse') }}</gl-button > - </div> + </gl-button-group> </div> </template> diff --git a/app/assets/stylesheets/framework.scss b/app/assets/stylesheets/framework.scss index e40b95cdce6..7931f4deea0 100644 --- a/app/assets/stylesheets/framework.scss +++ b/app/assets/stylesheets/framework.scss @@ -15,7 +15,6 @@ @import 'framework/badges'; @import 'framework/calendar'; @import 'framework/callout'; -@import 'framework/carousel'; @import 'framework/common'; @import 'framework/dropdowns'; @import 'framework/files'; diff --git a/app/assets/stylesheets/framework/carousel.scss b/app/assets/stylesheets/framework/carousel.scss deleted file mode 100644 index d51a9f9c173..00000000000 --- a/app/assets/stylesheets/framework/carousel.scss +++ /dev/null @@ -1,202 +0,0 @@ -// Notes on the classes: -// -// 1. .carousel.pointer-event should ideally be pan-y (to allow for users to scroll vertically) -// even when their scroll action started on a carousel, but for compatibility (with Firefox) -// we're preventing all actions instead -// 2. The .carousel-item-left and .carousel-item-right is used to indicate where -// the active slide is heading. -// 3. .active.carousel-item is the current slide. -// 4. .active.carousel-item-left and .active.carousel-item-right is the current -// slide in its in-transition state. Only one of these occurs at a time. -// 5. .carousel-item-next.carousel-item-left and .carousel-item-prev.carousel-item-right -// is the upcoming slide in transition. - -.carousel { - position: relative; - - &.pointer-event { - touch-action: pan-y; - } -} - - -.carousel-inner { - position: relative; - width: 100%; - overflow: hidden; - @include clearfix(); -} - -.carousel-item { - position: relative; - display: none; - float: left; - width: 100%; - margin-right: -100%; - backface-visibility: hidden; - @include transition($carousel-transition); -} - -.carousel-item.active, -.carousel-item-next, -.carousel-item-prev { - display: block; -} - -.carousel-item-next:not(.carousel-item-left), -.active.carousel-item-right { - transform: translateX(100%); -} - -.carousel-item-prev:not(.carousel-item-right), -.active.carousel-item-left { - transform: translateX(-100%); -} - - -// -// Alternate transitions -// - -.carousel-fade { - .carousel-item { - opacity: 0; - transition-property: opacity; - transform: none; - } - - .carousel-item.active, - .carousel-item-next.carousel-item-left, - .carousel-item-prev.carousel-item-right { - z-index: 1; - opacity: 1; - } - - .active.carousel-item-left, - .active.carousel-item-right { - z-index: 0; - opacity: 0; - @include transition(0s $carousel-transition-duration opacity); - } -} - - -// -// Left/right controls for nav -// - -.carousel-control-prev, -.carousel-control-next { - position: absolute; - top: 0; - bottom: 0; - z-index: 1; - // Use flex for alignment (1-3) - display: flex; // 1. allow flex styles - align-items: center; // 2. vertically center contents - justify-content: center; // 3. horizontally center contents - width: $carousel-control-width; - color: $carousel-control-color; - text-align: center; - opacity: $carousel-control-opacity; - @include transition($carousel-control-transition); - - // Hover/focus state - @include hover-focus { - color: $carousel-control-color; - text-decoration: none; - outline: 0; - opacity: $carousel-control-hover-opacity; - } -} - -.carousel-control-prev { - left: 0; - @if $enable-gradients { - background: linear-gradient(90deg, rgba($black, 0.25), rgba($black, 0.001)); - } -} - -.carousel-control-next { - right: 0; - @if $enable-gradients { - background: linear-gradient(270deg, rgba($black, 0.25), rgba($black, 0.001)); - } -} - -// Icons for within -.carousel-control-prev-icon, -.carousel-control-next-icon { - display: inline-block; - width: $carousel-control-icon-width; - height: $carousel-control-icon-width; - background: no-repeat 50% / 100% 100%; -} - -.carousel-control-prev-icon { - background-image: $carousel-control-prev-icon-bg; -} - -.carousel-control-next-icon { - background-image: $carousel-control-next-icon-bg; -} - - -// Optional indicator pips -// -// Add an ordered list with the following class and add a list item for each -// slide your carousel holds. - -.carousel-indicators { - position: absolute; - right: 0; - bottom: 0; - left: 0; - z-index: 15; - display: flex; - justify-content: center; - padding-left: 0; // override <ol> default - // Use the .carousel-control's width as margin so we don't overlay those - margin-right: $carousel-control-width; - margin-left: $carousel-control-width; - list-style: none; - - li { - box-sizing: content-box; - flex: 0 1 auto; - width: $carousel-indicator-width; - height: $carousel-indicator-height; - margin-right: $carousel-indicator-spacer; - margin-left: $carousel-indicator-spacer; - text-indent: -999px; - cursor: pointer; - background-color: $carousel-indicator-active-bg; - background-clip: padding-box; - // Use transparent borders to increase the hit area by 10px on top and bottom. - border-top: $carousel-indicator-hit-area-height solid transparent; - border-bottom: $carousel-indicator-hit-area-height solid transparent; - opacity: 0.5; - @include transition($carousel-indicator-transition); - } - - .active { - opacity: 1; - } -} - - -// Optional captions -// -// - -.carousel-caption { - position: absolute; - right: (100% - $carousel-caption-width) / 2; - bottom: 20px; - left: (100% - $carousel-caption-width) / 2; - z-index: 10; - padding-top: 20px; - padding-bottom: 20px; - color: $carousel-caption-color; - text-align: center; -} diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index ee0d76b0301..7db172c55a1 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -872,6 +872,27 @@ $add-to-slack-well-max-width: 750px; $add-to-slack-logo-size: 100px; /* +Security & Compliance Carousel +*/ +$security-and-compliance-carousel-image-carousel-width: 1000px; +$security-and-compliance-carousel-image-discover-button-width: 45%; +$security-and-compliance-carousel-image-discover-buttons-max-width: 280px; +$security-and-compliance-carousel-image-discover-footer-max-width: 500px; +$security-and-compliance-carousel-image-discover-feedback-width: 30%; +$security-and-compliance-carousel-image-discover-text-carousel-max-width: 650px; +$security-and-compliance-carousel-image-discover-text-carousel-caption-height: 100%; +$security-and-compliance-carousel-image-discover-text-carousel-caption-max-width: 500px; +$security-and-compliance-carousel-control-icon-width: 10px; +$security-and-compliance-carousel-control-position: -5%; +$security-and-compliance-carousel-inner-width: 90%; +$security-and-compliance-carousel-indicators-bottom: -20px; +$security-and-compliance-carousel-indicators-bottom-lg: -15px; +$security-and-compliance-carousel-indicators-dimension: 6px; +$security-and-compliance-carousel-indicators-border-radius: 100%; +$security-and-compliance-carousel-prev-icon-background: url("data:image/svg+xml;charset=utf8,%3Csvg xmlns='http://www.w3.org/2000/svg' fill='%23666666' viewBox='0 0 8 8'%3E%3Cpath d='M5.25 0l-4 4 4 4 1.5-1.5-2.5-2.5 2.5-2.5-1.5-1.5z'/%3E%3C/svg%3E"); +$security-and-compliance-carousel-next-icon-background: url("data:image/svg+xml;charset=utf8,%3Csvg xmlns='http://www.w3.org/2000/svg' fill='%23666666' viewBox='0 0 8 8'%3E%3Cpath d='M2.75 0l-1.5 1.5 2.5 2.5-2.5 2.5 1.5 1.5 4-4-4-4z'/%3E%3C/svg%3E"); + +/* Popup */ $popup-triangle-size: 15px; diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index d3b3341e0b2..9418fda97e0 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -59,7 +59,8 @@ class Projects::IssuesController < Projects::ApplicationController around_action :allow_gitaly_ref_name_caching, only: [:discussions] before_action :run_null_hypothesis_experiment, - only: [:index, :new, :create] + only: [:index, :new, :create], + if: -> { Feature.enabled?(:gitlab_experiments) } respond_to :html diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 9180b3f6b62..98ef9d918ae 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_controller.rb @@ -122,10 +122,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic end end - if render_merge_ref_head_diff? - return CompareService.new(@project, @merge_request.merge_ref_head.sha) - .execute(@project, @merge_request.target_branch) - end + return @merge_request.merge_head_diff if render_merge_ref_head_diff? if @start_sha @merge_request_diff.compare_with(@start_sha) diff --git a/app/experiments/application_experiment.rb b/app/experiments/application_experiment.rb index 9a1a37a6b19..7a8851d11ce 100644 --- a/app/experiments/application_experiment.rb +++ b/app/experiments/application_experiment.rb @@ -1,20 +1,13 @@ # frozen_string_literal: true class ApplicationExperiment < Gitlab::Experiment - def enabled? - return false if Feature::Definition.get(name).nil? # there has to be a feature flag yaml file - return false unless Gitlab.dev_env_or_com? # we're in an environment that allows experiments - - Feature.get(name).state != :off # rubocop:disable Gitlab/AvoidFeatureGet - end - def publish(_result) track(:assignment) # track that we've assigned a variant for this context Gon.global.push({ experiment: { name => signature } }, true) # push to client end def track(action, **event_args) - return unless should_track? # no events for opted out actors or excluded subjects + return if excluded? # no events for opted out actors or excluded subjects Gitlab::Tracking.event(name, action.to_s, **event_args.merge( context: (event_args[:context] || []) << SnowplowTracker::SelfDescribingJson.new( diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 725a8edbcda..cae0f0a5c55 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -50,12 +50,15 @@ class MergeRequest < ApplicationRecord end end - has_many :merge_request_diffs + has_many :merge_request_diffs, + -> { regular }, inverse_of: :merge_request has_many :merge_request_context_commits, inverse_of: :merge_request has_many :merge_request_context_commit_diff_files, through: :merge_request_context_commits, source: :diff_files has_one :merge_request_diff, - -> { order('merge_request_diffs.id DESC') }, inverse_of: :merge_request + -> { regular.order('merge_request_diffs.id DESC') }, inverse_of: :merge_request + has_one :merge_head_diff, + -> { merge_head }, inverse_of: :merge_request, class_name: 'MergeRequestDiff' has_one :cleanup_schedule, inverse_of: :merge_request belongs_to :latest_merge_request_diff, class_name: 'MergeRequestDiff' @@ -476,13 +479,17 @@ class MergeRequest < ApplicationRecord # This is used after project import, to reset the IDs to the correct # values. It is not intended to be called without having already scoped the # relation. + # + # Only set `regular` merge request diffs as latest so `merge_head` diff + # won't be considered as `MergeRequest#merge_request_diff`. def self.set_latest_merge_request_diff_ids! - update = ' + update = " latest_merge_request_diff_id = ( SELECT MAX(id) FROM merge_request_diffs WHERE merge_requests.id = merge_request_diffs.merge_request_id - )'.squish + AND merge_request_diffs.diff_type = #{MergeRequestDiff.diff_types[:regular]} + )".squish self.each_batch do |batch| batch.update_all(update) @@ -921,7 +928,7 @@ class MergeRequest < ApplicationRecord def create_merge_request_diff fetch_ref! - # n+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/37435 + # n+1: https://gitlab.com/gitlab-org/gitlab/-/issues/19377 Gitlab::GitalyClient.allow_n_plus_1_calls do merge_request_diffs.create! reload_merge_request_diff @@ -995,7 +1002,7 @@ class MergeRequest < ApplicationRecord # rubocop: enable CodeReuse/ServiceClass def diffable_merge_ref? - open? && merge_ref_head.present? && (Feature.enabled?(:display_merge_conflicts_in_diff, project) || can_be_merged?) + open? && merge_head_diff.present? && (Feature.enabled?(:display_merge_conflicts_in_diff, project) || can_be_merged?) end # Returns boolean indicating the merge_status should be rechecked in order to diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index d23e66b9697..5500ee7f74a 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -32,6 +32,7 @@ class MergeRequestDiff < ApplicationRecord has_many :merge_request_diff_commits, -> { order(:merge_request_diff_id, :relative_order) } validates :base_commit_sha, :head_commit_sha, :start_commit_sha, sha: true + validates :merge_request_id, uniqueness: { scope: :diff_type }, if: :merge_head? state_machine :state, initial: :empty do event :clean do @@ -50,6 +51,11 @@ class MergeRequestDiff < ApplicationRecord state :overflow_diff_lines_limit end + enum diff_type: { + regular: 1, + merge_head: 2 + } + scope :with_files, -> { without_states(:without_files, :empty) } scope :viewable, -> { without_state(:empty) } scope :by_commit_sha, ->(sha) do @@ -72,6 +78,7 @@ class MergeRequestDiff < ApplicationRecord join_condition = merge_requests[:id].eq(mr_diffs[:merge_request_id]) .and(mr_diffs[:id].not_eq(merge_requests[:latest_merge_request_diff_id])) + .and(mr_diffs[:diff_type].eq(diff_types[:regular])) arel_join = mr_diffs.join(merge_requests).on(join_condition) joins(arel_join.join_sources) @@ -196,6 +203,10 @@ class MergeRequestDiff < ApplicationRecord end def set_as_latest_diff + # Don't set merge_head diff as latest so it won't get considered as the + # MergeRequest#merge_request_diff. + return if merge_head? + MergeRequest .where('id = ? AND COALESCE(latest_merge_request_diff_id, 0) < ?', self.merge_request_id, self.id) .update_all(latest_merge_request_diff_id: self.id) @@ -203,8 +214,16 @@ class MergeRequestDiff < ApplicationRecord def ensure_commit_shas self.start_commit_sha ||= merge_request.target_branch_sha - self.head_commit_sha ||= merge_request.source_branch_sha - self.base_commit_sha ||= find_base_sha + + if merge_head? && merge_request.merge_ref_head.present? + diff_refs = merge_request.merge_ref_head.diff_refs + + self.head_commit_sha ||= diff_refs.head_sha + self.base_commit_sha ||= diff_refs.base_sha + else + self.head_commit_sha ||= merge_request.source_branch_sha + self.base_commit_sha ||= find_base_sha + end end # Override head_commit_sha to keep compatibility with merge request diff diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb index 96a2322f6a0..9fecab85cc1 100644 --- a/app/services/merge_requests/mergeability_check_service.rb +++ b/app/services/merge_requests/mergeability_check_service.rb @@ -114,6 +114,7 @@ module MergeRequests merge_to_ref_success = merge_to_ref + reload_merge_head_diff update_diff_discussion_positions! if merge_to_ref_success if merge_to_ref_success && can_git_merge? @@ -123,6 +124,10 @@ module MergeRequests end end + def reload_merge_head_diff + MergeRequests::ReloadMergeHeadDiffService.new(merge_request).execute + end + def update_diff_discussion_positions! Discussions::CaptureDiffNotePositionsService.new(merge_request).execute end @@ -153,6 +158,7 @@ module MergeRequests def merge_to_ref params = { allow_conflicts: Feature.enabled?(:display_merge_conflicts_in_diff, project) } result = MergeRequests::MergeToRefService.new(project, merge_request.author, params).execute(merge_request) + result[:status] == :success end diff --git a/app/services/merge_requests/reload_merge_head_diff_service.rb b/app/services/merge_requests/reload_merge_head_diff_service.rb new file mode 100644 index 00000000000..66fcb5c022b --- /dev/null +++ b/app/services/merge_requests/reload_merge_head_diff_service.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module MergeRequests + class ReloadMergeHeadDiffService + include BaseServiceUtility + + def initialize(merge_request) + @merge_request = merge_request + end + + def execute + return error("default_merge_ref_for_diffs feature flag is disabled") unless enabled? + return error("Merge request has no merge ref head.") unless merge_request.merge_ref_head.present? + + error_msg = recreate_merge_head_diff + + return error(error_msg) if error_msg + + success + end + + private + + attr_reader :merge_request + + def enabled? + Feature.enabled?(:default_merge_ref_for_diffs, merge_request.project) + end + + def recreate_merge_head_diff + merge_request.merge_head_diff&.destroy! + + # n+1: https://gitlab.com/gitlab-org/gitlab/-/issues/19377 + Gitlab::GitalyClient.allow_n_plus_1_calls do + merge_request.create_merge_head_diff! + end + + # Reset the merge request so it won't load the merge head diff as the + # MergeRequest#merge_request_diff. + merge_request.reset + + nil + rescue StandardError => e + message = "Failed to recreate merge head diff: #{e.message}" + + Gitlab::AppLogger.error(message: message, merge_request_id: merge_request.id) + message + end + end +end diff --git a/changelogs/unreleased/292507-batch-merge-ref-diff.yml b/changelogs/unreleased/292507-batch-merge-ref-diff.yml new file mode 100644 index 00000000000..fb000c4f160 --- /dev/null +++ b/changelogs/unreleased/292507-batch-merge-ref-diff.yml @@ -0,0 +1,5 @@ +--- +title: Support batch loading of merge head diffs +merge_request: 51078 +author: +type: performance diff --git a/config/application.rb b/config/application.rb index a2823b88e98..d95ef44c497 100644 --- a/config/application.rb +++ b/config/application.rb @@ -184,6 +184,7 @@ module Gitlab config.assets.precompile << "page_bundles/build.css" config.assets.precompile << "page_bundles/ci_status.css" config.assets.precompile << "page_bundles/cycle_analytics.css" + config.assets.precompile << "page_bundles/security_discover.css" config.assets.precompile << "page_bundles/dev_ops_report.css" config.assets.precompile << "page_bundles/environments.css" config.assets.precompile << "page_bundles/epics.css" diff --git a/config/feature_flags/development/gitlab_experiments.yml b/config/feature_flags/development/gitlab_experiments.yml new file mode 100644 index 00000000000..51fa6aa4529 --- /dev/null +++ b/config/feature_flags/development/gitlab_experiments.yml @@ -0,0 +1,8 @@ +--- +name: gitlab_experiments +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45840 +rollout_issue_url: +milestone: '13.7' +type: development +group: group::adoption +default_enabled: false diff --git a/db/migrate/20210107105306_add_diff_type_to_merge_request_diffs.rb b/db/migrate/20210107105306_add_diff_type_to_merge_request_diffs.rb new file mode 100644 index 00000000000..0b9b5e93054 --- /dev/null +++ b/db/migrate/20210107105306_add_diff_type_to_merge_request_diffs.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class AddDiffTypeToMergeRequestDiffs < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + UNIQUE_INDEX_NAME = 'index_merge_request_diffs_on_unique_merge_request_id' + + def up + unless column_exists?(:merge_request_diffs, :diff_type) + with_lock_retries do + add_column :merge_request_diffs, :diff_type, :integer, null: false, limit: 2, default: 1 + end + end + + add_concurrent_index :merge_request_diffs, :merge_request_id, unique: true, where: 'diff_type = 2', name: UNIQUE_INDEX_NAME + end + + def down + remove_concurrent_index_by_name(:merge_request_diffs, UNIQUE_INDEX_NAME) + + if column_exists?(:merge_request_diffs, :diff_type) + with_lock_retries do + remove_column :merge_request_diffs, :diff_type + end + end + end +end diff --git a/db/schema_migrations/20210107105306 b/db/schema_migrations/20210107105306 new file mode 100644 index 00000000000..fe66a041837 --- /dev/null +++ b/db/schema_migrations/20210107105306 @@ -0,0 +1 @@ +f76ce27a82f4773dcda324d79cc93a044f21317dbb9fdff035879502b5752da3
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index bae7dc05122..1a451a2430c 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -14011,6 +14011,7 @@ CREATE TABLE merge_request_diffs ( stored_externally boolean, files_count smallint, sorted boolean DEFAULT false NOT NULL, + diff_type smallint DEFAULT 1 NOT NULL, CONSTRAINT check_93ee616ac9 CHECK ((external_diff_store IS NOT NULL)) ); @@ -22221,6 +22222,8 @@ CREATE INDEX index_merge_request_diffs_on_external_diff_store ON merge_request_d CREATE INDEX index_merge_request_diffs_on_merge_request_id_and_id ON merge_request_diffs USING btree (merge_request_id, id); +CREATE UNIQUE INDEX index_merge_request_diffs_on_unique_merge_request_id ON merge_request_diffs USING btree (merge_request_id) WHERE (diff_type = 2); + CREATE INDEX index_merge_request_metrics_on_first_deployed_to_production_at ON merge_request_metrics USING btree (first_deployed_to_production_at); CREATE INDEX index_merge_request_metrics_on_latest_closed_at ON merge_request_metrics USING btree (latest_closed_at) WHERE (latest_closed_at IS NOT NULL); diff --git a/doc/administration/pages/index.md b/doc/administration/pages/index.md index 37dc03a297d..de9e083f39c 100644 --- a/doc/administration/pages/index.md +++ b/doc/administration/pages/index.md @@ -928,6 +928,20 @@ For example, if there is a connection timeout: error="failed to connect to internal Pages API: Get \"https://gitlab.example.com:3000/api/v4/internal/pages/status\": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)" ``` +### Pages cannot communicate with an instance of the GitLab API + +If you use the default value for `domain_config_source=auto` and run multiple instances of GitLab +Pages, you may see intermittent 502 error responses while serving Pages content. You may also see +the following warning in the Pages logs: + +```plaintext +WARN[0010] Pages cannot communicate with an instance of the GitLab API. Please sync your gitlab-secrets.json file https://gitlab.com/gitlab-org/gitlab-pages/-/issues/535#workaround. error="pages endpoint unauthorized" +``` + +This can happen if your `gitlab-secrets.json` file is out of date between GitLab Rails and GitLab +Pages. Follow steps 8-10 of [Running GitLab Pages on a separate server](#running-gitlab-pages-on-a-separate-server), +in all of your GitLab Pages instances. + ### 500 error with `securecookie: failed to generate random iv` and `Failed to save the session` This problem most likely results from an [out-dated operating system](https://docs.gitlab.com/omnibus/package-information/deprecated_os.html). diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 88010cebc97..870fbe08edf 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17550,6 +17550,9 @@ msgstr "" msgid "May" msgstr "" +msgid "Mean time to merge" +msgstr "" + msgid "Measured in bytes of code. Excludes generated and vendored code." msgstr "" @@ -33593,6 +33596,9 @@ msgid_plural "days" msgstr[0] "" msgstr[1] "" +msgid "days" +msgstr "" + msgid "default branch" msgstr "" diff --git a/qa/qa/specs/features/browser_ui/5_package/nuget_repository_spec.rb b/qa/qa/specs/features/browser_ui/5_package/nuget_repository_spec.rb index be806fcbb3e..e3bd1db297d 100644 --- a/qa/qa/specs/features/browser_ui/5_package/nuget_repository_spec.rb +++ b/qa/qa/specs/features/browser_ui/5_package/nuget_repository_spec.rb @@ -13,6 +13,13 @@ module QA end end + let(:another_project) do + Resource::Project.fabricate_via_api! do |project| + project.name = 'nuget-package-install-project' + project.template_name = 'dotnetcore' + end + end + let!(:runner) do Resource::Runner.fabricate! do |runner| runner.name = "qa-runner-#{Time.now.to_i}" @@ -22,11 +29,21 @@ module QA end end + let!(:another_runner) do + Resource::Runner.fabricate! do |runner| + runner.name = "qa-runner-#{Time.now.to_i}" + runner.tags = ["runner-for-#{another_project.name}"] + runner.executor = :docker + runner.project = another_project + end + end + after do runner.remove_via_api! + another_runner.remove_via_api! end - it 'publishes a nuget package and deletes it', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/1073' do + it 'publishes a nuget package at the project level, installs and deletes it at the group level', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/1073' do Flow::Login.sign_in Resource::Repository::Commit.fabricate_via_api! do |commit| @@ -37,23 +54,23 @@ module QA { file_path: '.gitlab-ci.yml', content: <<~YAML - image: mcr.microsoft.com/dotnet/core/sdk:3.1 - - stages: - - deploy - - deploy: - stage: deploy - script: - - dotnet restore -p:Configuration=Release - - dotnet build -c Release - - dotnet pack -c Release - - dotnet nuget add source "$CI_SERVER_URL/api/v4/projects/$CI_PROJECT_ID/packages/nuget/index.json" --name gitlab --username gitlab-ci-token --password $CI_JOB_TOKEN --store-password-in-clear-text - - dotnet nuget push "bin/Release/*.nupkg" --source gitlab - only: - - "#{project.default_branch}" - tags: - - "runner-for-#{project.name}" + image: mcr.microsoft.com/dotnet/core/sdk:3.1 + + stages: + - deploy + + deploy: + stage: deploy + script: + - dotnet restore -p:Configuration=Release + - dotnet build -c Release + - dotnet pack -c Release + - dotnet nuget add source "$CI_SERVER_URL/api/v4/projects/$CI_PROJECT_ID/packages/nuget/index.json" --name gitlab --username gitlab-ci-token --password $CI_JOB_TOKEN --store-password-in-clear-text + - dotnet nuget push "bin/Release/*.nupkg" --source gitlab + only: + - "#{project.default_branch}" + tags: + - "runner-for-#{project.name}" YAML } ] @@ -71,7 +88,67 @@ module QA expect(job).to be_successful(timeout: 800) end - Page::Project::Menu.perform(&:click_packages_link) + another_project.visit! + + Resource::Repository::Commit.fabricate_via_api! do |commit| + commit.project = another_project + commit.commit_message = 'Add new csproj file' + commit.add_files( + [ + { + file_path: 'otherdotnet.csproj', + content: <<~EOF + <Project Sdk="Microsoft.NET.Sdk"> + + <PropertyGroup> + <OutputType>Exe</OutputType> + <TargetFramework>netcoreapp3.1</TargetFramework> + </PropertyGroup> + + </Project> + EOF + } + ] + ) + commit.update_files( + [ + { + file_path: '.gitlab-ci.yml', + content: <<~YAML + image: mcr.microsoft.com/dotnet/core/sdk:3.1 + + stages: + - install + + install: + stage: install + script: + - dotnet nuget locals all --clear + - dotnet nuget add source "$CI_SERVER_URL/api/v4/groups/#{another_project.group.id}/-/packages/nuget/index.json" --name gitlab --username gitlab-ci-token --password $CI_JOB_TOKEN --store-password-in-clear-text + - "dotnet add otherdotnet.csproj package #{package_name} --version 1.0.0" + only: + - "#{another_project.default_branch}" + tags: + - "runner-for-#{another_project.name}" + YAML + } + ] + ) + end + + Flow::Pipeline.visit_latest_pipeline + + Page::Project::Pipeline::Show.perform do |pipeline| + pipeline.click_job('install') + end + + Page::Project::Job::Show.perform do |job| + expect(job).to be_successful(timeout: 800) + end + + project.group.sandbox.visit! + + Page::Group::Menu.perform(&:go_to_group_packages) Page::Project::Packages::Index.perform do |index| expect(index).to have_package(package_name) diff --git a/scripts/trigger-build b/scripts/trigger-build index ab6dcc63e11..aeea25f9ad8 100755 --- a/scripts/trigger-build +++ b/scripts/trigger-build @@ -3,13 +3,6 @@ require 'gitlab' -# -# Configure credentials to be used with gitlab gem -# -Gitlab.configure do |config| - config.endpoint = 'https://gitlab.com/api/v4' -end - module Trigger def self.ee? # Support former project name for `dev` @@ -34,18 +27,13 @@ module Trigger ENV['GITLAB_BOT_MULTI_PROJECT_PIPELINE_POLLING_TOKEN'] end - def initialize - # gitlab-bot's token "GitLab multi-project pipeline polling" - Gitlab.private_token = self.class.access_token - end - def invoke!(post_comment: false, downstream_job_name: nil) pipeline_variables = variables puts "Triggering downstream pipeline on #{downstream_project_path}" puts "with variables #{pipeline_variables}" - pipeline = Gitlab.run_trigger( + pipeline = gitlab_client(:downstream).run_trigger( downstream_project_path, trigger_token, ref, @@ -54,23 +42,34 @@ module Trigger puts "Triggered downstream pipeline: #{pipeline.web_url}\n" puts "Waiting for downstream pipeline status" - Trigger::CommitComment.post!(pipeline) if post_comment + Trigger::CommitComment.post!(pipeline, gitlab_client(:upstream)) if post_comment downstream_job = if downstream_job_name - Gitlab.pipeline_jobs(downstream_project_path, pipeline.id).auto_paginate.find do |potential_job| + gitlab_client(:downstream).pipeline_jobs(downstream_project_path, pipeline.id).auto_paginate.find do |potential_job| potential_job.name == downstream_job_name end end if downstream_job - Trigger::Job.new(downstream_project_path, downstream_job.id) + Trigger::Job.new(downstream_project_path, downstream_job.id, gitlab_client(:downstream)) else - Trigger::Pipeline.new(downstream_project_path, pipeline.id) + Trigger::Pipeline.new(downstream_project_path, pipeline.id, gitlab_client(:downstream)) end end private + # Override to trigger and work with pipeline on different GitLab instance + # type: :downstream -> downstream build and pipeline status + # type: :upstream -> this project, e.g. for posting comments + def gitlab_client(type) + # By default, always use the same client + @gitlab_client ||= Gitlab.client( + endpoint: 'https://gitlab.com/api/v4', + private_token: self.class.access_token + ) + end + # Must be overridden def downstream_project_path raise NotImplementedError @@ -305,9 +304,53 @@ module Trigger end end + class DatabaseTesting < Base + def self.access_token + ENV['GITLABCOM_DATABASE_TESTING_ACCESS_TOKEN'] + end + + private + + def gitlab_client(type) + @gitlab_clients ||= { + downstream: Gitlab.client( + endpoint: 'https://ops.gitlab.net/api/v4', + private_token: self.class.access_token + ), + upstream: Gitlab.client( + endpoint: 'https://gitlab.com/api/v4', + private_token: Base.access_token + ) + } + + @gitlab_clients[type] + end + + def trigger_token + ENV['GITLABCOM_DATABASE_TESTING_TRIGGER_TOKEN'] + end + + def downstream_project_path + ENV['GITLABCOM_DATABASE_TESTING_PROJECT_PATH'] || 'gitlab-com/database-team/gitlab-com-database-testing' + end + + def extra_variables + { + # Use CI_MERGE_REQUEST_SOURCE_BRANCH_SHA for omnibus checkouts due to pipeline for merged results + # and fallback to CI_COMMIT_SHA for the `detached` pipelines. + 'GITLAB_COMMIT_SHA' => Trigger.non_empty_variable_value('CI_MERGE_REQUEST_SOURCE_BRANCH_SHA') || ENV['CI_COMMIT_SHA'], + 'TRIGGERED_USER_LOGIN' => ENV['GITLAB_USER_LOGIN'] + } + end + + def ref + ENV['GITLABCOM_DATABASE_TESTING_TRIGGER_REF'] || 'master' + end + end + class CommitComment - def self.post!(downstream_pipeline) - Gitlab.create_commit_comment( + def self.post!(downstream_pipeline, gitlab_client) + gitlab_client.create_commit_comment( ENV['CI_PROJECT_PATH'], Trigger.non_empty_variable_value('CI_MERGE_REQUEST_SOURCE_BRANCH_SHA') || ENV['CI_COMMIT_SHA'], "The [`#{ENV['CI_JOB_NAME']}`](#{ENV['CI_JOB_URL']}) job from pipeline #{ENV['CI_PIPELINE_URL']} triggered #{downstream_pipeline.web_url} downstream.") @@ -329,9 +372,10 @@ module Trigger unscoped_class_name.downcase end - def initialize(project, id) + def initialize(project, id, gitlab_client) @project = project @id = id + @gitlab_client = gitlab_client @start_time = Time.now.to_i end @@ -359,7 +403,7 @@ module Trigger end def status - Gitlab.public_send(self.class.gitlab_api_method_name, project, id).status.to_sym # rubocop:disable GitlabSecurity/PublicSend + gitlab_client.public_send(self.class.gitlab_api_method_name, project, id).status.to_sym # rubocop:disable GitlabSecurity/PublicSend rescue Gitlab::Error::Error => error puts "Ignoring the following error: #{error}" # Ignore GitLab API hiccups. If GitLab is really down, we'll hit the job @@ -369,7 +413,7 @@ module Trigger private - attr_reader :project, :id, :start_time + attr_reader :project, :id, :gitlab_client, :start_time end Job = Class.new(Pipeline) @@ -380,6 +424,8 @@ when 'omnibus' Trigger::Omnibus.new.invoke!(post_comment: true, downstream_job_name: 'Trigger:qa-test').wait! when 'cng' Trigger::CNG.new.invoke!.wait! +when 'gitlab-com-database-testing' + Trigger::DatabaseTesting.new.invoke! when 'docs' docs_trigger = Trigger::Docs.new @@ -395,5 +441,6 @@ when 'docs' else puts "Please provide a valid option: omnibus - Triggers a pipeline that builds the omnibus-gitlab package - cng - Triggers a pipeline that builds images used by the GitLab helm chart" + cng - Triggers a pipeline that builds images used by the GitLab helm chart + gitlab-com-database-testing - Triggers a pipeline that tests database changes on GitLab.com data" end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 81ffd2c4512..597f7007a7f 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -63,20 +63,53 @@ RSpec.describe Projects::IssuesController do end end - describe 'the null hypothesis experiment', :experiment do - before do - stub_experiments(null_hypothesis: :candidate) - end - + describe 'the null hypothesis experiment', :snowplow do it 'defines the expected before actions' do expect(controller).to use_before_action(:run_null_hypothesis_experiment) end - it 'assigns the candidate experience and tracks the event' do - expect(experiment(:null_hypothesis)).to track('index').on_any_instance.for(:candidate) - .with_context(project: project) + context 'when rolled out to 100%' do + it 'assigns the candidate experience and tracks the event' do + get :index, params: { namespace_id: project.namespace, project_id: project } + + expect_snowplow_event( + category: 'null_hypothesis', + action: 'index', + context: [{ + schema: 'iglu:com.gitlab/gitlab_experiment/jsonschema/0-3-0', + data: { variant: 'candidate', experiment: 'null_hypothesis', key: anything } + }] + ) + end + end - get :index, params: { namespace_id: project.namespace, project_id: project } + context 'when not rolled out' do + before do + stub_feature_flags(null_hypothesis: false) + end + + it 'assigns the control experience and tracks the event' do + get :index, params: { namespace_id: project.namespace, project_id: project } + + expect_snowplow_event( + category: 'null_hypothesis', + action: 'index', + context: [{ + schema: 'iglu:com.gitlab/gitlab_experiment/jsonschema/0-3-0', + data: { variant: 'control', experiment: 'null_hypothesis', key: anything } + }] + ) + end + end + + context 'when gitlab_experiments is disabled' do + it 'does not run the experiment at all' do + stub_feature_flags(gitlab_experiments: false) + + expect(controller).not_to receive(:run_null_hypothesis_experiment) + + get :index, params: { namespace_id: project.namespace, project_id: project } + end end end end diff --git a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb index f54a07de853..50f8942d9d5 100644 --- a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb +++ b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb @@ -226,11 +226,7 @@ RSpec.describe Projects::MergeRequests::DiffsController do let(:diffable_merge_ref) { true } it 'compares diffs with the head' do - MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) - - expect(CompareService).to receive(:new).with( - project, merge_request.merge_ref_head.sha - ).and_call_original + create(:merge_request_diff, :merge_head, merge_request: merge_request) go(diff_head: true) @@ -242,8 +238,6 @@ RSpec.describe Projects::MergeRequests::DiffsController do let(:diffable_merge_ref) { false } it 'compares diffs with the base' do - expect(CompareService).not_to receive(:new) - go(diff_head: true) expect(response).to have_gitlab_http_status(:ok) diff --git a/spec/experiments/application_experiment_spec.rb b/spec/experiments/application_experiment_spec.rb index beefc09e591..ece52d37351 100644 --- a/spec/experiments/application_experiment_spec.rb +++ b/spec/experiments/application_experiment_spec.rb @@ -2,45 +2,9 @@ require 'spec_helper' -RSpec.describe ApplicationExperiment, :experiment do +RSpec.describe ApplicationExperiment do subject { described_class.new(:stub) } - before do - allow(subject).to receive(:enabled?).and_return(true) - end - - describe "enabled" do - before do - allow(subject).to receive(:enabled?).and_call_original - - allow(Feature::Definition).to receive(:get).and_return('_instance_') - allow(Gitlab).to receive(:dev_env_or_com?).and_return(true) - allow(Feature).to receive(:get).and_return(double(state: :on)) - end - - it "is enabled when all criteria are met" do - expect(subject).to be_enabled - end - - it "isn't enabled if the feature definition doesn't exist" do - expect(Feature::Definition).to receive(:get).with('stub').and_return(nil) - - expect(subject).not_to be_enabled - end - - it "isn't enabled if we're not in dev or dotcom environments" do - expect(Gitlab).to receive(:dev_env_or_com?).and_return(false) - - expect(subject).not_to be_enabled - end - - it "isn't enabled if the feature flag state is :off" do - expect(Feature).to receive(:get).with('stub').and_return(double(state: :off)) - - expect(subject).not_to be_enabled - end - end - describe "publishing results" do it "tracks the assignment" do expect(subject).to receive(:track).with(:assignment) @@ -67,8 +31,8 @@ RSpec.describe ApplicationExperiment, :experiment do end describe "tracking events", :snowplow do - it "doesn't track if we shouldn't track" do - allow(subject).to receive(:should_track?).and_return(false) + it "doesn't track if excluded" do + subject.exclude { true } subject.track(:action) diff --git a/spec/factories/merge_request_diffs.rb b/spec/factories/merge_request_diffs.rb index 481cabdae6d..f93f3f22109 100644 --- a/spec/factories/merge_request_diffs.rb +++ b/spec/factories/merge_request_diffs.rb @@ -10,12 +10,18 @@ FactoryBot.define do head_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) } start_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) } + diff_type { :regular } + trait :external do external_diff { fixture_file_upload("spec/fixtures/doc_sample.txt", "plain/txt") } stored_externally { true } importing { true } # this avoids setting the state to 'empty' end + trait :merge_head do + diff_type { :merge_head } + end + factory :external_merge_request_diff, traits: [:external] end end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index ab9b62e02f0..3f40e0d228b 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -146,6 +146,7 @@ merge_requests: - merge_user - merge_request_diffs - merge_request_diff +- merge_head_diff - merge_request_context_commits - merge_request_context_commit_diff_files - events diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 79769f7ba7a..fa1df7acd07 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -220,6 +220,7 @@ MergeRequestDiff: - commits_count - files_count - sorted +- diff_type MergeRequestDiffCommit: - merge_request_diff_id - relative_order diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index a5493d1650b..83024c587fa 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -16,6 +16,8 @@ RSpec.describe MergeRequestDiff do describe 'validations' do subject { diff_with_commits } + it { is_expected.not_to validate_uniqueness_of(:diff_type).scoped_to(:merge_request_id) } + it 'checks sha format of base_commit_sha, head_commit_sha and start_commit_sha' do subject.base_commit_sha = subject.head_commit_sha = subject.start_commit_sha = 'foobar' @@ -23,6 +25,24 @@ RSpec.describe MergeRequestDiff do expect(subject.errors.count).to eq 3 expect(subject.errors).to all(include('is not a valid SHA')) end + + it 'does not validate uniqueness by default' do + expect(build(:merge_request_diff, merge_request: subject.merge_request)).to be_valid + end + + context 'when merge request diff is a merge_head type' do + it 'is valid' do + expect(build(:merge_request_diff, :merge_head, merge_request: subject.merge_request)).to be_valid + end + + context 'when merge_head diff exists' do + let(:existing_merge_head_diff) { create(:merge_request_diff, :merge_head) } + + it 'validates uniqueness' do + expect(build(:merge_request_diff, :merge_head, merge_request: existing_merge_head_diff.merge_request)).not_to be_valid + end + end + end end describe 'create new record' do @@ -35,6 +55,26 @@ RSpec.describe MergeRequestDiff do it { expect(subject.head_commit_sha).to eq('b83d6e391c22777fca1ed3012fce84f633d7fed0') } it { expect(subject.base_commit_sha).to eq('ae73cb07c9eeaf35924a10f713b364d32b2dd34f') } it { expect(subject.start_commit_sha).to eq('0b4bc9a49b562e85de7cc9e834518ea6828729b9') } + + context 'when diff_type is merge_head' do + let_it_be(:merge_request) { create(:merge_request) } + + let_it_be(:merge_head) do + MergeRequests::MergeToRefService + .new(merge_request.project, merge_request.author) + .execute(merge_request) + + merge_request.create_merge_head_diff + end + + it { expect(merge_head).to be_valid } + it { expect(merge_head).to be_persisted } + it { expect(merge_head.commits.count).to eq(30) } + it { expect(merge_head.diffs.count).to eq(20) } + it { expect(merge_head.head_commit_sha).to eq(merge_request.merge_ref_head.diff_refs.head_sha) } + it { expect(merge_head.base_commit_sha).to eq(merge_request.merge_ref_head.diff_refs.base_sha) } + it { expect(merge_head.start_commit_sha).to eq(merge_request.target_branch_sha) } + end end describe '.by_commit_sha' do @@ -63,6 +103,7 @@ RSpec.describe MergeRequestDiff do let_it_be(:merge_request) { create(:merge_request) } let_it_be(:outdated) { merge_request.merge_request_diff } let_it_be(:latest) { merge_request.create_merge_request_diff } + let_it_be(:merge_head) { merge_request.create_merge_head_diff } let_it_be(:closed_mr) { create(:merge_request, :closed_last_month) } let(:closed) { closed_mr.merge_request_diff } @@ -103,14 +144,14 @@ RSpec.describe MergeRequestDiff do stub_external_diffs_setting(enabled: true) end - it { is_expected.to contain_exactly(outdated.id, latest.id, closed.id, merged.id, closed_recently.id, merged_recently.id) } + it { is_expected.to contain_exactly(outdated.id, latest.id, closed.id, merged.id, closed_recently.id, merged_recently.id, merge_head.id) } it 'ignores diffs with 0 files' do MergeRequestDiffFile.where(merge_request_diff_id: [closed_recently.id, merged_recently.id]).delete_all closed_recently.update!(files_count: 0) merged_recently.update!(files_count: 0) - is_expected.to contain_exactly(outdated.id, latest.id, closed.id, merged.id) + is_expected.to contain_exactly(outdated.id, latest.id, closed.id, merged.id, merge_head.id) end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 094bb370100..9ba379167ee 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -491,6 +491,7 @@ RSpec.describe MergeRequest, factory_default: :keep do create(:merge_request, params).tap do |mr| diffs.times { mr.merge_request_diffs.create } + mr.create_merge_head_diff end end @@ -4379,37 +4380,41 @@ RSpec.describe MergeRequest, factory_default: :keep do end describe '#diffable_merge_ref?' do + let(:merge_request) { create(:merge_request) } + context 'merge request can be merged' do - context 'merge_to_ref is not calculated' do + context 'merge_head diff is not created' do it 'returns true' do - expect(subject.diffable_merge_ref?).to eq(false) + expect(merge_request.diffable_merge_ref?).to eq(false) end end - context 'merge_to_ref is calculated' do + context 'merge_head diff is created' do before do - MergeRequests::MergeToRefService.new(subject.project, subject.author).execute(subject) + create(:merge_request_diff, :merge_head, merge_request: merge_request) end it 'returns true' do - expect(subject.diffable_merge_ref?).to eq(true) + expect(merge_request.diffable_merge_ref?).to eq(true) end context 'merge request is merged' do - subject { build_stubbed(:merge_request, :merged, project: project) } + before do + merge_request.mark_as_merged! + end it 'returns false' do - expect(subject.diffable_merge_ref?).to eq(false) + expect(merge_request.diffable_merge_ref?).to eq(false) end end context 'merge request cannot be merged' do before do - subject.mark_as_unchecked! + merge_request.mark_as_unchecked! end it 'returns false' do - expect(subject.diffable_merge_ref?).to eq(true) + expect(merge_request.diffable_merge_ref?).to eq(true) end context 'display_merge_conflicts_in_diff is disabled' do @@ -4418,7 +4423,7 @@ RSpec.describe MergeRequest, factory_default: :keep do end it 'returns false' do - expect(subject.diffable_merge_ref?).to eq(false) + expect(merge_request.diffable_merge_ref?).to eq(false) end end end diff --git a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb index cdaacaf5fca..d2070a466b1 100644 --- a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb +++ b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb @@ -12,6 +12,8 @@ RSpec.describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_s stub_const("#{described_class.name}::BATCH_SIZE", 2) 3.times { merge_request.create_merge_request_diff } + merge_request.create_merge_head_diff + merge_request.reset end it 'schedules non-latest merge request diffs removal' do diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index 17bfa9d7368..e0baf5af8b4 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -33,6 +33,14 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar expect(merge_request.merge_status).to eq('can_be_merged') end + it 'reloads merge head diff' do + expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |service| + expect(service).to receive(:execute) + end + + subject + end + it 'update diff discussion positions' do expect_next_instance_of(Discussions::CaptureDiffNotePositionsService) do |service| expect(service).to receive(:execute) @@ -142,7 +150,11 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar end it 'resets one merge request upon execution' do - expect_any_instance_of(MergeRequest).to receive(:reset).once + expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |svc| + expect(svc).to receive(:execute).and_return(status: :success) + end + + expect_any_instance_of(MergeRequest).to receive(:reset).once.and_call_original execute_within_threads(amount: 2) end @@ -266,6 +278,14 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar it_behaves_like 'unmergeable merge request' + it 'reloads merge head diff' do + expect_next_instance_of(MergeRequests::ReloadMergeHeadDiffService) do |service| + expect(service).to receive(:execute) + end + + subject + end + it 'returns ServiceResponse.error' do result = subject @@ -329,6 +349,12 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar subject end + + it 'does not reload merge head diff' do + expect(MergeRequests::ReloadMergeHeadDiffService).not_to receive(:new) + + subject + end end end diff --git a/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb b/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb new file mode 100644 index 00000000000..3152a4e3861 --- /dev/null +++ b/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::ReloadMergeHeadDiffService do + let(:merge_request) { create(:merge_request) } + + subject { described_class.new(merge_request).execute } + + describe '#execute' do + before do + MergeRequests::MergeToRefService + .new(merge_request.project, merge_request.author) + .execute(merge_request) + end + + it 'creates a merge head diff' do + expect(subject[:status]).to eq(:success) + expect(merge_request.reload.merge_head_diff).to be_present + end + + context 'when merge ref head is not present' do + before do + allow(merge_request).to receive(:merge_ref_head).and_return(nil) + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + end + end + + context 'when failed to create merge head diff' do + before do + allow(merge_request).to receive(:create_merge_head_diff!).and_raise('fail') + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + end + end + + context 'when there is existing merge head diff' do + let!(:existing_merge_head_diff) { create(:merge_request_diff, :merge_head, merge_request: merge_request) } + + it 'recreates merge head diff' do + expect(subject[:status]).to eq(:success) + expect(merge_request.reload.merge_head_diff).not_to eq(existing_merge_head_diff) + end + end + + context 'when default_merge_ref_for_diffs feature flag is disabled' do + before do + stub_feature_flags(default_merge_ref_for_diffs: false) + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + end + end + end +end diff --git a/spec/support/gitlab_experiment.rb b/spec/support/gitlab_experiment.rb index f8375a02b1e..1f283e4f06c 100644 --- a/spec/support/gitlab_experiment.rb +++ b/spec/support/gitlab_experiment.rb @@ -1,7 +1,4 @@ # frozen_string_literal: true -# Require the provided spec helper and matchers. -require 'gitlab/experiment/rspec' - # Disable all caching for experiments in tests. Gitlab::Experiment::Configuration.cache = nil |