From 984dc66b0766db14d010b89fbf652fd6fe3ea8db Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 7 Mar 2022 15:52:05 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- app/assets/javascripts/deprecated_notes.js | 9 ++- app/models/namespace.rb | 28 +++++++- .../ci/runners/assign_runner_service.rb | 12 +++- app/views/groups/imports/show.html.haml | 2 +- .../creations/_new_submit.html.haml | 2 +- .../shared/milestones/_tab_loading.html.haml | 3 +- ...espaces_cache_first_auto_devops_config.yml | 8 +++ spec/models/group_spec.rb | 71 ++++++++++++++++++- .../ci/runners/assign_runner_service_spec.rb | 4 +- 9 files changed, 124 insertions(+), 15 deletions(-) create mode 100644 config/feature_flags/development/namespaces_cache_first_auto_devops_config.yml diff --git a/app/assets/javascripts/deprecated_notes.js b/app/assets/javascripts/deprecated_notes.js index 91e39dcd466..0707ae02872 100644 --- a/app/assets/javascripts/deprecated_notes.js +++ b/app/assets/javascripts/deprecated_notes.js @@ -17,6 +17,7 @@ import { escape, uniqueId } from 'lodash'; import Vue from 'vue'; import '~/lib/utils/jquery_at_who'; import AjaxCache from '~/lib/utils/ajax_cache'; +import { loadingIconForLegacyJS } from '~/loading_icon_for_legacy_js'; import syntaxHighlight from '~/syntax_highlight'; import CommentTypeDropdown from '~/notes/components/comment_type_dropdown.vue'; import * as constants from '~/notes/constants'; @@ -1761,9 +1762,11 @@ export default class Notes { // Show updated comment content temporarily $noteBodyText.html(formContent); $editingNote.removeClass('is-editing fade-in-full').addClass('being-posted fade-in-half'); - $editingNote - .find('.note-headline-meta a') - .html(''); + + const $timeAgo = $editingNote.find('.note-headline-meta a'); + + $timeAgo.empty(); + $timeAgo.append(loadingIconForLegacyJS({ inline: true, size: 'sm' })); // Make request to update comment on server axios diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 97e0df89e41..ffaeb2071f6 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -117,6 +117,7 @@ class Namespace < ApplicationRecord before_create :sync_share_with_group_lock_with_parent before_update :sync_share_with_group_lock_with_parent, if: :parent_changed? after_update :force_share_with_group_lock_on_descendants, if: -> { saved_change_to_share_with_group_lock? && share_with_group_lock? } + after_update :expire_first_auto_devops_config_cache, if: -> { saved_change_to_auto_devops_enabled? } # Legacy Storage specific hooks @@ -401,7 +402,11 @@ class Namespace < ApplicationRecord return { scope: :group, status: auto_devops_enabled } unless auto_devops_enabled.nil? strong_memoize(:first_auto_devops_config) do - if has_parent? + if has_parent? && cache_first_auto_devops_config? + Rails.cache.fetch(first_auto_devops_config_cache_key_for(id), expires_in: 1.day) do + parent.first_auto_devops_config + end + elsif has_parent? parent.first_auto_devops_config else { scope: :instance, status: Gitlab::CurrentSettings.auto_devops_enabled? } @@ -617,6 +622,20 @@ class Namespace < ApplicationRecord .update_all(share_with_group_lock: true) end + def expire_first_auto_devops_config_cache + return unless cache_first_auto_devops_config? + + descendants_to_expire = self_and_descendants.as_ids + return if descendants_to_expire.load.empty? + + keys = descendants_to_expire.map { |group| first_auto_devops_config_cache_key_for(group.id) } + Rails.cache.delete_multi(keys) + end + + def cache_first_auto_devops_config? + ::Feature.enabled?(:namespaces_cache_first_auto_devops_config, default_enabled: :yaml) + end + def write_projects_repository_config all_projects.find_each do |project| project.set_full_path @@ -634,6 +653,13 @@ class Namespace < ApplicationRecord Namespaces::SyncEvent.enqueue_worker end end + + def first_auto_devops_config_cache_key_for(group_id) + return "namespaces:{first_auto_devops_config}:#{group_id}" unless sync_traversal_ids? + + # Use SHA2 of `traversal_ids` to account for moving a namespace within the same root ancestor hierarchy. + "namespaces:{#{traversal_ids.first}}:first_auto_devops_config:#{group_id}:#{Digest::SHA2.hexdigest(traversal_ids.join(' '))}" + end end Namespace.prepend_mod_with('Namespace') diff --git a/app/services/ci/runners/assign_runner_service.rb b/app/services/ci/runners/assign_runner_service.rb index 0e16ac35231..886cd3a4e44 100644 --- a/app/services/ci/runners/assign_runner_service.rb +++ b/app/services/ci/runners/assign_runner_service.rb @@ -3,9 +3,9 @@ module Ci module Runners class AssignRunnerService - # @param [Ci::Runner] runner the runner to assign to a project - # @param [Project] project the new project to assign the runner to - # @param [User] user the user performing the operation + # @param [Ci::Runner] runner: the runner to assign to a project + # @param [Project] project: the new project to assign the runner to + # @param [User] user: the user performing the operation def initialize(runner, project, user) @runner = runner @project = project @@ -17,6 +17,12 @@ module Ci @runner.assign_to(@project, @user) end + + private + + attr_reader :runner, :project, :user end end end + +Ci::Runners::AssignRunnerService.prepend_mod diff --git a/app/views/groups/imports/show.html.haml b/app/views/groups/imports/show.html.haml index 79cac364016..9cfb58da7e4 100644 --- a/app/views/groups/imports/show.html.haml +++ b/app/views/groups/imports/show.html.haml @@ -4,7 +4,7 @@ .save-group-loader .center %h2 - %i.loading.gl-spinner + = gl_loading_icon(size: 'md') = page_title %p = s_('GroupImport|Please wait while we import the group for you. Refresh at will.') diff --git a/app/views/projects/merge_requests/creations/_new_submit.html.haml b/app/views/projects/merge_requests/creations/_new_submit.html.haml index 0036f1b4bde..253f50d5090 100644 --- a/app/views/projects/merge_requests/creations/_new_submit.html.haml +++ b/app/views/projects/merge_requests/creations/_new_submit.html.haml @@ -48,4 +48,4 @@ .mr-loading-status .loading.hide - .gl-spinner.gl-spinner-md + = gl_loading_icon(size: 'md') diff --git a/app/views/shared/milestones/_tab_loading.html.haml b/app/views/shared/milestones/_tab_loading.html.haml index b19e994ef80..ebd4ef7d4c3 100644 --- a/app/views/shared/milestones/_tab_loading.html.haml +++ b/app/views/shared/milestones/_tab_loading.html.haml @@ -1,2 +1 @@ -.text-center.gl-mt-3 - .gl-spinner.gl-spinner-md += gl_loading_icon(size: 'md', css_class: 'gl-mt-3') diff --git a/config/feature_flags/development/namespaces_cache_first_auto_devops_config.yml b/config/feature_flags/development/namespaces_cache_first_auto_devops_config.yml new file mode 100644 index 00000000000..6d15df2bb91 --- /dev/null +++ b/config/feature_flags/development/namespaces_cache_first_auto_devops_config.yml @@ -0,0 +1,8 @@ +--- +name: namespaces_cache_first_auto_devops_config +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80937 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/353503 +milestone: '14.9' +type: development +group: group::authentication and authorization +default_enabled: false diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 297b11bd72a..98fad544abd 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -2191,7 +2191,7 @@ RSpec.describe Group do let(:group) { create(:group) } - subject { group.first_auto_devops_config } + subject(:fetch_config) { group.first_auto_devops_config } where(:instance_value, :group_value, :config) do # Instance level enabled @@ -2216,6 +2216,8 @@ RSpec.describe Group do end context 'with parent groups' do + let(:parent) { create(:group) } + where(:instance_value, :parent_value, :group_value, :config) do # Instance level enabled true | nil | nil | { status: true, scope: :instance } @@ -2245,17 +2247,82 @@ RSpec.describe Group do end with_them do + def define_cache_expectations(cache_key) + if group_value.nil? + expect(Rails.cache).to receive(:fetch).with(start_with(cache_key), expires_in: 1.day) + else + expect(Rails.cache).not_to receive(:fetch).with(start_with(cache_key), expires_in: 1.day) + end + end + before do stub_application_setting(auto_devops_enabled: instance_value) - parent = create(:group, auto_devops_enabled: parent_value) group.update!( auto_devops_enabled: group_value, parent: parent ) + parent.update!(auto_devops_enabled: parent_value) + + group.reload # Reload so we get the populated traversal IDs end it { is_expected.to eq(config) } + + it 'caches the parent config when group auto_devops_enabled is nil' do + cache_key = "namespaces:{#{group.traversal_ids.first}}:first_auto_devops_config:#{group.id}" + define_cache_expectations(cache_key) + + fetch_config + end + + context 'when traversal ID feature flags are disabled' do + before do + stub_feature_flags(sync_traversal_ids: false) + end + + it 'caches the parent config when group auto_devops_enabled is nil' do + cache_key = "namespaces:{first_auto_devops_config}:#{group.id}" + define_cache_expectations(cache_key) + + fetch_config + end + end + end + + context 'cache expiration' do + before do + group.update!(parent: parent) + + reload_models(parent) + end + + it 'clears both self and descendant cache when the parent value is updated' do + expect(Rails.cache).to receive(:delete_multi) + .with( + match_array([ + start_with("namespaces:{#{parent.traversal_ids.first}}:first_auto_devops_config:#{parent.id}"), + start_with("namespaces:{#{parent.traversal_ids.first}}:first_auto_devops_config:#{group.id}") + ]) + ) + + parent.update!(auto_devops_enabled: true) + end + + it 'only clears self cache when there are no dependents' do + expect(Rails.cache).to receive(:delete_multi) + .with([start_with("namespaces:{#{group.traversal_ids.first}}:first_auto_devops_config:#{group.id}")]) + + group.update!(auto_devops_enabled: true) + end + + it 'does not clear cache when the feature is disabled' do + stub_feature_flags(namespaces_cache_first_auto_devops_config: false) + + expect(Rails.cache).not_to receive(:delete_multi) + + parent.update!(auto_devops_enabled: true) + end end end end diff --git a/spec/services/ci/runners/assign_runner_service_spec.rb b/spec/services/ci/runners/assign_runner_service_spec.rb index 2997dfd7027..00b176bb759 100644 --- a/spec/services/ci/runners/assign_runner_service_spec.rb +++ b/spec/services/ci/runners/assign_runner_service_spec.rb @@ -5,8 +5,8 @@ require 'spec_helper' RSpec.describe ::Ci::Runners::AssignRunnerService, '#execute' do subject { described_class.new(runner, project, user).execute } - let_it_be(:runner) { build(:ci_runner) } - let_it_be(:project) { build(:project) } + let_it_be(:runner) { create(:ci_runner, :project, projects: [project]) } + let_it_be(:project) { create(:project) } context 'without user' do let(:user) { nil }