Automatic merge of gitlab-org/gitlab master

app/views/admin/groups/_form.html.haml

 = form_for [:admin, @group] do |f|
   = form_errors(@group)
   = render 'shared/group_form', f: f
+  = render 'shared/group_form_description', f: f
 
   = render_if_exists 'shared/old_repository_size_limit_setting', form: f, type: :group
   = render_if_exists 'admin/namespace_plan', f: f

app/views/groups/_new_group_fields.html.haml

@@ -2,11 +2,6 @@
 = render 'shared/group_form', f: f, autofocus: true
 
 .row
-  .form-group.group-description-holder.col-sm-12
-    = f.label :avatar, _("Group avatar"), class: 'label-bold'
-    %div
-      = render 'shared/choose_avatar_button', f: f
-
   .form-group.col-sm-12
     %label.label-bold
       = _('Visibility level')

app/views/shared/_group_form.html.haml

@@ -47,11 +47,3 @@
       = f.label :id, class: 'label-bold' do
         = _("Group ID")
       = f.text_field :id, class: 'form-control', readonly: true
-
-.row
-  .form-group.group-description-holder.col-sm-8
-    = f.label :description, class: 'label-bold' do
-      = _("Group description")
-      %span (optional)
-    = f.text_area :description, maxlength: 250,
-        class: 'form-control js-gfm-input', rows: 4

app/views/shared/_group_form_description.html.haml

+.row
+  .form-group.group-description-holder.col-sm-8
+    = f.label :description, _('Group description (optional)'), class: 'label-bold'
+    = f.text_area :description, maxlength: 250,
+        class: 'form-control js-gfm-input', rows: 4

changelogs/unreleased/219241-refine-group-creation-form.yml

+---
+title: Refine group creation form
+merge_request: 48490
+author:
+type: changed

changelogs/unreleased/281586_monorepo_nodejsscan.yml

+---
+title: Update nodejs-scan rule to wildcard prefix
+merge_request: 48902
+author:
+type: changed

doc/development/elasticsearch.md

@@ -216,6 +216,29 @@ cron worker sequentially.
 
 Any update to the Elastic index mappings should be replicated in [`Elastic::Latest::Config`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/lib/elastic/latest/config.rb).
 
+### Migration options supported by the [`Elastic::MigrationWorker`](https://gitlab.com/gitlab-org/gitlab/blob/master/ee/app/workers/elastic/migration_worker.rb)
+
+- `batched!` - Allow the migration to run in batches. If set, the [`Elastic::MigrationWorker`](https://gitlab.com/gitlab-org/gitlab/blob/master/ee/app/workers/elastic/migration_worker.rb) 
+will re-enqueue itself with a delay which is set using the `throttle_delay` option described below. The batching
+must be handled within the `migrate` method, this setting controls the re-enqueuing only.  
+ 
+- `throttle_delay` - Sets the wait time in between batch runs. This time should be set high enough to allow each migration batch
+enough time to finish. Additionally, the time should be less than 30 minutes since that is how often the
+[`Elastic::MigrationWorker`](https://gitlab.com/gitlab-org/gitlab/blob/master/ee/app/workers/elastic/migration_worker.rb)
+cron worker runs. Default value is 5 minutes.
+
+```ruby
+# frozen_string_literal: true
+
+class BatchedMigrationName < Elastic::Migration
+  # Declares a migration should be run in batches
+  batched!
+  throttle_delay 10.minutes
+
+  # ...
+end
+```
+
 ## Performance Monitoring
 
 ### Prometheus

ee/app/models/elastic/migration_record.rb

@@ -4,7 +4,7 @@ module Elastic
   class MigrationRecord
     attr_reader :version, :name, :filename
 
-    delegate :migrate, :skip_migration?, :completed?, to: :migration
+    delegate :migrate, :skip_migration?, :completed?, :batched?, :throttle_delay, to: :migration
 
     def initialize(version:, name:, filename:)
       @version = version

ee/app/workers/concerns/elastic/migration_options.rb

+# frozen_string_literal: true
+
+module Elastic
+  module MigrationOptions
+    extend ActiveSupport::Concern
+    include Gitlab::ClassAttributes
+
+    DEFAULT_THROTTLE_DELAY = 5.minutes
+
+    def batched?
+      self.class.get_batched
+    end
+
+    def throttle_delay
+      self.class.get_throttle_delay
+    end
+
+    class_methods do
+      def batched!
+        class_attributes[:batched] = true
+      end
+
+      def get_batched
+        class_attributes[:batched]
+      end
+
+      def throttle_delay(value)
+        class_attributes[:throttle_delay] = value
+      end
+
+      def get_throttle_delay
+        class_attributes[:throttle_delay] || DEFAULT_THROTTLE_DELAY
+      end
+    end
+  end
+end

ee/app/workers/elastic/migration_worker.rb

@@ -42,11 +42,16 @@ module Elastic
     private
 
     def execute_migration(migration)
-      if migration.persisted?
+      if migration.persisted? && !migration.batched?
         logger.info "MigrationWorker: migration[#{migration.name}] did not execute migrate method since it was already executed. Waiting for migration to complete"
       else
         logger.info "MigrationWorker: migration[#{migration.name}] executing migrate method"
         migration.migrate
+
+        if migration.batched? && !migration.completed?
+          logger.info "MigrationWorker: migration[#{migration.name}] kicking off next migration batch"
+          Elastic::MigrationWorker.perform_in(migration.throttle_delay)
+        end
       end
     end
 

ee/elastic/migrate/20201116142400_add_new_data_to_issues_documents.rb

+# frozen_string_literal: true
+
+class AddNewDataToIssuesDocuments < Elastic::Migration
+  batched!
+  throttle_delay 5.minutes
+
+  QUERY_BATCH_SIZE = 5000
+  UPDATE_BATCH_SIZE = 100
+
+  def migrate
+    if completed?
+      log "Skipping adding visibility_level field to issues documents migration since it is already applied"
+      return
+    end
+
+    log "Adding visibility_level field to issues documents for batch of #{QUERY_BATCH_SIZE} documents"
+
+    query = {
+      size: QUERY_BATCH_SIZE,
+      query: {
+        bool: {
+          filter: issues_missing_visibility_level_filter
+        }
+      }
+    }
+
+    results = client.search(index: helper.target_index_name, body: query)
+    hits = results.dig('hits', 'hits') || []
+
+    document_references = hits.map do |hit|
+      id = hit.dig('_source', 'id')
+      es_id = hit.dig('_id')
+      es_parent = hit.dig('_source', 'join_field', 'parent')
+
+      # ensure that any issues missing from the database will be removed from Elasticsearch
+      # as the data is back-filled
+      Gitlab::Elastic::DocumentReference.new(Issue, id, es_id, es_parent)
+    end
+
+    document_references.each_slice(UPDATE_BATCH_SIZE) do |refs|
+      Elastic::ProcessBookkeepingService.track!(*refs)
+    end
+
+    log "Adding visibility_level field to issues documents is completed for batch of #{document_references.size} documents"
+  end
+
+  def completed?
+    query = {
+      size: 0,
+      aggs: {
+        issues: {
+          filter: issues_missing_visibility_level_filter
+        }
+      }
+    }
+
+    results = client.search(index: helper.target_index_name, body: query)
+    doc_count = results.dig('aggregations', 'issues', 'doc_count')
+    doc_count && doc_count == 0
+  end
+
+  private
+
+  def issues_missing_visibility_level_filter
+    {
+      bool: {
+        must_not: field_exists('visibility_level'),
+        filter: issue_type_filter
+      }
+    }
+  end
+
+  def issue_type_filter
+    {
+      term: {
+        type: {
+          value: 'issue'
+        }
+      }
+    }
+  end
+
+  def field_exists(field)
+    {
+      exists: {
+        field: field
+      }
+    }
+  end
+end

ee/lib/elastic/migration.rb

@@ -2,6 +2,8 @@
 
 module Elastic
   class Migration
+    include Elastic::MigrationOptions
+
     attr_reader :version
 
     def initialize(version)

ee/spec/elastic/migrate/20201116142400_add_new_data_to_issues_documents_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require File.expand_path('ee/elastic/migrate/20201116142400_add_new_data_to_issues_documents.rb')
+
+RSpec.describe AddNewDataToIssuesDocuments, :elastic, :sidekiq_inline do
+  let(:version) { 20201116142400 }
+  let(:migration) { described_class.new(version) }
+  let(:issues) { create_list(:issue, 3) }
+
+  before do
+    stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
+
+    # ensure issues are indexed
+    issues
+
+    ensure_elasticsearch_index!
+  end
+
+  describe 'migration_options' do
+    it 'has migration options set', :aggregate_failures do
+      expect(migration.batched?).to be_truthy
+      expect(migration.throttle_delay).to eq(5.minutes)
+    end
+  end
+
+  describe '.migrate' do
+    subject { migration.migrate }
+
+    context 'when migration is already completed' do
+      before do
+        add_visibility_level_for_issues(issues)
+      end
+
+      it 'does not modify data', :aggregate_failures do
+        expect(::Elastic::ProcessBookkeepingService).not_to receive(:track!)
+
+        subject
+      end
+    end
+
+    context 'migration process' do
+      before do
+        remove_visibility_level_for_issues(issues)
+      end
+
+      it 'updates all issue documents' do
+        # track calls are batched in groups of 100
+        expect(::Elastic::ProcessBookkeepingService).to receive(:track!).once do |*tracked_refs|
+          expect(tracked_refs.count).to eq(3)
+        end
+
+        subject
+      end
+
+      it 'only updates issue documents missing visibility_level', :aggregate_failures do
+        issue = issues.first
+        add_visibility_level_for_issues(issues[1..-1])
+
+        expected = [Gitlab::Elastic::DocumentReference.new(Issue, issue.id, issue.es_id, issue.es_parent)]
+        expect(::Elastic::ProcessBookkeepingService).to receive(:track!).with(*expected).once
+
+        subject
+      end
+
+      it 'processes in batches', :aggregate_failures do
+        stub_const("#{described_class}::QUERY_BATCH_SIZE", 2)
+        stub_const("#{described_class}::UPDATE_BATCH_SIZE", 1)
+
+        expect(::Elastic::ProcessBookkeepingService).to receive(:track!).exactly(3).times.and_call_original
+
+        # cannot use subject in spec because it is memoized
+        migration.migrate
+
+        ensure_elasticsearch_index!
+
+        migration.migrate
+      end
+    end
+  end
+
+  describe '.completed?' do
+    subject { migration.completed? }
+
+    context 'when documents are missing visibility_level' do
+      before do
+        remove_visibility_level_for_issues(issues)
+      end
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when no documents are missing visibility_level' do
+      before do
+        add_visibility_level_for_issues(issues)
+      end
+
+      it { is_expected.to be_truthy }
+    end
+  end
+
+  private
+
+  def add_visibility_level_for_issues(issues)
+    script =  {
+      source: "ctx._source['visibility_level'] = params.visibility_level;",
+      lang: "painless",
+      params: {
+        visibility_level: Gitlab::VisibilityLevel::PRIVATE
+      }
+    }
+
+    update_by_query(issues, script)
+  end
+
+  def remove_visibility_level_for_issues(issues)
+    script =  {
+      source: "ctx._source.remove('visibility_level')"
+    }
+
+    update_by_query(issues, script)
+  end
+
+  def update_by_query(issues, script)
+    issue_ids = issues.map { |i| i.id }
+
+    client = Issue.__elasticsearch__.client
+    client.update_by_query({
+      index: Issue.__elasticsearch__.index_name,
+      wait_for_completion: true, # run synchronously
+      refresh: true, # make operation visible to search
+      body: {
+        script: script,
+        query: {
+          bool: {
+            must: [
+              {
+                terms: {
+                  id: issue_ids
+                }
+              },
+              {
+                term: {
+                  type: {
+                    value: 'issue'
+                  }
+                }
+              }
+            ]
+          }
+        }
+      }
+    })
+  end
+end

ee/spec/workers/concerns/elastic/migration_options_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Elastic::MigrationOptions do
+  let(:migration_class) do
+    Class.new do
+      include Elastic::MigrationOptions
+    end
+  end
+
+  describe '#batched?' do
+    subject { migration_class.new.batched? }
+
+    it 'defaults to false' do
+      expect(subject).to be_falsey
+    end
+
+    it 'respects when batched! is set for the class' do
+      migration_class.batched!
+
+      expect(subject).to be_truthy
+    end
+  end
+
+  describe '#throttle_delay' do
+    subject { migration_class.new.throttle_delay }
+
+    it 'has a default' do
+      expect(subject).to eq(described_class::DEFAULT_THROTTLE_DELAY)
+    end
+
+    it 'respects when throttle_delay is set for the class' do
+      migration_class.throttle_delay 30.seconds
+
+      expect(subject).to eq(30.seconds)
+    end
+  end
+end

ee/spec/workers/elastic/migration_worker_spec.rb

@@ -18,8 +18,12 @@ RSpec.describe Elastic::MigrationWorker, :elastic do
     end
 
     context 'indexing is enabled' do
+      let(:migration) { Elastic::DataMigrationService.migrations.first }
+
       before do
         stub_ee_application_setting(elasticsearch_indexing: true)
+
+        allow(subject).to receive(:current_migration).and_return(migration)
       end
 
       it 'creates an index if it does not exist' do
@@ -41,25 +45,28 @@ RSpec.describe Elastic::MigrationWorker, :elastic do
       end
 
       context 'migration process' do
-        let(:migration) { Elastic::DataMigrationService.migrations.first }
-
         before do
-          allow(subject).to receive(:current_migration).and_return(migration)
           allow(migration).to receive(:persisted?).and_return(persisted)
           allow(migration).to receive(:completed?).and_return(completed)
+          allow(migration).to receive(:batched?).and_return(batched)
         end
 
         using RSpec::Parameterized::TableSyntax
 
-        where(:persisted, :completed, :execute_migration) do
-          false | false | true
-          false | true  | true
-          true  | false | false
-          true  | true  | false
+        # completed is evaluated after migrate method is executed
+        where(:persisted, :completed, :execute_migration, :batched) do
+          false | false | true  | false
+          false | true  | true  | false
+          false | false | true  | true
+          false | true  | true  | true
+          true  | false | false | false
+          true  | true  | false | false
+          true  | false | true  | true
+          true  | true  | true | true
         end
 
         with_them do
-          it 'calls migration only when needed' do
+          it 'calls migration only when needed', :aggregate_failures do
             if execute_migration
               expect(migration).to receive(:migrate).once
             else
@@ -71,6 +78,18 @@ RSpec.describe Elastic::MigrationWorker, :elastic do
 
             subject.perform
           end
+
+          it 'handles batched migrations' do
+            if batched && !completed
+              # default throttle_delay is 5.minutes
+              expect( Elastic::MigrationWorker).to receive(:perform_in)
+                .with(5.minutes)
+            else
+              expect( Elastic::MigrationWorker).not_to receive(:perform_in)
+            end
+
+            subject.perform
+          end
         end
       end
     end

lib/gitlab/ci/templates/Security/SAST.gitlab-ci.yml

@@ -173,7 +173,7 @@ nodejs-scan-sast:
     - if: $CI_COMMIT_BRANCH &&
           $SAST_DEFAULT_ANALYZERS =~ /nodejs-scan/
       exists:
-        - 'package.json'
+        - '**/package.json'
 
 phpcs-security-audit-sast:
   extends: .sast-analyzer

lib/quality/test_level.rb

@@ -21,6 +21,8 @@ module Quality
         config
         db
         dependencies
+        elastic
+        elastic_integration
         factories
         finders
         frontend
@@ -46,7 +48,6 @@ module Quality
         validators
         views
         workers
-        elastic_integration
         tooling
       ],
       integration: %w[

locale/gitlab.pot

@@ -13241,9 +13241,6 @@ msgstr ""
 msgid "Group by"
 msgstr ""
 
-msgid "Group description"
-msgstr ""
-
 msgid "Group description (optional)"
 msgstr ""
 

qa/qa/page/group/new.rb

@@ -7,7 +7,6 @@ module QA
         view 'app/views/shared/_group_form.html.haml' do
           element :group_path_field, 'text_field :path' # rubocop:disable QA/ElementWithPattern
           element :group_name_field, 'text_field :name' # rubocop:disable QA/ElementWithPattern
-          element :group_description_field, 'text_area :description' # rubocop:disable QA/ElementWithPattern
         end
 
         view 'app/views/groups/_new_group_fields.html.haml' do
@@ -20,10 +19,6 @@ module QA
           fill_in 'group_name', with: path
         end
 
-        def set_description(description)
-          fill_in 'group_description', with: description
-        end
-
         def set_visibility(visibility)
           choose visibility
         end

qa/qa/page/project/pipeline/index.rb

@@ -31,6 +31,8 @@ module QA
           end
 
           def wait_for_latest_pipeline_status
+            wait_until(max_duration: 30, reload: true, sleep_interval: 5) { has_pipeline? }
+
             wait_until(reload: false, max_duration: 360) do
               within_element_by_index(:pipeline_commit_status, 0) { yield }
             end

qa/qa/resource/group.rb

@@ -36,7 +36,6 @@ module QA
 
             Page::Group::New.perform do |group_new|
               group_new.set_path(path)
-              group_new.set_description(description)
               group_new.set_visibility('Public')
               group_new.create
             end

qa/qa/resource/sandbox.rb

@@ -31,7 +31,6 @@ module QA
 
             Page::Group::New.perform do |group|
               group.set_path(path)
-              group.set_description('GitLab QA Sandbox Group')
               group.set_visibility('Public')
               group.create
             end

spec/features/admin/admin_groups_spec.rb

@@ -7,9 +7,10 @@ RSpec.describe 'Admin Groups' do
   include Spec::Support::Helpers::Features::MembersHelpers
 
   let(:internal) { Gitlab::VisibilityLevel::INTERNAL }
-  let(:user) { create :user }
-  let!(:group) { create :group }
-  let!(:current_user) { create(:admin) }
+
+  let_it_be(:user) { create :user }
+  let_it_be(:group) { create :group }
+  let_it_be(:current_user) { create(:admin) }
 
   before do
     sign_in(current_user)
@@ -26,6 +27,17 @@ RSpec.describe 'Admin Groups' do
   end
 
   describe 'create a group' do
+    describe 'with expected fields' do
+      it 'renders from as expected', :aggregate_failures do
+        visit new_admin_group_path
+
+        expect(page).to have_field('name')
+        expect(page).to have_field('group_path')
+        expect(page).to have_field('group_visibility_level_0')
+        expect(page).to have_field('description')
+      end
+    end
+
     it 'creates new group' do
       visit admin_groups_path
 

spec/features/dashboard/group_spec.rb

@@ -17,14 +17,11 @@ RSpec.describe 'Dashboard Group' do
     visit dashboard_groups_path
     find('.btn-success').click
     new_name = 'Samurai'
-    new_description = 'Tokugawa Shogunate'
 
     fill_in 'group_name', with: new_name
-    fill_in 'group_description', with: new_description
     click_button 'Create group'
 
     expect(current_path).to eq group_path(Group.find_by(name: new_name))
     expect(page).to have_content(new_name)
-    expect(page).to have_content(new_description)
   end
 end

spec/features/groups_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe 'Group' do
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
 
   before do
     sign_in(user)
@@ -36,6 +36,15 @@ RSpec.describe 'Group' do
       end
     end
 
+    describe 'with expected fields' do
+      it 'renders from as expected', :aggregate_failures do
+        expect(page).to have_field('name')
+        expect(page).to have_field('group_path')
+        expect(page).to have_field('group_visibility_level_0')
+        expect(page).not_to have_field('description')
+      end
+    end
+
     describe 'with space in group path' do
       it 'renders new group form with validation errors' do
         fill_in 'Group URL', with: 'space group'
@@ -135,7 +144,7 @@ RSpec.describe 'Group' do
   end
 
   describe 'create a nested group', :js do
-    let(:group) { create(:group, path: 'foo') }
+    let_it_be(:group) { create(:group, path: 'foo') }
 
     context 'as admin' do
       let(:user) { create(:admin) }
@@ -185,8 +194,8 @@ RSpec.describe 'Group' do
   end
 
   describe 'group edit', :js do
-    let(:group) { create(:group, :public) }
-    let(:path)  { edit_group_path(group) }
+    let_it_be(:group) { create(:group, :public) }
+    let(:path) { edit_group_path(group) }
     let(:new_name) { 'new-name' }
 
     before do
@@ -230,8 +239,8 @@ RSpec.describe 'Group' do
   end
 
   describe 'group page with markdown description' do
-    let(:group) { create(:group) }
-    let(:path)  { group_path(group) }
+    let_it_be(:group) { create(:group) }
+    let(:path) { group_path(group) }
 
     before do
       group.add_owner(user)
@@ -271,9 +280,9 @@ RSpec.describe 'Group' do
   end
 
   describe 'group page with nested groups', :js do
-    let!(:group) { create(:group) }
-    let!(:nested_group) { create(:group, parent: group) }
-    let!(:project) { create(:project, namespace: group) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:nested_group) { create(:group, parent: group) }
+    let_it_be(:project) { create(:project, namespace: group) }
 
     before do
       group.add_owner(user)
@@ -304,7 +313,11 @@ RSpec.describe 'Group' do
   end
 
   describe 'new subgroup / project button' do
-    let(:group) { create(:group, project_creation_level: Gitlab::Access::NO_ONE_PROJECT_ACCESS, subgroup_creation_level: Gitlab::Access::OWNER_SUBGROUP_ACCESS) }
+    let_it_be(:group, reload: true) do
+      create(:group,
+             project_creation_level: Gitlab::Access::NO_ONE_PROJECT_ACCESS,
+             subgroup_creation_level: Gitlab::Access::OWNER_SUBGROUP_ACCESS)
+    end
 
     before do
       group.add_owner(user)
@@ -341,6 +354,7 @@ RSpec.describe 'Group' do
     context 'when user has project and subgroup creation permissions' do
       it 'displays "New subgroup" and "New project" buttons' do
         group.update!(project_creation_level: Gitlab::Access::MAINTAINER_PROJECT_ACCESS)
+
         visit group_path(group)
 
         page.within '[data-testid="group-buttons"]' do

spec/lib/quality/test_level_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe Quality::TestLevel do
     context 'when level is unit' do
       it 'returns a pattern' do
         expect(subject.pattern(:unit))
-          .to eq("spec/{bin,channels,config,db,dependencies,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,models,policies,presenters,rack_servers,replicators,routing,rubocop,serializers,services,sidekiq,support_specs,tasks,uploaders,validators,views,workers,elastic_integration,tooling}{,/**/}*_spec.rb")
+          .to eq("spec/{bin,channels,config,db,dependencies,elastic,elastic_integration,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,models,policies,presenters,rack_servers,replicators,routing,rubocop,serializers,services,sidekiq,support_specs,tasks,uploaders,validators,views,workers,tooling}{,/**/}*_spec.rb")
       end
     end
 
@@ -103,7 +103,7 @@ RSpec.describe Quality::TestLevel do
     context 'when level is unit' do
       it 'returns a regexp' do
         expect(subject.regexp(:unit))
-          .to eq(%r{spec/(bin|channels|config|db|dependencies|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|models|policies|presenters|rack_servers|replicators|routing|rubocop|serializers|services|sidekiq|support_specs|tasks|uploaders|validators|views|workers|elastic_integration|tooling)})
+          .to eq(%r{spec/(bin|channels|config|db|dependencies|elastic|elastic_integration|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|models|policies|presenters|rack_servers|replicators|routing|rubocop|serializers|services|sidekiq|support_specs|tasks|uploaders|validators|views|workers|tooling)})
       end
     end