diff options
23 files changed, 288 insertions, 71 deletions
diff --git a/app/assets/javascripts/vue_shared/components/file_row.vue b/app/assets/javascripts/vue_shared/components/file_row.vue index e3a606571c0..0a5cc7b693c 100644 --- a/app/assets/javascripts/vue_shared/components/file_row.vue +++ b/app/assets/javascripts/vue_shared/components/file_row.vue @@ -111,6 +111,7 @@ export default { v-else :class="fileClass" :title="textForTitle" + :data-level="level" class="file-row" role="button" @click="clickFile" diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 3ce44a066ae..8a3ca2e758c 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -736,6 +736,7 @@ module Ci MergeRequest.where(id: merge_request_id) else MergeRequest.where(source_project_id: project_id, source_branch: ref) + .by_commit_sha(sha) end end diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 30dfcc11417..1a85289a04f 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -153,6 +153,6 @@ class PrometheusService < MonitoringService def create_default_alerts return unless project_id - Prometheus::CreateDefaultAlertsWorker.perform_async(project_id: project_id) + Prometheus::CreateDefaultAlertsWorker.perform_async(project_id) end end diff --git a/changelogs/unreleased/filter-pipeline-merge-requests-by-sha.yml b/changelogs/unreleased/filter-pipeline-merge-requests-by-sha.yml new file mode 100644 index 00000000000..3dc1fbfac02 --- /dev/null +++ b/changelogs/unreleased/filter-pipeline-merge-requests-by-sha.yml @@ -0,0 +1,5 @@ +--- +title: Prevent false positives in Ci::Pipeline#all_merge_requests +merge_request: 28800 +author: +type: fixed diff --git a/doc/administration/geo/replication/version_specific_updates.md b/doc/administration/geo/replication/version_specific_updates.md index a697d07ded4..3c047c8374a 100644 --- a/doc/administration/geo/replication/version_specific_updates.md +++ b/doc/administration/geo/replication/version_specific_updates.md @@ -4,6 +4,13 @@ Check this document if it includes instructions for the version you are updating These steps go together with the [general steps](updating_the_geo_nodes.md#general-update-steps) for updating Geo nodes. +## Updating to GitLab 12.9 + +CAUTION: **Warning:** +GitLab 12.9.0 through GitLab 12.9.3 are affected by [a bug that stops +repository verification](https://gitlab.com/gitlab-org/gitlab/-/issues/213523). +The issue is fixed in GitLab 12.9.4. Please upgrade to GitLab 12.9.4 or later. + ## Updating to GitLab 12.7 DANGER: **Danger:** diff --git a/doc/development/documentation/site_architecture/index.md b/doc/development/documentation/site_architecture/index.md index bd870399978..56dd3821b1c 100644 --- a/doc/development/documentation/site_architecture/index.md +++ b/doc/development/documentation/site_architecture/index.md @@ -20,29 +20,27 @@ from where content is sourced, the `gitlab-docs` project, and the published outp ```mermaid graph LR - A[gitlab-foss/doc] - B[gitlab/doc] - C[gitlab-runner/docs] - D[omnibus-gitlab/doc] - E[charts/doc] - F[gitlab-docs] - A --> F - B --> F - C --> F - D --> F - E --> F - F -- Build pipeline --> G - G[docs.gitlab.com] - H[/ce/] - I[/ee/] - J[/runner/] - K[/omnibus/] - L[/charts/] - G --> H - G --> I - G --> J - G --> K - G --> L + A[gitlab/doc] + B[gitlab-runner/docs] + C[omnibus-gitlab/doc] + D[charts/doc] + E[gitlab-docs] + A --> E + B --> E + C --> E + D --> E + E -- Build pipeline --> F + F[docs.gitlab.com] + G[/ce/] + H[/ee/] + I[/runner/] + J[/omnibus/] + K[/charts/] + F --> H + F --> I + F --> J + F --> K + H -- symlink --> G ``` You will not find any GitLab docs content in the `gitlab-docs` repository. diff --git a/doc/development/elasticsearch.md b/doc/development/elasticsearch.md index feff0ba7c8a..b6f863b8bea 100644 --- a/doc/development/elasticsearch.md +++ b/doc/development/elasticsearch.md @@ -60,12 +60,15 @@ Please see the `sha_tokenizer` explanation later below for an example. #### `code_analyzer` -Used when indexing a blob's filename and content. Uses the `whitespace` tokenizer and the filters: `code`, `edgeNGram_filter`, `lowercase`, and `asciifolding` +Used when indexing a blob's filename and content. Uses the `whitespace` tokenizer and the filters: [`code`](#code), [`edgeNGram_filter`](#edgengram_filter), `lowercase`, and `asciifolding` The `whitespace` tokenizer was selected in order to have more control over how tokens are split. For example the string `Foo::bar(4)` needs to generate tokens like `Foo` and `bar(4)` in order to be properly searched. Please see the `code` filter for an explanation on how tokens are split. +NOTE: **Known Issues**: +Currently the [Elasticsearch code_analyzer doesn't account for all code cases](../integration/elasticsearch.md#known-issues). + #### `code_search_analyzer` Not directly used for indexing, but rather used to transform a search input. Uses the `whitespace` tokenizer and the `lowercase` and `asciifolding` filters. diff --git a/doc/integration/elasticsearch.md b/doc/integration/elasticsearch.md index 48f39ea4bc9..fe7c3855d9a 100644 --- a/doc/integration/elasticsearch.md +++ b/doc/integration/elasticsearch.md @@ -634,6 +634,14 @@ Here are some common pitfalls and how to overcome them: You probably have not used either `http://` or `https://` as part of your value in the **"URL"** field of the Elasticseach Integration Menu. Please make sure you are using either `http://` or `https://` in this field as the [Elasticsearch client for Go](https://github.com/olivere/elastic) that we are using [needs the prefix for the URL to be accepted as valid](https://github.com/olivere/elastic/commit/a80af35aa41856dc2c986204e2b64eab81ccac3a). Once you have corrected the formatting of the URL, delete the index (via the [dedicated Rake task](#gitlab-elasticsearch-rake-tasks)) and [reindex the content of your instance](#adding-gitlabs-data-to-the-elasticsearch-index). +### Known Issues + +- **[Elasticsearch `code_analyzer` doesn't account for all code cases](https://gitlab.com/gitlab-org/gitlab/issues/10693)** + + The `code_analyzer` pattern and filter configuration is being evaluated for improvement. We have noticed [several edge cases](https://gitlab.com/gitlab-org/gitlab/-/issues/10693#note_158382332) that are not returning expected search results due to our pattern and filter configuration. + + An improved strategy for the `code_analyzer` pattern and filters are being discussed in [issue 29443](https://gitlab.com/gitlab-org/gitlab/-/issues/29443). + ### Reverting to basic search Sometimes there may be issues with your Elasticsearch index data and as such diff --git a/doc/subscriptions/index.md b/doc/subscriptions/index.md index f3488b45bbb..12d5aa1e29f 100644 --- a/doc/subscriptions/index.md +++ b/doc/subscriptions/index.md @@ -448,7 +448,7 @@ of the group/namespace. You can [purchase additional CI minutes](#purchasing-add If you're using GitLab.com, you can purchase additional CI minutes so your pipelines won't be blocked after you have used all your CI minutes from your -main quota. Additional minutes: +main quota. You can find pricing for additional CI/CD minutes in the [GitLab Customers Portal](https://customers.gitlab.com/plans). Additional minutes: - Are only used once the shared quota included in your subscription runs out. - Roll over month to month. diff --git a/doc/user/application_security/dast/index.md b/doc/user/application_security/dast/index.md index 57d2a383768..c9c7129dd7b 100644 --- a/doc/user/application_security/dast/index.md +++ b/doc/user/application_security/dast/index.md @@ -148,6 +148,9 @@ The results will be saved as a that you can later download and analyze. Due to implementation limitations, we always take the latest DAST artifact available. +DANGER: **Danger:** +**DO NOT** run an authenticated scan against a production server. When an authenticated scan is run, it may perform *any* function that the authenticated user can. This includes modifying and deleting data, submitting forms, following links, and so on. Only run an authenticated scan against a test server. + ### Full scan DAST can be configured to perform [ZAP Full Scan](https://github.com/zaproxy/zaproxy/wiki/ZAP-Full-Scan), which diff --git a/lib/gitlab/set_cache.rb b/lib/gitlab/set_cache.rb index d1151a431bb..e891b805879 100644 --- a/lib/gitlab/set_cache.rb +++ b/lib/gitlab/set_cache.rb @@ -64,7 +64,9 @@ module Gitlab else redis.del(*keys) end - rescue ::Redis::CommandError + rescue ::Redis::CommandError => e + Gitlab::ErrorTracking.log_exception(e) + redis.del(*keys) end end diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb index e0478097148..257dd3337ba 100644 --- a/spec/factories/ci/pipelines.rb +++ b/spec/factories/ci/pipelines.rb @@ -5,7 +5,7 @@ FactoryBot.define do factory :ci_empty_pipeline, class: 'Ci::Pipeline' do source { :push } ref { 'master' } - sha { '97de212e80737a608d939f648d959671fb0a0142' } + sha { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' } status { 'pending' } add_attribute(:protected) { false } diff --git a/spec/features/dashboard/projects_spec.rb b/spec/features/dashboard/projects_spec.rb index 73f759f8a54..218cbf871a9 100644 --- a/spec/features/dashboard/projects_spec.rb +++ b/spec/features/dashboard/projects_spec.rb @@ -244,13 +244,15 @@ describe 'Dashboard Projects' do ActiveRecord::QueryRecorder.new { visit dashboard_projects_path }.count - # There are three known N+1 queries: + # There are seven known N+1 queries: https://gitlab.com/gitlab-org/gitlab/-/issues/214037 # 1. Project#open_issues_count # 2. Project#open_merge_requests_count # 3. Project#forks_count - # - # In addition, ProjectsHelper#load_pipeline_status also adds an - # additional query. - expect { visit dashboard_projects_path }.not_to exceed_query_limit(control_count + 4) + # 4. ProjectsHelper#load_pipeline_status + # 5. RendersMemberAccess#preload_max_member_access_for_collection + # 6. User#max_member_access_for_project_ids + # 7. CommitWithPipeline#last_pipeline + + expect { visit dashboard_projects_path }.not_to exceed_query_limit(control_count + 7) end end diff --git a/spec/features/ide/user_commits_changes_spec.rb b/spec/features/ide/user_commits_changes_spec.rb new file mode 100644 index 00000000000..f53abde1523 --- /dev/null +++ b/spec/features/ide/user_commits_changes_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'IDE user commits changes', :js do + include WebIdeSpecHelpers + + let(:project) { create(:project, :public, :repository) } + let(:user) { project.owner } + + before do + sign_in(user) + + ide_visit(project) + end + + it 'user updates nested files' do + content = <<~HEREDOC + Lorem ipsum + Dolar sit + Amit + HEREDOC + + ide_create_new_file('foo/bar/lorem_ipsum.md', content: content) + ide_delete_file('foo/bar/.gitkeep') + + ide_commit + + expect(page).to have_content('All changes are committed') + expect(project.repository.blob_at('master', 'foo/bar/.gitkeep')).to be_nil + expect(project.repository.blob_at('master', 'foo/bar/lorem_ipsum.md').data).to eql(content) + end +end diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index 561c0552007..e8846b5b617 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -133,15 +133,8 @@ describe 'Pipeline', :js do context 'when there are two related merge requests' do before do - create(:merge_request, - source_project: project, - source_branch: pipeline.ref, - target_branch: 'feature-1') - - create(:merge_request, - source_project: project, - source_branch: pipeline.ref, - target_branch: 'feature-2') + create(:merge_request, source_project: project, source_branch: pipeline.ref) + create(:merge_request, source_project: project, source_branch: pipeline.ref, target_branch: 'fix') end it 'links to the most recent related merge request' do diff --git a/spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb b/spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb index 02c6409a9a6..b894dce3e17 100644 --- a/spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb +++ b/spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb @@ -14,7 +14,7 @@ describe Resolvers::MergeRequestPipelinesResolver do sha: merge_request.diff_head_sha ) end - let_it_be(:other_project_pipeline) { create(:ci_pipeline, project: merge_request.source_project) } + let_it_be(:other_project_pipeline) { create(:ci_pipeline, project: merge_request.source_project, ref: 'other-ref') } let_it_be(:other_pipeline) { create(:ci_pipeline) } let(:current_user) { create(:user) } diff --git a/spec/lib/gitlab/repository_set_cache_spec.rb b/spec/lib/gitlab/repository_set_cache_spec.rb index 6221d6fb45f..b09194e7d0b 100644 --- a/spec/lib/gitlab/repository_set_cache_spec.rb +++ b/spec/lib/gitlab/repository_set_cache_spec.rb @@ -103,6 +103,12 @@ describe Gitlab::RepositorySetCache, :clean_gitlab_redis_cache do expect(cache.expire(:foo)).to eq(1) expect(cache.read(:foo)).to be_empty end + + it 'logs the failure' do + expect(Gitlab::ErrorTracking).to receive(:log_exception) + + cache.expire(:foo) + end end end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 844e50dbb58..90412136c1d 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -2367,18 +2367,31 @@ describe Ci::Pipeline, :mailer do end end - describe "#all_merge_requests" do + describe '#all_merge_requests' do let(:project) { create(:project) } shared_examples 'a method that returns all merge requests for a given pipeline' do let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: pipeline_project, ref: 'master') } - it "returns all merge requests having the same source branch" do + it 'returns all merge requests having the same source branch and the pipeline sha' do merge_request = create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: pipeline.ref) + create(:merge_request_diff, merge_request: merge_request).tap do |diff| + create(:merge_request_diff_commit, merge_request_diff: diff, sha: pipeline.sha) + end + expect(pipeline.all_merge_requests).to eq([merge_request]) end + it "doesn't return merge requests having the same source branch without the pipeline sha" do + merge_request = create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: pipeline.ref) + create(:merge_request_diff, merge_request: merge_request).tap do |diff| + create(:merge_request_diff_commit, merge_request_diff: diff, sha: 'unrelated') + end + + expect(pipeline.all_merge_requests).to be_empty + end + it "doesn't return merge requests having a different source branch" do create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: 'feature', target_branch: 'master') diff --git a/spec/models/project_services/prometheus_service_spec.rb b/spec/models/project_services/prometheus_service_spec.rb index 415d634d405..5565d30d8c1 100644 --- a/spec/models/project_services/prometheus_service_spec.rb +++ b/spec/models/project_services/prometheus_service_spec.rb @@ -133,7 +133,7 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do it 'creates default alerts' do expect(Prometheus::CreateDefaultAlertsWorker) .to receive(:perform_async) - .with(project_id: project.id) + .with(project.id) create_service end diff --git a/spec/presenters/ci/pipeline_presenter_spec.rb b/spec/presenters/ci/pipeline_presenter_spec.rb index 28eb6804703..e8b66682b97 100644 --- a/spec/presenters/ci/pipeline_presenter_spec.rb +++ b/spec/presenters/ci/pipeline_presenter_spec.rb @@ -236,7 +236,7 @@ describe Ci::PipelinePresenter do context 'for a branch pipeline with two open MRs' do let!(:one) { create(:merge_request, source_project: project, source_branch: pipeline.ref) } - let!(:two) { create(:merge_request, source_project: project, source_branch: pipeline.ref, target_branch: 'wip') } + let!(:two) { create(:merge_request, source_project: project, source_branch: pipeline.ref, target_branch: 'fix') } it { is_expected.to contain_exactly(one, two) } end diff --git a/spec/serializers/build_details_entity_spec.rb b/spec/serializers/build_details_entity_spec.rb index 15f605b183d..92917f6ea25 100644 --- a/spec/serializers/build_details_entity_spec.rb +++ b/spec/serializers/build_details_entity_spec.rb @@ -15,7 +15,7 @@ describe BuildDetailsEntity do let(:project) { create(:project, :repository) } let(:pipeline) { create(:ci_pipeline, project: project) } let(:build) { create(:ci_build, :failed, pipeline: pipeline) } - let(:request) { double('request') } + let(:request) { double('request', project: project) } let(:entity) do described_class.new(build, request: request, diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index a316c8a4219..bd50d6b1001 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -368,10 +368,12 @@ describe Issues::CreateService do end context 'checking spam' do + let(:title) { 'Legit issue' } + let(:description) { 'please fix' } let(:opts) do { - title: 'Awesome issue', - description: 'please fix', + title: title, + description: description, request: double(:request, env: {}) } end @@ -382,7 +384,7 @@ describe Issues::CreateService do context 'when recaptcha was verified' do let(:log_user) { user } - let(:spam_logs) { create_list(:spam_log, 2, user: log_user, title: 'Awesome issue') } + let(:spam_logs) { create_list(:spam_log, 2, user: log_user, title: title) } let(:target_spam_log) { spam_logs.last } before do @@ -396,7 +398,7 @@ describe Issues::CreateService do expect(issue).not_to be_spam end - it 'issue is valid ' do + it 'creates a valid issue' do expect(issue).to be_valid end @@ -405,14 +407,14 @@ describe Issues::CreateService do end it 'marks related spam_log as recaptcha_verified' do - expect { issue }.to change {SpamLog.last.recaptcha_verified}.from(false).to(true) + expect { issue }.to change { target_spam_log.reload.recaptcha_verified }.from(false).to(true) end context 'when spam log does not belong to a user' do let(:log_user) { create(:user) } it 'does not mark spam_log as recaptcha_verified' do - expect { issue }.not_to change {SpamLog.last.recaptcha_verified} + expect { issue }.not_to change { target_spam_log.reload.recaptcha_verified } end end end @@ -431,8 +433,8 @@ describe Issues::CreateService do end end - context 'when issuables_recaptcha_enabled feature flag is true' do - it 'marks an issue as spam' do + context 'when allow_possible_spam feature flag is false' do + it 'marks the issue as spam' do expect(issue).to be_spam end @@ -442,34 +444,26 @@ describe Issues::CreateService do it 'creates a new spam_log' do expect { issue } - .to have_spam_log(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue') - end - - it 'assigns a spam_log to the issue' do - expect(issue.spam_log).to eq(SpamLog.last) + .to have_spam_log(title: title, description: description, user_id: user.id, noteable_type: 'Issue') end end - context 'when issuable_recaptcha_enabled feature flag is false' do + context 'when allow_possible_spam feature flag is true' do before do stub_feature_flags(allow_possible_spam: true) end - it 'does not mark an issue as spam' do + it 'does not mark the issue as spam' do expect(issue).not_to be_spam end - it 'accepts the issue as valid' do + it 'creates a valid issue' do expect(issue).to be_valid end it 'creates a new spam_log' do expect { issue } - .to have_spam_log(title: issue.title, description: issue.description, user_id: user.id, noteable_type: 'Issue') - end - - it 'assigns a spam_log to an issue' do - expect(issue.spam_log).to eq(SpamLog.last) + .to have_spam_log(title: title, description: description, user_id: user.id, noteable_type: 'Issue') end end end @@ -485,8 +479,8 @@ describe Issues::CreateService do expect(issue).not_to be_spam end - it 'an issue is valid ' do - expect(issue.valid?).to be_truthy + it 'creates a valid issue' do + expect(issue).to be_valid end it 'does not assign a spam_log to an issue' do diff --git a/spec/support/helpers/features/web_ide_spec_helpers.rb b/spec/support/helpers/features/web_ide_spec_helpers.rb new file mode 100644 index 00000000000..37c8345a4e5 --- /dev/null +++ b/spec/support/helpers/features/web_ide_spec_helpers.rb @@ -0,0 +1,148 @@ +# frozen_string_literal: true + +# These helpers help you interact within the Web IDE. +# +# Usage: +# describe "..." do +# include WebIdeSpecHelpers +# ... +# +# ide_visit(project) +# ide_create_new_file('path/to/file.txt', content: 'Lorem ipsum') +# ide_commit +# +module WebIdeSpecHelpers + include ActionView::Helpers::JavaScriptHelper + + def ide_visit(project) + visit project_path(project) + + wait_for_requests + + click_link('Web IDE') + + wait_for_requests + end + + def ide_tree_body + page.find('.ide-tree-body') + end + + def ide_tree_actions + page.find('.ide-tree-actions') + end + + def ide_file_row_open?(row) + row.matches_css?('.is-open') + end + + # Creates a file in the IDE by expanding directories + # then using the dropdown next to the parent directory + # + # - Throws an error if the parent directory is not found + def ide_create_new_file(path, content: '') + parent_path = path.split('/')[0...-1].join('/') + + container = ide_traverse_to_file(parent_path) + + if container + click_file_action(container, 'New file') + else + ide_tree_actions.click_button('New file') + end + + within '#ide-new-entry' do + find('input').fill_in(with: path) + click_button('Create file') + end + + ide_set_editor_value(content) + end + + # Deletes a file by traversing to `path` + # then clicking the 'Delete' action. + # + # - Throws an error if the file is not found + def ide_delete_file(path) + container = ide_traverse_to_file(path) + + click_file_action(container, 'Delete') + end + + # Opens parent directories until the file at `path` + # is exposed. + # + # - Returns a reference to the file row at `path` + # - Throws an error if the file is not found + def ide_traverse_to_file(path) + paths = path.split('/') + container = nil + + paths.each_with_index do |path, index| + ide_open_file_row(container) if container + container = find_file_child(container, path, level: index) + end + + container + end + + def ide_open_file_row(row) + return if ide_file_row_open?(row) + + row.click + end + + def ide_set_editor_value(value) + editor = find('.monaco-editor') + uri = editor['data-uri'] + + execute_script("monaco.editor.getModel('#{uri}').setValue('#{escape_javascript(value)}')") + end + + def ide_editor_value + editor = find('.monaco-editor') + uri = editor['data-uri'] + + evaluate_script("monaco.editor.getModel('#{uri}').getValue()") + end + + def ide_commit + ide_switch_mode('commit') + + commit_to_current_branch + end + + def ide_switch_mode(mode) + find(".js-ide-#{mode}-mode").click + end + + private + + def file_row_container(row) + row ? row.find(:xpath, '..') : ide_tree_body + end + + def find_file_child(row, name, level: nil) + container = file_row_container(row) + container.find(".file-row[data-level=\"#{level}\"]", text: name) + end + + def click_file_action(row, text) + row.hover + dropdown = row.find('.ide-new-btn') + dropdown.find('button').click + dropdown.find('button', text: text).click + end + + def commit_to_current_branch(option: 'Commit to master branch', message: '') + within '.multi-file-commit-form' do + fill_in('commit-message', with: message) if message + + choose(option) + + click_button('Commit') + + wait_for_requests + end + end +end |