From 0ecdcf59f4342de55a7d2c3ee4ac0d9c3b116aa5 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 20 Nov 2019 03:06:28 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- app/models/ci/build.rb | 3 ++- app/models/ci/persistent_ref.rb | 12 ++++++++- app/models/merge_request_diff.rb | 3 ++- ...pty-diffs-for-external-diffs-migration.yml | 5 ++++ ...-persistent-ref-outside-of-transaction.yml | 5 ++++ doc/ci/pipelines.md | 12 +++++++++ doc/development/fe_guide/frontend_faq.md | 27 +++++++++++++++++-- doc/user/project/clusters/serverless/aws.md | 2 +- spec/models/ci/build_spec.rb | 16 ++++++++--- spec/models/ci/persistent_ref_spec.rb | 12 +++++++++ spec/models/merge_request_diff_spec.rb | 6 +++++ spec/requests/api/runner_spec.rb | 10 +++++++ 12 files changed, 104 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/36646-ignore-empty-diffs-for-external-diffs-migration.yml create mode 100644 changelogs/unreleased/create-persistent-ref-outside-of-transaction.yml diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 59a2c09bd28..59bff4e2d2b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -247,10 +247,11 @@ module Ci end after_transition pending: :running do |build| - build.pipeline.persistent_ref.create build.deployment&.run build.run_after_commit do + build.pipeline.persistent_ref.create + BuildHooksWorker.perform_async(id) end end diff --git a/app/models/ci/persistent_ref.rb b/app/models/ci/persistent_ref.rb index be3d4aa3203..54c3135ac9e 100644 --- a/app/models/ci/persistent_ref.rb +++ b/app/models/ci/persistent_ref.rb @@ -14,13 +14,15 @@ module Ci delegate :ref_exists?, :create_ref, :delete_refs, to: :repository def exist? + return unless enabled? + ref_exists?(path) rescue false end def create - return if exist? + return unless enabled? && !exist? create_ref(sha, path) rescue => e @@ -29,6 +31,8 @@ module Ci end def delete + return unless enabled? + delete_refs(path) rescue Gitlab::Git::Repository::NoRepository # no-op @@ -40,5 +44,11 @@ module Ci def path "refs/#{Repository::REF_PIPELINES}/#{pipeline.id}" end + + private + + def enabled? + Feature.enabled?(:depend_on_persistent_pipeline_ref, project, default_enabled: true) + end end end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 70ce4df5678..e91a529fecc 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -49,13 +49,14 @@ class MergeRequestDiff < ApplicationRecord scope :by_commit_sha, ->(sha) do joins(:merge_request_diff_commits).where(merge_request_diff_commits: { sha: sha }).reorder(nil) end + scope :has_diff_files, -> { where(id: MergeRequestDiffFile.select(:merge_request_diff_id)) } scope :by_project_id, -> (project_id) do joins(:merge_request).where(merge_requests: { target_project_id: project_id }) end scope :recent, -> { order(id: :desc).limit(100) } - scope :files_in_database, -> { where(stored_externally: [false, nil]) } + scope :files_in_database, -> { has_diff_files.where(stored_externally: [false, nil]) } scope :not_latest_diffs, -> do merge_requests = MergeRequest.arel_table diff --git a/changelogs/unreleased/36646-ignore-empty-diffs-for-external-diffs-migration.yml b/changelogs/unreleased/36646-ignore-empty-diffs-for-external-diffs-migration.yml new file mode 100644 index 00000000000..7633f562bc4 --- /dev/null +++ b/changelogs/unreleased/36646-ignore-empty-diffs-for-external-diffs-migration.yml @@ -0,0 +1,5 @@ +--- +title: Ignore empty MR diffs when migrating to external storage +merge_request: 20296 +author: +type: fixed diff --git a/changelogs/unreleased/create-persistent-ref-outside-of-transaction.yml b/changelogs/unreleased/create-persistent-ref-outside-of-transaction.yml new file mode 100644 index 00000000000..c268f376fa7 --- /dev/null +++ b/changelogs/unreleased/create-persistent-ref-outside-of-transaction.yml @@ -0,0 +1,5 @@ +--- +title: Move persistent_ref.create into run_after_commit +merge_request: 20422 +author: +type: fixed diff --git a/doc/ci/pipelines.md b/doc/ci/pipelines.md index 590a02b306c..c4cca944804 100644 --- a/doc/ci/pipelines.md +++ b/doc/ci/pipelines.md @@ -468,3 +468,15 @@ To illustrate its life cycle: even if the commit history of the `example` branch has been overwritten by force-push. 1. GitLab Runner fetches the persistent pipeline ref and gets source code from the checkout-SHA. 1. When the pipeline finished, its persistent ref is cleaned up in a background process. + +NOTE: **NOTE**: At this moment, this feature is on by default and can be manually disabled +by disabling `depend_on_persistent_pipeline_ref` feature flag. If you're interested in +manually disabling this behavior, please ask the administrator +to execute the following commands in rails console. + +```shell +> sudo gitlab-rails console # Login to Rails console of GitLab instance. +> project = Project.find_by_full_path('namespace/project-name') # Get the project instance. +> Feature.disable(:depend_on_persistent_pipeline_ref, project) # Disable the feature flag for specific project +> Feature.disable(:depend_on_persistent_pipeline_ref) # Disable the feature flag system-wide +``` diff --git a/doc/development/fe_guide/frontend_faq.md b/doc/development/fe_guide/frontend_faq.md index 36c8a03a241..aaa2fb0246b 100644 --- a/doc/development/fe_guide/frontend_faq.md +++ b/doc/development/fe_guide/frontend_faq.md @@ -15,7 +15,7 @@ ## FAQ -### How do I find the Rails route for a page? +### 1. How do I find the Rails route for a page? #### Check the 'page' data attribute @@ -37,7 +37,7 @@ The output includes the request types available, route parameters and the releva bundle exec rake routes | grep "issues" ``` -### `modal_copy_button` vs `clipboard_button` +### 2. `modal_copy_button` vs `clipboard_button` The `clipboard_button` uses the `copy_to_clipboard.js` behaviour, which is initialized on page load, so if there are vue-based clipboard buttons that @@ -50,3 +50,26 @@ the instance of that component, which means that clipboard events are bound on mounting and destroyed when the button is, mitigating the above issue. It also has bindings to a particular container or modal ID available, to work with the focus trap created by our GlModal. + +### 3. A gitlab-ui component not conforming to [Pajamas Design System](https://design.gitlab.com/) + +Some [Pajamas Design System](https://design.gitlab.com/) components implemented in +gitlab-ui do not conform with the design system specs because they lack some +planned features or are not correctly styled yet. In the Pajamas website, a +banner on top of the component examples indicates that: + +> This component does not yet conform to the correct styling defined in our Design +> System. Refer to the Design System documentation when referencing visuals for this +> component. + +For example, at the time of writing, this type of warning can be observed for +[all form components](https://design.gitlab.com/components/forms). It, however, +doesn't imply that the component should not be used. + +GitLab always asks to use `` components whenever a suitable component exists. +It makes codebase unified and more comfortable to maintain/refactor in the future. + +Ensure a [Product Designer](https://about.gitlab.com/company/team/?department=ux-department) +reviews the use of the non-conforming component as part of the MR review. Make a +follow up issue and attach it to the component implementation epic found within the +[Components of Pajamas Design System epic](https://gitlab.com/groups/gitlab-org/-/epics/973). diff --git a/doc/user/project/clusters/serverless/aws.md b/doc/user/project/clusters/serverless/aws.md index 2c16748a3ee..f1a9f156d7f 100644 --- a/doc/user/project/clusters/serverless/aws.md +++ b/doc/user/project/clusters/serverless/aws.md @@ -119,7 +119,7 @@ In order to interact with your AWS account, the .gitlab-ci.yml requires both `AW For more information please see: NOTE: **Note:** - The AWS credentials you provide must include IAM policies that provision correct access control to AWS Lambda, API Gateway, and CloudFormation resources. + The AWS credentials you provide must include IAM policies that provision correct access control to AWS Lambda, API Gateway, CloudFormation, and IAM resources. ### Deploying your function diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 24fa3b9b1ea..cc713443a0a 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -3086,10 +3086,20 @@ describe Ci::Build do rescue StateMachines::InvalidTransition end - it 'ensures pipeline ref existence' do - expect(job.pipeline.persistent_ref).to receive(:create).once + context 'for pipeline ref existence' do + it 'ensures pipeline ref creation' do + expect(job.pipeline.persistent_ref).to receive(:create).once - run_job_without_exception + run_job_without_exception + end + + it 'ensures that it is not run in database transaction' do + expect(job.pipeline.persistent_ref).to receive(:create) do + expect(Gitlab::Database).not_to be_inside_transaction + end + + run_job_without_exception + end end shared_examples 'saves data on transition' do diff --git a/spec/models/ci/persistent_ref_spec.rb b/spec/models/ci/persistent_ref_spec.rb index be447476e2c..71e0b03dff9 100644 --- a/spec/models/ci/persistent_ref_spec.rb +++ b/spec/models/ci/persistent_ref_spec.rb @@ -45,6 +45,18 @@ describe Ci::PersistentRef do expect(pipeline.persistent_ref).to be_exist end + context 'when depend_on_persistent_pipeline_ref feature flag is disabled' do + before do + stub_feature_flags(depend_on_persistent_pipeline_ref: false) + end + + it 'does not create a persistent ref' do + expect(project.repository).not_to receive(:create_ref) + + subject + end + end + context 'when sha does not exist in the repository' do let(:sha) { 'not-exist' } diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index 0f7f68e0b38..b5404658a69 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -98,6 +98,12 @@ describe MergeRequestDiff do end it { is_expected.to contain_exactly(outdated.id, latest.id, closed.id, merged.id, closed_recently.id, merged_recently.id) } + + it 'ignores diffs with 0 files' do + MergeRequestDiffFile.where(merge_request_diff_id: [closed_recently.id, merged_recently.id]).delete_all + + is_expected.to contain_exactly(outdated.id, latest.id, closed.id, merged.id) + end end context 'external diffs are enabled for outdated diffs' do diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 6138036b0af..cc6cadb190a 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -513,6 +513,16 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(json_response['features']).to eq(expected_features) end + it 'creates persistent ref' do + expect_any_instance_of(Ci::PersistentRef).to receive(:create_ref) + .with(job.sha, "refs/#{Repository::REF_PIPELINES}/#{job.commit_id}") + + request_job info: { platform: :darwin } + + expect(response).to have_gitlab_http_status(201) + expect(json_response['id']).to eq(job.id) + end + context 'when job is made for tag' do let!(:job) { create(:ci_build, :tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }