From aaae74c6c3099a7d590f8bd968ba06d371df533c Mon Sep 17 00:00:00 2001 From: Lyle Kozloff Date: Fri, 1 Dec 2017 15:27:56 +0000 Subject: Updating trouble shooting links --- doc/administration/raketasks/maintenance.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/administration/raketasks/maintenance.md b/doc/administration/raketasks/maintenance.md index 5b6ee354887..136192191f9 100644 --- a/doc/administration/raketasks/maintenance.md +++ b/doc/administration/raketasks/maintenance.md @@ -58,7 +58,9 @@ Runs the following rake tasks: It will check that each component was setup according to the installation guide and suggest fixes for issues found. -You may also have a look at our [Trouble Shooting Guide](https://github.com/gitlabhq/gitlab-public-wiki/wiki/Trouble-Shooting-Guide). +You may also have a look at our Trouble Shooting Guides: +- [Trouble Shooting Guide (GitLab)](http://docs.gitlab.com/ee/README.html#troubleshooting) +- [Trouble Shooting Guide (Omnibus Gitlab)](http://docs.gitlab.com/omnibus/README.html#troubleshooting) **Omnibus Installation** -- cgit v1.2.1 From 636376dd4d41856cc965718725f06bb8caacfd34 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 18 Sep 2017 11:15:33 +0200 Subject: WIP --- app/models/ci/artifact.rb | 12 ++++++++++++ db/migrate/20170918072948_create_artifacts.rb | 8 ++++++++ spec/models/ci/artifact_spec.rb | 5 +++++ 3 files changed, 25 insertions(+) create mode 100644 app/models/ci/artifact.rb create mode 100644 db/migrate/20170918072948_create_artifacts.rb create mode 100644 spec/models/ci/artifact_spec.rb diff --git a/app/models/ci/artifact.rb b/app/models/ci/artifact.rb new file mode 100644 index 00000000000..c66c560037e --- /dev/null +++ b/app/models/ci/artifact.rb @@ -0,0 +1,12 @@ +module Ci + class Artifact < ActiveRecord::Base + belongs_to :build, class_name: "Ci::Build" + belongs_to :project, class_name: "Ci::Build" + + enum type { + archive: 0, + metadata: 1, + trace: 2 + } + end +end diff --git a/db/migrate/20170918072948_create_artifacts.rb b/db/migrate/20170918072948_create_artifacts.rb new file mode 100644 index 00000000000..0b3241070ce --- /dev/null +++ b/db/migrate/20170918072948_create_artifacts.rb @@ -0,0 +1,8 @@ +class CreateArtifacts < ActiveRecord::Migration + def change + create_table :artifacts do |t| + + t.timestamps null: false + end + end +end diff --git a/spec/models/ci/artifact_spec.rb b/spec/models/ci/artifact_spec.rb new file mode 100644 index 00000000000..438964fd787 --- /dev/null +++ b/spec/models/ci/artifact_spec.rb @@ -0,0 +1,5 @@ +require 'rails_helper' + +RSpec.describe Artifact, type: :model do + pending "add some examples to (or delete) #{__FILE__}" +end -- cgit v1.2.1 From 8ac7f29726989bc0a20ee32780aa18625159f8b4 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 18 Sep 2017 15:33:24 +0200 Subject: Create ci_artifacts table --- db/migrate/20170918072948_create_artifacts.rb | 22 +++++++++++++++++++--- db/schema.rb | 16 ++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/db/migrate/20170918072948_create_artifacts.rb b/db/migrate/20170918072948_create_artifacts.rb index 0b3241070ce..dd0af2a7066 100644 --- a/db/migrate/20170918072948_create_artifacts.rb +++ b/db/migrate/20170918072948_create_artifacts.rb @@ -1,8 +1,24 @@ class CreateArtifacts < ActiveRecord::Migration - def change - create_table :artifacts do |t| + def up + create_table :ci_artifacts do |t| + t.belongs_to :project, null: false, foreign_key: { on_delete: :cascade } + t.belongs_to :ci_build, null: false, foreign_key: { on_delete: :cascade } + t.integer :size, limit: 8, default: 0 - t.timestamps null: false + t.datetime_with_timezone :created_at, null: false + t.datetime_with_timezone :updated_at, null: false + + t.datetime_with_timezone :expire_at + t.integer :erased_by_id, null: false + t.datetime_with_timezone :erased_at + + t.text :file end + + add_index(:ci_artifacts, [:project_id, :ci_build_id], unique: true) + end + + def down + drop_table(:ci_artifacts) end end diff --git a/db/schema.rb b/db/schema.rb index effb2604af2..b37e0eabbd6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -226,6 +226,20 @@ ActiveRecord::Schema.define(version: 20171124150326) do add_index "chat_teams", ["namespace_id"], name: "index_chat_teams_on_namespace_id", unique: true, using: :btree + create_table "ci_artifacts", force: :cascade do |t| + t.integer "project_id", null: false + t.integer "ci_build_id", null: false + t.integer "size", limit: 8, default: 0 + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.datetime "expire_at" + t.integer "erased_by_id", null: false + t.datetime "erased_at" + t.text "file" + end + + add_index "ci_artifacts", ["project_id", "ci_build_id"], name: "index_ci_artifacts_on_project_id_and_ci_build_id", unique: true, using: :btree + create_table "ci_build_trace_section_names", force: :cascade do |t| t.integer "project_id", null: false t.string "name", null: false @@ -1901,6 +1915,8 @@ ActiveRecord::Schema.define(version: 20171124150326) do add_foreign_key "boards", "projects", name: "fk_f15266b5f9", on_delete: :cascade add_foreign_key "chat_teams", "namespaces", on_delete: :cascade + add_foreign_key "ci_artifacts", "ci_builds", on_delete: :cascade + add_foreign_key "ci_artifacts", "projects", on_delete: :cascade add_foreign_key "ci_build_trace_section_names", "projects", on_delete: :cascade add_foreign_key "ci_build_trace_sections", "ci_build_trace_section_names", column: "section_name_id", name: "fk_264e112c66", on_delete: :cascade add_foreign_key "ci_build_trace_sections", "ci_builds", column: "build_id", name: "fk_4ebe41f502", on_delete: :cascade -- cgit v1.2.1 From 25df666156279e5b392b429519b4f4ba01eefaac Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 19 Sep 2017 09:14:06 +0200 Subject: Create Ci::Artifacts To allow jobs/builds to have multiple artifacts, and to start seperating concerns from Ci::Build a new model is created: Ci::Artifact. Changes include the updating of the ArtifactUploader to adapt to a slightly different interface. The uploader expects to be initialized with a `Ci::Build`. Futher a migration with the minimal fields, the needed foreign keys and an index. Last, the way this works is by prepending a module to Ci::Build so we can basically override behaviour but if needed use `super` to get the original behaviour. --- app/models/ci/artifact.rb | 28 +++++++++---- app/models/ci/build.rb | 23 ++++------ app/models/concerns/artifact_migratable.rb | 37 +++++++++++++++++ app/uploaders/artifact_uploader.rb | 4 ++ db/migrate/20170918072948_create_artifacts.rb | 5 +-- db/schema.rb | 8 ++-- lib/api/entities.rb | 2 +- spec/factories/ci/artifacts.rb | 22 ++++++++++ spec/factories/ci/builds.rb | 27 ++---------- spec/models/ci/artifact_spec.rb | 60 +++++++++++++++++++++++++-- spec/models/ci/build_spec.rb | 17 +++----- 11 files changed, 164 insertions(+), 69 deletions(-) create mode 100644 app/models/concerns/artifact_migratable.rb create mode 100644 spec/factories/ci/artifacts.rb diff --git a/app/models/ci/artifact.rb b/app/models/ci/artifact.rb index c66c560037e..858609883ce 100644 --- a/app/models/ci/artifact.rb +++ b/app/models/ci/artifact.rb @@ -1,12 +1,24 @@ module Ci class Artifact < ActiveRecord::Base - belongs_to :build, class_name: "Ci::Build" - belongs_to :project, class_name: "Ci::Build" - - enum type { - archive: 0, - metadata: 1, - trace: 2 - } + extend Gitlab::Ci::Model + + belongs_to :project + belongs_to :build, class_name: "Ci::Build", foreign_key: :ci_build_id + + before_save :set_size, if: :file_changed? + + mount_uploader :file, ArtifactUploader + + enum type: { archive: 0, metadata: 1 } + + # Allow us to use `type` as column name, without Rails thinking we're using + # STI: https://stackoverflow.com/a/29663933 + def self.inheritance_column + nil + end + + def set_size + self.size = file.exists? ? file.size : 0 + end end end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 4ea040dfad5..fae2f5590b4 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -1,5 +1,6 @@ module Ci class Build < CommitStatus + prepend ArtifactMigratable include TokenAuthenticatable include AfterCommitQueue include Presentable @@ -10,6 +11,7 @@ module Ci belongs_to :erased_by, class_name: 'User' has_many :deployments, as: :deployable + has_many :artifacts, class_name: 'Ci::Artifact', foreign_key: :ci_build_id has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' has_many :trace_sections, class_name: 'Ci::BuildTraceSection' @@ -326,14 +328,6 @@ module Ci project.running_or_pending_build_count(force: true) end - def artifacts? - !artifacts_expired? && artifacts_file.exists? - end - - def artifacts_metadata? - artifacts? && artifacts_metadata.exists? - end - def artifacts_metadata_entry(path, **options) metadata = Gitlab::Ci::Build::Artifacts::Metadata.new( artifacts_metadata.path, @@ -429,7 +423,7 @@ module Ci Gitlab::Ci::Build::Image.from_services(self) end - def artifacts + def artifacts_options [options[:artifacts]] end @@ -470,6 +464,12 @@ module Ci super(options).merge(when: read_attribute(:when)) end + def update_project_statistics + return unless project + + ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size]) + end + private def update_artifacts_size @@ -560,11 +560,6 @@ module Ci pipeline.config_processor.build_attributes(name) end - def update_project_statistics - return unless project - - ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size]) - end def update_project_statistics_after_save if previous_changes.include?('artifacts_size') diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb new file mode 100644 index 00000000000..a14a278df9f --- /dev/null +++ b/app/models/concerns/artifact_migratable.rb @@ -0,0 +1,37 @@ +# Adapter class to unify the interface between mounted uploaders and the +# Ci::Artifact model +# Meant to be prepended so the interface can stay the same +module ArtifactMigratable + def artifacts_file + artifacts.archive.first&.file || super + end + + def artifacts_metadata + artifacts.metadata.first&.file || super + end + + def artifacts? + byebug + !artifacts_expired? && artifacts_file.exists? + end + + def artifacts_metadata? + artifacts? && artifacts_metadata.exists? + end + + def remove_artifacts_file! + artifacts_file.remove! + end + + def remove_artifacts_metadata! + artifacts_metadata.remove! + end + + def artifacts_file=(file) + artifacts.create(project: project, type: :archive, file: file) + end + + def artifacts_metadata=(file) + artifacts.create(project: project, type: :metadata, file: file) + end +end diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb index 14addb6cf14..8ac0e2fe5a2 100644 --- a/app/uploaders/artifact_uploader.rb +++ b/app/uploaders/artifact_uploader.rb @@ -12,6 +12,10 @@ class ArtifactUploader < GitlabUploader end def initialize(job, field) + # Temporairy conditional, needed to move artifacts to their own table, + # but keeping compat with Ci::Build for the time being + job = job.build if job.respond_to?(:build) + @job, @field = job, field end diff --git a/db/migrate/20170918072948_create_artifacts.rb b/db/migrate/20170918072948_create_artifacts.rb index dd0af2a7066..1dd5edc3f15 100644 --- a/db/migrate/20170918072948_create_artifacts.rb +++ b/db/migrate/20170918072948_create_artifacts.rb @@ -3,15 +3,12 @@ class CreateArtifacts < ActiveRecord::Migration create_table :ci_artifacts do |t| t.belongs_to :project, null: false, foreign_key: { on_delete: :cascade } t.belongs_to :ci_build, null: false, foreign_key: { on_delete: :cascade } + t.integer :type, default: 0, null: false t.integer :size, limit: 8, default: 0 t.datetime_with_timezone :created_at, null: false t.datetime_with_timezone :updated_at, null: false - t.datetime_with_timezone :expire_at - t.integer :erased_by_id, null: false - t.datetime_with_timezone :erased_at - t.text :file end diff --git a/db/schema.rb b/db/schema.rb index b37e0eabbd6..b4048371676 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -229,12 +229,10 @@ ActiveRecord::Schema.define(version: 20171124150326) do create_table "ci_artifacts", force: :cascade do |t| t.integer "project_id", null: false t.integer "ci_build_id", null: false + t.integer "type", default: 0, null: false t.integer "size", limit: 8, default: 0 - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.datetime "expire_at" - t.integer "erased_by_id", null: false - t.datetime "erased_at" + t.datetime_with_timezone "created_at", null: false + t.datetime_with_timezone "updated_at", null: false t.text "file" end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index ce332fe85d2..9eb2c98c436 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1036,7 +1036,7 @@ module API expose :steps, using: Step expose :image, using: Image expose :services, using: Service - expose :artifacts, using: Artifacts + expose :artifacts_options, as: :artifacts, using: Artifacts expose :cache, using: Cache expose :credentials, using: Credentials expose :dependencies, using: Dependency diff --git a/spec/factories/ci/artifacts.rb b/spec/factories/ci/artifacts.rb new file mode 100644 index 00000000000..085e707d686 --- /dev/null +++ b/spec/factories/ci/artifacts.rb @@ -0,0 +1,22 @@ +include ActionDispatch::TestProcess + +FactoryGirl.define do + factory :artifact, class: Ci::Artifact do + project + build factory: :ci_build + + after :create do |artifact| + artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') + artifact.save + end + + factory :artifact_metadata do + type :metadata + + after :create do |artifact| + artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), 'application/x-gzip') + artifact.save + end + end + end +end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index cf38066dedc..69d58f367ac 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -156,32 +156,13 @@ FactoryGirl.define do trait :artifacts do after(:create) do |build, _| - build.artifacts_file = - fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), - 'application/zip') - - build.artifacts_metadata = - fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), - 'application/x-gzip') - - build.save! + create(:artifact, build: build) + create(:artifact_metadata, build: build) end end - trait :artifacts_expired do - after(:create) do |build, _| - build.artifacts_file = - fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), - 'application/zip') - - build.artifacts_metadata = - fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), - 'application/x-gzip') - - build.artifacts_expire_at = 1.minute.ago - - build.save! - end + trait :expired do + artifacts_expire_at = 1.minute.ago end trait :with_commit do diff --git a/spec/models/ci/artifact_spec.rb b/spec/models/ci/artifact_spec.rb index 438964fd787..5e39f73763b 100644 --- a/spec/models/ci/artifact_spec.rb +++ b/spec/models/ci/artifact_spec.rb @@ -1,5 +1,59 @@ -require 'rails_helper' +require 'spec_helper' -RSpec.describe Artifact, type: :model do - pending "add some examples to (or delete) #{__FILE__}" +describe Ci::Artifact do + it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:build) } + + it { is_expected.to respond_to(:file) } + it { is_expected.to respond_to(:created_at) } + it { is_expected.to respond_to(:updated_at) } + it { is_expected.to respond_to(:type) } + + let(:artifact) { create(:artifact) } + + describe '#type' do + it 'defaults to archive' do + expect(artifact.type).to eq("archive") + end + end + + describe '#set_size' do + it 'sets the size' do + expect(artifact.size).to eq(106365) + end + end + + describe '#file' do + subject { artifact.file } + + context 'the uploader api' do + it { is_expected.to respond_to(:store_dir) } + it { is_expected.to respond_to(:cache_dir) } + it { is_expected.to respond_to(:work_dir) } + end + end + + describe '#remove_file' do + it 'removes the file from the database' do + artifact.remove_file! + + expect(artifact.file.exists?).to be_falsy + end + end + + describe '#exists?' do + context 'when the artifact file is present' do + it 'is returns true' do + expect(artifact.exists?).to be(true) + end + end + + context 'when the file has been removed' do + it 'does not exist' do + artifact.remove_file! + + expect(artifact.exists?).to be_falsy + end + end + end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 1795ee8e9a4..52a3732d0cd 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -23,6 +23,8 @@ describe Ci::Build do it { is_expected.to respond_to(:has_trace?) } it { is_expected.to respond_to(:trace) } + it { is_expected.to be_a(ArtifactMigratable) } + describe 'callbacks' do context 'when running after_create callback' do it 'triggers asynchronous build hooks worker' do @@ -130,33 +132,26 @@ describe Ci::Build do end describe '#artifacts?' do + let(:build) { create(:ci_build, :artifacts) } + subject { build.artifacts? } context 'artifacts archive does not exist' do - before do - build.update_attributes(artifacts_file: nil) - end + let(:build) { create(:ci_build) } it { is_expected.to be_falsy } end context 'artifacts archive exists' do - let(:build) { create(:ci_build, :artifacts) } it { is_expected.to be_truthy } context 'is expired' do - before do - build.update(artifacts_expire_at: Time.now - 7.days) - end + let(:build) { create(:ci_build, :artifacts, :expired) } it { is_expected.to be_falsy } end context 'is not expired' do - before do - build.update(artifacts_expire_at: Time.now + 7.days) - end - it { is_expected.to be_truthy } end end -- cgit v1.2.1 From 61864a5a5bb523953589c9398a431c4369fbfc76 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 21 Sep 2017 10:34:12 +0200 Subject: Rename Artifact to JobArtifact, split metadata out Two things at ones, as there was no clean way to seperate the commit and give me feedback from the tests. But the model Artifact is now JobArtifact, and the table does not have a type anymore, but the metadata is now its own model: Ci::JobArtifactMetadata. --- app/models/ci/artifact.rb | 24 --------- app/models/ci/build.rb | 24 +++++---- app/models/ci/job_artifact.rb | 26 ++++++++++ app/models/concerns/artifact_migratable.rb | 30 +++++++---- app/models/project_statistics.rb | 5 +- app/uploaders/artifact_uploader.rb | 8 ++- app/uploaders/job_artifact_uploader.rb | 34 +++++++++++++ db/migrate/20170918072948_create_artifacts.rb | 21 -------- db/migrate/20170918072948_create_job_artifacts.rb | 19 +++++++ db/schema.rb | 31 ++++++------ lib/api/entities.rb | 8 +-- lib/api/runner.rb | 7 +-- spec/factories/ci/artifacts.rb | 22 -------- spec/factories/ci/builds.rb | 8 ++- spec/factories/ci/job_artifacts.rb | 28 ++++++++++ spec/models/ci/artifact_spec.rb | 59 ---------------------- spec/models/ci/build_spec.rb | 2 +- spec/models/ci/job_artifact_spec.rb | 30 +++++++++++ spec/models/project_statistics_spec.rb | 8 +-- spec/requests/api/runner_spec.rb | 4 +- spec/serializers/pipeline_serializer_spec.rb | 2 +- spec/uploaders/artifact_uploader_spec.rb | 4 +- spec/uploaders/job_artifact_uploader_spec.rb | 15 ++++++ .../expire_build_instance_artifacts_worker_spec.rb | 22 ++++---- 24 files changed, 238 insertions(+), 203 deletions(-) delete mode 100644 app/models/ci/artifact.rb create mode 100644 app/models/ci/job_artifact.rb create mode 100644 app/uploaders/job_artifact_uploader.rb delete mode 100644 db/migrate/20170918072948_create_artifacts.rb create mode 100644 db/migrate/20170918072948_create_job_artifacts.rb delete mode 100644 spec/factories/ci/artifacts.rb create mode 100644 spec/factories/ci/job_artifacts.rb delete mode 100644 spec/models/ci/artifact_spec.rb create mode 100644 spec/models/ci/job_artifact_spec.rb create mode 100644 spec/uploaders/job_artifact_uploader_spec.rb diff --git a/app/models/ci/artifact.rb b/app/models/ci/artifact.rb deleted file mode 100644 index 858609883ce..00000000000 --- a/app/models/ci/artifact.rb +++ /dev/null @@ -1,24 +0,0 @@ -module Ci - class Artifact < ActiveRecord::Base - extend Gitlab::Ci::Model - - belongs_to :project - belongs_to :build, class_name: "Ci::Build", foreign_key: :ci_build_id - - before_save :set_size, if: :file_changed? - - mount_uploader :file, ArtifactUploader - - enum type: { archive: 0, metadata: 1 } - - # Allow us to use `type` as column name, without Rails thinking we're using - # STI: https://stackoverflow.com/a/29663933 - def self.inheritance_column - nil - end - - def set_size - self.size = file.exists? ? file.size : 0 - end - end -end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index fae2f5590b4..6d0ebd7f932 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -11,10 +11,15 @@ module Ci belongs_to :erased_by, class_name: 'User' has_many :deployments, as: :deployable - has_many :artifacts, class_name: 'Ci::Artifact', foreign_key: :ci_build_id + has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' has_many :trace_sections, class_name: 'Ci::BuildTraceSection' + has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id + has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id + has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id + + # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @persisted_environment ||= Environment.find_by( @@ -33,7 +38,9 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } - scope :with_artifacts, ->() { where.not(artifacts_file: [nil, '']) } + scope :with_artifacts, ->() do + where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.ci_job_id')) + end scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } @@ -423,7 +430,7 @@ module Ci Gitlab::Ci::Build::Image.from_services(self) end - def artifacts_options + def artifacts [options[:artifacts]] end @@ -464,12 +471,6 @@ module Ci super(options).merge(when: read_attribute(:when)) end - def update_project_statistics - return unless project - - ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size]) - end - private def update_artifacts_size @@ -560,6 +561,11 @@ module Ci pipeline.config_processor.build_attributes(name) end + def update_project_statistics + return unless project + + ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size]) + end def update_project_statistics_after_save if previous_changes.include?('artifacts_size') diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb new file mode 100644 index 00000000000..9c709077ac6 --- /dev/null +++ b/app/models/ci/job_artifact.rb @@ -0,0 +1,26 @@ +module Ci + class JobArtifact < ActiveRecord::Base + extend Gitlab::Ci::Model + + belongs_to :project + belongs_to :job, class_name: "Ci::Build", foreign_key: :ci_job_id + + before_save :set_size, if: :file_changed? + after_commit :remove_file!, on: :destroy + + mount_uploader :file, JobArtifactUploader + + enum file_type: { + archive: 1, + metadata: 2 + } + + def self.artifacts_size_for(project) + self.where(project: project).sum(:size) + end + + def set_size + self.size = file.size + end + end +end diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index a14a278df9f..8e331617cfa 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -3,15 +3,14 @@ # Meant to be prepended so the interface can stay the same module ArtifactMigratable def artifacts_file - artifacts.archive.first&.file || super + job_archive&.file || super end def artifacts_metadata - artifacts.metadata.first&.file || super + job_metadata&.file || super end def artifacts? - byebug !artifacts_expired? && artifacts_file.exists? end @@ -19,19 +18,28 @@ module ArtifactMigratable artifacts? && artifacts_metadata.exists? end - def remove_artifacts_file! - artifacts_file.remove! + def artifacts_file_changed? + job_archive&.file_changed? || super end - def remove_artifacts_metadata! - artifacts_metadata.remove! + def remove_artifacts_file! + if job_archive + job_archive.destroy + else + super + end end - def artifacts_file=(file) - artifacts.create(project: project, type: :archive, file: file) + def remove_artifacts_metadata! + if job_metadata + job_metadata.destroy + else + super + end end - def artifacts_metadata=(file) - artifacts.create(project: project, type: :metadata, file: file) + def artifacts_size + read_attribute(:artifacts_size).to_i + + job_archive&.size.to_i + job_metadata&.size.to_i end end diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index 715b215d1db..a9c22d9cf18 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -35,7 +35,10 @@ class ProjectStatistics < ActiveRecord::Base end def update_build_artifacts_size - self.build_artifacts_size = project.builds.sum(:artifacts_size) + self.build_artifacts_size = + project.builds.sum(:artifacts_size) + + Ci::JobArtifact.artifacts_size_for(self) + + Ci::JobArtifactMetadata.artifacts_size_for(self) end def update_storage_size diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb index 8ac0e2fe5a2..d4dda8e9e67 100644 --- a/app/uploaders/artifact_uploader.rb +++ b/app/uploaders/artifact_uploader.rb @@ -12,10 +12,6 @@ class ArtifactUploader < GitlabUploader end def initialize(job, field) - # Temporairy conditional, needed to move artifacts to their own table, - # but keeping compat with Ci::Build for the time being - job = job.build if job.respond_to?(:build) - @job, @field = job, field end @@ -38,6 +34,8 @@ class ArtifactUploader < GitlabUploader end def default_path - File.join(job.created_at.utc.strftime('%Y_%m'), job.project_id.to_s, job.id.to_s) + File.join(job.project_id.to_s, + job.created_at.utc.strftime('%Y_%m'), + job.id.to_s) end end diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb new file mode 100644 index 00000000000..6ea6a85b4a2 --- /dev/null +++ b/app/uploaders/job_artifact_uploader.rb @@ -0,0 +1,34 @@ +class JobArtifactUploader < ArtifactUploader + def initialize(artifact, _field) + @artifact = artifact + end + + # If this record exists, the associatied artifact is there. Every artifact + # persisted will have an associated file + def exists? + true + end + + def size + return super unless @artifact.size + + @artifact.size + end + + private + + def disk_hash + @disk_hash ||= Digest::SHA2.hexdigest(job.project_id.to_s) + end + + def default_path + creation_date = job.created_at.utc.strftime('%Y_%m_%d') + + File.join(disk_hash[0..1], disk_hash[2..3], disk_hash, + creation_date, job.id.to_s, @artifact.id.to_s) + end + + def job + @artifact.job + end +end diff --git a/db/migrate/20170918072948_create_artifacts.rb b/db/migrate/20170918072948_create_artifacts.rb deleted file mode 100644 index 1dd5edc3f15..00000000000 --- a/db/migrate/20170918072948_create_artifacts.rb +++ /dev/null @@ -1,21 +0,0 @@ -class CreateArtifacts < ActiveRecord::Migration - def up - create_table :ci_artifacts do |t| - t.belongs_to :project, null: false, foreign_key: { on_delete: :cascade } - t.belongs_to :ci_build, null: false, foreign_key: { on_delete: :cascade } - t.integer :type, default: 0, null: false - t.integer :size, limit: 8, default: 0 - - t.datetime_with_timezone :created_at, null: false - t.datetime_with_timezone :updated_at, null: false - - t.text :file - end - - add_index(:ci_artifacts, [:project_id, :ci_build_id], unique: true) - end - - def down - drop_table(:ci_artifacts) - end -end diff --git a/db/migrate/20170918072948_create_job_artifacts.rb b/db/migrate/20170918072948_create_job_artifacts.rb new file mode 100644 index 00000000000..6d1756f368c --- /dev/null +++ b/db/migrate/20170918072948_create_job_artifacts.rb @@ -0,0 +1,19 @@ +class CreateJobArtifacts < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + create_table :ci_job_artifacts do |t| + t.belongs_to :project, null: false, index: true, foreign_key: { on_delete: :cascade } + t.integer :ci_job_id, null: false, index: true + t.integer :size, limit: 8 + t.integer :file_type, null: false, index: true + + t.datetime_with_timezone :created_at, null: false + t.datetime_with_timezone :updated_at, null: false + + t.string :file + end + end +end diff --git a/db/schema.rb b/db/schema.rb index b4048371676..46a3b147198 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -226,18 +226,6 @@ ActiveRecord::Schema.define(version: 20171124150326) do add_index "chat_teams", ["namespace_id"], name: "index_chat_teams_on_namespace_id", unique: true, using: :btree - create_table "ci_artifacts", force: :cascade do |t| - t.integer "project_id", null: false - t.integer "ci_build_id", null: false - t.integer "type", default: 0, null: false - t.integer "size", limit: 8, default: 0 - t.datetime_with_timezone "created_at", null: false - t.datetime_with_timezone "updated_at", null: false - t.text "file" - end - - add_index "ci_artifacts", ["project_id", "ci_build_id"], name: "index_ci_artifacts_on_project_id_and_ci_build_id", unique: true, using: :btree - create_table "ci_build_trace_section_names", force: :cascade do |t| t.integer "project_id", null: false t.string "name", null: false @@ -331,6 +319,20 @@ ActiveRecord::Schema.define(version: 20171124150326) do add_index "ci_group_variables", ["group_id", "key"], name: "index_ci_group_variables_on_group_id_and_key", unique: true, using: :btree + create_table "ci_job_artifacts", force: :cascade do |t| + t.integer "project_id", null: false + t.integer "ci_job_id", null: false + t.integer "size", limit: 8 + t.integer "file_type", null: false + t.datetime_with_timezone "created_at", null: false + t.datetime_with_timezone "updated_at", null: false + t.string "file" + end + + add_index "ci_job_artifacts", ["ci_job_id"], name: "index_ci_job_artifacts_on_ci_job_id", using: :btree + add_index "ci_job_artifacts", ["file_type"], name: "index_ci_job_artifacts_on_file_type", using: :btree + add_index "ci_job_artifacts", ["project_id"], name: "index_ci_job_artifacts_on_project_id", using: :btree + create_table "ci_pipeline_schedule_variables", force: :cascade do |t| t.string "key", null: false t.text "value" @@ -391,9 +393,9 @@ ActiveRecord::Schema.define(version: 20171124150326) do t.integer "auto_canceled_by_id" t.integer "pipeline_schedule_id" t.integer "source" - t.integer "config_source" t.boolean "protected" t.integer "failure_reason" + t.integer "config_source" end add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree @@ -1913,8 +1915,6 @@ ActiveRecord::Schema.define(version: 20171124150326) do add_foreign_key "boards", "projects", name: "fk_f15266b5f9", on_delete: :cascade add_foreign_key "chat_teams", "namespaces", on_delete: :cascade - add_foreign_key "ci_artifacts", "ci_builds", on_delete: :cascade - add_foreign_key "ci_artifacts", "projects", on_delete: :cascade add_foreign_key "ci_build_trace_section_names", "projects", on_delete: :cascade add_foreign_key "ci_build_trace_sections", "ci_build_trace_section_names", column: "section_name_id", name: "fk_264e112c66", on_delete: :cascade add_foreign_key "ci_build_trace_sections", "ci_builds", column: "build_id", name: "fk_4ebe41f502", on_delete: :cascade @@ -1923,6 +1923,7 @@ ActiveRecord::Schema.define(version: 20171124150326) do add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade + add_foreign_key "ci_job_artifacts", "projects", on_delete: :cascade add_foreign_key "ci_pipeline_schedule_variables", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_41c35fda51", on_delete: :cascade add_foreign_key "ci_pipeline_schedules", "projects", name: "fk_8ead60fcc4", on_delete: :cascade add_foreign_key "ci_pipeline_schedules", "users", column: "owner_id", name: "fk_9ea99f58d2", on_delete: :nullify diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 9eb2c98c436..0964bd98fbb 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1006,13 +1006,9 @@ module API expose :type, :url, :username, :password end - class ArtifactFile < Grape::Entity - expose :filename, :size - end - class Dependency < Grape::Entity expose :id, :name, :token - expose :artifacts_file, using: ArtifactFile, if: ->(job, _) { job.artifacts? } + expose :artifacts_file, using: JobArtifactFile, if: ->(job, _) { job.artifacts? } end class Response < Grape::Entity @@ -1036,7 +1032,7 @@ module API expose :steps, using: Step expose :image, using: Image expose :services, using: Service - expose :artifacts_options, as: :artifacts, using: Artifacts + expose :artifacts, using: Artifacts expose :cache, using: Cache expose :credentials, using: Credentials expose :dependencies, using: Dependency diff --git a/lib/api/runner.rb b/lib/api/runner.rb index a3987c560dd..8de0868c063 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -215,15 +215,16 @@ module API job = authenticate_job! forbidden!('Job is not running!') unless job.running? - artifacts_upload_path = ArtifactUploader.artifacts_upload_path + artifacts_upload_path = JobArtifactUploader.artifacts_upload_path artifacts = uploaded_file(:file, artifacts_upload_path) metadata = uploaded_file(:metadata, artifacts_upload_path) bad_request!('Missing artifacts file!') unless artifacts file_to_large! unless artifacts.size < max_artifacts_size - job.artifacts_file = artifacts - job.artifacts_metadata = metadata + job.job_artifacts.build(project: job.project, file_type: :archive, file: artifacts) + job.job_artifacts.build(project: job.project, file_type: :metadata, file: metadata) + job.artifacts_expire_in = params['expire_in'] || Gitlab::CurrentSettings.current_application_settings.default_artifacts_expire_in diff --git a/spec/factories/ci/artifacts.rb b/spec/factories/ci/artifacts.rb deleted file mode 100644 index 085e707d686..00000000000 --- a/spec/factories/ci/artifacts.rb +++ /dev/null @@ -1,22 +0,0 @@ -include ActionDispatch::TestProcess - -FactoryGirl.define do - factory :artifact, class: Ci::Artifact do - project - build factory: :ci_build - - after :create do |artifact| - artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') - artifact.save - end - - factory :artifact_metadata do - type :metadata - - after :create do |artifact| - artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), 'application/x-gzip') - artifact.save - end - end - end -end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 69d58f367ac..6cb612a58d2 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -155,14 +155,12 @@ FactoryGirl.define do end trait :artifacts do - after(:create) do |build, _| - create(:artifact, build: build) - create(:artifact_metadata, build: build) - end + job_archive factory: :ci_job_artifact + job_metadata factory: :ci_job_metadata end trait :expired do - artifacts_expire_at = 1.minute.ago + artifacts_expire_at 1.minute.ago end trait :with_commit do diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb new file mode 100644 index 00000000000..8a7e04c747f --- /dev/null +++ b/spec/factories/ci/job_artifacts.rb @@ -0,0 +1,28 @@ +include ActionDispatch::TestProcess + +FactoryGirl.define do + factory :ci_job_artifact, class: Ci::JobArtifact do + project + job factory: :ci_build + file_type :archive + + after :create do |artifact| + if artifact.archive? + artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), + 'application/zip') + + artifact.save + end + end + end + + factory :ci_job_metadata, parent: :ci_job_artifact do + file_type :metadata + + after :create do |artifact| + artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), + 'application/x-gzip') + artifact.save + end + end +end diff --git a/spec/models/ci/artifact_spec.rb b/spec/models/ci/artifact_spec.rb deleted file mode 100644 index 5e39f73763b..00000000000 --- a/spec/models/ci/artifact_spec.rb +++ /dev/null @@ -1,59 +0,0 @@ -require 'spec_helper' - -describe Ci::Artifact do - it { is_expected.to belong_to(:project) } - it { is_expected.to belong_to(:build) } - - it { is_expected.to respond_to(:file) } - it { is_expected.to respond_to(:created_at) } - it { is_expected.to respond_to(:updated_at) } - it { is_expected.to respond_to(:type) } - - let(:artifact) { create(:artifact) } - - describe '#type' do - it 'defaults to archive' do - expect(artifact.type).to eq("archive") - end - end - - describe '#set_size' do - it 'sets the size' do - expect(artifact.size).to eq(106365) - end - end - - describe '#file' do - subject { artifact.file } - - context 'the uploader api' do - it { is_expected.to respond_to(:store_dir) } - it { is_expected.to respond_to(:cache_dir) } - it { is_expected.to respond_to(:work_dir) } - end - end - - describe '#remove_file' do - it 'removes the file from the database' do - artifact.remove_file! - - expect(artifact.file.exists?).to be_falsy - end - end - - describe '#exists?' do - context 'when the artifact file is present' do - it 'is returns true' do - expect(artifact.exists?).to be(true) - end - end - - context 'when the file has been removed' do - it 'does not exist' do - artifact.remove_file! - - expect(artifact.exists?).to be_falsy - end - end - end -end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 52a3732d0cd..f8dbed91c1a 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -146,7 +146,7 @@ describe Ci::Build do it { is_expected.to be_truthy } context 'is expired' do - let(:build) { create(:ci_build, :artifacts, :expired) } + let!(:build) { create(:ci_build, :artifacts, :expired) } it { is_expected.to be_falsy } end diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb new file mode 100644 index 00000000000..5202a8183af --- /dev/null +++ b/spec/models/ci/job_artifact_spec.rb @@ -0,0 +1,30 @@ +require 'spec_helper' + +describe Ci::JobArtifact do + set(:artifact) { create(:ci_job_artifact) } + + describe "Associations" do + it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:job) } + end + + it { is_expected.to respond_to(:file) } + it { is_expected.to respond_to(:created_at) } + it { is_expected.to respond_to(:updated_at) } + + describe '#set_size' do + it 'sets the size' do + expect(artifact.size).to eq(106365) + end + end + + describe '#file' do + subject { artifact.file } + + context 'the uploader api' do + it { is_expected.to respond_to(:store_dir) } + it { is_expected.to respond_to(:cache_dir) } + it { is_expected.to respond_to(:work_dir) } + end + end +end diff --git a/spec/models/project_statistics_spec.rb b/spec/models/project_statistics_spec.rb index 59e20e84c2f..95e8d519bdd 100644 --- a/spec/models/project_statistics_spec.rb +++ b/spec/models/project_statistics_spec.rb @@ -133,15 +133,17 @@ describe ProjectStatistics do describe '#update_build_artifacts_size' do let!(:pipeline) { create(:ci_pipeline, project: project) } - let!(:build1) { create(:ci_build, pipeline: pipeline, artifacts_size: 45.megabytes) } - let!(:build2) { create(:ci_build, pipeline: pipeline, artifacts_size: 56.megabytes) } + let!(:ci_build) { create(:ci_build, pipeline: pipeline, artifacts_size: 45.megabytes) } before do + create(:ci_build, pipeline: pipeline, artifacts_size: 56.megabytes) + create(:ci_job_artifact, project: pipeline.project, job: ci_build) + statistics.update_build_artifacts_size end it "stores the size of related build artifacts" do - expect(statistics.build_artifacts_size).to eq 101.megabytes + expect(statistics.build_artifacts_size).to eq(106012541) end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 47f4ccd4887..f320a366e6e 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -945,7 +945,7 @@ describe API::Runner do context 'when artifacts are being stored inside of tmp path' do before do # by configuring this path we allow to pass temp file from any path - allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return('/') + allow(JobArtifactUploader).to receive(:artifacts_upload_path).and_return('/') end context 'when job has been erased' do @@ -1106,7 +1106,7 @@ describe API::Runner do expect(response).to have_gitlab_http_status(201) expect(stored_artifacts_file.original_filename).to eq(artifacts.original_filename) expect(stored_metadata_file.original_filename).to eq(metadata.original_filename) - expect(stored_artifacts_size).to eq(71759) + expect(stored_artifacts_size).to eq(72821) end end diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 8fc1ceedc34..7144b66585c 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe PipelineSerializer do - let(:user) { create(:user) } + set(:user) { create(:user) } let(:serializer) do described_class.new(current_user: user) diff --git a/spec/uploaders/artifact_uploader_spec.rb b/spec/uploaders/artifact_uploader_spec.rb index 2a3bd0e3bb2..9cb2c090b43 100644 --- a/spec/uploaders/artifact_uploader_spec.rb +++ b/spec/uploaders/artifact_uploader_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' describe ArtifactUploader do - let(:job) { create(:ci_build) } + set(:job) { create(:ci_build) } let(:uploader) { described_class.new(job, :artifacts_file) } let(:path) { Gitlab.config.artifacts.path } @@ -26,7 +26,7 @@ describe ArtifactUploader do subject { uploader.store_dir } it { is_expected.to start_with(path) } - it { is_expected.to end_with("#{job.project_id}/#{job.id}") } + it { is_expected.to end_with("#{job.project_id}/#{job.created_at.utc.strftime('%Y_%m')}/#{job.id}") } end describe '#cache_dir' do diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb new file mode 100644 index 00000000000..d045acf9089 --- /dev/null +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -0,0 +1,15 @@ +require 'spec_helper' + +describe JobArtifactUploader do + set(:job_artifact) { create(:ci_job_artifact) } + let(:job) { job_artifact.job } + let(:uploader) { described_class.new(job_artifact, :file) } + + describe '#store_dir' do + subject { uploader.store_dir } + + it { is_expected.to start_with(Gitlab.config.artifacts.path) } + it { is_expected.not_to end_with("#{job.project_id}/#{job.created_at.utc.strftime('%Y_%m')}/#{job.id}") } + it { is_expected.to match(/\h{2}\/\h{2}\/\h{64}\/\d{4}_\d{1,2}_\d{1,2}\/\d+\/\d+\z/) } + end +end diff --git a/spec/workers/expire_build_instance_artifacts_worker_spec.rb b/spec/workers/expire_build_instance_artifacts_worker_spec.rb index bed5c5e2ecb..c0d2b1b7411 100644 --- a/spec/workers/expire_build_instance_artifacts_worker_spec.rb +++ b/spec/workers/expire_build_instance_artifacts_worker_spec.rb @@ -11,12 +11,8 @@ describe ExpireBuildInstanceArtifactsWorker do end context 'with expired artifacts' do - let(:artifacts_expiry) { { artifacts_expire_at: Time.now - 7.days } } - context 'when associated project is valid' do - let(:build) do - create(:ci_build, :artifacts, artifacts_expiry) - end + let(:build) { create(:ci_build, :artifacts, :expired) } it 'does expire' do expect(build.reload.artifacts_expired?).to be_truthy @@ -26,14 +22,14 @@ describe ExpireBuildInstanceArtifactsWorker do expect(build.reload.artifacts_file.exists?).to be_falsey end - it 'does nullify artifacts_file column' do - expect(build.reload.artifacts_file_identifier).to be_nil + it 'does remove the job artifact record' do + expect(build.reload.job_archive).to be_nil end end end context 'with not yet expired artifacts' do - let(:build) do + set(:build) do create(:ci_build, :artifacts, artifacts_expire_at: Time.now + 7.days) end @@ -45,8 +41,8 @@ describe ExpireBuildInstanceArtifactsWorker do expect(build.reload.artifacts_file.exists?).to be_truthy end - it 'does not nullify artifacts_file column' do - expect(build.reload.artifacts_file_identifier).not_to be_nil + it 'does not remove the job artifact record' do + expect(build.reload.job_archive).not_to be_nil end end @@ -61,13 +57,13 @@ describe ExpireBuildInstanceArtifactsWorker do expect(build.reload.artifacts_file.exists?).to be_truthy end - it 'does not nullify artifacts_file column' do - expect(build.reload.artifacts_file_identifier).not_to be_nil + it 'does not remove the job artifact record' do + expect(build.reload.job_archive).not_to be_nil end end context 'for expired artifacts' do - let(:build) { create(:ci_build, artifacts_expire_at: Time.now - 7.days) } + let(:build) { create(:ci_build, :expired) } it 'is still expired' do expect(build.reload.artifacts_expired?).to be_truthy -- cgit v1.2.1 From 303f165cbae8367c19ea273fc52170c2a354a8d6 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 2 Nov 2017 12:16:50 +0100 Subject: Fix creation of job_artifact_uploader --- app/models/ci/build.rb | 4 ++-- app/uploaders/job_artifact_uploader.rb | 6 ------ spec/factories/ci/builds.rb | 6 ++++-- spec/factories/ci/job_artifacts.rb | 6 ++++-- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 6d0ebd7f932..8d0c62fcb49 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -15,11 +15,11 @@ module Ci has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' has_many :trace_sections, class_name: 'Ci::BuildTraceSection' - has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id + # TODO: what to do with dependent destroy + has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id, dependent: :destroy has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id - # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @persisted_environment ||= Environment.find_by( diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb index 6ea6a85b4a2..cc6e1927f9e 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -3,12 +3,6 @@ class JobArtifactUploader < ArtifactUploader @artifact = artifact end - # If this record exists, the associatied artifact is there. Every artifact - # persisted will have an associated file - def exists? - true - end - def size return super unless @artifact.size diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 6cb612a58d2..ee449bfe5db 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -155,8 +155,10 @@ FactoryGirl.define do end trait :artifacts do - job_archive factory: :ci_job_artifact - job_metadata factory: :ci_job_metadata + after(:create) do |build| + create(:ci_job_artifact, job: build) + create(:ci_job_metadata, job: build) + end end trait :expired do diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb index 8a7e04c747f..0abebd14286 100644 --- a/spec/factories/ci/job_artifacts.rb +++ b/spec/factories/ci/job_artifacts.rb @@ -2,15 +2,17 @@ include ActionDispatch::TestProcess FactoryGirl.define do factory :ci_job_artifact, class: Ci::JobArtifact do - project job factory: :ci_build file_type :archive + after :build do |artifact| + artifact.project ||= artifact.job.project + end + after :create do |artifact| if artifact.archive? artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') - artifact.save end end -- cgit v1.2.1 From cc750539ec852e9f42ea25b31cc14a45962fc975 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 2 Nov 2017 12:20:47 +0100 Subject: Use tmp/test/artifacts for files --- config/gitlab.yml.example | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 7f6e68ceed6..c8b6018bc1b 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -649,6 +649,8 @@ test: # user: YOUR_USERNAME pages: path: tmp/tests/pages + artifacts: + path: tmp/tests/artifacts repositories: storages: default: -- cgit v1.2.1 From c7d945758accc6f80ee63c7c3b25782cf7f6f3b0 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 2 Nov 2017 19:38:25 +0100 Subject: Fix most test failures --- app/models/ci/build.rb | 9 ++++----- app/models/ci/job_artifact.rb | 2 +- app/models/project_statistics.rb | 3 +-- app/uploaders/job_artifact_uploader.rb | 12 ++++-------- db/migrate/20170918072948_create_job_artifacts.rb | 4 +++- db/schema.rb | 5 +++-- spec/factories/ci/builds.rb | 1 + spec/features/projects/pipelines/pipelines_spec.rb | 2 +- spec/serializers/pipeline_serializer_spec.rb | 2 +- spec/services/ci/retry_build_service_spec.rb | 4 ++-- spec/tasks/gitlab/backup_rake_spec.rb | 1 + 11 files changed, 22 insertions(+), 23 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8d0c62fcb49..161800e9061 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -15,10 +15,9 @@ module Ci has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' has_many :trace_sections, class_name: 'Ci::BuildTraceSection' - # TODO: what to do with dependent destroy - has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id, dependent: :destroy - has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id - has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id + has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @@ -39,7 +38,7 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } scope :with_artifacts, ->() do - where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.ci_job_id')) + where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id')) end scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 9c709077ac6..cc28092978c 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -3,7 +3,7 @@ module Ci extend Gitlab::Ci::Model belongs_to :project - belongs_to :job, class_name: "Ci::Build", foreign_key: :ci_job_id + belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id before_save :set_size, if: :file_changed? after_commit :remove_file!, on: :destroy diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index a9c22d9cf18..17b9d2cf7b4 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -37,8 +37,7 @@ class ProjectStatistics < ActiveRecord::Base def update_build_artifacts_size self.build_artifacts_size = project.builds.sum(:artifacts_size) + - Ci::JobArtifact.artifacts_size_for(self) + - Ci::JobArtifactMetadata.artifacts_size_for(self) + Ci::JobArtifact.artifacts_size_for(self) end def update_storage_size diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb index cc6e1927f9e..7185e24287f 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -4,7 +4,7 @@ class JobArtifactUploader < ArtifactUploader end def size - return super unless @artifact.size + return super if @artifact.size.nil? @artifact.size end @@ -12,17 +12,13 @@ class JobArtifactUploader < ArtifactUploader private def disk_hash - @disk_hash ||= Digest::SHA2.hexdigest(job.project_id.to_s) + @disk_hash ||= Digest::SHA2.hexdigest(@artifact.project_id.to_s) end def default_path - creation_date = job.created_at.utc.strftime('%Y_%m_%d') + creation_date = @artifact.created_at.utc.strftime('%Y_%m_%d') File.join(disk_hash[0..1], disk_hash[2..3], disk_hash, - creation_date, job.id.to_s, @artifact.id.to_s) - end - - def job - @artifact.job + creation_date, @artifact.ci_job_id.to_s, @artifact.id.to_s) end end diff --git a/db/migrate/20170918072948_create_job_artifacts.rb b/db/migrate/20170918072948_create_job_artifacts.rb index 6d1756f368c..078e3e9dc4e 100644 --- a/db/migrate/20170918072948_create_job_artifacts.rb +++ b/db/migrate/20170918072948_create_job_artifacts.rb @@ -6,7 +6,7 @@ class CreateJobArtifacts < ActiveRecord::Migration def change create_table :ci_job_artifacts do |t| t.belongs_to :project, null: false, index: true, foreign_key: { on_delete: :cascade } - t.integer :ci_job_id, null: false, index: true + t.integer :job_id, null: false, index: true t.integer :size, limit: 8 t.integer :file_type, null: false, index: true @@ -15,5 +15,7 @@ class CreateJobArtifacts < ActiveRecord::Migration t.string :file end + + add_foreign_key :ci_job_artifacts, :ci_builds, column: :job_id, on_delete: :cascade end end diff --git a/db/schema.rb b/db/schema.rb index 46a3b147198..0f71463acd8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -321,7 +321,7 @@ ActiveRecord::Schema.define(version: 20171124150326) do create_table "ci_job_artifacts", force: :cascade do |t| t.integer "project_id", null: false - t.integer "ci_job_id", null: false + t.integer "job_id", null: false t.integer "size", limit: 8 t.integer "file_type", null: false t.datetime_with_timezone "created_at", null: false @@ -329,8 +329,8 @@ ActiveRecord::Schema.define(version: 20171124150326) do t.string "file" end - add_index "ci_job_artifacts", ["ci_job_id"], name: "index_ci_job_artifacts_on_ci_job_id", using: :btree add_index "ci_job_artifacts", ["file_type"], name: "index_ci_job_artifacts_on_file_type", using: :btree + add_index "ci_job_artifacts", ["job_id"], name: "index_ci_job_artifacts_on_job_id", using: :btree add_index "ci_job_artifacts", ["project_id"], name: "index_ci_job_artifacts_on_project_id", using: :btree create_table "ci_pipeline_schedule_variables", force: :cascade do |t| @@ -1923,6 +1923,7 @@ ActiveRecord::Schema.define(version: 20171124150326) do add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade + add_foreign_key "ci_job_artifacts", "ci_builds", column: "job_id", on_delete: :cascade add_foreign_key "ci_job_artifacts", "projects", on_delete: :cascade add_foreign_key "ci_pipeline_schedule_variables", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_41c35fda51", on_delete: :cascade add_foreign_key "ci_pipeline_schedules", "projects", name: "fk_8ead60fcc4", on_delete: :cascade diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index ee449bfe5db..2ce49ede4fa 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -158,6 +158,7 @@ FactoryGirl.define do after(:create) do |build| create(:ci_job_artifact, job: build) create(:ci_job_metadata, job: build) + build.reload end end diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb index a1b1d94ae06..b87b47d0e1a 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -304,7 +304,7 @@ describe 'Pipelines', :js do context 'with artifacts expired' do let!(:with_artifacts_expired) do - create(:ci_build, :artifacts_expired, :success, + create(:ci_build, :expired, :success, pipeline: pipeline, name: 'rspec', stage: 'test') diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 7144b66585c..88d347322a6 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -117,7 +117,7 @@ describe PipelineSerializer do shared_examples 'no N+1 queries' do it 'verifies number of queries', :request_store do recorded = ActiveRecord::QueryRecorder.new { subject } - expect(recorded.count).to be_within(1).of(57) + expect(recorded.count).to be_within(1).of(36) expect(recorded.cached_count).to eq(0) end end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index b61d1cb765e..20fe80beade 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -17,7 +17,7 @@ describe Ci::RetryBuildService do %i[id status user token coverage trace runner artifacts_expire_at artifacts_file artifacts_metadata artifacts_size created_at updated_at started_at finished_at queued_at erased_by - erased_at auto_canceled_by].freeze + erased_at auto_canceled_by job_artifacts job_archive job_metadata].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections @@ -34,7 +34,7 @@ describe Ci::RetryBuildService do end let(:build) do - create(:ci_build, :failed, :artifacts_expired, :erased, + create(:ci_build, :failed, :artifacts, :expired, :erased, :queued, :coverage, :tags, :allowed_to_fail, :on_tag, :triggered, :trace, :teardown_environment, description: 'my-job', stage: 'test', pipeline: pipeline, diff --git a/spec/tasks/gitlab/backup_rake_spec.rb b/spec/tasks/gitlab/backup_rake_spec.rb index bf2e11bc360..52f81106829 100644 --- a/spec/tasks/gitlab/backup_rake_spec.rb +++ b/spec/tasks/gitlab/backup_rake_spec.rb @@ -24,6 +24,7 @@ describe 'gitlab:app namespace rake task' do # We need this directory to run `gitlab:backup:create` task FileUtils.mkdir_p('public/uploads') + FileUtils.mkdir_p(Rails.root.join('tmp/tests/artifacts')) end before do -- cgit v1.2.1 From 1756604f90588a746ce6df7e4386830db9b3a485 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 7 Nov 2017 10:18:32 +0100 Subject: JobArtifactsUploader does not inherrit from ArtifactsUploader --- app/uploaders/job_artifact_uploader.rb | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb index 7185e24287f..8a5200504fc 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -1,4 +1,14 @@ -class JobArtifactUploader < ArtifactUploader +class JobArtifactUploader < GitlabUploader + storage :file + + def self.local_artifacts_store + Gitlab.config.artifacts.path + end + + def self.artifacts_upload_path + File.join(self.local_artifacts_store, 'tmp/uploads/') + end + def initialize(artifact, _field) @artifact = artifact end @@ -9,16 +19,20 @@ class JobArtifactUploader < ArtifactUploader @artifact.size end - private - - def disk_hash - @disk_hash ||= Digest::SHA2.hexdigest(@artifact.project_id.to_s) + def store_dir + File.join(self.class.local_artifacts_store, default_path) end + private + def default_path creation_date = @artifact.created_at.utc.strftime('%Y_%m_%d') File.join(disk_hash[0..1], disk_hash[2..3], disk_hash, - creation_date, @artifact.ci_job_id.to_s, @artifact.id.to_s) + creation_date, @artifact.job_id.to_s, @artifact.id.to_s) + end + + def disk_hash + @disk_hash ||= Digest::SHA2.hexdigest(@artifact.project_id.to_s) end end -- cgit v1.2.1 From ba5697fd809563cb2fd619d7c50362303ab86434 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 8 Nov 2017 10:46:47 +0100 Subject: Fix legacy migration test --- app/uploaders/artifact_uploader.rb | 4 +-- db/migrate/20170918072948_create_job_artifacts.rb | 4 +-- spec/migrations/migrate_old_artifacts_spec.rb | 37 ++++++++++++++++++----- spec/support/test_env.rb | 5 +++ spec/uploaders/artifact_uploader_spec.rb | 2 +- 5 files changed, 38 insertions(+), 14 deletions(-) diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb index d4dda8e9e67..14addb6cf14 100644 --- a/app/uploaders/artifact_uploader.rb +++ b/app/uploaders/artifact_uploader.rb @@ -34,8 +34,6 @@ class ArtifactUploader < GitlabUploader end def default_path - File.join(job.project_id.to_s, - job.created_at.utc.strftime('%Y_%m'), - job.id.to_s) + File.join(job.created_at.utc.strftime('%Y_%m'), job.project_id.to_s, job.id.to_s) end end diff --git a/db/migrate/20170918072948_create_job_artifacts.rb b/db/migrate/20170918072948_create_job_artifacts.rb index 078e3e9dc4e..90152127ed5 100644 --- a/db/migrate/20170918072948_create_job_artifacts.rb +++ b/db/migrate/20170918072948_create_job_artifacts.rb @@ -14,8 +14,8 @@ class CreateJobArtifacts < ActiveRecord::Migration t.datetime_with_timezone :updated_at, null: false t.string :file - end - add_foreign_key :ci_job_artifacts, :ci_builds, column: :job_id, on_delete: :cascade + t.foreign_key :ci_builds, column: :job_id, on_delete: :cascade + end end end diff --git a/spec/migrations/migrate_old_artifacts_spec.rb b/spec/migrations/migrate_old_artifacts_spec.rb index 81366d15b34..b09243e5c95 100644 --- a/spec/migrations/migrate_old_artifacts_spec.rb +++ b/spec/migrations/migrate_old_artifacts_spec.rb @@ -16,20 +16,22 @@ describe MigrateOldArtifacts do end context 'with migratable data' do - let(:project1) { create(:project, ci_id: 2) } - let(:project2) { create(:project, ci_id: 3) } - let(:project3) { create(:project) } + set(:project1) { create(:project, ci_id: 2) } + set(:project2) { create(:project, ci_id: 3) } + set(:project3) { create(:project) } - let(:pipeline1) { create(:ci_empty_pipeline, project: project1) } - let(:pipeline2) { create(:ci_empty_pipeline, project: project2) } - let(:pipeline3) { create(:ci_empty_pipeline, project: project3) } + set(:pipeline1) { create(:ci_empty_pipeline, project: project1) } + set(:pipeline2) { create(:ci_empty_pipeline, project: project2) } + set(:pipeline3) { create(:ci_empty_pipeline, project: project3) } let!(:build_with_legacy_artifacts) { create(:ci_build, pipeline: pipeline1) } let!(:build_without_artifacts) { create(:ci_build, pipeline: pipeline1) } - let!(:build2) { create(:ci_build, :artifacts, pipeline: pipeline2) } - let!(:build3) { create(:ci_build, :artifacts, pipeline: pipeline3) } + let!(:build2) { create(:ci_build, pipeline: pipeline2) } + let!(:build3) { create(:ci_build, pipeline: pipeline3) } before do + setup_builds(build2, build3) + store_artifacts_in_legacy_path(build_with_legacy_artifacts) end @@ -113,5 +115,24 @@ describe MigrateOldArtifacts do build.project.ci_id.to_s, build.id.to_s) end + + def new_legacy_path(build) + File.join(directory, + build.created_at.utc.strftime('%Y_%m'), + build.project_id.to_s, + build.id.to_s) + end + + def setup_builds(*builds) + builds.each do |build| + FileUtils.mkdir_p(new_legacy_path(build)) + + build.update_columns( + artifacts_file: 'ci_build_artifacts.zip', + artifacts_metadata: 'ci_build_artifacts_metadata.gz') + + build.reload + end + end end end diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index fff120fcb88..b300b493f86 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -120,6 +120,7 @@ module TestEnv FileUtils.mkdir_p(repos_path) FileUtils.mkdir_p(backup_path) FileUtils.mkdir_p(pages_path) + FileUtils.mkdir_p(artifacts_path) end def clean_gitlab_test_path @@ -233,6 +234,10 @@ module TestEnv Gitlab.config.pages.path end + def artifacts_path + Gitlab.config.artifacts.path + end + # When no cached assets exist, manually hit the root path to create them # # Otherwise they'd be created by the first test, often timing out and diff --git a/spec/uploaders/artifact_uploader_spec.rb b/spec/uploaders/artifact_uploader_spec.rb index 9cb2c090b43..0a07a7337b5 100644 --- a/spec/uploaders/artifact_uploader_spec.rb +++ b/spec/uploaders/artifact_uploader_spec.rb @@ -26,7 +26,7 @@ describe ArtifactUploader do subject { uploader.store_dir } it { is_expected.to start_with(path) } - it { is_expected.to end_with("#{job.project_id}/#{job.created_at.utc.strftime('%Y_%m')}/#{job.id}") } + it { is_expected.to end_with("#{job.project_id}/#{job.id}") } end describe '#cache_dir' do -- cgit v1.2.1 From e2242cdf75d2734f78f694ab3191fcbb31947a6f Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Sun, 12 Nov 2017 13:02:38 +0100 Subject: Last test fixes multiple artifacts --- spec/requests/api/runner_spec.rb | 2 +- spec/tasks/gitlab/backup_rake_spec.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index f320a366e6e..72962f5d0b4 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -1131,7 +1131,7 @@ describe API::Runner do # by configuring this path we allow to pass file from @tmpdir only # but all temporary files are stored in system tmp directory @tmpdir = Dir.mktmpdir - allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return(@tmpdir) + allow(JobArtifactUploader).to receive(:artifacts_upload_path).and_return(@tmpdir) end after do diff --git a/spec/tasks/gitlab/backup_rake_spec.rb b/spec/tasks/gitlab/backup_rake_spec.rb index 52f81106829..bf2e11bc360 100644 --- a/spec/tasks/gitlab/backup_rake_spec.rb +++ b/spec/tasks/gitlab/backup_rake_spec.rb @@ -24,7 +24,6 @@ describe 'gitlab:app namespace rake task' do # We need this directory to run `gitlab:backup:create` task FileUtils.mkdir_p('public/uploads') - FileUtils.mkdir_p(Rails.root.join('tmp/tests/artifacts')) end before do -- cgit v1.2.1 From 871de0f18581bb03fed5c0d800f8183598a0195f Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 23 Nov 2017 16:57:27 +0100 Subject: Rename artifacts_* to legacy_artifacts_* --- app/models/ci/build.rb | 4 +- app/models/concerns/artifact_migratable.rb | 14 +++-- app/uploaders/artifact_uploader.rb | 39 -------------- app/uploaders/job_artifact_uploader.rb | 18 ++----- app/uploaders/legacy_artifact_uploader.rb | 33 ++++++++++++ db/fixtures/development/14_pipelines.rb | 4 +- features/steps/project/pages.rb | 4 +- features/steps/shared/builds.rb | 4 +- lib/backup/artifacts.rb | 2 +- lib/gitlab/workhorse.rb | 2 +- spec/factories/ci/builds.rb | 11 ++++ spec/features/commits_spec.rb | 6 +-- .../merge_requests/mini_pipeline_graph_spec.rb | 4 +- spec/features/projects/jobs_spec.rb | 8 +-- .../services/projects/update_pages_service_spec.rb | 14 ++--- spec/uploaders/artifact_uploader_spec.rb | 61 ---------------------- spec/uploaders/legacy_artifact_uploader_spec.rb | 61 ++++++++++++++++++++++ 17 files changed, 145 insertions(+), 144 deletions(-) delete mode 100644 app/uploaders/artifact_uploader.rb create mode 100644 app/uploaders/legacy_artifact_uploader.rb delete mode 100644 spec/uploaders/artifact_uploader_spec.rb create mode 100644 spec/uploaders/legacy_artifact_uploader_spec.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 161800e9061..91747da28a1 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -46,8 +46,8 @@ module Ci scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } scope :ref_protected, -> { where(protected: true) } - mount_uploader :artifacts_file, ArtifactUploader - mount_uploader :artifacts_metadata, ArtifactUploader + mount_uploader :legacy_artifacts_file, LegacyArtifactUploader, mount_on: :artifacts_file + mount_uploader :legacy_artifacts_metadata, LegacyArtifactUploader, mount_on: :artifacts_metadata acts_as_taggable diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index 8e331617cfa..5c647bacf1b 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -3,11 +3,15 @@ # Meant to be prepended so the interface can stay the same module ArtifactMigratable def artifacts_file - job_archive&.file || super + job_archive&.file || legacy_artifacts_file + end + + def artifacts_file? + job_archive&.file? || legacy_artifacts_file? end def artifacts_metadata - job_metadata&.file || super + job_metadata&.file || legacy_artifacts_metadata end def artifacts? @@ -19,14 +23,14 @@ module ArtifactMigratable end def artifacts_file_changed? - job_archive&.file_changed? || super + job_archive&.file_changed? || attribute_changed?(:artifacts_file) end def remove_artifacts_file! if job_archive job_archive.destroy else - super + remove_legacy_artifacts_file! end end @@ -34,7 +38,7 @@ module ArtifactMigratable if job_metadata job_metadata.destroy else - super + remove_legacy_artifacts_metadata! end end diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb deleted file mode 100644 index 14addb6cf14..00000000000 --- a/app/uploaders/artifact_uploader.rb +++ /dev/null @@ -1,39 +0,0 @@ -class ArtifactUploader < GitlabUploader - storage :file - - attr_reader :job, :field - - def self.local_artifacts_store - Gitlab.config.artifacts.path - end - - def self.artifacts_upload_path - File.join(self.local_artifacts_store, 'tmp/uploads/') - end - - def initialize(job, field) - @job, @field = job, field - end - - def store_dir - default_local_path - end - - def cache_dir - File.join(self.class.local_artifacts_store, 'tmp/cache') - end - - def work_dir - File.join(self.class.local_artifacts_store, 'tmp/work') - end - - private - - def default_local_path - File.join(self.class.local_artifacts_store, default_path) - end - - def default_path - File.join(job.created_at.utc.strftime('%Y_%m'), job.project_id.to_s, job.id.to_s) - end -end diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb index 8a5200504fc..d54411e198f 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -9,30 +9,22 @@ class JobArtifactUploader < GitlabUploader File.join(self.local_artifacts_store, 'tmp/uploads/') end - def initialize(artifact, _field) - @artifact = artifact - end - def size - return super if @artifact.size.nil? - - @artifact.size - end + return super if model.size.nil? - def store_dir - File.join(self.class.local_artifacts_store, default_path) + model.size end private def default_path - creation_date = @artifact.created_at.utc.strftime('%Y_%m_%d') + creation_date = model.created_at.utc.strftime('%Y_%m_%d') File.join(disk_hash[0..1], disk_hash[2..3], disk_hash, - creation_date, @artifact.job_id.to_s, @artifact.id.to_s) + creation_date, model.job_id.to_s, model.id.to_s) end def disk_hash - @disk_hash ||= Digest::SHA2.hexdigest(@artifact.project_id.to_s) + @disk_hash ||= Digest::SHA2.hexdigest(model.project_id.to_s) end end diff --git a/app/uploaders/legacy_artifact_uploader.rb b/app/uploaders/legacy_artifact_uploader.rb new file mode 100644 index 00000000000..0c23e05b680 --- /dev/null +++ b/app/uploaders/legacy_artifact_uploader.rb @@ -0,0 +1,33 @@ +class LegacyArtifactUploader < GitlabUploader + storage :file + + def self.local_store_path + Gitlab.config.artifacts.path + end + + def self.artifacts_upload_path + File.join(self.local_artifacts_store, 'tmp/uploads/') + end + + def store_dir + default_local_path + end + + def cache_dir + File.join(self.class.local_store_path, 'tmp/cache') + end + + def work_dir + File.join(self.class.local_store_path, 'tmp/work') + end + + private + + def default_local_path + File.join(self.class.local_store_path, default_path) + end + + def default_path + File.join(model.created_at.utc.strftime('%Y_%m'), model.project_id.to_s, model.id.to_s) + end +end diff --git a/db/fixtures/development/14_pipelines.rb b/db/fixtures/development/14_pipelines.rb index 5de5339b70e..d3a63aa2a78 100644 --- a/db/fixtures/development/14_pipelines.rb +++ b/db/fixtures/development/14_pipelines.rb @@ -124,11 +124,11 @@ class Gitlab::Seeder::Pipelines return unless %w[build test].include?(build.stage) artifacts_cache_file(artifacts_archive_path) do |file| - build.artifacts_file = file + build.job_artifacts.build(project: build.project, file_type: :archive, file: file) end artifacts_cache_file(artifacts_metadata_path) do |file| - build.artifacts_metadata = file + build.job_artifacts.build(project: build.project, file_type: :metadata, file: file) end end diff --git a/features/steps/project/pages.rb b/features/steps/project/pages.rb index 124a132d688..f03630e5a91 100644 --- a/features/steps/project/pages.rb +++ b/features/steps/project/pages.rb @@ -44,8 +44,8 @@ class Spinach::Features::ProjectPages < Spinach::FeatureSteps project: @project, pipeline: pipeline, ref: 'HEAD', - artifacts_file: fixture_file_upload(Rails.root + 'spec/fixtures/pages.zip'), - artifacts_metadata: fixture_file_upload(Rails.root + 'spec/fixtures/pages.zip.meta') + legacy_artifacts_file: fixture_file_upload(Rails.root + 'spec/fixtures/pages.zip'), + legacy_artifacts_metadata: fixture_file_upload(Rails.root + 'spec/fixtures/pages.zip.meta') ) result = ::Projects::UpdatePagesService.new(@project, build).execute diff --git a/features/steps/shared/builds.rb b/features/steps/shared/builds.rb index 3b4c98ec00d..c267195f0e8 100644 --- a/features/steps/shared/builds.rb +++ b/features/steps/shared/builds.rb @@ -37,13 +37,13 @@ module SharedBuilds step 'recent build has artifacts available' do artifacts = Rails.root + 'spec/fixtures/ci_build_artifacts.zip' archive = fixture_file_upload(artifacts, 'application/zip') - @build.update_attributes(artifacts_file: archive) + @build.update_attributes(legacy_artifacts_file: archive) end step 'recent build has artifacts metadata available' do metadata = Rails.root + 'spec/fixtures/ci_build_artifacts_metadata.gz' gzip = fixture_file_upload(metadata, 'application/x-gzip') - @build.update_attributes(artifacts_metadata: gzip) + @build.update_attributes(legacy_artifacts_metadata: gzip) end step 'recent build has a build trace' do diff --git a/lib/backup/artifacts.rb b/lib/backup/artifacts.rb index 1f4bda6f588..7a582a20056 100644 --- a/lib/backup/artifacts.rb +++ b/lib/backup/artifacts.rb @@ -3,7 +3,7 @@ require 'backup/files' module Backup class Artifacts < Files def initialize - super('artifacts', ArtifactUploader.local_artifacts_store) + super('artifacts', LegacyArtifactUploader.local_store_path) end def create_files_dir diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index 864a9e04888..c3e2742306d 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -58,7 +58,7 @@ module Gitlab end def artifact_upload_ok - { TempPath: ArtifactUploader.artifacts_upload_path } + { TempPath: LegacyArtifactUploader.artifacts_upload_path } end def send_git_blob(repository, blob) diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 2ce49ede4fa..441f740e1e5 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -154,6 +154,17 @@ FactoryGirl.define do runner factory: :ci_runner end + trait :legacy_artifacts do + after(:create) do |build, _| + build.update!( + legacy_artifacts_file: fixture_file_upload( + Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip'), + legacy_artifacts_metadata: fixture_file_upload( + Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), 'application/x-gzip') + ) + end + end + trait :artifacts do after(:create) do |build| create(:ci_job_artifact, job: build) diff --git a/spec/features/commits_spec.rb b/spec/features/commits_spec.rb index 98586ddbd81..c870910c8ea 100644 --- a/spec/features/commits_spec.rb +++ b/spec/features/commits_spec.rb @@ -89,7 +89,7 @@ describe 'Commits' do context 'Download artifacts' do before do - build.update_attributes(artifacts_file: artifacts_file) + build.update_attributes(legacy_artifacts_file: artifacts_file) end it do @@ -146,7 +146,7 @@ describe 'Commits' do context "when logged as reporter" do before do project.team << [user, :reporter] - build.update_attributes(artifacts_file: artifacts_file) + build.update_attributes(legacy_artifacts_file: artifacts_file) visit pipeline_path(pipeline) end @@ -168,7 +168,7 @@ describe 'Commits' do project.update( visibility_level: Gitlab::VisibilityLevel::INTERNAL, public_builds: false) - build.update_attributes(artifacts_file: artifacts_file) + build.update_attributes(legacy_artifacts_file: artifacts_file) visit pipeline_path(pipeline) end diff --git a/spec/features/merge_requests/mini_pipeline_graph_spec.rb b/spec/features/merge_requests/mini_pipeline_graph_spec.rb index bac56270362..93c5e945453 100644 --- a/spec/features/merge_requests/mini_pipeline_graph_spec.rb +++ b/spec/features/merge_requests/mini_pipeline_graph_spec.rb @@ -28,14 +28,14 @@ feature 'Mini Pipeline Graph', :js do let(:artifacts_file2) { fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'image/png') } before do - create(:ci_build, pipeline: pipeline, artifacts_file: artifacts_file1) + create(:ci_build, pipeline: pipeline, legacy_artifacts_file: artifacts_file1) create(:ci_build, pipeline: pipeline, when: 'manual') end it 'avoids repeated database queries' do before = ActiveRecord::QueryRecorder.new { visit_merge_request(:json) } - create(:ci_build, pipeline: pipeline, artifacts_file: artifacts_file2) + create(:ci_build, pipeline: pipeline, legacy_artifacts_file: artifacts_file2) create(:ci_build, pipeline: pipeline, when: 'manual') after = ActiveRecord::QueryRecorder.new { visit_merge_request(:json) } diff --git a/spec/features/projects/jobs_spec.rb b/spec/features/projects/jobs_spec.rb index c2a0d2395a9..0b0d5a2dce8 100644 --- a/spec/features/projects/jobs_spec.rb +++ b/spec/features/projects/jobs_spec.rb @@ -187,7 +187,7 @@ feature 'Jobs' do context "Download artifacts" do before do - job.update_attributes(artifacts_file: artifacts_file) + job.update_attributes(legacy_artifacts_file: artifacts_file) visit project_job_path(project, job) end @@ -198,7 +198,7 @@ feature 'Jobs' do context 'Artifacts expire date' do before do - job.update_attributes(artifacts_file: artifacts_file, + job.update_attributes(legacy_artifacts_file: artifacts_file, artifacts_expire_at: expire_at) visit project_job_path(project, job) @@ -422,14 +422,14 @@ feature 'Jobs' do describe "GET /:project/jobs/:id/download" do before do - job.update_attributes(artifacts_file: artifacts_file) + job.update_attributes(legacy_artifacts_file: artifacts_file) visit project_job_path(project, job) click_link 'Download' end context "Build from other project" do before do - job2.update_attributes(artifacts_file: artifacts_file) + job2.update_attributes(legacy_artifacts_file: artifacts_file) visit download_project_job_artifacts_path(project, job2) end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index d4ac1f6ad81..a0c83600f39 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -22,8 +22,8 @@ describe Projects::UpdatePagesService do end before do - build.update_attributes(artifacts_file: file) - build.update_attributes(artifacts_metadata: metadata) + build.update_attributes(legacy_artifacts_file: file) + build.update_attributes(legacy_artifacts_metadata: metadata) end describe 'pages artifacts' do @@ -75,12 +75,12 @@ describe Projects::UpdatePagesService do it 'fails if sha on branch is not latest' do pipeline.update_attributes(sha: 'old_sha') - build.update_attributes(artifacts_file: file) + build.update_attributes(legacy_artifacts_file: file) expect(execute).not_to eq(:success) end it 'fails for empty file fails' do - build.update_attributes(artifacts_file: empty_file) + build.update_attributes(legacy_artifacts_file: empty_file) expect(execute).not_to eq(:success) end end @@ -97,7 +97,7 @@ describe Projects::UpdatePagesService do end it 'fails for invalid archive' do - build.update_attributes(artifacts_file: invalid_file) + build.update_attributes(legacy_artifacts_file: invalid_file) expect(execute).not_to eq(:success) end @@ -108,8 +108,8 @@ describe Projects::UpdatePagesService do file = fixture_file_upload(Rails.root + 'spec/fixtures/pages.zip') metafile = fixture_file_upload(Rails.root + 'spec/fixtures/pages.zip.meta') - build.update_attributes(artifacts_file: file) - build.update_attributes(artifacts_metadata: metafile) + build.update_attributes(legacy_artifacts_file: file) + build.update_attributes(legacy_artifacts_metadata: metafile) allow(build).to receive(:artifacts_metadata_entry) .and_return(metadata) diff --git a/spec/uploaders/artifact_uploader_spec.rb b/spec/uploaders/artifact_uploader_spec.rb deleted file mode 100644 index 0a07a7337b5..00000000000 --- a/spec/uploaders/artifact_uploader_spec.rb +++ /dev/null @@ -1,61 +0,0 @@ -require 'rails_helper' - -describe ArtifactUploader do - set(:job) { create(:ci_build) } - let(:uploader) { described_class.new(job, :artifacts_file) } - let(:path) { Gitlab.config.artifacts.path } - - describe '.local_artifacts_store' do - subject { described_class.local_artifacts_store } - - it "delegate to artifacts path" do - expect(Gitlab.config.artifacts).to receive(:path) - - subject - end - end - - describe '.artifacts_upload_path' do - subject { described_class.artifacts_upload_path } - - it { is_expected.to start_with(path) } - it { is_expected.to end_with('tmp/uploads/') } - end - - describe '#store_dir' do - subject { uploader.store_dir } - - it { is_expected.to start_with(path) } - it { is_expected.to end_with("#{job.project_id}/#{job.id}") } - end - - describe '#cache_dir' do - subject { uploader.cache_dir } - - it { is_expected.to start_with(path) } - it { is_expected.to end_with('/tmp/cache') } - end - - describe '#work_dir' do - subject { uploader.work_dir } - - it { is_expected.to start_with(path) } - it { is_expected.to end_with('/tmp/work') } - end - - describe '#filename' do - # we need to use uploader, as this makes to use mounter - # which initialises uploader.file object - let(:uploader) { job.artifacts_file } - - subject { uploader.filename } - - it { is_expected.to be_nil } - - context 'with artifacts' do - let(:job) { create(:ci_build, :artifacts) } - - it { is_expected.not_to be_nil } - end - end -end diff --git a/spec/uploaders/legacy_artifact_uploader_spec.rb b/spec/uploaders/legacy_artifact_uploader_spec.rb new file mode 100644 index 00000000000..1d9adaccd8d --- /dev/null +++ b/spec/uploaders/legacy_artifact_uploader_spec.rb @@ -0,0 +1,61 @@ +require 'rails_helper' + +describe LegacyArtifactUploader do + set(:job) { create(:ci_build) } + let(:uploader) { described_class.new(job, :artifacts_file) } + let(:path) { Gitlab.config.artifacts.path } + + describe '.local_artifacts_store' do + subject { described_class.local_artifacts_store } + + it "delegate to artifacts path" do + expect(Gitlab.config.artifacts).to receive(:path) + + subject + end + end + + describe '.artifacts_upload_path' do + subject { described_class.artifacts_upload_path } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with('tmp/uploads/') } + end + + describe '#store_dir' do + subject { uploader.store_dir } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with("#{job.project_id}/#{job.id}") } + end + + describe '#cache_dir' do + subject { uploader.cache_dir } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with('/tmp/cache') } + end + + describe '#work_dir' do + subject { uploader.work_dir } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with('/tmp/work') } + end + + describe '#filename' do + # we need to use uploader, as this makes to use mounter + # which initialises uploader.file object + let(:uploader) { job.artifacts_file } + + subject { uploader.filename } + + it { is_expected.to be_nil } + + context 'with artifacts' do + let(:job) { create(:ci_build, :artifacts) } + + it { is_expected.not_to be_nil } + end + end +end -- cgit v1.2.1 From 38c61ab6df15fbd1eab22a8dff8da01b17c075f3 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 23 Nov 2017 18:51:20 +0100 Subject: Fix specs failures, and use factory with `:ci_job_artifact, :archive` --- app/uploaders/job_artifact_uploader.rb | 20 +++++++++++-- app/uploaders/legacy_artifact_uploader.rb | 2 +- spec/factories/ci/builds.rb | 4 +-- spec/factories/ci/job_artifacts.rb | 28 +++++++++--------- spec/migrations/migrate_old_artifacts_spec.rb | 2 +- spec/models/ci/job_artifact_spec.rb | 2 +- spec/models/project_statistics_spec.rb | 2 +- spec/uploaders/job_artifact_uploader_spec.rb | 38 +++++++++++++++++++++++-- spec/uploaders/legacy_artifact_uploader_spec.rb | 22 ++++++++++++-- 9 files changed, 94 insertions(+), 26 deletions(-) diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb index d54411e198f..15dfb5a5763 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -1,12 +1,12 @@ class JobArtifactUploader < GitlabUploader storage :file - def self.local_artifacts_store + def self.local_store_path Gitlab.config.artifacts.path end def self.artifacts_upload_path - File.join(self.local_artifacts_store, 'tmp/uploads/') + File.join(self.local_store_path, 'tmp/uploads/') end def size @@ -15,8 +15,24 @@ class JobArtifactUploader < GitlabUploader model.size end + def store_dir + default_local_path + end + + def cache_dir + File.join(self.class.local_store_path, 'tmp/cache') + end + + def work_dir + File.join(self.class.local_store_path, 'tmp/work') + end + private + def default_local_path + File.join(self.class.local_store_path, default_path) + end + def default_path creation_date = model.created_at.utc.strftime('%Y_%m_%d') diff --git a/app/uploaders/legacy_artifact_uploader.rb b/app/uploaders/legacy_artifact_uploader.rb index 0c23e05b680..4f7f8a63108 100644 --- a/app/uploaders/legacy_artifact_uploader.rb +++ b/app/uploaders/legacy_artifact_uploader.rb @@ -6,7 +6,7 @@ class LegacyArtifactUploader < GitlabUploader end def self.artifacts_upload_path - File.join(self.local_artifacts_store, 'tmp/uploads/') + File.join(self.local_store_path, 'tmp/uploads/') end def store_dir diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 441f740e1e5..c868525cbc0 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -167,8 +167,8 @@ FactoryGirl.define do trait :artifacts do after(:create) do |build| - create(:ci_job_artifact, job: build) - create(:ci_job_metadata, job: build) + create(:ci_job_artifact, :archive, job: build) + create(:ci_job_artifact, :metadata, job: build) build.reload end end diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb index 0abebd14286..47c9842e698 100644 --- a/spec/factories/ci/job_artifacts.rb +++ b/spec/factories/ci/job_artifacts.rb @@ -9,22 +9,24 @@ FactoryGirl.define do artifact.project ||= artifact.job.project end - after :create do |artifact| - if artifact.archive? - artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), - 'application/zip') - artifact.save + trait :archive do + after(:create) do |artifact, _| + artifact.update!( + file_type: :archive, + file: fixture_file_upload( + Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') + ) end end - end - - factory :ci_job_metadata, parent: :ci_job_artifact do - file_type :metadata - after :create do |artifact| - artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), - 'application/x-gzip') - artifact.save + trait :metadata do + after(:create) do |artifact, _| + artifact.update!( + file_type: :metadata, + file: fixture_file_upload( + Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), 'application/x-gzip') + ) + end end end end diff --git a/spec/migrations/migrate_old_artifacts_spec.rb b/spec/migrations/migrate_old_artifacts_spec.rb index b09243e5c95..92eb1d9ce86 100644 --- a/spec/migrations/migrate_old_artifacts_spec.rb +++ b/spec/migrations/migrate_old_artifacts_spec.rb @@ -40,7 +40,7 @@ describe MigrateOldArtifacts do end it "legacy artifacts are set" do - expect(build_with_legacy_artifacts.artifacts_file_identifier).not_to be_nil + expect(build_with_legacy_artifacts.legacy_artifacts_file_identifier).not_to be_nil end describe '#min_id' do diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index 5202a8183af..9dd5cba150d 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Ci::JobArtifact do - set(:artifact) { create(:ci_job_artifact) } + set(:artifact) { create(:ci_job_artifact, :archive) } describe "Associations" do it { is_expected.to belong_to(:project) } diff --git a/spec/models/project_statistics_spec.rb b/spec/models/project_statistics_spec.rb index 95e8d519bdd..4496f91fc5e 100644 --- a/spec/models/project_statistics_spec.rb +++ b/spec/models/project_statistics_spec.rb @@ -137,7 +137,7 @@ describe ProjectStatistics do before do create(:ci_build, pipeline: pipeline, artifacts_size: 56.megabytes) - create(:ci_job_artifact, project: pipeline.project, job: ci_build) + create(:ci_job_artifact, :archive, project: pipeline.project, job: ci_build) statistics.update_build_artifacts_size end diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb index d045acf9089..bb2cc52381d 100644 --- a/spec/uploaders/job_artifact_uploader_spec.rb +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -2,14 +2,46 @@ require 'spec_helper' describe JobArtifactUploader do set(:job_artifact) { create(:ci_job_artifact) } - let(:job) { job_artifact.job } let(:uploader) { described_class.new(job_artifact, :file) } + let(:path) { Gitlab.config.artifacts.path } describe '#store_dir' do subject { uploader.store_dir } - it { is_expected.to start_with(Gitlab.config.artifacts.path) } - it { is_expected.not_to end_with("#{job.project_id}/#{job.created_at.utc.strftime('%Y_%m')}/#{job.id}") } + it { is_expected.to start_with(path) } + it { is_expected.not_to end_with("#{job_artifact.project_id}/#{job_artifact.created_at.utc.strftime('%Y_%m')}/#{job_artifact.id}") } it { is_expected.to match(/\h{2}\/\h{2}\/\h{64}\/\d{4}_\d{1,2}_\d{1,2}\/\d+\/\d+\z/) } end + + describe '#cache_dir' do + subject { uploader.cache_dir } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with('/tmp/cache') } + end + + describe '#work_dir' do + subject { uploader.work_dir } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with('/tmp/work') } + end + + context 'file is stored in valid path' do + let(:file) do + fixture_file_upload(Rails.root.join( + 'spec/fixtures/ci_build_artifacts.zip'), 'application/zip') + end + + before do + uploader.store!(file) + end + + subject { uploader.file.path } + + it { is_expected.to start_with(path) } + it { is_expected.to include("/#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/") } + it { is_expected.to include("/#{job_artifact.project_id.to_s}/") } + it { is_expected.to end_with("ci_build_artifacts.zip") } + end end diff --git a/spec/uploaders/legacy_artifact_uploader_spec.rb b/spec/uploaders/legacy_artifact_uploader_spec.rb index 1d9adaccd8d..203630de91c 100644 --- a/spec/uploaders/legacy_artifact_uploader_spec.rb +++ b/spec/uploaders/legacy_artifact_uploader_spec.rb @@ -5,8 +5,8 @@ describe LegacyArtifactUploader do let(:uploader) { described_class.new(job, :artifacts_file) } let(:path) { Gitlab.config.artifacts.path } - describe '.local_artifacts_store' do - subject { described_class.local_artifacts_store } + describe '.local_store_path' do + subject { described_class.local_store_path } it "delegate to artifacts path" do expect(Gitlab.config.artifacts).to receive(:path) @@ -58,4 +58,22 @@ describe LegacyArtifactUploader do it { is_expected.not_to be_nil } end end + + context 'file is stored in valid path' do + let(:file) do + fixture_file_upload(Rails.root.join( + 'spec/fixtures/ci_build_artifacts.zip'), 'application/zip') + end + + before do + uploader.store!(file) + end + + subject { uploader.file.path } + + it { is_expected.to start_with(path) } + it { is_expected.to include("/#{job.created_at.utc.strftime('%Y_%m')}/") } + it { is_expected.to include("/#{job.project_id.to_s}/") } + it { is_expected.to end_with("ci_build_artifacts.zip") } + end end -- cgit v1.2.1 From ee8efb3d67ba8b8b2ece9962cd2aa79063fffaa0 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 23 Nov 2017 19:41:49 +0100 Subject: Sync ArtifactUploader specs with EE --- spec/uploaders/job_artifact_uploader_spec.rb | 22 +++++++++++-------- spec/uploaders/legacy_artifact_uploader_spec.rb | 28 ++++++++++++------------- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb index bb2cc52381d..e80d5272a4a 100644 --- a/spec/uploaders/job_artifact_uploader_spec.rb +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -1,33 +1,37 @@ require 'spec_helper' describe JobArtifactUploader do - set(:job_artifact) { create(:ci_job_artifact) } + let(:job_artifact) { create(:ci_job_artifact) } let(:uploader) { described_class.new(job_artifact, :file) } - let(:path) { Gitlab.config.artifacts.path } + let(:local_path) { Gitlab.config.artifacts.path } describe '#store_dir' do subject { uploader.store_dir } - it { is_expected.to start_with(path) } - it { is_expected.not_to end_with("#{job_artifact.project_id}/#{job_artifact.created_at.utc.strftime('%Y_%m')}/#{job_artifact.id}") } - it { is_expected.to match(/\h{2}\/\h{2}\/\h{64}\/\d{4}_\d{1,2}_\d{1,2}\/\d+\/\d+\z/) } + let(:path) { "#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/#{job_artifact.project_id}/#{job_artifact.id}" } + + context 'when using local storage' do + it { is_expected.to start_with(local_path) } + it { is_expected.to match(/\h{2}\/\h{2}\/\h{64}\/\d{4}_\d{1,2}_\d{1,2}\/\d+\/\d+\z/) } + it { is_expected.to end_with(path) } + end end describe '#cache_dir' do subject { uploader.cache_dir } - it { is_expected.to start_with(path) } + it { is_expected.to start_with(local_path) } it { is_expected.to end_with('/tmp/cache') } end describe '#work_dir' do subject { uploader.work_dir } - it { is_expected.to start_with(path) } + it { is_expected.to start_with(local_path) } it { is_expected.to end_with('/tmp/work') } end - context 'file is stored in valid path' do + context 'file is stored in valid local_path' do let(:file) do fixture_file_upload(Rails.root.join( 'spec/fixtures/ci_build_artifacts.zip'), 'application/zip') @@ -39,7 +43,7 @@ describe JobArtifactUploader do subject { uploader.file.path } - it { is_expected.to start_with(path) } + it { is_expected.to start_with(local_path) } it { is_expected.to include("/#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/") } it { is_expected.to include("/#{job_artifact.project_id.to_s}/") } it { is_expected.to end_with("ci_build_artifacts.zip") } diff --git a/spec/uploaders/legacy_artifact_uploader_spec.rb b/spec/uploaders/legacy_artifact_uploader_spec.rb index 203630de91c..714976b92c1 100644 --- a/spec/uploaders/legacy_artifact_uploader_spec.rb +++ b/spec/uploaders/legacy_artifact_uploader_spec.rb @@ -1,9 +1,9 @@ require 'rails_helper' describe LegacyArtifactUploader do - set(:job) { create(:ci_build) } - let(:uploader) { described_class.new(job, :artifacts_file) } - let(:path) { Gitlab.config.artifacts.path } + let(:job) { create(:ci_build) } + let(:uploader) { described_class.new(job, :legacy_artifacts_file) } + let(:local_path) { Gitlab.config.artifacts.path } describe '.local_store_path' do subject { described_class.local_store_path } @@ -18,28 +18,32 @@ describe LegacyArtifactUploader do describe '.artifacts_upload_path' do subject { described_class.artifacts_upload_path } - it { is_expected.to start_with(path) } + it { is_expected.to start_with(local_path) } it { is_expected.to end_with('tmp/uploads/') } end describe '#store_dir' do subject { uploader.store_dir } - it { is_expected.to start_with(path) } - it { is_expected.to end_with("#{job.project_id}/#{job.id}") } + let(:path) { "#{job.created_at.utc.strftime('%Y_%m')}/#{job.project_id}/#{job.id}" } + + context 'when using local storage' do + it { is_expected.to start_with(local_path) } + it { is_expected.to end_with(path) } + end end describe '#cache_dir' do subject { uploader.cache_dir } - it { is_expected.to start_with(path) } + it { is_expected.to start_with(local_path) } it { is_expected.to end_with('/tmp/cache') } end describe '#work_dir' do subject { uploader.work_dir } - it { is_expected.to start_with(path) } + it { is_expected.to start_with(local_path) } it { is_expected.to end_with('/tmp/work') } end @@ -51,12 +55,6 @@ describe LegacyArtifactUploader do subject { uploader.filename } it { is_expected.to be_nil } - - context 'with artifacts' do - let(:job) { create(:ci_build, :artifacts) } - - it { is_expected.not_to be_nil } - end end context 'file is stored in valid path' do @@ -71,7 +69,7 @@ describe LegacyArtifactUploader do subject { uploader.file.path } - it { is_expected.to start_with(path) } + it { is_expected.to start_with(local_path) } it { is_expected.to include("/#{job.created_at.utc.strftime('%Y_%m')}/") } it { is_expected.to include("/#{job.project_id.to_s}/") } it { is_expected.to end_with("ci_build_artifacts.zip") } -- cgit v1.2.1 From 6881d0917458f9b62542ee2908e2d258c75a8537 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 23 Nov 2017 19:53:31 +0100 Subject: Fix rubocop --- spec/uploaders/job_artifact_uploader_spec.rb | 6 +++--- spec/uploaders/legacy_artifact_uploader_spec.rb | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb index e80d5272a4a..14fd5f3600f 100644 --- a/spec/uploaders/job_artifact_uploader_spec.rb +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -33,8 +33,8 @@ describe JobArtifactUploader do context 'file is stored in valid local_path' do let(:file) do - fixture_file_upload(Rails.root.join( - 'spec/fixtures/ci_build_artifacts.zip'), 'application/zip') + fixture_file_upload( + Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') end before do @@ -45,7 +45,7 @@ describe JobArtifactUploader do it { is_expected.to start_with(local_path) } it { is_expected.to include("/#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/") } - it { is_expected.to include("/#{job_artifact.project_id.to_s}/") } + it { is_expected.to include("/#{job_artifact.project_id}/") } it { is_expected.to end_with("ci_build_artifacts.zip") } end end diff --git a/spec/uploaders/legacy_artifact_uploader_spec.rb b/spec/uploaders/legacy_artifact_uploader_spec.rb index 714976b92c1..efeffb78772 100644 --- a/spec/uploaders/legacy_artifact_uploader_spec.rb +++ b/spec/uploaders/legacy_artifact_uploader_spec.rb @@ -59,8 +59,8 @@ describe LegacyArtifactUploader do context 'file is stored in valid path' do let(:file) do - fixture_file_upload(Rails.root.join( - 'spec/fixtures/ci_build_artifacts.zip'), 'application/zip') + fixture_file_upload( + Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') end before do @@ -71,7 +71,7 @@ describe LegacyArtifactUploader do it { is_expected.to start_with(local_path) } it { is_expected.to include("/#{job.created_at.utc.strftime('%Y_%m')}/") } - it { is_expected.to include("/#{job.project_id.to_s}/") } + it { is_expected.to include("/#{job.project_id}/") } it { is_expected.to end_with("ci_build_artifacts.zip") } end end -- cgit v1.2.1 From e35f16074d065cc60f6cd99b4b46c09e437f129c Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Fri, 24 Nov 2017 09:23:56 +0100 Subject: Test new artifacts for pages deploy --- app/services/projects/update_pages_service.rb | 2 +- .../services/projects/update_pages_service_spec.rb | 108 ++++++++++++++++++--- 2 files changed, 93 insertions(+), 17 deletions(-) diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index d34903c9989..a773222bf17 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -18,7 +18,7 @@ module Projects @status.enqueue! @status.run! - raise 'missing pages artifacts' unless build.artifacts_file? + raise 'missing pages artifacts' unless build.artifacts? raise 'pages are outdated' unless latest? # Create temporary directory in which we will extract the artifacts diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index a0c83600f39..8e0965b444b 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -1,10 +1,18 @@ require "spec_helper" describe Projects::UpdatePagesService do - let(:project) { create(:project, :repository) } - let(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) } - let(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') } + set(:project) { create(:project, :repository) } + set(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit('HEAD').sha) } + set(:build) { create(:ci_build, pipeline: pipeline, ref: 'HEAD') } let(:invalid_file) { fixture_file_upload(Rails.root + 'spec/fixtures/dk.png') } + let(:extension) { 'zip' } + + let(:file) { fixture_file_upload(Rails.root + "spec/fixtures/pages.#{extension}") } + let(:empty_file) { fixture_file_upload(Rails.root + "spec/fixtures/pages_empty.#{extension}") } + let(:metadata) do + filename = Rails.root + "spec/fixtures/pages.#{extension}.meta" + fixture_file_upload(filename) if File.exist?(filename) + end subject { described_class.new(project, build) } @@ -12,18 +20,85 @@ describe Projects::UpdatePagesService do project.remove_pages end - %w(tar.gz zip).each do |format| - context "for valid #{format}" do - let(:file) { fixture_file_upload(Rails.root + "spec/fixtures/pages.#{format}") } - let(:empty_file) { fixture_file_upload(Rails.root + "spec/fixtures/pages_empty.#{format}") } - let(:metadata) do - filename = Rails.root + "spec/fixtures/pages.#{format}.meta" - fixture_file_upload(filename) if File.exist?(filename) + context 'legacy artifacts' do + %w(tar.gz zip).each do |format| + let(:extension) { format } + + context "for valid #{format}" do + before do + build.update_attributes(legacy_artifacts_file: file) + build.update_attributes(legacy_artifacts_metadata: metadata) + end + + describe 'pages artifacts' do + context 'with expiry date' do + before do + build.artifacts_expire_in = "2 days" + end + + it "doesn't delete artifacts" do + expect(execute).to eq(:success) + + expect(build.reload.artifacts_file?).to eq(true) + end + end + + context 'without expiry date' do + it "does delete artifacts" do + expect(execute).to eq(:success) + + expect(build.reload.artifacts_file?).to eq(false) + end + end + end + + it 'succeeds' do + expect(project.pages_deployed?).to be_falsey + expect(execute).to eq(:success) + expect(project.pages_deployed?).to be_truthy + + # Check that all expected files are extracted + %w[index.html zero .hidden/file].each do |filename| + expect(File.exist?(File.join(project.public_pages_path, filename))).to be_truthy + end + end + + it 'limits pages size' do + stub_application_setting(max_pages_size: 1) + expect(execute).not_to eq(:success) + end + + it 'removes pages after destroy' do + expect(PagesWorker).to receive(:perform_in) + expect(project.pages_deployed?).to be_falsey + expect(execute).to eq(:success) + expect(project.pages_deployed?).to be_truthy + project.destroy + expect(project.pages_deployed?).to be_falsey + end + + it 'fails if sha on branch is not latest' do + build.update_attributes(ref: 'feature') + + expect(execute).not_to eq(:success) + end + + it 'fails for empty file fails' do + build.update_attributes(legacy_artifacts_file: empty_file) + + expect(execute).not_to eq(:success) + end end + end + end + context 'for new artifacts' do + context "for a valid job" do before do - build.update_attributes(legacy_artifacts_file: file) - build.update_attributes(legacy_artifacts_metadata: metadata) + create(:ci_job_artifact, file: file, job: build) + create(:ci_job_artifact, file_type: :metadata, file: metadata, job: build) + + build.reload end describe 'pages artifacts' do @@ -35,7 +110,7 @@ describe Projects::UpdatePagesService do it "doesn't delete artifacts" do expect(execute).to eq(:success) - expect(build.reload.artifacts_file?).to eq(true) + expect(build.artifacts_file?).to eq(true) end end @@ -74,13 +149,14 @@ describe Projects::UpdatePagesService do end it 'fails if sha on branch is not latest' do - pipeline.update_attributes(sha: 'old_sha') - build.update_attributes(legacy_artifacts_file: file) + build.update_attributes(ref: 'feature') + expect(execute).not_to eq(:success) end it 'fails for empty file fails' do - build.update_attributes(legacy_artifacts_file: empty_file) + build.job_archive.update_attributes(file: empty_file) + expect(execute).not_to eq(:success) end end -- cgit v1.2.1 From bf6126b1ecd63825070e391ebde86da9cd9dfd5e Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 27 Nov 2017 11:09:35 +0100 Subject: Add coverage on legacy artifacts for Ci::Build --- spec/models/ci/build_spec.rb | 241 +++++++++++++++++++++++---------- spec/models/project_statistics_spec.rb | 26 +++- 2 files changed, 190 insertions(+), 77 deletions(-) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index f8dbed91c1a..fefb54ad6f7 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -132,27 +132,55 @@ describe Ci::Build do end describe '#artifacts?' do - let(:build) { create(:ci_build, :artifacts) } + context 'when new artifacts are used' do + let(:build) { create(:ci_build, :artifacts) } - subject { build.artifacts? } + subject { build.artifacts? } - context 'artifacts archive does not exist' do - let(:build) { create(:ci_build) } + context 'artifacts archive does not exist' do + let(:build) { create(:ci_build) } - it { is_expected.to be_falsy } + it { is_expected.to be_falsy } + end + + context 'artifacts archive exists' do + it { is_expected.to be_truthy } + + context 'is expired' do + let!(:build) { create(:ci_build, :artifacts, :expired) } + + it { is_expected.to be_falsy } + end + + context 'is not expired' do + it { is_expected.to be_truthy } + end + end end - context 'artifacts archive exists' do - it { is_expected.to be_truthy } + context 'when legacy artifacts are used' do + let(:build) { create(:ci_build, :legacy_artifacts) } + + subject { build.artifacts? } - context 'is expired' do - let!(:build) { create(:ci_build, :artifacts, :expired) } + context 'artifacts archive does not exist' do + let(:build) { create(:ci_build) } it { is_expected.to be_falsy } end - context 'is not expired' do + context 'artifacts archive exists' do it { is_expected.to be_truthy } + + context 'is expired' do + let!(:build) { create(:ci_build, :legacy_artifacts, :expired) } + + it { is_expected.to be_falsy } + end + + context 'is not expired' do + it { is_expected.to be_truthy } + end end end end @@ -607,71 +635,144 @@ describe Ci::Build do describe '#erasable?' do subject { build.erasable? } + it { is_expected.to eq false } end end context 'build is erasable' do - let!(:build) { create(:ci_build, :trace, :success, :artifacts) } + context 'new artifacts' do + let!(:build) { create(:ci_build, :trace, :success, :artifacts) } - describe '#erase' do - before do - build.erase(erased_by: user) - end + describe '#erase' do + before do + build.erase(erased_by: user) + end - context 'erased by user' do - let!(:user) { create(:user, username: 'eraser') } + context 'erased by user' do + let!(:user) { create(:user, username: 'eraser') } - include_examples 'erasable' + include_examples 'erasable' - it 'records user who erased a build' do - expect(build.erased_by).to eq user + it 'records user who erased a build' do + expect(build.erased_by).to eq user + end end - end - context 'erased by system' do - let(:user) { nil } + context 'erased by system' do + let(:user) { nil } - include_examples 'erasable' + include_examples 'erasable' - it 'does not set user who erased a build' do - expect(build.erased_by).to be_nil + it 'does not set user who erased a build' do + expect(build.erased_by).to be_nil + end end end - end - describe '#erasable?' do - subject { build.erasable? } - it { is_expected.to be_truthy } - end + describe '#erasable?' do + subject { build.erasable? } + it { is_expected.to be_truthy } + end - describe '#erased?' do - let!(:build) { create(:ci_build, :trace, :success, :artifacts) } - subject { build.erased? } + describe '#erased?' do + let!(:build) { create(:ci_build, :trace, :success, :artifacts) } + subject { build.erased? } - context 'job has not been erased' do - it { is_expected.to be_falsey } + context 'job has not been erased' do + it { is_expected.to be_falsey } + end + + context 'job has been erased' do + before do + build.erase + end + + it { is_expected.to be_truthy } + end end - context 'job has been erased' do + context 'metadata and build trace are not available' do + let!(:build) { create(:ci_build, :success, :artifacts) } + before do - build.erase + build.remove_artifacts_metadata! end - it { is_expected.to be_truthy } + describe '#erase' do + it 'does not raise error' do + expect { build.erase }.not_to raise_error + end + end end end + end - context 'metadata and build trace are not available' do - let!(:build) { create(:ci_build, :success, :artifacts) } + context 'old artifacts' do + context 'build is erasable' do + context 'new artifacts' do + let!(:build) { create(:ci_build, :trace, :success, :legacy_artifacts) } - before do - build.remove_artifacts_metadata! - end + describe '#erase' do + before do + build.erase(erased_by: user) + end - describe '#erase' do - it 'does not raise error' do - expect { build.erase }.not_to raise_error + context 'erased by user' do + let!(:user) { create(:user, username: 'eraser') } + + include_examples 'erasable' + + it 'records user who erased a build' do + expect(build.erased_by).to eq user + end + end + + context 'erased by system' do + let(:user) { nil } + + include_examples 'erasable' + + it 'does not set user who erased a build' do + expect(build.erased_by).to be_nil + end + end + end + + describe '#erasable?' do + subject { build.erasable? } + it { is_expected.to be_truthy } + end + + describe '#erased?' do + let!(:build) { create(:ci_build, :trace, :success, :legacy_artifacts) } + subject { build.erased? } + + context 'job has not been erased' do + it { is_expected.to be_falsey } + end + + context 'job has been erased' do + before do + build.erase + end + + it { is_expected.to be_truthy } + end + end + + context 'metadata and build trace are not available' do + let!(:build) { create(:ci_build, :success, :legacy_artifacts) } + + before do + build.remove_artifacts_metadata! + end + + describe '#erase' do + it 'does not raise error' do + expect { build.erase }.not_to raise_error + end + end end end end @@ -917,9 +1018,9 @@ describe Ci::Build do describe '#merge_request' do def create_mr(build, pipeline, factory: :merge_request, created_at: Time.now) create(factory, source_project: pipeline.project, - target_project: pipeline.project, - source_branch: build.ref, - created_at: created_at) + target_project: pipeline.project, + source_branch: build.ref, + created_at: created_at) end context 'when a MR has a reference to the pipeline' do @@ -1218,7 +1319,7 @@ describe Ci::Build do context 'when `when` is undefined' do before do build.when = nil - end + end context 'use from gitlab-ci.yml' do let(:pipeline) { create(:ci_pipeline) } @@ -1236,10 +1337,10 @@ describe Ci::Build do context 'when config does not have a questioned job' do let(:config) do YAML.dump({ - test_other: { - script: 'Hello World' - } - }) + test_other: { + script: 'Hello World' + } + }) end it { is_expected.to eq('on_success') } @@ -1248,18 +1349,18 @@ describe Ci::Build do context 'when config has `when`' do let(:config) do YAML.dump({ - test: { - script: 'Hello World', - when: 'always' - } - }) + test: { + script: 'Hello World', + when: 'always' + } + }) end it { is_expected.to eq('always') } end end end - end + end describe '#variables' do let(:container_registry_enabled) { false } @@ -1333,10 +1434,10 @@ describe Ci::Build do let!(:environment) do create(:environment, - project: build.project, - name: 'production', - slug: 'prod-slug', - external_url: '') + project: build.project, + name: 'production', + slug: 'prod-slug', + external_url: '') end before do @@ -1362,7 +1463,7 @@ describe Ci::Build do before do environment_variables << - { key: 'CI_ENVIRONMENT_URL', value: url, public: true } + { key: 'CI_ENVIRONMENT_URL', value: url, public: true } end context 'when the URL was set from the job' do @@ -1560,8 +1661,8 @@ describe Ci::Build do let!(:pipeline_schedule_variable) do create(:ci_pipeline_schedule_variable, - key: 'SCHEDULE_VARIABLE_KEY', - pipeline_schedule: pipeline_schedule) + key: 'SCHEDULE_VARIABLE_KEY', + pipeline_schedule: pipeline_schedule) end before do @@ -1703,8 +1804,8 @@ describe Ci::Build do allow_any_instance_of(Project) .to receive(:secret_variables_for) .with(ref: 'master', environment: nil) do - [create(:ci_variable, key: 'secret', value: 'value')] - end + [create(:ci_variable, key: 'secret', value: 'value')] + end allow_any_instance_of(Ci::Pipeline) .to receive(:predefined_variables) { [pipeline_pre_var] } diff --git a/spec/models/project_statistics_spec.rb b/spec/models/project_statistics_spec.rb index 4496f91fc5e..e78ed1df821 100644 --- a/spec/models/project_statistics_spec.rb +++ b/spec/models/project_statistics_spec.rb @@ -133,17 +133,29 @@ describe ProjectStatistics do describe '#update_build_artifacts_size' do let!(:pipeline) { create(:ci_pipeline, project: project) } - let!(:ci_build) { create(:ci_build, pipeline: pipeline, artifacts_size: 45.megabytes) } - before do - create(:ci_build, pipeline: pipeline, artifacts_size: 56.megabytes) - create(:ci_job_artifact, :archive, project: pipeline.project, job: ci_build) + context 'when new job artifacts are calculated' do + let(:ci_build) { create(:ci_build, pipeline: pipeline) } + + before do + create(:ci_job_artifact, :archive, project: pipeline.project, job: ci_build) + end + + it "stores the size of related build artifacts" do + statistics.update_build_artifacts_size - statistics.update_build_artifacts_size + expect(statistics.build_artifacts_size).to be(106365) + end end - it "stores the size of related build artifacts" do - expect(statistics.build_artifacts_size).to eq(106012541) + context 'when legacy artifacts are used' do + let!(:ci_build) { create(:ci_build, pipeline: pipeline, artifacts_size: 10.megabytes) } + + it "stores the size of related build artifacts" do + statistics.update_build_artifacts_size + + expect(statistics.build_artifacts_size).to eq(10.megabytes) + end end end -- cgit v1.2.1 From f5d3b92155b76d2459014372e00d877b21408f49 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 27 Nov 2017 14:31:26 +0100 Subject: Remove Ci::Build#artifacts_file? --- app/models/concerns/artifact_migratable.rb | 4 ---- lib/gitlab/workhorse.rb | 2 +- spec/services/projects/update_pages_service_spec.rb | 8 ++++---- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index 5c647bacf1b..811a8252459 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -6,10 +6,6 @@ module ArtifactMigratable job_archive&.file || legacy_artifacts_file end - def artifacts_file? - job_archive&.file? || legacy_artifacts_file? - end - def artifacts_metadata job_metadata&.file || legacy_artifacts_metadata end diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index c3e2742306d..5ab6cd5a4ef 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -58,7 +58,7 @@ module Gitlab end def artifact_upload_ok - { TempPath: LegacyArtifactUploader.artifacts_upload_path } + { TempPath: JobArtifactUploader.artifacts_upload_path } end def send_git_blob(repository, blob) diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 8e0965b444b..b669a0102bd 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -39,7 +39,7 @@ describe Projects::UpdatePagesService do it "doesn't delete artifacts" do expect(execute).to eq(:success) - expect(build.reload.artifacts_file?).to eq(true) + expect(build.reload.artifacts?).to eq(true) end end @@ -47,7 +47,7 @@ describe Projects::UpdatePagesService do it "does delete artifacts" do expect(execute).to eq(:success) - expect(build.reload.artifacts_file?).to eq(false) + expect(build.reload.artifacts?).to eq(false) end end end @@ -110,7 +110,7 @@ describe Projects::UpdatePagesService do it "doesn't delete artifacts" do expect(execute).to eq(:success) - expect(build.artifacts_file?).to eq(true) + expect(build.artifacts?).to eq(true) end end @@ -118,7 +118,7 @@ describe Projects::UpdatePagesService do it "does delete artifacts" do expect(execute).to eq(:success) - expect(build.reload.artifacts_file?).to eq(false) + expect(build.reload.artifacts?).to eq(false) end end end -- cgit v1.2.1 From 2a620e7412c8a658bae9a5b7c9b55b9ac97dd58e Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 28 Nov 2017 09:38:24 +0100 Subject: Remove hook set by carrierwave too --- app/models/ci/job_artifact.rb | 1 - spec/models/ci/build_spec.rb | 12 ++++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index cc28092978c..e02fa021409 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -6,7 +6,6 @@ module Ci belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id before_save :set_size, if: :file_changed? - after_commit :remove_file!, on: :destroy mount_uploader :file, JobArtifactUploader diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index fefb54ad6f7..212ff36633b 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1018,9 +1018,9 @@ describe Ci::Build do describe '#merge_request' do def create_mr(build, pipeline, factory: :merge_request, created_at: Time.now) create(factory, source_project: pipeline.project, - target_project: pipeline.project, - source_branch: build.ref, - created_at: created_at) + target_project: pipeline.project, + source_branch: build.ref, + created_at: created_at) end context 'when a MR has a reference to the pipeline' do @@ -1319,7 +1319,7 @@ describe Ci::Build do context 'when `when` is undefined' do before do build.when = nil - end + end context 'use from gitlab-ci.yml' do let(:pipeline) { create(:ci_pipeline) } @@ -1360,7 +1360,7 @@ describe Ci::Build do end end end - end + end describe '#variables' do let(:container_registry_enabled) { false } @@ -1463,7 +1463,7 @@ describe Ci::Build do before do environment_variables << - { key: 'CI_ENVIRONMENT_URL', value: url, public: true } + { key: 'CI_ENVIRONMENT_URL', value: url, public: true } end context 'when the URL was set from the job' do -- cgit v1.2.1 From 2045a771bfa5a1a32ff04f5bc23d58f1170b6359 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 30 Nov 2017 11:06:43 +0100 Subject: Rename `job_archive|metadata` to `artifacts_archive|metadata` --- app/models/ci/build.rb | 4 ++-- app/models/concerns/artifact_migratable.rb | 16 ++++++++-------- app/models/project_statistics.rb | 3 +++ spec/services/ci/retry_build_service_spec.rb | 2 +- .../expire_build_instance_artifacts_worker_spec.rb | 6 +++--- 5 files changed, 17 insertions(+), 14 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 91747da28a1..842323e26e3 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -16,8 +16,8 @@ module Ci has_many :trace_sections, class_name: 'Ci::BuildTraceSection' has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id - has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :artifacts_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :artifacts_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index 811a8252459..b99585a0b65 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -3,11 +3,11 @@ # Meant to be prepended so the interface can stay the same module ArtifactMigratable def artifacts_file - job_archive&.file || legacy_artifacts_file + artifacts_archive&.file || legacy_artifacts_file end def artifacts_metadata - job_metadata&.file || legacy_artifacts_metadata + artifacts_metadata&.file || legacy_artifacts_metadata end def artifacts? @@ -19,20 +19,20 @@ module ArtifactMigratable end def artifacts_file_changed? - job_archive&.file_changed? || attribute_changed?(:artifacts_file) + artifacts_archive&.file_changed? || attribute_changed?(:artifacts_file) end def remove_artifacts_file! - if job_archive - job_archive.destroy + if artifacts_archive + artifacts_archive.destroy else remove_legacy_artifacts_file! end end def remove_artifacts_metadata! - if job_metadata - job_metadata.destroy + if artifacts_metadata + artifacts_metadata.destroy else remove_legacy_artifacts_metadata! end @@ -40,6 +40,6 @@ module ArtifactMigratable def artifacts_size read_attribute(:artifacts_size).to_i + - job_archive&.size.to_i + job_metadata&.size.to_i + artifacts_archive&.size.to_i + artifacts_metadata&.size.to_i end end diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index 17b9d2cf7b4..b3d4c82583c 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -35,6 +35,9 @@ class ProjectStatistics < ActiveRecord::Base end def update_build_artifacts_size + # REMARK: + # We perform dual calculation as we run SQL query: sum does not instantiate AR model. + # The Ci::Build#artifacts_size returns sum of ci_builds.artifacts_size and ci_job_artifacts.file_size self.build_artifacts_size = project.builds.sum(:artifacts_size) + Ci::JobArtifact.artifacts_size_for(self) diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 20fe80beade..3078e5b35e4 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -17,7 +17,7 @@ describe Ci::RetryBuildService do %i[id status user token coverage trace runner artifacts_expire_at artifacts_file artifacts_metadata artifacts_size created_at updated_at started_at finished_at queued_at erased_by - erased_at auto_canceled_by job_artifacts job_archive job_metadata].freeze + erased_at auto_canceled_by job_artifacts artifacts_archive artifacts_metadata].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections diff --git a/spec/workers/expire_build_instance_artifacts_worker_spec.rb b/spec/workers/expire_build_instance_artifacts_worker_spec.rb index c0d2b1b7411..05e2aa0703b 100644 --- a/spec/workers/expire_build_instance_artifacts_worker_spec.rb +++ b/spec/workers/expire_build_instance_artifacts_worker_spec.rb @@ -23,7 +23,7 @@ describe ExpireBuildInstanceArtifactsWorker do end it 'does remove the job artifact record' do - expect(build.reload.job_archive).to be_nil + expect(build.reload.artifacts_archive).to be_nil end end end @@ -42,7 +42,7 @@ describe ExpireBuildInstanceArtifactsWorker do end it 'does not remove the job artifact record' do - expect(build.reload.job_archive).not_to be_nil + expect(build.reload.artifacts_archive).not_to be_nil end end @@ -58,7 +58,7 @@ describe ExpireBuildInstanceArtifactsWorker do end it 'does not remove the job artifact record' do - expect(build.reload.job_archive).not_to be_nil + expect(build.reload.artifacts_archive).not_to be_nil end end -- cgit v1.2.1 From 8f01e67980d4ab8a7879800156cdc1dee07a4e8b Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 30 Nov 2017 14:19:07 +0100 Subject: Revert "Rename `job_archive|metadata` to `artifacts_archive|metadata`" This reverts commit 714082e65304ae2ec5c5400c59a68ab63e724aa9. --- app/models/ci/build.rb | 4 ++-- app/models/concerns/artifact_migratable.rb | 16 ++++++++-------- app/models/project_statistics.rb | 3 --- spec/services/ci/retry_build_service_spec.rb | 2 +- .../expire_build_instance_artifacts_worker_spec.rb | 6 +++--- 5 files changed, 14 insertions(+), 17 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 842323e26e3..91747da28a1 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -16,8 +16,8 @@ module Ci has_many :trace_sections, class_name: 'Ci::BuildTraceSection' has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_one :artifacts_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id - has_one :artifacts_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index b99585a0b65..811a8252459 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -3,11 +3,11 @@ # Meant to be prepended so the interface can stay the same module ArtifactMigratable def artifacts_file - artifacts_archive&.file || legacy_artifacts_file + job_archive&.file || legacy_artifacts_file end def artifacts_metadata - artifacts_metadata&.file || legacy_artifacts_metadata + job_metadata&.file || legacy_artifacts_metadata end def artifacts? @@ -19,20 +19,20 @@ module ArtifactMigratable end def artifacts_file_changed? - artifacts_archive&.file_changed? || attribute_changed?(:artifacts_file) + job_archive&.file_changed? || attribute_changed?(:artifacts_file) end def remove_artifacts_file! - if artifacts_archive - artifacts_archive.destroy + if job_archive + job_archive.destroy else remove_legacy_artifacts_file! end end def remove_artifacts_metadata! - if artifacts_metadata - artifacts_metadata.destroy + if job_metadata + job_metadata.destroy else remove_legacy_artifacts_metadata! end @@ -40,6 +40,6 @@ module ArtifactMigratable def artifacts_size read_attribute(:artifacts_size).to_i + - artifacts_archive&.size.to_i + artifacts_metadata&.size.to_i + job_archive&.size.to_i + job_metadata&.size.to_i end end diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index b3d4c82583c..17b9d2cf7b4 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -35,9 +35,6 @@ class ProjectStatistics < ActiveRecord::Base end def update_build_artifacts_size - # REMARK: - # We perform dual calculation as we run SQL query: sum does not instantiate AR model. - # The Ci::Build#artifacts_size returns sum of ci_builds.artifacts_size and ci_job_artifacts.file_size self.build_artifacts_size = project.builds.sum(:artifacts_size) + Ci::JobArtifact.artifacts_size_for(self) diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 3078e5b35e4..20fe80beade 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -17,7 +17,7 @@ describe Ci::RetryBuildService do %i[id status user token coverage trace runner artifacts_expire_at artifacts_file artifacts_metadata artifacts_size created_at updated_at started_at finished_at queued_at erased_by - erased_at auto_canceled_by job_artifacts artifacts_archive artifacts_metadata].freeze + erased_at auto_canceled_by job_artifacts job_archive job_metadata].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections diff --git a/spec/workers/expire_build_instance_artifacts_worker_spec.rb b/spec/workers/expire_build_instance_artifacts_worker_spec.rb index 05e2aa0703b..c0d2b1b7411 100644 --- a/spec/workers/expire_build_instance_artifacts_worker_spec.rb +++ b/spec/workers/expire_build_instance_artifacts_worker_spec.rb @@ -23,7 +23,7 @@ describe ExpireBuildInstanceArtifactsWorker do end it 'does remove the job artifact record' do - expect(build.reload.artifacts_archive).to be_nil + expect(build.reload.job_archive).to be_nil end end end @@ -42,7 +42,7 @@ describe ExpireBuildInstanceArtifactsWorker do end it 'does not remove the job artifact record' do - expect(build.reload.artifacts_archive).not_to be_nil + expect(build.reload.job_archive).not_to be_nil end end @@ -58,7 +58,7 @@ describe ExpireBuildInstanceArtifactsWorker do end it 'does not remove the job artifact record' do - expect(build.reload.artifacts_archive).not_to be_nil + expect(build.reload.job_archive).not_to be_nil end end -- cgit v1.2.1 From ab02bd69425fe17e65717bed91e99b62e11c0e74 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 30 Nov 2017 14:18:38 +0100 Subject: Use `job_artifacts_archive|metadata` --- app/models/ci/build.rb | 4 ++-- app/models/concerns/artifact_migratable.rb | 16 ++++++++-------- spec/services/ci/retry_build_service_spec.rb | 2 +- spec/services/projects/update_pages_service_spec.rb | 2 +- .../expire_build_instance_artifacts_worker_spec.rb | 6 +++--- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 91747da28a1..d5c7cd0bb6a 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -16,8 +16,8 @@ module Ci has_many :trace_sections, class_name: 'Ci::BuildTraceSection' has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id - has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_artifacts_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_artifacts_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index 811a8252459..0460439e9e6 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -3,11 +3,11 @@ # Meant to be prepended so the interface can stay the same module ArtifactMigratable def artifacts_file - job_archive&.file || legacy_artifacts_file + job_artifacts_archive&.file || legacy_artifacts_file end def artifacts_metadata - job_metadata&.file || legacy_artifacts_metadata + job_artifacts_metadata&.file || legacy_artifacts_metadata end def artifacts? @@ -19,20 +19,20 @@ module ArtifactMigratable end def artifacts_file_changed? - job_archive&.file_changed? || attribute_changed?(:artifacts_file) + job_artifacts_archive&.file_changed? || attribute_changed?(:artifacts_file) end def remove_artifacts_file! - if job_archive - job_archive.destroy + if job_artifacts_archive + job_artifacts_archive.destroy else remove_legacy_artifacts_file! end end def remove_artifacts_metadata! - if job_metadata - job_metadata.destroy + if job_artifacts_metadata + job_artifacts_metadata.destroy else remove_legacy_artifacts_metadata! end @@ -40,6 +40,6 @@ module ArtifactMigratable def artifacts_size read_attribute(:artifacts_size).to_i + - job_archive&.size.to_i + job_metadata&.size.to_i + job_artifacts_archive&.size.to_i + job_artifacts_metadata&.size.to_i end end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 20fe80beade..d48a44fa57f 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -17,7 +17,7 @@ describe Ci::RetryBuildService do %i[id status user token coverage trace runner artifacts_expire_at artifacts_file artifacts_metadata artifacts_size created_at updated_at started_at finished_at queued_at erased_by - erased_at auto_canceled_by job_artifacts job_archive job_metadata].freeze + erased_at auto_canceled_by job_artifacts job_artifacts_archive job_artifacts_metadata].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index b669a0102bd..bfb86284d86 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -155,7 +155,7 @@ describe Projects::UpdatePagesService do end it 'fails for empty file fails' do - build.job_archive.update_attributes(file: empty_file) + build.job_artifacts_archive.update_attributes(file: empty_file) expect(execute).not_to eq(:success) end diff --git a/spec/workers/expire_build_instance_artifacts_worker_spec.rb b/spec/workers/expire_build_instance_artifacts_worker_spec.rb index c0d2b1b7411..e1a56c72162 100644 --- a/spec/workers/expire_build_instance_artifacts_worker_spec.rb +++ b/spec/workers/expire_build_instance_artifacts_worker_spec.rb @@ -23,7 +23,7 @@ describe ExpireBuildInstanceArtifactsWorker do end it 'does remove the job artifact record' do - expect(build.reload.job_archive).to be_nil + expect(build.reload.job_artifacts_archive).to be_nil end end end @@ -42,7 +42,7 @@ describe ExpireBuildInstanceArtifactsWorker do end it 'does not remove the job artifact record' do - expect(build.reload.job_archive).not_to be_nil + expect(build.reload.job_artifacts_archive).not_to be_nil end end @@ -58,7 +58,7 @@ describe ExpireBuildInstanceArtifactsWorker do end it 'does not remove the job artifact record' do - expect(build.reload.job_archive).not_to be_nil + expect(build.reload.job_artifacts_archive).not_to be_nil end end -- cgit v1.2.1 From 592e0877f470efbb224043a6f887265afa070e0b Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 30 Nov 2017 16:05:17 +0100 Subject: Add unique index on job_id and file_type --- ...145523_uniqueness_contraint_job_artifact_file_type.rb | 16 ++++++++++++++++ db/schema.rb | 3 ++- 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20171130145523_uniqueness_contraint_job_artifact_file_type.rb diff --git a/db/migrate/20171130145523_uniqueness_contraint_job_artifact_file_type.rb b/db/migrate/20171130145523_uniqueness_contraint_job_artifact_file_type.rb new file mode 100644 index 00000000000..7ecbac8a81d --- /dev/null +++ b/db/migrate/20171130145523_uniqueness_contraint_job_artifact_file_type.rb @@ -0,0 +1,16 @@ +class UniquenessContraintJobArtifactFileType < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :ci_job_artifacts, [:job_id, :file_type], unique: true + end + + def down + remove_concurrent_index :ci_job_artifacts, [:job_id, :file_type] + end +end diff --git a/db/schema.rb b/db/schema.rb index 0f71463acd8..3ec183f1c05 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20171124150326) do +ActiveRecord::Schema.define(version: 20171130145523) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -330,6 +330,7 @@ ActiveRecord::Schema.define(version: 20171124150326) do end add_index "ci_job_artifacts", ["file_type"], name: "index_ci_job_artifacts_on_file_type", using: :btree + add_index "ci_job_artifacts", ["job_id", "file_type"], name: "index_ci_job_artifacts_on_job_id_and_file_type", unique: true, using: :btree add_index "ci_job_artifacts", ["job_id"], name: "index_ci_job_artifacts_on_job_id", using: :btree add_index "ci_job_artifacts", ["project_id"], name: "index_ci_job_artifacts_on_project_id", using: :btree -- cgit v1.2.1 From 0e1821973da36d995cf1f9673300c59af8c82294 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 30 Nov 2017 18:32:16 +0100 Subject: Fix factory for artifacts --- spec/factories/ci/job_artifacts.rb | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb index 47c9842e698..cf05472c369 100644 --- a/spec/factories/ci/job_artifacts.rb +++ b/spec/factories/ci/job_artifacts.rb @@ -10,22 +10,20 @@ FactoryGirl.define do end trait :archive do - after(:create) do |artifact, _| - artifact.update!( - file_type: :archive, - file: fixture_file_upload( + file_type :archive + + after(:build) do |artifact, _| + artifact.file = fixture_file_upload( Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') - ) end end trait :metadata do - after(:create) do |artifact, _| - artifact.update!( - file_type: :metadata, - file: fixture_file_upload( - Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), 'application/x-gzip') - ) + file_type :metadata + + after(:build) do |artifact, _| + artifact.file = fixture_file_upload( + Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), 'application/x-gzip') end end end -- cgit v1.2.1 From 0464c25f602dc2e4d147ca2ac714d49bf96ddcf2 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sun, 3 Dec 2017 12:02:11 +0100 Subject: Store expire_at in ci_job_artifacts --- app/models/ci/build.rb | 7 ++-- app/models/ci/job_artifact.rb | 11 ++++++ db/migrate/20170918072948_create_job_artifacts.rb | 3 +- db/schema.rb | 3 +- lib/api/runner.rb | 9 ++--- spec/models/ci/build_spec.rb | 14 +++++++- spec/models/ci/job_artifact_spec.rb | 44 +++++++++++++++++++++++ 7 files changed, 81 insertions(+), 10 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index d5c7cd0bb6a..a19273484ef 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -16,8 +16,8 @@ module Ci has_many :trace_sections, class_name: 'Ci::BuildTraceSection' has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_one :job_artifacts_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id - has_one :job_artifacts_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_artifacts_archive, -> { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id + has_one :job_artifacts_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @@ -38,7 +38,7 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } scope :with_artifacts, ->() do - where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id')) + where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.build_id')) end scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } @@ -386,6 +386,7 @@ module Ci def keep_artifacts! self.update(artifacts_expire_at: nil) + self.job_artifacts.update_all(expire_at: nil) end def coverage_regex diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index e02fa021409..84fc6863567 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -21,5 +21,16 @@ module Ci def set_size self.size = file.size end + + def expire_in + expire_at - Time.now if expire_at + end + + def expire_in=(value) + self.expire_at = + if value + ChronicDuration.parse(value)&.seconds&.from_now + end + end end end diff --git a/db/migrate/20170918072948_create_job_artifacts.rb b/db/migrate/20170918072948_create_job_artifacts.rb index 90152127ed5..d6c6a228ed1 100644 --- a/db/migrate/20170918072948_create_job_artifacts.rb +++ b/db/migrate/20170918072948_create_job_artifacts.rb @@ -7,11 +7,12 @@ class CreateJobArtifacts < ActiveRecord::Migration create_table :ci_job_artifacts do |t| t.belongs_to :project, null: false, index: true, foreign_key: { on_delete: :cascade } t.integer :job_id, null: false, index: true - t.integer :size, limit: 8 t.integer :file_type, null: false, index: true + t.integer :size, limit: 8 t.datetime_with_timezone :created_at, null: false t.datetime_with_timezone :updated_at, null: false + t.datetime_with_timezone :expire_at t.string :file diff --git a/db/schema.rb b/db/schema.rb index 3ec183f1c05..64f67574b3f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -322,10 +322,11 @@ ActiveRecord::Schema.define(version: 20171130145523) do create_table "ci_job_artifacts", force: :cascade do |t| t.integer "project_id", null: false t.integer "job_id", null: false - t.integer "size", limit: 8 t.integer "file_type", null: false + t.integer "size", limit: 8 t.datetime_with_timezone "created_at", null: false t.datetime_with_timezone "updated_at", null: false + t.datetime_with_timezone "expire_at" t.string "file" end diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 8de0868c063..80feb629d54 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -222,12 +222,13 @@ module API bad_request!('Missing artifacts file!') unless artifacts file_to_large! unless artifacts.size < max_artifacts_size - job.job_artifacts.build(project: job.project, file_type: :archive, file: artifacts) - job.job_artifacts.build(project: job.project, file_type: :metadata, file: metadata) - - job.artifacts_expire_in = params['expire_in'] || + expire_in = params['expire_in'] || Gitlab::CurrentSettings.current_application_settings.default_artifacts_expire_in + job.build_job_artifacts_archive(project: job.project, file_type: :archive, file: artifacts, expire_in: expire_in) + job.build_job_artifacts_metadata(project: job.project, file_type: :metadata, file: metadata, expire_in: expire_in) if metadata + job.artifacts_expire_in = expire_in + if job.save present job, with: Entities::JobRequest::Response else diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 212ff36633b..9070692abfe 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1008,11 +1008,23 @@ describe Ci::Build do describe '#keep_artifacts!' do let(:build) { create(:ci_build, artifacts_expire_at: Time.now + 7.days) } + subject { build.keep_artifacts! } + it 'to reset expire_at' do - build.keep_artifacts! + subject expect(build.artifacts_expire_at).to be_nil end + + context 'when having artifacts files' do + let!(:artifact) { create(:ci_job_artifact, job: build, expire_in: '7 days') } + + it 'to reset dependent objects' do + subject + + expect(artifact.reload.expire_at).to be_nil + end + end end describe '#merge_request' do diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index 9dd5cba150d..cdde1ed207a 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -27,4 +27,48 @@ describe Ci::JobArtifact do it { is_expected.to respond_to(:work_dir) } end end + + describe '#expire_in' do + subject { artifact.expire_in } + + it { is_expected.to be_nil } + + context 'when expire_at is specified' do + let(:expire_at) { Time.now + 7.days } + + before do + artifact.expire_at = expire_at + end + + it { is_expected.to be_within(5).of(expire_at - Time.now) } + end + end + + describe '#expire_in=' do + subject { artifact.expire_in } + + it 'when assigning valid duration' do + artifact.expire_in = '7 days' + + is_expected.to be_within(10).of(7.days.to_i) + end + + it 'when assigning invalid duration' do + expect { artifact.expire_in = '7 elephants' }.to raise_error(ChronicDuration::DurationParseError) + + is_expected.to be_nil + end + + it 'when resetting value' do + artifact.expire_in = nil + + is_expected.to be_nil + end + + it 'when setting to 0' do + artifact.expire_in = '0' + + is_expected.to be_nil + end + end end -- cgit v1.2.1 From 02831af9da8f50a824bae223c5f5e6bdc0e0a225 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sun, 3 Dec 2017 12:02:35 +0100 Subject: Remove update artifacts test (it should not be needed) --- spec/requests/api/runner_spec.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 72962f5d0b4..679d391caa5 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -985,15 +985,6 @@ describe API::Runner do it_behaves_like 'successful artifacts upload' end - context 'when updates artifact' do - before do - upload_artifacts(file_upload2, headers_with_token) - upload_artifacts(file_upload, headers_with_token) - end - - it_behaves_like 'successful artifacts upload' - end - context 'when using runners token' do it 'responds with forbidden' do upload_artifacts(file_upload, headers.merge(API::Helpers::Runner::JOB_TOKEN_HEADER => job.project.runners_token)) -- cgit v1.2.1 From 6b6fb11a235893335bc8d67b86c9328d027b72e0 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sun, 3 Dec 2017 12:04:16 +0100 Subject: Code style --- app/models/ci/build.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index a19273484ef..c41757cf394 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -38,7 +38,8 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } scope :with_artifacts, ->() do - where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.build_id')) + where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', + '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id')) end scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } -- cgit v1.2.1 From 676f48794663e65a7e7290bba19038cd60e6ffa1 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sun, 3 Dec 2017 16:21:59 +0100 Subject: Fix failures --- app/models/ci/build.rb | 6 +----- spec/factories/ci/job_artifacts.rb | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index c41757cf394..8738e094510 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -475,11 +475,7 @@ module Ci private def update_artifacts_size - self.artifacts_size = if artifacts_file.exists? - artifacts_file.size - else - nil - end + self.artifacts_size = legacy_artifacts_file&.size end def erase_trace! diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb index cf05472c369..538dc422832 100644 --- a/spec/factories/ci/job_artifacts.rb +++ b/spec/factories/ci/job_artifacts.rb @@ -14,7 +14,7 @@ FactoryGirl.define do after(:build) do |artifact, _| artifact.file = fixture_file_upload( - Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') + Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') end end -- cgit v1.2.1 From dd0e3412034bf9227c415455f230fe4b28439f2c Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sun, 3 Dec 2017 16:21:59 +0100 Subject: Fix Rubocop --- spec/models/ci/job_artifact_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index cdde1ed207a..0e18a326c68 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -55,7 +55,7 @@ describe Ci::JobArtifact do it 'when assigning invalid duration' do expect { artifact.expire_in = '7 elephants' }.to raise_error(ChronicDuration::DurationParseError) - + is_expected.to be_nil end -- cgit v1.2.1 From b8a393192529015bc2fa9d04c2782cf96e7ec896 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Wed, 22 Nov 2017 14:48:38 +0100 Subject: Add custom brand text on new project pages --- app/controllers/admin/appearances_controller.rb | 6 ++-- app/helpers/appearances_helper.rb | 20 +++++------ app/models/appearance.rb | 3 +- app/views/admin/appearances/_form.html.haml | 38 ++++++++++++++------- app/views/admin/appearances/preview.html.haml | 12 ------- .../admin/appearances/preview_sign_in.html.haml | 12 +++++++ app/views/layouts/devise.html.haml | 2 +- app/views/projects/new.html.haml | 1 + .../feature-custom-text-for-new-projects.yml | 5 +++ config/routes/admin.rb | 2 +- ...00_add_new_project_guidelines_to_appearances.rb | 12 +++++++ db/schema.rb | 2 ++ doc/README.md | 1 + doc/customization/new_project_page.md | 20 +++++++++++ .../new_project_page/appearance_settings.png | Bin 0 -> 71178 bytes .../new_project_page/custom_new_project_page.png | Bin 0 -> 164962 bytes .../new_project_page/default_new_project_page.png | Bin 0 -> 146906 bytes spec/factories/appearances.rb | 1 + spec/features/admin/admin_appearance_spec.rb | 33 +++++++++++++++--- spec/models/appearance_spec.rb | 3 -- 20 files changed, 121 insertions(+), 52 deletions(-) delete mode 100644 app/views/admin/appearances/preview.html.haml create mode 100644 app/views/admin/appearances/preview_sign_in.html.haml create mode 100644 changelogs/unreleased/feature-custom-text-for-new-projects.yml create mode 100644 db/migrate/20171122131600_add_new_project_guidelines_to_appearances.rb create mode 100644 doc/customization/new_project_page.md create mode 100644 doc/customization/new_project_page/appearance_settings.png create mode 100644 doc/customization/new_project_page/custom_new_project_page.png create mode 100644 doc/customization/new_project_page/default_new_project_page.png diff --git a/app/controllers/admin/appearances_controller.rb b/app/controllers/admin/appearances_controller.rb index 92df1c8dff0..dd0b38970bd 100644 --- a/app/controllers/admin/appearances_controller.rb +++ b/app/controllers/admin/appearances_controller.rb @@ -4,8 +4,8 @@ class Admin::AppearancesController < Admin::ApplicationController def show end - def preview - render 'preview', layout: 'devise' + def preview_sign_in + render 'preview_sign_in', layout: 'devise' end def create @@ -52,7 +52,7 @@ class Admin::AppearancesController < Admin::ApplicationController def appearance_params params.require(:appearance).permit( :title, :description, :logo, :logo_cache, :header_logo, :header_logo_cache, - :updated_by + :new_project_guidelines, :updated_by ) end end diff --git a/app/helpers/appearances_helper.rb b/app/helpers/appearances_helper.rb index df590cf47c8..c037de33c22 100644 --- a/app/helpers/appearances_helper.rb +++ b/app/helpers/appearances_helper.rb @@ -1,30 +1,26 @@ module AppearancesHelper def brand_title - if brand_item && brand_item.title - brand_item.title - else - 'GitLab Community Edition' - end + brand_item&.title.presence || 'GitLab Community Edition' end def brand_image - if brand_item.logo? - image_tag brand_item.logo - else - nil - end + image_tag(brand_item.logo) if brand_item&.logo? end def brand_text markdown_field(brand_item, :description) end + def brand_new_project_guidelines + markdown_field(brand_item, :new_project_guidelines) + end + def brand_item @appearance ||= Appearance.current end def brand_header_logo - if brand_item && brand_item.header_logo? + if brand_item&.header_logo? image_tag brand_item.header_logo else render 'shared/logo.svg' @@ -33,7 +29,7 @@ module AppearancesHelper # Skip the 'GitLab' type logo when custom brand logo is set def brand_header_logo_type - unless brand_item && brand_item.header_logo? + unless brand_item&.header_logo? render 'shared/logo_type.svg' end end diff --git a/app/models/appearance.rb b/app/models/appearance.rb index ff15689ecac..76cfe28742a 100644 --- a/app/models/appearance.rb +++ b/app/models/appearance.rb @@ -2,9 +2,8 @@ class Appearance < ActiveRecord::Base include CacheMarkdownField cache_markdown_field :description + cache_markdown_field :new_project_guidelines - validates :title, presence: true - validates :description, presence: true validates :logo, file_size: { maximum: 1.megabyte } validates :header_logo, file_size: { maximum: 1.megabyte } diff --git a/app/views/admin/appearances/_form.html.haml b/app/views/admin/appearances/_form.html.haml index 4a2238fe277..15bda97c3b5 100644 --- a/app/views/admin/appearances/_form.html.haml +++ b/app/views/admin/appearances/_form.html.haml @@ -1,6 +1,23 @@ = form_for @appearance, url: admin_appearances_path, html: { class: 'form-horizontal'} do |f| = form_errors(@appearance) + %fieldset.app_logo + %legend + Navigation bar: + .form-group + = f.label :header_logo, 'Header logo', class: 'control-label' + .col-sm-10 + - if @appearance.header_logo? + = image_tag @appearance.header_logo_url, class: 'appearance-light-logo-preview' + - if @appearance.persisted? + %br + = link_to 'Remove header logo', header_logos_admin_appearances_path, data: { confirm: "Header logo will be removed. Are you sure?"}, method: :delete, class: "btn btn-remove btn-sm remove-logo" + %hr + = f.hidden_field :header_logo_cache + = f.file_field :header_logo, class: "" + .hint + Maximum file size is 1MB. Pages are optimized for a 28px tall header logo + %fieldset.sign-in %legend Sign in/Sign up pages: @@ -28,27 +45,22 @@ .hint Maximum file size is 1MB. Pages are optimized for a 640x360 px logo. - %fieldset.app_logo + %fieldset %legend - Navigation bar: + New project pages: .form-group - = f.label :header_logo, 'Header logo', class: 'control-label' + = f.label :new_project_guidelines, class: 'control-label' .col-sm-10 - - if @appearance.header_logo? - = image_tag @appearance.header_logo_url, class: 'appearance-light-logo-preview' - - if @appearance.persisted? - %br - = link_to 'Remove header logo', header_logos_admin_appearances_path, data: { confirm: "Header logo will be removed. Are you sure?"}, method: :delete, class: "btn btn-remove btn-sm remove-logo" - %hr - = f.hidden_field :header_logo_cache - = f.file_field :header_logo, class: "" + = f.text_area :new_project_guidelines, class: "form-control", rows: 10 .hint - Maximum file size is 1MB. Pages are optimized for a 28px tall header logo + Guidelines parsed with #{link_to "GitLab Flavored Markdown", help_page_path('user/markdown'), target: '_blank'}. .form-actions = f.submit 'Save', class: 'btn btn-save append-right-10' - if @appearance.persisted? - = link_to 'Preview last save', preview_admin_appearances_path, class: 'btn', target: '_blank', rel: 'noopener noreferrer' + Preview last save: + = link_to 'Sign-in page', preview_sign_in_admin_appearances_path, class: 'btn', target: '_blank', rel: 'noopener noreferrer' + = link_to 'New project page', new_project_path, class: 'btn', target: '_blank', rel: 'noopener noreferrer' - if @appearance.updated_at %span.pull-right diff --git a/app/views/admin/appearances/preview.html.haml b/app/views/admin/appearances/preview.html.haml deleted file mode 100644 index 1af7dd5bb67..00000000000 --- a/app/views/admin/appearances/preview.html.haml +++ /dev/null @@ -1,12 +0,0 @@ -= render 'devise/shared/tab_single', tab_title: 'Sign in preview' -.login-box - %form.gl-show-field-errors - .form-group - = label_tag :login - = text_field_tag :login, nil, class: "form-control top", title: 'Please provide your username or email address.' - .form-group - = label_tag :password - = password_field_tag :password, nil, class: "form-control bottom", title: 'This field is required.' - .form-group - = button_tag "Sign in", class: "btn-create btn" - diff --git a/app/views/admin/appearances/preview_sign_in.html.haml b/app/views/admin/appearances/preview_sign_in.html.haml new file mode 100644 index 00000000000..1af7dd5bb67 --- /dev/null +++ b/app/views/admin/appearances/preview_sign_in.html.haml @@ -0,0 +1,12 @@ += render 'devise/shared/tab_single', tab_title: 'Sign in preview' +.login-box + %form.gl-show-field-errors + .form-group + = label_tag :login + = text_field_tag :login, nil, class: "form-control top", title: 'Please provide your username or email address.' + .form-group + = label_tag :password + = password_field_tag :password, nil, class: "form-control bottom", title: 'This field is required.' + .form-group + = button_tag "Sign in", class: "btn-create btn" + diff --git a/app/views/layouts/devise.html.haml b/app/views/layouts/devise.html.haml index 97016d28535..691d2528022 100644 --- a/app/views/layouts/devise.html.haml +++ b/app/views/layouts/devise.html.haml @@ -15,8 +15,8 @@ .col-sm-7.brand-holder.pull-left %h1 = brand_title - - if brand_item = brand_image + - if brand_item&.description? = brand_text - else %h3 Open source software to collaborate on code diff --git a/app/views/projects/new.html.haml b/app/views/projects/new.html.haml index 0a7880ce4cd..cad7c2e83db 100644 --- a/app/views/projects/new.html.haml +++ b/app/views/projects/new.html.haml @@ -18,6 +18,7 @@ A project is where you house your files (repository), plan your work (issues), and publish your documentation (wiki), #{link_to 'among other things', help_page_path("user/project/index.md", anchor: "projects-features"), target: '_blank'}. %p All features are enabled when you create a project, but you can disable the ones you don’t need in the project settings. + = brand_new_project_guidelines .col-lg-9.js-toggle-container %ul.nav-links.gitlab-tabs{ role: 'tablist' } %li.active{ role: 'presentation' } diff --git a/changelogs/unreleased/feature-custom-text-for-new-projects.yml b/changelogs/unreleased/feature-custom-text-for-new-projects.yml new file mode 100644 index 00000000000..905d76dab33 --- /dev/null +++ b/changelogs/unreleased/feature-custom-text-for-new-projects.yml @@ -0,0 +1,5 @@ +--- +title: Add custom brand text on new project pages +merge_request: 15541 +author: Markus Koller +type: changed diff --git a/config/routes/admin.rb b/config/routes/admin.rb index c0748231813..e22fb440abc 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -97,7 +97,7 @@ namespace :admin do resource :appearances, only: [:show, :create, :update], path: 'appearance' do member do - get :preview + get :preview_sign_in delete :logo delete :header_logos end diff --git a/db/migrate/20171122131600_add_new_project_guidelines_to_appearances.rb b/db/migrate/20171122131600_add_new_project_guidelines_to_appearances.rb new file mode 100644 index 00000000000..f141c442d97 --- /dev/null +++ b/db/migrate/20171122131600_add_new_project_guidelines_to_appearances.rb @@ -0,0 +1,12 @@ +class AddNewProjectGuidelinesToAppearances < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + change_table :appearances do |t| + t.text :new_project_guidelines + t.text :new_project_guidelines_html + end + end +end diff --git a/db/schema.rb b/db/schema.rb index effb2604af2..ad4c7f690d1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -36,6 +36,8 @@ ActiveRecord::Schema.define(version: 20171124150326) do t.datetime_with_timezone "updated_at", null: false t.text "description_html" t.integer "cached_markdown_version" + t.text "new_project_guidelines" + t.text "new_project_guidelines_html" end create_table "application_settings", force: :cascade do |t| diff --git a/doc/README.md b/doc/README.md index d4119d35162..95cb9683a15 100644 --- a/doc/README.md +++ b/doc/README.md @@ -189,6 +189,7 @@ have access to GitLab administration tools and settings. - [Issue closing pattern](administration/issue_closing_pattern.md): Customize how to close an issue from commit messages. - [Libravatar](customization/libravatar.md): Use Libravatar instead of Gravatar for user avatars. - [Welcome message](customization/welcome_message.md): Add a custom welcome message to the sign-in page. +- [New project page](customization/new_project_page.md): Customize the new project page. ### Admin tools diff --git a/doc/customization/new_project_page.md b/doc/customization/new_project_page.md new file mode 100644 index 00000000000..148bf9512c6 --- /dev/null +++ b/doc/customization/new_project_page.md @@ -0,0 +1,20 @@ +# Customizing the new project page + +It is possible to add a markdown-formatted message to your GitLab +new project page. + +By default, the new project page shows a sidebar with general information: + +![](new_project_page/default_new_project_page.png) + +## Changing the appearance of the new project page + +Navigate to the **Admin** area and go to the **Appearance** page. + +Fill in your project guidelines: + +![](new_project_page/appearance_settings.png) + +After saving the page, your new project page will show the guidelines in the sidebar, below the general information: + +![](new_project_page/custom_new_project_page.png) diff --git a/doc/customization/new_project_page/appearance_settings.png b/doc/customization/new_project_page/appearance_settings.png new file mode 100644 index 00000000000..08eea684e14 Binary files /dev/null and b/doc/customization/new_project_page/appearance_settings.png differ diff --git a/doc/customization/new_project_page/custom_new_project_page.png b/doc/customization/new_project_page/custom_new_project_page.png new file mode 100644 index 00000000000..662c715f193 Binary files /dev/null and b/doc/customization/new_project_page/custom_new_project_page.png differ diff --git a/doc/customization/new_project_page/default_new_project_page.png b/doc/customization/new_project_page/default_new_project_page.png new file mode 100644 index 00000000000..4a0bcf09903 Binary files /dev/null and b/doc/customization/new_project_page/default_new_project_page.png differ diff --git a/spec/factories/appearances.rb b/spec/factories/appearances.rb index cf2a2b76bcb..860973024c9 100644 --- a/spec/factories/appearances.rb +++ b/spec/factories/appearances.rb @@ -4,5 +4,6 @@ FactoryGirl.define do factory :appearance do title "MepMep" description "This is my Community Edition instance" + new_project_guidelines "Custom project guidelines" end end diff --git a/spec/features/admin/admin_appearance_spec.rb b/spec/features/admin/admin_appearance_spec.rb index 5f3a37c1dcc..d91dcf76191 100644 --- a/spec/features/admin/admin_appearance_spec.rb +++ b/spec/features/admin/admin_appearance_spec.rb @@ -9,6 +9,7 @@ feature 'Admin Appearance' do fill_in 'appearance_title', with: 'MyCompany' fill_in 'appearance_description', with: 'dev server' + fill_in 'appearance_new_project_guidelines', with: 'Custom project guidelines' click_button 'Save' expect(current_path).to eq admin_appearances_path @@ -16,21 +17,39 @@ feature 'Admin Appearance' do expect(page).to have_field('appearance_title', with: 'MyCompany') expect(page).to have_field('appearance_description', with: 'dev server') + expect(page).to have_field('appearance_new_project_guidelines', with: 'Custom project guidelines') expect(page).to have_content 'Last edit' end - scenario 'Preview appearance' do + scenario 'Preview sign-in page appearance' do sign_in(create(:admin)) visit admin_appearances_path - click_link "Preview" + click_link "Sign-in page" - expect_page_has_custom_appearance(appearance) + expect_custom_sign_in_appearance(appearance) + end + + scenario 'Preview new project page appearance' do + sign_in(create(:admin)) + + visit admin_appearances_path + click_link "New project page" + + expect_custom_new_project_appearance(appearance) end scenario 'Custom sign-in page' do visit new_user_session_path - expect_page_has_custom_appearance(appearance) + + expect_custom_sign_in_appearance(appearance) + end + + scenario 'Custom new project page' do + sign_in create(:user) + visit new_project_path + + expect_custom_new_project_appearance(appearance) end scenario 'Appearance logo' do @@ -57,11 +76,15 @@ feature 'Admin Appearance' do expect(page).not_to have_css(header_logo_selector) end - def expect_page_has_custom_appearance(appearance) + def expect_custom_sign_in_appearance(appearance) expect(page).to have_content appearance.title expect(page).to have_content appearance.description end + def expect_custom_new_project_appearance(appearance) + expect(page).to have_content appearance.new_project_guidelines + end + def logo_selector '//img[data-src^="/uploads/-/system/appearance/logo"]' end diff --git a/spec/models/appearance_spec.rb b/spec/models/appearance_spec.rb index 49f44525b29..56b5d616284 100644 --- a/spec/models/appearance_spec.rb +++ b/spec/models/appearance_spec.rb @@ -5,9 +5,6 @@ describe Appearance do it { is_expected.to be_valid } - it { is_expected.to validate_presence_of(:title) } - it { is_expected.to validate_presence_of(:description) } - it { is_expected.to have_many(:uploads).dependent(:destroy) } describe '.current', :use_clean_rails_memory_store_caching do -- cgit v1.2.1 From e58ce2b72eec6b92986884ee80b974b83134b7f4 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Mon, 4 Dec 2017 19:23:44 +0100 Subject: Remove index on file_type --- db/migrate/20170918072948_create_job_artifacts.rb | 4 ++-- db/schema.rb | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/db/migrate/20170918072948_create_job_artifacts.rb b/db/migrate/20170918072948_create_job_artifacts.rb index d6c6a228ed1..304b3fce5be 100644 --- a/db/migrate/20170918072948_create_job_artifacts.rb +++ b/db/migrate/20170918072948_create_job_artifacts.rb @@ -6,8 +6,8 @@ class CreateJobArtifacts < ActiveRecord::Migration def change create_table :ci_job_artifacts do |t| t.belongs_to :project, null: false, index: true, foreign_key: { on_delete: :cascade } - t.integer :job_id, null: false, index: true - t.integer :file_type, null: false, index: true + t.integer :job_id, null: false + t.integer :file_type, null: false t.integer :size, limit: 8 t.datetime_with_timezone :created_at, null: false diff --git a/db/schema.rb b/db/schema.rb index 64f67574b3f..de84d861530 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -330,9 +330,7 @@ ActiveRecord::Schema.define(version: 20171130145523) do t.string "file" end - add_index "ci_job_artifacts", ["file_type"], name: "index_ci_job_artifacts_on_file_type", using: :btree add_index "ci_job_artifacts", ["job_id", "file_type"], name: "index_ci_job_artifacts_on_job_id_and_file_type", unique: true, using: :btree - add_index "ci_job_artifacts", ["job_id"], name: "index_ci_job_artifacts_on_job_id", using: :btree add_index "ci_job_artifacts", ["project_id"], name: "index_ci_job_artifacts_on_project_id", using: :btree create_table "ci_pipeline_schedule_variables", force: :cascade do |t| -- cgit v1.2.1 From 29e058ba953dab54c32b4662c13da0b88c351ffa Mon Sep 17 00:00:00 2001 From: Victor Wu Date: Mon, 4 Dec 2017 19:55:16 +0000 Subject: Update slash_commands.md --- doc/integration/slash_commands.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/integration/slash_commands.md b/doc/integration/slash_commands.md index aa52b5415cf..36a8844e953 100644 --- a/doc/integration/slash_commands.md +++ b/doc/integration/slash_commands.md @@ -17,6 +17,9 @@ Taking the trigger term as `project-name`, the commands are: | `/project-name issue search ` | Shows up to 5 issues matching `` | | `/project-name deploy to ` | Deploy from the `` environment to the `` environment | +Note that if you are using the [GitLab Slack application](https://docs.gitlab.com/ee/user/project/integrations/gitlab_slack_application.html) for +your GitLab.com projects, you need to [add the `gitlab` keyword at the beginning of the command](https://docs.gitlab.com/ee/user/project/integrations/gitlab_slack_application.html#usage). + ## Issue commands It is possible to create new issue, display issue details and search up to 5 issues. -- cgit v1.2.1 From 0b15570e497d3c5c515be59a43b686087b985f5c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 28 Nov 2017 17:08:30 +0100 Subject: Add ApplicationWorker and make every worker include it --- app/workers/admin_email_worker.rb | 2 +- app/workers/authorized_projects_worker.rb | 3 +- app/workers/background_migration_worker.rb | 3 +- app/workers/build_coverage_worker.rb | 2 +- app/workers/build_finished_worker.rb | 2 +- app/workers/build_hooks_worker.rb | 2 +- app/workers/build_queue_worker.rb | 2 +- app/workers/build_success_worker.rb | 2 +- app/workers/build_trace_sections_worker.rb | 2 +- app/workers/cluster_install_app_worker.rb | 2 +- app/workers/cluster_provision_worker.rb | 2 +- .../cluster_wait_for_app_installation_worker.rb | 2 +- app/workers/concerns/application_worker.rb | 25 +++++++++++ app/workers/concerns/dedicated_sidekiq_queue.rb | 9 ---- .../gitlab/github_import/object_importer.rb | 2 +- app/workers/create_gpg_signature_worker.rb | 3 +- app/workers/create_pipeline_worker.rb | 2 +- app/workers/delete_merged_branches_worker.rb | 3 +- app/workers/delete_user_worker.rb | 3 +- app/workers/email_receiver_worker.rb | 3 +- app/workers/emails_on_push_worker.rb | 3 +- app/workers/expire_build_artifacts_worker.rb | 2 +- .../expire_build_instance_artifacts_worker.rb | 3 +- app/workers/expire_job_cache_worker.rb | 2 +- app/workers/expire_pipeline_cache_worker.rb | 2 +- app/workers/git_garbage_collect_worker.rb | 3 +- .../gitlab/github_import/advance_stage_worker.rb | 2 +- .../github_import/refresh_import_jid_worker.rb | 2 +- .../github_import/stage/finish_import_worker.rb | 2 +- .../github_import/stage/import_base_data_worker.rb | 2 +- .../stage/import_issues_and_diff_notes_worker.rb | 2 +- .../github_import/stage/import_notes_worker.rb | 2 +- .../stage/import_pull_requests_worker.rb | 2 +- .../stage/import_repository_worker.rb | 2 +- app/workers/gitlab_shell_worker.rb | 3 +- app/workers/gitlab_usage_ping_worker.rb | 2 +- app/workers/group_destroy_worker.rb | 3 +- .../import_export_project_cleanup_worker.rb | 2 +- app/workers/invalid_gpg_signature_update_worker.rb | 3 +- app/workers/irker_worker.rb | 3 +- app/workers/merge_worker.rb | 3 +- .../namespaceless_project_destroy_worker.rb | 3 +- app/workers/new_issue_worker.rb | 3 +- app/workers/new_merge_request_worker.rb | 3 +- app/workers/new_note_worker.rb | 3 +- app/workers/pages_worker.rb | 2 +- app/workers/pipeline_hooks_worker.rb | 2 +- app/workers/pipeline_metrics_worker.rb | 2 +- app/workers/pipeline_notification_worker.rb | 2 +- app/workers/pipeline_process_worker.rb | 2 +- app/workers/pipeline_schedule_worker.rb | 2 +- app/workers/pipeline_success_worker.rb | 2 +- app/workers/pipeline_update_worker.rb | 2 +- app/workers/post_receive.rb | 3 +- app/workers/process_commit_worker.rb | 3 +- app/workers/project_cache_worker.rb | 3 +- app/workers/project_destroy_worker.rb | 3 +- app/workers/project_export_worker.rb | 3 +- .../project_migrate_hashed_storage_worker.rb | 3 +- app/workers/project_service_worker.rb | 3 +- app/workers/propagate_service_template_worker.rb | 3 +- app/workers/prune_old_events_worker.rb | 2 +- app/workers/reactive_caching_worker.rb | 3 +- app/workers/remove_expired_group_links_worker.rb | 2 +- app/workers/remove_expired_members_worker.rb | 2 +- app/workers/remove_old_web_hook_logs_worker.rb | 2 +- .../remove_unreferenced_lfs_objects_worker.rb | 2 +- app/workers/repository_archive_cache_worker.rb | 2 +- app/workers/repository_check/batch_worker.rb | 2 +- app/workers/repository_check/clear_worker.rb | 2 +- .../repository_check/single_repository_worker.rb | 2 +- app/workers/repository_fork_worker.rb | 3 +- app/workers/repository_import_worker.rb | 3 +- app/workers/requests_profiles_worker.rb | 2 +- .../schedule_update_user_activity_worker.rb | 2 +- app/workers/stage_update_worker.rb | 2 +- app/workers/storage_migrator_worker.rb | 3 +- app/workers/stuck_ci_jobs_worker.rb | 2 +- app/workers/stuck_import_jobs_worker.rb | 2 +- app/workers/stuck_merge_jobs_worker.rb | 2 +- app/workers/system_hook_push_worker.rb | 3 +- app/workers/trending_projects_worker.rb | 2 +- app/workers/update_merge_requests_worker.rb | 3 +- app/workers/update_user_activity_worker.rb | 3 +- app/workers/upload_checksum_worker.rb | 3 +- app/workers/wait_for_cluster_creation_worker.rb | 2 +- app/workers/web_hook_worker.rb | 3 +- config/initializers/sidekiq.rb | 8 ++-- doc/development/sidekiq_style_guide.md | 3 +- lib/gitlab/sidekiq_config.rb | 50 ++++++++++++++++++++++ spec/lib/gitlab/sidekiq_config_spec.rb | 24 +++++++++++ spec/workers/concerns/application_worker_spec.rb | 27 ++++++++++++ spec/workers/concerns/cluster_queue_spec.rb | 6 ++- spec/workers/concerns/cronjob_queue_spec.rb | 6 ++- .../concerns/dedicated_sidekiq_queue_spec.rb | 20 --------- .../gitlab/github_import/object_importer_spec.rb | 4 ++ .../concerns/gitlab/github_import/queue_spec.rb | 6 ++- spec/workers/concerns/pipeline_queue_spec.rb | 6 ++- .../concerns/repository_check_queue_spec.rb | 6 ++- spec/workers/every_sidekiq_worker_spec.rb | 35 +++------------ 100 files changed, 251 insertions(+), 189 deletions(-) create mode 100644 app/workers/concerns/application_worker.rb delete mode 100644 app/workers/concerns/dedicated_sidekiq_queue.rb create mode 100644 lib/gitlab/sidekiq_config.rb create mode 100644 spec/lib/gitlab/sidekiq_config_spec.rb create mode 100644 spec/workers/concerns/application_worker_spec.rb delete mode 100644 spec/workers/concerns/dedicated_sidekiq_queue_spec.rb diff --git a/app/workers/admin_email_worker.rb b/app/workers/admin_email_worker.rb index c2dc955b27c..bec0a003a1c 100644 --- a/app/workers/admin_email_worker.rb +++ b/app/workers/admin_email_worker.rb @@ -1,5 +1,5 @@ class AdminEmailWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/authorized_projects_worker.rb b/app/workers/authorized_projects_worker.rb index 55d8d0c69d1..d4f334ec3b8 100644 --- a/app/workers/authorized_projects_worker.rb +++ b/app/workers/authorized_projects_worker.rb @@ -1,6 +1,5 @@ class AuthorizedProjectsWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker # Schedules multiple jobs and waits for them to be completed. def self.bulk_perform_and_wait(args_list) diff --git a/app/workers/background_migration_worker.rb b/app/workers/background_migration_worker.rb index 45ce49bb5c0..65791c4eaba 100644 --- a/app/workers/background_migration_worker.rb +++ b/app/workers/background_migration_worker.rb @@ -1,6 +1,5 @@ class BackgroundMigrationWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker # Enqueues a number of jobs in bulk. # diff --git a/app/workers/build_coverage_worker.rb b/app/workers/build_coverage_worker.rb index cd4af85d047..62b212c79be 100644 --- a/app/workers/build_coverage_worker.rb +++ b/app/workers/build_coverage_worker.rb @@ -1,5 +1,5 @@ class BuildCoverageWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue def perform(build_id) diff --git a/app/workers/build_finished_worker.rb b/app/workers/build_finished_worker.rb index 52e7d346e74..5efa9180f5e 100644 --- a/app/workers/build_finished_worker.rb +++ b/app/workers/build_finished_worker.rb @@ -1,5 +1,5 @@ class BuildFinishedWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :processing diff --git a/app/workers/build_hooks_worker.rb b/app/workers/build_hooks_worker.rb index dedaf2835e6..6705a1c2709 100644 --- a/app/workers/build_hooks_worker.rb +++ b/app/workers/build_hooks_worker.rb @@ -1,5 +1,5 @@ class BuildHooksWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :hooks diff --git a/app/workers/build_queue_worker.rb b/app/workers/build_queue_worker.rb index e5ceb9ef715..fc775a84dc0 100644 --- a/app/workers/build_queue_worker.rb +++ b/app/workers/build_queue_worker.rb @@ -1,5 +1,5 @@ class BuildQueueWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :processing diff --git a/app/workers/build_success_worker.rb b/app/workers/build_success_worker.rb index 20ec24bd18a..ec049821ad7 100644 --- a/app/workers/build_success_worker.rb +++ b/app/workers/build_success_worker.rb @@ -1,5 +1,5 @@ class BuildSuccessWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :processing diff --git a/app/workers/build_trace_sections_worker.rb b/app/workers/build_trace_sections_worker.rb index 8c57e8f767b..c0f5c144e10 100644 --- a/app/workers/build_trace_sections_worker.rb +++ b/app/workers/build_trace_sections_worker.rb @@ -1,5 +1,5 @@ class BuildTraceSectionsWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue def perform(build_id) diff --git a/app/workers/cluster_install_app_worker.rb b/app/workers/cluster_install_app_worker.rb index 899aed904e4..f771cb4939f 100644 --- a/app/workers/cluster_install_app_worker.rb +++ b/app/workers/cluster_install_app_worker.rb @@ -1,5 +1,5 @@ class ClusterInstallAppWorker - include Sidekiq::Worker + include ApplicationWorker include ClusterQueue include ClusterApplications diff --git a/app/workers/cluster_provision_worker.rb b/app/workers/cluster_provision_worker.rb index b01f9708424..1ab4de3b647 100644 --- a/app/workers/cluster_provision_worker.rb +++ b/app/workers/cluster_provision_worker.rb @@ -1,5 +1,5 @@ class ClusterProvisionWorker - include Sidekiq::Worker + include ApplicationWorker include ClusterQueue def perform(cluster_id) diff --git a/app/workers/cluster_wait_for_app_installation_worker.rb b/app/workers/cluster_wait_for_app_installation_worker.rb index 4bb8c293e5d..d564d5e48bf 100644 --- a/app/workers/cluster_wait_for_app_installation_worker.rb +++ b/app/workers/cluster_wait_for_app_installation_worker.rb @@ -1,5 +1,5 @@ class ClusterWaitForAppInstallationWorker - include Sidekiq::Worker + include ApplicationWorker include ClusterQueue include ClusterApplications diff --git a/app/workers/concerns/application_worker.rb b/app/workers/concerns/application_worker.rb new file mode 100644 index 00000000000..bf1ecaa0c6d --- /dev/null +++ b/app/workers/concerns/application_worker.rb @@ -0,0 +1,25 @@ +Sidekiq::Worker.extend ActiveSupport::Concern + +module ApplicationWorker + extend ActiveSupport::Concern + + include Sidekiq::Worker + + included do + sidekiq_options queue: base_queue_name + end + + module ClassMethods + def base_queue_name + name + .sub(/\AGitlab::/, '') + .sub(/Worker\z/, '') + .underscore + .tr('/', '_') + end + + def queue + get_sidekiq_options['queue'].to_s + end + end +end diff --git a/app/workers/concerns/dedicated_sidekiq_queue.rb b/app/workers/concerns/dedicated_sidekiq_queue.rb deleted file mode 100644 index 132bae6022b..00000000000 --- a/app/workers/concerns/dedicated_sidekiq_queue.rb +++ /dev/null @@ -1,9 +0,0 @@ -# Concern that sets the queue of a Sidekiq worker based on the worker's class -# name/namespace. -module DedicatedSidekiqQueue - extend ActiveSupport::Concern - - included do - sidekiq_options queue: name.sub(/Worker\z/, '').underscore.tr('/', '_') - end -end diff --git a/app/workers/concerns/gitlab/github_import/object_importer.rb b/app/workers/concerns/gitlab/github_import/object_importer.rb index 67e36c811de..9a9fbaad653 100644 --- a/app/workers/concerns/gitlab/github_import/object_importer.rb +++ b/app/workers/concerns/gitlab/github_import/object_importer.rb @@ -8,7 +8,7 @@ module Gitlab extend ActiveSupport::Concern included do - include Sidekiq::Worker + include ApplicationWorker include GithubImport::Queue include ReschedulingMethods include NotifyUponDeath diff --git a/app/workers/create_gpg_signature_worker.rb b/app/workers/create_gpg_signature_worker.rb index 9b5ff17aafa..f371731f68c 100644 --- a/app/workers/create_gpg_signature_worker.rb +++ b/app/workers/create_gpg_signature_worker.rb @@ -1,6 +1,5 @@ class CreateGpgSignatureWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(commit_sha, project_id) project = Project.find_by(id: project_id) diff --git a/app/workers/create_pipeline_worker.rb b/app/workers/create_pipeline_worker.rb index 865ad1ba420..00cd7b85b9f 100644 --- a/app/workers/create_pipeline_worker.rb +++ b/app/workers/create_pipeline_worker.rb @@ -1,5 +1,5 @@ class CreatePipelineWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :creation diff --git a/app/workers/delete_merged_branches_worker.rb b/app/workers/delete_merged_branches_worker.rb index f870da4ecfd..07cd1f02fb5 100644 --- a/app/workers/delete_merged_branches_worker.rb +++ b/app/workers/delete_merged_branches_worker.rb @@ -1,6 +1,5 @@ class DeleteMergedBranchesWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(project_id, user_id) begin diff --git a/app/workers/delete_user_worker.rb b/app/workers/delete_user_worker.rb index 3340a7be4fe..6c431b02979 100644 --- a/app/workers/delete_user_worker.rb +++ b/app/workers/delete_user_worker.rb @@ -1,6 +1,5 @@ class DeleteUserWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(current_user_id, delete_user_id, options = {}) delete_user = User.find(delete_user_id) diff --git a/app/workers/email_receiver_worker.rb b/app/workers/email_receiver_worker.rb index 1afa24c8e2a..fff7cf9d26c 100644 --- a/app/workers/email_receiver_worker.rb +++ b/app/workers/email_receiver_worker.rb @@ -1,6 +1,5 @@ class EmailReceiverWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(raw) return unless Gitlab::IncomingEmail.enabled? diff --git a/app/workers/emails_on_push_worker.rb b/app/workers/emails_on_push_worker.rb index f5ccc84c160..21da27973fe 100644 --- a/app/workers/emails_on_push_worker.rb +++ b/app/workers/emails_on_push_worker.rb @@ -1,6 +1,5 @@ class EmailsOnPushWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker attr_reader :email, :skip_premailer diff --git a/app/workers/expire_build_artifacts_worker.rb b/app/workers/expire_build_artifacts_worker.rb index a27585fd389..73ab4211080 100644 --- a/app/workers/expire_build_artifacts_worker.rb +++ b/app/workers/expire_build_artifacts_worker.rb @@ -1,5 +1,5 @@ class ExpireBuildArtifactsWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/expire_build_instance_artifacts_worker.rb b/app/workers/expire_build_instance_artifacts_worker.rb index 7b59e976492..234b4357cf7 100644 --- a/app/workers/expire_build_instance_artifacts_worker.rb +++ b/app/workers/expire_build_instance_artifacts_worker.rb @@ -1,6 +1,5 @@ class ExpireBuildInstanceArtifactsWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(build_id) build = Ci::Build diff --git a/app/workers/expire_job_cache_worker.rb b/app/workers/expire_job_cache_worker.rb index 98a7500bffe..a591e2da519 100644 --- a/app/workers/expire_job_cache_worker.rb +++ b/app/workers/expire_job_cache_worker.rb @@ -1,5 +1,5 @@ class ExpireJobCacheWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :cache diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb index 1a0e7f92875..a3ac32b437d 100644 --- a/app/workers/expire_pipeline_cache_worker.rb +++ b/app/workers/expire_pipeline_cache_worker.rb @@ -1,5 +1,5 @@ class ExpirePipelineCacheWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :cache diff --git a/app/workers/git_garbage_collect_worker.rb b/app/workers/git_garbage_collect_worker.rb index ec65d3ff65e..8e26275669e 100644 --- a/app/workers/git_garbage_collect_worker.rb +++ b/app/workers/git_garbage_collect_worker.rb @@ -1,6 +1,5 @@ class GitGarbageCollectWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker include Gitlab::CurrentSettings sidekiq_options retry: false diff --git a/app/workers/gitlab/github_import/advance_stage_worker.rb b/app/workers/gitlab/github_import/advance_stage_worker.rb index 877f88c043f..400396d5755 100644 --- a/app/workers/gitlab/github_import/advance_stage_worker.rb +++ b/app/workers/gitlab/github_import/advance_stage_worker.rb @@ -7,7 +7,7 @@ module Gitlab # been completed this worker will advance the import process to the next # stage. class AdvanceStageWorker - include Sidekiq::Worker + include ApplicationWorker sidekiq_options queue: 'github_importer_advance_stage', dead: false diff --git a/app/workers/gitlab/github_import/refresh_import_jid_worker.rb b/app/workers/gitlab/github_import/refresh_import_jid_worker.rb index 45a38927225..7108b531bc2 100644 --- a/app/workers/gitlab/github_import/refresh_import_jid_worker.rb +++ b/app/workers/gitlab/github_import/refresh_import_jid_worker.rb @@ -3,7 +3,7 @@ module Gitlab module GithubImport class RefreshImportJidWorker - include Sidekiq::Worker + include ApplicationWorker include GithubImport::Queue # The interval to schedule new instances of this job at. diff --git a/app/workers/gitlab/github_import/stage/finish_import_worker.rb b/app/workers/gitlab/github_import/stage/finish_import_worker.rb index 1a09497780a..073d6608082 100644 --- a/app/workers/gitlab/github_import/stage/finish_import_worker.rb +++ b/app/workers/gitlab/github_import/stage/finish_import_worker.rb @@ -4,7 +4,7 @@ module Gitlab module GithubImport module Stage class FinishImportWorker - include Sidekiq::Worker + include ApplicationWorker include GithubImport::Queue include StageMethods diff --git a/app/workers/gitlab/github_import/stage/import_base_data_worker.rb b/app/workers/gitlab/github_import/stage/import_base_data_worker.rb index f8a3684c6ba..5726fbb573d 100644 --- a/app/workers/gitlab/github_import/stage/import_base_data_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_base_data_worker.rb @@ -4,7 +4,7 @@ module Gitlab module GithubImport module Stage class ImportBaseDataWorker - include Sidekiq::Worker + include ApplicationWorker include GithubImport::Queue include StageMethods diff --git a/app/workers/gitlab/github_import/stage/import_issues_and_diff_notes_worker.rb b/app/workers/gitlab/github_import/stage/import_issues_and_diff_notes_worker.rb index e110b7c1c36..7007754ff2e 100644 --- a/app/workers/gitlab/github_import/stage/import_issues_and_diff_notes_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_issues_and_diff_notes_worker.rb @@ -4,7 +4,7 @@ module Gitlab module GithubImport module Stage class ImportIssuesAndDiffNotesWorker - include Sidekiq::Worker + include ApplicationWorker include GithubImport::Queue include StageMethods diff --git a/app/workers/gitlab/github_import/stage/import_notes_worker.rb b/app/workers/gitlab/github_import/stage/import_notes_worker.rb index 9810ed25cf9..5f4678a595f 100644 --- a/app/workers/gitlab/github_import/stage/import_notes_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_notes_worker.rb @@ -4,7 +4,7 @@ module Gitlab module GithubImport module Stage class ImportNotesWorker - include Sidekiq::Worker + include ApplicationWorker include GithubImport::Queue include StageMethods diff --git a/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb b/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb index c531f26e897..1c5a7139802 100644 --- a/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb @@ -4,7 +4,7 @@ module Gitlab module GithubImport module Stage class ImportPullRequestsWorker - include Sidekiq::Worker + include ApplicationWorker include GithubImport::Queue include StageMethods diff --git a/app/workers/gitlab/github_import/stage/import_repository_worker.rb b/app/workers/gitlab/github_import/stage/import_repository_worker.rb index aa5762e773d..4d16cef1130 100644 --- a/app/workers/gitlab/github_import/stage/import_repository_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_repository_worker.rb @@ -4,7 +4,7 @@ module Gitlab module GithubImport module Stage class ImportRepositoryWorker - include Sidekiq::Worker + include ApplicationWorker include GithubImport::Queue include StageMethods diff --git a/app/workers/gitlab_shell_worker.rb b/app/workers/gitlab_shell_worker.rb index 0ec871e00e1..a0028e41332 100644 --- a/app/workers/gitlab_shell_worker.rb +++ b/app/workers/gitlab_shell_worker.rb @@ -1,7 +1,6 @@ class GitlabShellWorker - include Sidekiq::Worker + include ApplicationWorker include Gitlab::ShellAdapter - include DedicatedSidekiqQueue def perform(action, *arg) gitlab_shell.__send__(action, *arg) # rubocop:disable GitlabSecurity/PublicSend diff --git a/app/workers/gitlab_usage_ping_worker.rb b/app/workers/gitlab_usage_ping_worker.rb index 0a55aab63fd..6dd281b1147 100644 --- a/app/workers/gitlab_usage_ping_worker.rb +++ b/app/workers/gitlab_usage_ping_worker.rb @@ -1,7 +1,7 @@ class GitlabUsagePingWorker LEASE_TIMEOUT = 86400 - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/group_destroy_worker.rb b/app/workers/group_destroy_worker.rb index bd8e212e928..f577b310b20 100644 --- a/app/workers/group_destroy_worker.rb +++ b/app/workers/group_destroy_worker.rb @@ -1,6 +1,5 @@ class GroupDestroyWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker include ExceptionBacktrace def perform(group_id, user_id) diff --git a/app/workers/import_export_project_cleanup_worker.rb b/app/workers/import_export_project_cleanup_worker.rb index 7957ed807ab..9788c8df3a3 100644 --- a/app/workers/import_export_project_cleanup_worker.rb +++ b/app/workers/import_export_project_cleanup_worker.rb @@ -1,5 +1,5 @@ class ImportExportProjectCleanupWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/invalid_gpg_signature_update_worker.rb b/app/workers/invalid_gpg_signature_update_worker.rb index db6b1ea8e8d..6774ab307c6 100644 --- a/app/workers/invalid_gpg_signature_update_worker.rb +++ b/app/workers/invalid_gpg_signature_update_worker.rb @@ -1,6 +1,5 @@ class InvalidGpgSignatureUpdateWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(gpg_key_id) gpg_key = GpgKey.find_by(id: gpg_key_id) diff --git a/app/workers/irker_worker.rb b/app/workers/irker_worker.rb index 311fc187e49..9ae5456be4c 100644 --- a/app/workers/irker_worker.rb +++ b/app/workers/irker_worker.rb @@ -2,8 +2,7 @@ require 'json' require 'socket' class IrkerWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(project_id, chans, colors, push_data, settings) project = Project.find(project_id) diff --git a/app/workers/merge_worker.rb b/app/workers/merge_worker.rb index 48e2da338f6..ba832fe30c6 100644 --- a/app/workers/merge_worker.rb +++ b/app/workers/merge_worker.rb @@ -1,6 +1,5 @@ class MergeWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(merge_request_id, current_user_id, params) params = params.with_indifferent_access diff --git a/app/workers/namespaceless_project_destroy_worker.rb b/app/workers/namespaceless_project_destroy_worker.rb index f1cd1769421..13d750e5876 100644 --- a/app/workers/namespaceless_project_destroy_worker.rb +++ b/app/workers/namespaceless_project_destroy_worker.rb @@ -5,8 +5,7 @@ # The worker will reject doing anything for projects that *do* have a # namespace. For those use ProjectDestroyWorker instead. class NamespacelessProjectDestroyWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker include ExceptionBacktrace def self.bulk_perform_async(args_list) diff --git a/app/workers/new_issue_worker.rb b/app/workers/new_issue_worker.rb index d9a8e892e90..3bc030f9c62 100644 --- a/app/workers/new_issue_worker.rb +++ b/app/workers/new_issue_worker.rb @@ -1,6 +1,5 @@ class NewIssueWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker include NewIssuable def perform(issue_id, user_id) diff --git a/app/workers/new_merge_request_worker.rb b/app/workers/new_merge_request_worker.rb index 1910c490159..bda2a0ab59d 100644 --- a/app/workers/new_merge_request_worker.rb +++ b/app/workers/new_merge_request_worker.rb @@ -1,6 +1,5 @@ class NewMergeRequestWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker include NewIssuable def perform(merge_request_id, user_id) diff --git a/app/workers/new_note_worker.rb b/app/workers/new_note_worker.rb index 926162b8c53..67c54fbf10e 100644 --- a/app/workers/new_note_worker.rb +++ b/app/workers/new_note_worker.rb @@ -1,6 +1,5 @@ class NewNoteWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker # Keep extra parameter to preserve backwards compatibility with # old `NewNoteWorker` jobs (can remove later) diff --git a/app/workers/pages_worker.rb b/app/workers/pages_worker.rb index 64788da7299..62f733c02fc 100644 --- a/app/workers/pages_worker.rb +++ b/app/workers/pages_worker.rb @@ -1,5 +1,5 @@ class PagesWorker - include Sidekiq::Worker + include ApplicationWorker sidekiq_options queue: :pages, retry: false diff --git a/app/workers/pipeline_hooks_worker.rb b/app/workers/pipeline_hooks_worker.rb index 30a75ec8435..661c29efe88 100644 --- a/app/workers/pipeline_hooks_worker.rb +++ b/app/workers/pipeline_hooks_worker.rb @@ -1,5 +1,5 @@ class PipelineHooksWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :hooks diff --git a/app/workers/pipeline_metrics_worker.rb b/app/workers/pipeline_metrics_worker.rb index 070943f1ecc..d46d1f122fc 100644 --- a/app/workers/pipeline_metrics_worker.rb +++ b/app/workers/pipeline_metrics_worker.rb @@ -1,5 +1,5 @@ class PipelineMetricsWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue def perform(pipeline_id) diff --git a/app/workers/pipeline_notification_worker.rb b/app/workers/pipeline_notification_worker.rb index cdb860b6675..a9a1168a6e3 100644 --- a/app/workers/pipeline_notification_worker.rb +++ b/app/workers/pipeline_notification_worker.rb @@ -1,5 +1,5 @@ class PipelineNotificationWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue def perform(pipeline_id, recipients = nil) diff --git a/app/workers/pipeline_process_worker.rb b/app/workers/pipeline_process_worker.rb index 8c067d05081..07dbf6a971e 100644 --- a/app/workers/pipeline_process_worker.rb +++ b/app/workers/pipeline_process_worker.rb @@ -1,5 +1,5 @@ class PipelineProcessWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :processing diff --git a/app/workers/pipeline_schedule_worker.rb b/app/workers/pipeline_schedule_worker.rb index 7320db1065e..c49758878a4 100644 --- a/app/workers/pipeline_schedule_worker.rb +++ b/app/workers/pipeline_schedule_worker.rb @@ -1,5 +1,5 @@ class PipelineScheduleWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/pipeline_success_worker.rb b/app/workers/pipeline_success_worker.rb index cb8bb2ffe75..68c40a259e1 100644 --- a/app/workers/pipeline_success_worker.rb +++ b/app/workers/pipeline_success_worker.rb @@ -1,5 +1,5 @@ class PipelineSuccessWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :processing diff --git a/app/workers/pipeline_update_worker.rb b/app/workers/pipeline_update_worker.rb index 5fa399dff4c..24a8a9fbed5 100644 --- a/app/workers/pipeline_update_worker.rb +++ b/app/workers/pipeline_update_worker.rb @@ -1,5 +1,5 @@ class PipelineUpdateWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :processing diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index b8f8d3750d9..f2b2c4428d3 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -1,6 +1,5 @@ class PostReceive - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(gl_repository, identifier, changes) project, is_wiki = Gitlab::GlRepository.parse(gl_repository) diff --git a/app/workers/process_commit_worker.rb b/app/workers/process_commit_worker.rb index c0c03848a40..52eebe475ec 100644 --- a/app/workers/process_commit_worker.rb +++ b/app/workers/process_commit_worker.rb @@ -5,8 +5,7 @@ # Consider using an extra worker if you need to add any extra (and potentially # slow) processing of commits. class ProcessCommitWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker # project_id - The ID of the project this commit belongs to. # user_id - The ID of the user that pushed the commit. diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb index 505ff9e086e..f19bcbf946a 100644 --- a/app/workers/project_cache_worker.rb +++ b/app/workers/project_cache_worker.rb @@ -1,7 +1,6 @@ # Worker for updating any project specific caches. class ProjectCacheWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker LEASE_TIMEOUT = 15.minutes.to_i diff --git a/app/workers/project_destroy_worker.rb b/app/workers/project_destroy_worker.rb index 3be7e686609..1ba854ca4cb 100644 --- a/app/workers/project_destroy_worker.rb +++ b/app/workers/project_destroy_worker.rb @@ -1,6 +1,5 @@ class ProjectDestroyWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker include ExceptionBacktrace def perform(project_id, user_id, params) diff --git a/app/workers/project_export_worker.rb b/app/workers/project_export_worker.rb index f13ac9e5db2..c100852374a 100644 --- a/app/workers/project_export_worker.rb +++ b/app/workers/project_export_worker.rb @@ -1,6 +1,5 @@ class ProjectExportWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker include ExceptionBacktrace sidekiq_options retry: 3 diff --git a/app/workers/project_migrate_hashed_storage_worker.rb b/app/workers/project_migrate_hashed_storage_worker.rb index 127aa6b9d7d..d01eb744e5d 100644 --- a/app/workers/project_migrate_hashed_storage_worker.rb +++ b/app/workers/project_migrate_hashed_storage_worker.rb @@ -1,6 +1,5 @@ class ProjectMigrateHashedStorageWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker LEASE_TIMEOUT = 30.seconds.to_i diff --git a/app/workers/project_service_worker.rb b/app/workers/project_service_worker.rb index 4883d848c53..75c4b8b3663 100644 --- a/app/workers/project_service_worker.rb +++ b/app/workers/project_service_worker.rb @@ -1,6 +1,5 @@ class ProjectServiceWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker sidekiq_options dead: false diff --git a/app/workers/propagate_service_template_worker.rb b/app/workers/propagate_service_template_worker.rb index 6b607451c7a..635a97c99af 100644 --- a/app/workers/propagate_service_template_worker.rb +++ b/app/workers/propagate_service_template_worker.rb @@ -1,7 +1,6 @@ # Worker for updating any project specific caches. class PropagateServiceTemplateWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker LEASE_TIMEOUT = 4.hours.to_i diff --git a/app/workers/prune_old_events_worker.rb b/app/workers/prune_old_events_worker.rb index 2b43bb19ad1..5ff62ab1369 100644 --- a/app/workers/prune_old_events_worker.rb +++ b/app/workers/prune_old_events_worker.rb @@ -1,5 +1,5 @@ class PruneOldEventsWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/reactive_caching_worker.rb b/app/workers/reactive_caching_worker.rb index 18b8daf4e1e..ef3ddb9024b 100644 --- a/app/workers/reactive_caching_worker.rb +++ b/app/workers/reactive_caching_worker.rb @@ -1,6 +1,5 @@ class ReactiveCachingWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(class_name, id, *args) klass = begin diff --git a/app/workers/remove_expired_group_links_worker.rb b/app/workers/remove_expired_group_links_worker.rb index 2a619f83410..7e64c3070a8 100644 --- a/app/workers/remove_expired_group_links_worker.rb +++ b/app/workers/remove_expired_group_links_worker.rb @@ -1,5 +1,5 @@ class RemoveExpiredGroupLinksWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/remove_expired_members_worker.rb b/app/workers/remove_expired_members_worker.rb index 31f652e5f9b..d80b3b15840 100644 --- a/app/workers/remove_expired_members_worker.rb +++ b/app/workers/remove_expired_members_worker.rb @@ -1,5 +1,5 @@ class RemoveExpiredMembersWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/remove_old_web_hook_logs_worker.rb b/app/workers/remove_old_web_hook_logs_worker.rb index 555e1bb8691..87fed42d7ce 100644 --- a/app/workers/remove_old_web_hook_logs_worker.rb +++ b/app/workers/remove_old_web_hook_logs_worker.rb @@ -1,5 +1,5 @@ class RemoveOldWebHookLogsWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue WEB_HOOK_LOG_LIFETIME = 2.days diff --git a/app/workers/remove_unreferenced_lfs_objects_worker.rb b/app/workers/remove_unreferenced_lfs_objects_worker.rb index b80f131d5f7..8daf079fc31 100644 --- a/app/workers/remove_unreferenced_lfs_objects_worker.rb +++ b/app/workers/remove_unreferenced_lfs_objects_worker.rb @@ -1,5 +1,5 @@ class RemoveUnreferencedLfsObjectsWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/repository_archive_cache_worker.rb b/app/workers/repository_archive_cache_worker.rb index e47069df189..86a258cf94f 100644 --- a/app/workers/repository_archive_cache_worker.rb +++ b/app/workers/repository_archive_cache_worker.rb @@ -1,5 +1,5 @@ class RepositoryArchiveCacheWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/repository_check/batch_worker.rb b/app/workers/repository_check/batch_worker.rb index b94d83bd709..76688cf51c1 100644 --- a/app/workers/repository_check/batch_worker.rb +++ b/app/workers/repository_check/batch_worker.rb @@ -1,6 +1,6 @@ module RepositoryCheck class BatchWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue RUN_TIME = 3600 diff --git a/app/workers/repository_check/clear_worker.rb b/app/workers/repository_check/clear_worker.rb index 85bc9103538..97b89dc3db5 100644 --- a/app/workers/repository_check/clear_worker.rb +++ b/app/workers/repository_check/clear_worker.rb @@ -1,6 +1,6 @@ module RepositoryCheck class ClearWorker - include Sidekiq::Worker + include ApplicationWorker include RepositoryCheckQueue def perform diff --git a/app/workers/repository_check/single_repository_worker.rb b/app/workers/repository_check/single_repository_worker.rb index 3d4bee15f1c..4e3c691e8da 100644 --- a/app/workers/repository_check/single_repository_worker.rb +++ b/app/workers/repository_check/single_repository_worker.rb @@ -1,6 +1,6 @@ module RepositoryCheck class SingleRepositoryWorker - include Sidekiq::Worker + include ApplicationWorker include RepositoryCheckQueue def perform(project_id) diff --git a/app/workers/repository_fork_worker.rb b/app/workers/repository_fork_worker.rb index 001c11b73e1..a07ef1705a1 100644 --- a/app/workers/repository_fork_worker.rb +++ b/app/workers/repository_fork_worker.rb @@ -1,9 +1,8 @@ class RepositoryForkWorker ForkError = Class.new(StandardError) - include Sidekiq::Worker + include ApplicationWorker include Gitlab::ShellAdapter - include DedicatedSidekiqQueue include ProjectStartImport sidekiq_options status_expiration: StuckImportJobsWorker::IMPORT_JOBS_EXPIRATION diff --git a/app/workers/repository_import_worker.rb b/app/workers/repository_import_worker.rb index 4e90b137b26..55715c83cb1 100644 --- a/app/workers/repository_import_worker.rb +++ b/app/workers/repository_import_worker.rb @@ -1,8 +1,7 @@ class RepositoryImportWorker ImportError = Class.new(StandardError) - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker include ExceptionBacktrace include ProjectStartImport diff --git a/app/workers/requests_profiles_worker.rb b/app/workers/requests_profiles_worker.rb index 703b025d76e..55c236e9e9d 100644 --- a/app/workers/requests_profiles_worker.rb +++ b/app/workers/requests_profiles_worker.rb @@ -1,5 +1,5 @@ class RequestsProfilesWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/schedule_update_user_activity_worker.rb b/app/workers/schedule_update_user_activity_worker.rb index 6c2c3e437f3..d9376577597 100644 --- a/app/workers/schedule_update_user_activity_worker.rb +++ b/app/workers/schedule_update_user_activity_worker.rb @@ -1,5 +1,5 @@ class ScheduleUpdateUserActivityWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform(batch_size = 500) diff --git a/app/workers/stage_update_worker.rb b/app/workers/stage_update_worker.rb index c301cea5ad6..69f2318d83b 100644 --- a/app/workers/stage_update_worker.rb +++ b/app/workers/stage_update_worker.rb @@ -1,5 +1,5 @@ class StageUpdateWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :processing diff --git a/app/workers/storage_migrator_worker.rb b/app/workers/storage_migrator_worker.rb index b48ead799b9..f92421a667d 100644 --- a/app/workers/storage_migrator_worker.rb +++ b/app/workers/storage_migrator_worker.rb @@ -1,6 +1,5 @@ class StorageMigratorWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker BATCH_SIZE = 100 diff --git a/app/workers/stuck_ci_jobs_worker.rb b/app/workers/stuck_ci_jobs_worker.rb index 367e227f680..fb26fa4c515 100644 --- a/app/workers/stuck_ci_jobs_worker.rb +++ b/app/workers/stuck_ci_jobs_worker.rb @@ -1,5 +1,5 @@ class StuckCiJobsWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue EXCLUSIVE_LEASE_KEY = 'stuck_ci_builds_worker_lease'.freeze diff --git a/app/workers/stuck_import_jobs_worker.rb b/app/workers/stuck_import_jobs_worker.rb index f850e459cd9..e0e6d1418de 100644 --- a/app/workers/stuck_import_jobs_worker.rb +++ b/app/workers/stuck_import_jobs_worker.rb @@ -1,5 +1,5 @@ class StuckImportJobsWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue IMPORT_JOBS_EXPIRATION = 15.hours.to_i diff --git a/app/workers/stuck_merge_jobs_worker.rb b/app/workers/stuck_merge_jobs_worker.rb index a396c0f27b2..36d2a2e6466 100644 --- a/app/workers/stuck_merge_jobs_worker.rb +++ b/app/workers/stuck_merge_jobs_worker.rb @@ -1,5 +1,5 @@ class StuckMergeJobsWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/system_hook_push_worker.rb b/app/workers/system_hook_push_worker.rb index e43bbe35de9..ceeaaf8d189 100644 --- a/app/workers/system_hook_push_worker.rb +++ b/app/workers/system_hook_push_worker.rb @@ -1,6 +1,5 @@ class SystemHookPushWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(push_data, hook_id) SystemHooksService.new.execute_hooks(push_data, hook_id) diff --git a/app/workers/trending_projects_worker.rb b/app/workers/trending_projects_worker.rb index 0531630d13a..7eb65452a7d 100644 --- a/app/workers/trending_projects_worker.rb +++ b/app/workers/trending_projects_worker.rb @@ -1,5 +1,5 @@ class TrendingProjectsWorker - include Sidekiq::Worker + include ApplicationWorker include CronjobQueue def perform diff --git a/app/workers/update_merge_requests_worker.rb b/app/workers/update_merge_requests_worker.rb index afc47fc63d6..74bb9993275 100644 --- a/app/workers/update_merge_requests_worker.rb +++ b/app/workers/update_merge_requests_worker.rb @@ -1,6 +1,5 @@ class UpdateMergeRequestsWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker LOG_TIME_THRESHOLD = 90 # seconds diff --git a/app/workers/update_user_activity_worker.rb b/app/workers/update_user_activity_worker.rb index 31bbdb69edb..27ec5cd33fb 100644 --- a/app/workers/update_user_activity_worker.rb +++ b/app/workers/update_user_activity_worker.rb @@ -1,6 +1,5 @@ class UpdateUserActivityWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(pairs) pairs = cast_data(pairs) diff --git a/app/workers/upload_checksum_worker.rb b/app/workers/upload_checksum_worker.rb index 78931f1258f..9222760c031 100644 --- a/app/workers/upload_checksum_worker.rb +++ b/app/workers/upload_checksum_worker.rb @@ -1,6 +1,5 @@ class UploadChecksumWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker def perform(upload_id) upload = Upload.find(upload_id) diff --git a/app/workers/wait_for_cluster_creation_worker.rb b/app/workers/wait_for_cluster_creation_worker.rb index 241ed3901dc..19cdb279aaa 100644 --- a/app/workers/wait_for_cluster_creation_worker.rb +++ b/app/workers/wait_for_cluster_creation_worker.rb @@ -1,5 +1,5 @@ class WaitForClusterCreationWorker - include Sidekiq::Worker + include ApplicationWorker include ClusterQueue def perform(cluster_id) diff --git a/app/workers/web_hook_worker.rb b/app/workers/web_hook_worker.rb index 713c0228040..dfc3f33ad9d 100644 --- a/app/workers/web_hook_worker.rb +++ b/app/workers/web_hook_worker.rb @@ -1,6 +1,5 @@ class WebHookWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker sidekiq_options retry: 4, dead: false diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index a1cc9655319..ba4481ae602 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -64,13 +64,13 @@ end # The Sidekiq client API always adds the queue to the Sidekiq queue # list, but mail_room and gitlab-shell do not. This is only necessary # for monitoring. -config = YAML.load_file(Rails.root.join('config', 'sidekiq_queues.yml').to_s) - begin + queues = Gitlab::SidekiqConfig.worker_queues + Sidekiq.redis do |conn| conn.pipelined do - config[:queues].each do |queue| - conn.sadd('queues', queue[0]) + queues.each do |queue| + conn.sadd('queues', queue) end end end diff --git a/doc/development/sidekiq_style_guide.md b/doc/development/sidekiq_style_guide.md index 1e9fdbc65e2..a120f3e4771 100644 --- a/doc/development/sidekiq_style_guide.md +++ b/doc/development/sidekiq_style_guide.md @@ -18,8 +18,7 @@ include the `DedicatedSidekiqQueue` concern as follows: ```ruby class ProcessSomethingWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue + include ApplicationWorker end ``` diff --git a/lib/gitlab/sidekiq_config.rb b/lib/gitlab/sidekiq_config.rb new file mode 100644 index 00000000000..dc9886732b5 --- /dev/null +++ b/lib/gitlab/sidekiq_config.rb @@ -0,0 +1,50 @@ +require 'yaml' + +module Gitlab + module SidekiqConfig + def self.redis_queues + @redis_queues ||= Sidekiq::Queue.all.map(&:name) + end + + # This method is called by `bin/sidekiq-cluster` in EE, which runs outside + # of bundler/Rails context, so we cannot use any gem or Rails methods. + def self.config_queues(rails_path = Rails.root.to_s) + @config_queues ||= begin + config = YAML.load_file(File.join(rails_path, 'config', 'sidekiq_queues.yml')) + config[:queues].map(&:first) + end + end + + def self.cron_workers + @cron_workers ||= Settings.cron_jobs.map { |job_name, options| options['job_class'].constantize } + end + + def self.workers + @workers ||= find_workers(Rails.root.join('app', 'workers')) + end + + def self.default_queues + [ActionMailer::DeliveryJob.queue_name, 'default'] + end + + def self.worker_queues + @worker_queues ||= (workers.map(&:queue) + default_queues).uniq + end + + def self.find_workers(root) + concerns = root.join('concerns').to_s + + workers = Dir[root.join('**', '*.rb')] + .reject { |path| path.start_with?(concerns) } + + workers.map! do |path| + ns = Pathname.new(path).relative_path_from(root).to_s.gsub('.rb', '') + + ns.camelize.constantize + end + + # Skip concerns + workers.select { |w| w < Sidekiq::Worker } + end + end +end diff --git a/spec/lib/gitlab/sidekiq_config_spec.rb b/spec/lib/gitlab/sidekiq_config_spec.rb new file mode 100644 index 00000000000..09f95be2213 --- /dev/null +++ b/spec/lib/gitlab/sidekiq_config_spec.rb @@ -0,0 +1,24 @@ +require 'rails_helper' + +describe Gitlab::SidekiqConfig do + describe '.workers' do + it 'includes all workers' do + workers = described_class.workers + + expect(workers).to include(PostReceive) + expect(workers).to include(MergeWorker) + end + end + + describe '.worker_queues' do + it 'includes all queues' do + queues = described_class.worker_queues + + expect(queues).to include('post_receive') + expect(queues).to include('merge') + expect(queues).to include('cronjob') + expect(queues).to include('mailers') + expect(queues).to include('default') + end + end +end diff --git a/spec/workers/concerns/application_worker_spec.rb b/spec/workers/concerns/application_worker_spec.rb new file mode 100644 index 00000000000..fd434f58602 --- /dev/null +++ b/spec/workers/concerns/application_worker_spec.rb @@ -0,0 +1,27 @@ +require 'spec_helper' + +describe ApplicationWorker do + let(:worker) do + Class.new do + def self.name + 'Gitlab::Foo::Bar::DummyWorker' + end + + include ApplicationWorker + end + end + + describe 'Sidekiq options' do + it 'sets the queue name based on the class name' do + expect(worker.sidekiq_options['queue']).to eq('foo_bar_dummy') + end + end + + describe '.queue' do + it 'returns the queue name' do + worker.sidekiq_options queue: :some_queue + + expect(worker.queue).to eq('some_queue') + end + end +end diff --git a/spec/workers/concerns/cluster_queue_spec.rb b/spec/workers/concerns/cluster_queue_spec.rb index 1050651fa51..5049886b55c 100644 --- a/spec/workers/concerns/cluster_queue_spec.rb +++ b/spec/workers/concerns/cluster_queue_spec.rb @@ -3,7 +3,11 @@ require 'spec_helper' describe ClusterQueue do let(:worker) do Class.new do - include Sidekiq::Worker + def self.name + 'DummyWorker' + end + + include ApplicationWorker include ClusterQueue end end diff --git a/spec/workers/concerns/cronjob_queue_spec.rb b/spec/workers/concerns/cronjob_queue_spec.rb index 5d1336c21a6..3ae1c5f54d8 100644 --- a/spec/workers/concerns/cronjob_queue_spec.rb +++ b/spec/workers/concerns/cronjob_queue_spec.rb @@ -3,7 +3,11 @@ require 'spec_helper' describe CronjobQueue do let(:worker) do Class.new do - include Sidekiq::Worker + def self.name + 'DummyWorker' + end + + include ApplicationWorker include CronjobQueue end end diff --git a/spec/workers/concerns/dedicated_sidekiq_queue_spec.rb b/spec/workers/concerns/dedicated_sidekiq_queue_spec.rb deleted file mode 100644 index 512baec8b7e..00000000000 --- a/spec/workers/concerns/dedicated_sidekiq_queue_spec.rb +++ /dev/null @@ -1,20 +0,0 @@ -require 'spec_helper' - -describe DedicatedSidekiqQueue do - let(:worker) do - Class.new do - def self.name - 'Foo::Bar::DummyWorker' - end - - include Sidekiq::Worker - include DedicatedSidekiqQueue - end - end - - describe 'queue names' do - it 'sets the queue name based on the class name' do - expect(worker.sidekiq_options['queue']).to eq('foo_bar_dummy') - end - end -end diff --git a/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb b/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb index 3ccf06f2d7d..68cfe9d5545 100644 --- a/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb +++ b/spec/workers/concerns/gitlab/github_import/object_importer_spec.rb @@ -3,6 +3,10 @@ require 'spec_helper' describe Gitlab::GithubImport::ObjectImporter do let(:worker) do Class.new do + def self.name + 'DummyWorker' + end + include(Gitlab::GithubImport::ObjectImporter) def counter_name diff --git a/spec/workers/concerns/gitlab/github_import/queue_spec.rb b/spec/workers/concerns/gitlab/github_import/queue_spec.rb index 321ae3fe978..9c69ee32da1 100644 --- a/spec/workers/concerns/gitlab/github_import/queue_spec.rb +++ b/spec/workers/concerns/gitlab/github_import/queue_spec.rb @@ -3,7 +3,11 @@ require 'spec_helper' describe Gitlab::GithubImport::Queue do it 'sets the Sidekiq options for the worker' do worker = Class.new do - include Sidekiq::Worker + def self.name + 'DummyWorker' + end + + include ApplicationWorker include Gitlab::GithubImport::Queue end diff --git a/spec/workers/concerns/pipeline_queue_spec.rb b/spec/workers/concerns/pipeline_queue_spec.rb index eac5a770e5f..dd911760948 100644 --- a/spec/workers/concerns/pipeline_queue_spec.rb +++ b/spec/workers/concerns/pipeline_queue_spec.rb @@ -3,7 +3,11 @@ require 'spec_helper' describe PipelineQueue do let(:worker) do Class.new do - include Sidekiq::Worker + def self.name + 'DummyWorker' + end + + include ApplicationWorker include PipelineQueue end end diff --git a/spec/workers/concerns/repository_check_queue_spec.rb b/spec/workers/concerns/repository_check_queue_spec.rb index 8868e969829..fdbbfcc90a5 100644 --- a/spec/workers/concerns/repository_check_queue_spec.rb +++ b/spec/workers/concerns/repository_check_queue_spec.rb @@ -3,7 +3,11 @@ require 'spec_helper' describe RepositoryCheckQueue do let(:worker) do Class.new do - include Sidekiq::Worker + def self.name + 'DummyWorker' + end + + include ApplicationWorker include RepositoryCheckQueue end end diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index 30908534eb3..7ee0a51a263 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -1,44 +1,21 @@ require 'spec_helper' describe 'Every Sidekiq worker' do - let(:workers) do - root = Rails.root.join('app', 'workers') - concerns = root.join('concerns').to_s - - workers = Dir[root.join('**', '*.rb')] - .reject { |path| path.start_with?(concerns) } - - workers.map do |path| - ns = Pathname.new(path).relative_path_from(root).to_s.gsub('.rb', '') - - ns.camelize.constantize - end + it 'includes ApplicationWorker' do + expect(Gitlab::SidekiqConfig.workers).to all(include(ApplicationWorker)) end it 'does not use the default queue' do - workers.each do |worker| - expect(worker.sidekiq_options['queue'].to_s).not_to eq('default') - end + expect(Gitlab::SidekiqConfig.workers.map(&:queue)).not_to include('default') end it 'uses the cronjob queue when the worker runs as a cronjob' do - cron_workers = Settings.cron_jobs - .map { |job_name, options| options['job_class'].constantize } - .to_set - - workers.each do |worker| - next unless cron_workers.include?(worker) - - expect(worker.sidekiq_options['queue'].to_s).to eq('cronjob') - end + expect(Gitlab::SidekiqConfig.cron_workers.map(&:queue)).to all(eq('cronjob')) end it 'defines the queue in the Sidekiq configuration file' do - config = YAML.load_file(Rails.root.join('config', 'sidekiq_queues.yml').to_s) - queue_names = config[:queues].map { |(queue, _)| queue }.to_set + config_queue_names = Gitlab::SidekiqConfig.config_queues.to_set - workers.each do |worker| - expect(queue_names).to include(worker.sidekiq_options['queue'].to_s) - end + expect(Gitlab::SidekiqConfig.worker_queues).to all(be_in(config_queue_names)) end end -- cgit v1.2.1 From a5c3f1c8ff7da20183b172b2b0693a6010c9e86d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 30 Nov 2017 16:28:09 +0100 Subject: Update docs --- doc/development/sidekiq_style_guide.md | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/doc/development/sidekiq_style_guide.md b/doc/development/sidekiq_style_guide.md index a120f3e4771..085fb8f902c 100644 --- a/doc/development/sidekiq_style_guide.md +++ b/doc/development/sidekiq_style_guide.md @@ -3,6 +3,12 @@ This document outlines various guidelines that should be followed when adding or modifying Sidekiq workers. +## ApplicationWorker + +All workers should include `ApplicationWorker` instead of `Sidekiq::Worker`, +which adds some convenience methods and automatically sets the queue based on +the worker's name. + ## Default Queue Use of the "default" queue is not allowed. Every worker should use a queue that @@ -13,18 +19,10 @@ A list of all available queues can be found in `config/sidekiq_queues.yml`. ## Dedicated Queues -Most workers should use their own queue. To ease this process a worker can -include the `DedicatedSidekiqQueue` concern as follows: - -```ruby -class ProcessSomethingWorker - include ApplicationWorker -end -``` - -This will set the queue name based on the class' name, minus the `Worker` -suffix. In the above example this would lead to the queue being -`process_something`. +Most workers should use their own queue, which is automatically set based on the +worker class name. For a worker named `ProcessSomethingWorker`, the queue name +would be `process_something`. If you're not sure what a worker's queue name is, +you can find it using `SomeWorker.queue`. In some cases multiple workers do use the same queue. For example, the various workers for updating CI pipelines all use the `pipeline` queue. Adding workers @@ -38,7 +36,7 @@ tests should be placed in `spec/workers`. ## Removing or renaming queues -Try to avoid renaming or removing queues in minor and patch releases. -During online update instance can have pending jobs and removing the queue can -lead to those jobs being stuck forever. If you can't write migration for those -Sidekiq jobs, please consider doing rename or remove queue in major release only. \ No newline at end of file +Try to avoid renaming or removing queues in minor and patch releases. +During online update instance can have pending jobs and removing the queue can +lead to those jobs being stuck forever. If you can't write migration for those +Sidekiq jobs, please consider doing rename or remove queue in major release only. -- cgit v1.2.1 From 1e6ca3c41ead23c5e433460c8c807ea73d9ec0ef Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 29 Nov 2017 16:30:17 +0100 Subject: Consistently schedule Sidekiq jobs --- app/models/group.rb | 1 + app/models/key.rb | 1 + app/models/member.rb | 1 + app/models/service.rb | 2 +- app/models/user.rb | 2 ++ app/services/system_hooks_service.rb | 6 ++++- app/services/web_hook_service.rb | 2 +- app/workers/authorized_projects_worker.rb | 5 ---- app/workers/background_migration_worker.rb | 28 ------------------- app/workers/concerns/application_worker.rb | 15 +++++++++++ app/workers/expire_build_artifacts_worker.rb | 2 +- .../namespaceless_project_destroy_worker.rb | 4 --- .../initializers/forbid_sidekiq_in_transactions.rb | 19 +++++++------ .../20170627101016_schedule_event_migrations.rb | 4 +-- ...chedule_create_gpg_key_subkeys_from_gpg_keys.rb | 2 +- doc/development/background_migrations.md | 10 +++---- lib/after_commit_queue.rb | 26 ++++++++++++++++-- lib/gitlab/database/migration_helpers.rb | 4 +-- spec/lib/gitlab/database/migration_helpers_spec.rb | 8 +++--- spec/services/web_hook_service_spec.rb | 2 +- spec/workers/authorized_projects_worker_spec.rb | 1 - spec/workers/background_migration_worker_spec.rb | 31 ---------------------- spec/workers/concerns/application_worker_spec.rb | 31 ++++++++++++++++++++++ 23 files changed, 107 insertions(+), 100 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 27287f079a4..505e943e464 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -2,6 +2,7 @@ require 'carrierwave/orm/activerecord' class Group < Namespace include Gitlab::ConfigHelper + include AfterCommitQueue include AccessRequestable include Avatarable include Referable diff --git a/app/models/key.rb b/app/models/key.rb index 815fd1de909..a3f8a5d6dc7 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -2,6 +2,7 @@ require 'digest/md5' class Key < ActiveRecord::Base include Gitlab::CurrentSettings + include AfterCommitQueue include Sortable belongs_to :user diff --git a/app/models/member.rb b/app/models/member.rb index cbbd58f2eaf..2fe5fda985f 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -1,4 +1,5 @@ class Member < ActiveRecord::Base + include AfterCommitQueue include Sortable include Importable include Expirable diff --git a/app/models/service.rb b/app/models/service.rb index fdd2605e3e3..3c4f1885dd0 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -211,7 +211,7 @@ class Service < ActiveRecord::Base def async_execute(data) return unless supported_events.include?(data[:object_kind]) - Sidekiq::Client.enqueue(ProjectServiceWorker, id, data) + ProjectServiceWorker.perform_async(id, data) end def issue_tracker? diff --git a/app/models/user.rb b/app/models/user.rb index 76fd395be9a..38ee4ed50c1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -7,6 +7,7 @@ class User < ActiveRecord::Base include Gitlab::ConfigHelper include Gitlab::CurrentSettings include Gitlab::SQL::Pattern + include AfterCommitQueue include Avatarable include Referable include Sortable @@ -903,6 +904,7 @@ class User < ActiveRecord::Base def post_destroy_hook log_info("User \"#{name}\" (#{email}) was removed") + system_hook_service.execute_hooks_for(self, :destroy) end diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb index 911cc919bb8..690918b4a00 100644 --- a/app/services/system_hooks_service.rb +++ b/app/services/system_hooks_service.rb @@ -1,6 +1,10 @@ class SystemHooksService def execute_hooks_for(model, event) - execute_hooks(build_event_data(model, event)) + data = build_event_data(model, event) + + model.run_after_commit_or_now do + SystemHooksService.new.execute_hooks(data) + end end def execute_hooks(data, hooks_scope = :all) diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index cd99e0b90f9..6ebc7c89500 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -63,7 +63,7 @@ class WebHookService end def async_execute - Sidekiq::Client.enqueue(WebHookWorker, hook.id, data, hook_name) + WebHookWorker.perform_async(hook.id, data, hook_name) end private diff --git a/app/workers/authorized_projects_worker.rb b/app/workers/authorized_projects_worker.rb index d4f334ec3b8..09559e3b696 100644 --- a/app/workers/authorized_projects_worker.rb +++ b/app/workers/authorized_projects_worker.rb @@ -16,11 +16,6 @@ class AuthorizedProjectsWorker waiter.wait end - # Schedules multiple jobs to run in sidekiq without waiting for completion - def self.bulk_perform_async(args_list) - Sidekiq::Client.push_bulk('class' => self, 'queue' => sidekiq_options['queue'], 'args' => args_list) - end - # Performs multiple jobs directly. Failed jobs will be put into sidekiq so # they can benefit from retries def self.bulk_perform_inline(args_list) diff --git a/app/workers/background_migration_worker.rb b/app/workers/background_migration_worker.rb index 65791c4eaba..aeb3bc019b9 100644 --- a/app/workers/background_migration_worker.rb +++ b/app/workers/background_migration_worker.rb @@ -1,34 +1,6 @@ class BackgroundMigrationWorker include ApplicationWorker - # Enqueues a number of jobs in bulk. - # - # The `jobs` argument should be an Array of Arrays, each sub-array must be in - # the form: - # - # [migration-class, [arg1, arg2, ...]] - def self.perform_bulk(jobs) - Sidekiq::Client.push_bulk('class' => self, - 'queue' => sidekiq_options['queue'], - 'args' => jobs) - end - - # Schedules multiple jobs in bulk, with a delay. - # - def self.perform_bulk_in(delay, jobs) - now = Time.now.to_i - schedule = now + delay.to_i - - if schedule <= now - raise ArgumentError, 'The schedule time must be in the future!' - end - - Sidekiq::Client.push_bulk('class' => self, - 'queue' => sidekiq_options['queue'], - 'args' => jobs, - 'at' => schedule) - end - # Performs the background migration. # # See Gitlab::BackgroundMigration.perform for more information. diff --git a/app/workers/concerns/application_worker.rb b/app/workers/concerns/application_worker.rb index bf1ecaa0c6d..9c3bdabc49e 100644 --- a/app/workers/concerns/application_worker.rb +++ b/app/workers/concerns/application_worker.rb @@ -21,5 +21,20 @@ module ApplicationWorker def queue get_sidekiq_options['queue'].to_s end + + def bulk_perform_async(args_list) + Sidekiq::Client.push_bulk('class' => self, 'args' => args_list) + end + + def bulk_perform_in(delay, args_list) + now = Time.now.to_i + schedule = now + delay.to_i + + if schedule <= now + raise ArgumentError, 'The schedule time must be in the future!' + end + + Sidekiq::Client.push_bulk('class' => self, 'args' => args_list, 'at' => schedule) + end end end diff --git a/app/workers/expire_build_artifacts_worker.rb b/app/workers/expire_build_artifacts_worker.rb index 73ab4211080..87e5dca01fd 100644 --- a/app/workers/expire_build_artifacts_worker.rb +++ b/app/workers/expire_build_artifacts_worker.rb @@ -8,6 +8,6 @@ class ExpireBuildArtifactsWorker build_ids = Ci::Build.with_expired_artifacts.pluck(:id) build_ids = build_ids.map { |build_id| [build_id] } - Sidekiq::Client.push_bulk('class' => ExpireBuildInstanceArtifactsWorker, 'args' => build_ids ) + ExpireBuildInstanceArtifactsWorker.bulk_perform_async(build_ids) end end diff --git a/app/workers/namespaceless_project_destroy_worker.rb b/app/workers/namespaceless_project_destroy_worker.rb index 13d750e5876..adb25c2a170 100644 --- a/app/workers/namespaceless_project_destroy_worker.rb +++ b/app/workers/namespaceless_project_destroy_worker.rb @@ -8,10 +8,6 @@ class NamespacelessProjectDestroyWorker include ApplicationWorker include ExceptionBacktrace - def self.bulk_perform_async(args_list) - Sidekiq::Client.push_bulk('class' => self, 'queue' => sidekiq_options['queue'], 'args' => args_list) - end - def perform(project_id) begin project = Project.unscoped.find(project_id) diff --git a/config/initializers/forbid_sidekiq_in_transactions.rb b/config/initializers/forbid_sidekiq_in_transactions.rb index a78711fe599..bedd57ede04 100644 --- a/config/initializers/forbid_sidekiq_in_transactions.rb +++ b/config/initializers/forbid_sidekiq_in_transactions.rb @@ -13,20 +13,19 @@ module Sidekiq module ClassMethods module NoSchedulingFromTransactions - NESTING = ::Rails.env.test? ? 1 : 0 - %i(perform_async perform_at perform_in).each do |name| define_method(name) do |*args| - return super(*args) if Sidekiq::Worker.skip_transaction_check - return super(*args) unless ActiveRecord::Base.connection.open_transactions > NESTING + if !Sidekiq::Worker.skip_transaction_check && AfterCommitQueue.inside_transaction? + raise <<-MSG.strip_heredoc + `#{self}.#{name}` cannot be called inside a transaction as this can lead to + race conditions when the worker runs before the transaction is committed and + tries to access a model that has not been saved yet. - raise <<-MSG.strip_heredoc - `#{self}.#{name}` cannot be called inside a transaction as this can lead to - race conditions when the worker runs before the transaction is committed and - tries to access a model that has not been saved yet. + Use an `after_commit` hook, or include `AfterCommitQueue` and use a `run_after_commit` block instead. + MSG + end - Use an `after_commit` hook, or include `AfterCommitQueue` and use a `run_after_commit` block instead. - MSG + super(*args) end end end diff --git a/db/post_migrate/20170627101016_schedule_event_migrations.rb b/db/post_migrate/20170627101016_schedule_event_migrations.rb index 1f34375ff0d..1e020d05f78 100644 --- a/db/post_migrate/20170627101016_schedule_event_migrations.rb +++ b/db/post_migrate/20170627101016_schedule_event_migrations.rb @@ -25,14 +25,14 @@ class ScheduleEventMigrations < ActiveRecord::Migration # We push multiple jobs at a time to reduce the time spent in # Sidekiq/Redis operations. We're using this buffer based approach so we # don't need to run additional queries for every range. - BackgroundMigrationWorker.perform_bulk(jobs) + BackgroundMigrationWorker.bulk_perform_async(jobs) jobs.clear end jobs << ['MigrateEventsToPushEventPayloads', [min, max]] end - BackgroundMigrationWorker.perform_bulk(jobs) unless jobs.empty? + BackgroundMigrationWorker.bulk_perform_async(jobs) unless jobs.empty? end def down diff --git a/db/post_migrate/20171005130944_schedule_create_gpg_key_subkeys_from_gpg_keys.rb b/db/post_migrate/20171005130944_schedule_create_gpg_key_subkeys_from_gpg_keys.rb index 01d56fbd490..467c584c2e0 100644 --- a/db/post_migrate/20171005130944_schedule_create_gpg_key_subkeys_from_gpg_keys.rb +++ b/db/post_migrate/20171005130944_schedule_create_gpg_key_subkeys_from_gpg_keys.rb @@ -19,7 +19,7 @@ class ScheduleCreateGpgKeySubkeysFromGpgKeys < ActiveRecord::Migration [MIGRATION, [id]] end - BackgroundMigrationWorker.perform_bulk(jobs) + BackgroundMigrationWorker.bulk_perform_async(jobs) end end diff --git a/doc/development/background_migrations.md b/doc/development/background_migrations.md index 5452b0e7a2f..fd2b9d0e908 100644 --- a/doc/development/background_migrations.md +++ b/doc/development/background_migrations.md @@ -68,10 +68,10 @@ BackgroundMigrationWorker.perform_async('BackgroundMigrationClassName', [arg1, a ``` Usually it's better to enqueue jobs in bulk, for this you can use -`BackgroundMigrationWorker.perform_bulk`: +`BackgroundMigrationWorker.bulk_perform_async`: ```ruby -BackgroundMigrationWorker.perform_bulk( +BackgroundMigrationWorker.bulk_perform_async( [['BackgroundMigrationClassName', [1]], ['BackgroundMigrationClassName', [2]]] ) @@ -85,13 +85,13 @@ updates. Removals in turn can be handled by simply defining foreign keys with cascading deletes. If you would like to schedule jobs in bulk with a delay, you can use -`BackgroundMigrationWorker.perform_bulk_in`: +`BackgroundMigrationWorker.bulk_perform_in`: ```ruby jobs = [['BackgroundMigrationClassName', [1]], ['BackgroundMigrationClassName', [2]]] -BackgroundMigrationWorker.perform_bulk_in(5.minutes, jobs) +BackgroundMigrationWorker.bulk_perform_in(5.minutes, jobs) ``` ## Cleaning Up @@ -201,7 +201,7 @@ class ScheduleExtractServicesUrl < ActiveRecord::Migration ['ExtractServicesUrl', [id]] end - BackgroundMigrationWorker.perform_bulk(jobs) + BackgroundMigrationWorker.bulk_perform_async(jobs) end end diff --git a/lib/after_commit_queue.rb b/lib/after_commit_queue.rb index 4750a2c373a..db63c5038ae 100644 --- a/lib/after_commit_queue.rb +++ b/lib/after_commit_queue.rb @@ -6,12 +6,34 @@ module AfterCommitQueue after_rollback :_clear_after_commit_queue end - def run_after_commit(method = nil, &block) - _after_commit_queue << proc { self.send(method) } if method # rubocop:disable GitlabSecurity/PublicSend + def run_after_commit(&block) _after_commit_queue << block if block + + true + end + + def run_after_commit_or_now(&block) + if AfterCommitQueue.inside_transaction? + run_after_commit(&block) + else + instance_eval(&block) + end + true end + def self.open_transactions_baseline + if ::Rails.env.test? + return DatabaseCleaner.connections.count { |conn| conn.strategy.is_a?(DatabaseCleaner::ActiveRecord::Transaction) } + end + + 0 + end + + def self.inside_transaction? + ActiveRecord::Base.connection.open_transactions > open_transactions_baseline + end + protected def _run_after_commit_queue diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index c276c3566b4..3f65bc912de 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -703,14 +703,14 @@ into similar problems in the future (e.g. when new tables are created). # We push multiple jobs at a time to reduce the time spent in # Sidekiq/Redis operations. We're using this buffer based approach so we # don't need to run additional queries for every range. - BackgroundMigrationWorker.perform_bulk(jobs) + BackgroundMigrationWorker.bulk_perform_async(jobs) jobs.clear end jobs << [job_class_name, [start_id, end_id]] end - BackgroundMigrationWorker.perform_bulk(jobs) unless jobs.empty? + BackgroundMigrationWorker.bulk_perform_async(jobs) unless jobs.empty? end # Queues background migration jobs for an entire table, batched by ID range. diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 3c8350b3aad..664ba0f7234 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -942,8 +942,8 @@ describe Gitlab::Database::MigrationHelpers do end it 'queues jobs in groups of buffer size 1' do - expect(BackgroundMigrationWorker).to receive(:perform_bulk).with([['FooJob', [id1, id2]]]) - expect(BackgroundMigrationWorker).to receive(:perform_bulk).with([['FooJob', [id3, id3]]]) + expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with([['FooJob', [id1, id2]]]) + expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with([['FooJob', [id3, id3]]]) model.bulk_queue_background_migration_jobs_by_range(User, 'FooJob', batch_size: 2) end @@ -960,8 +960,8 @@ describe Gitlab::Database::MigrationHelpers do end it 'queues jobs in bulk all at once (big buffer size)' do - expect(BackgroundMigrationWorker).to receive(:perform_bulk).with([['FooJob', [id1, id2]], - ['FooJob', [id3, id3]]]) + expect(BackgroundMigrationWorker).to receive(:bulk_perform_async).with([['FooJob', [id1, id2]], + ['FooJob', [id3, id3]]]) model.bulk_queue_background_migration_jobs_by_range(User, 'FooJob', batch_size: 2) end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index a669429ce3e..21910e69d2e 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -146,7 +146,7 @@ describe WebHookService do let(:system_hook) { create(:system_hook) } it 'enqueue WebHookWorker' do - expect(Sidekiq::Client).to receive(:enqueue).with(WebHookWorker, project_hook.id, data, 'push_hooks') + expect(WebHookWorker).to receive(:perform_async).with(project_hook.id, data, 'push_hooks') described_class.new(project_hook, data, 'push_hooks').async_execute end diff --git a/spec/workers/authorized_projects_worker_spec.rb b/spec/workers/authorized_projects_worker_spec.rb index 90ed1309d4a..0d6eb536c33 100644 --- a/spec/workers/authorized_projects_worker_spec.rb +++ b/spec/workers/authorized_projects_worker_spec.rb @@ -65,7 +65,6 @@ describe AuthorizedProjectsWorker do args_list = build_args_list(project.owner.id) push_bulk_args = { 'class' => described_class, - 'queue' => described_class.sidekiq_options['queue'], 'args' => args_list } diff --git a/spec/workers/background_migration_worker_spec.rb b/spec/workers/background_migration_worker_spec.rb index 4f6e3474634..1c54cf55fa0 100644 --- a/spec/workers/background_migration_worker_spec.rb +++ b/spec/workers/background_migration_worker_spec.rb @@ -10,35 +10,4 @@ describe BackgroundMigrationWorker, :sidekiq do described_class.new.perform('Foo', [10, 20]) end end - - describe '.perform_bulk' do - it 'enqueues background migrations in bulk' do - Sidekiq::Testing.fake! do - described_class.perform_bulk([['Foo', [1]], ['Foo', [2]]]) - - expect(described_class.jobs.count).to eq 2 - expect(described_class.jobs).to all(include('enqueued_at')) - end - end - end - - describe '.perform_bulk_in' do - context 'when delay is valid' do - it 'correctly schedules background migrations' do - Sidekiq::Testing.fake! do - described_class.perform_bulk_in(1.minute, [['Foo', [1]], ['Foo', [2]]]) - - expect(described_class.jobs.count).to eq 2 - expect(described_class.jobs).to all(include('at')) - end - end - end - - context 'when delay is invalid' do - it 'raises an ArgumentError exception' do - expect { described_class.perform_bulk_in(-60, [['Foo']]) } - .to raise_error(ArgumentError) - end - end - end end diff --git a/spec/workers/concerns/application_worker_spec.rb b/spec/workers/concerns/application_worker_spec.rb index fd434f58602..0145563e0ed 100644 --- a/spec/workers/concerns/application_worker_spec.rb +++ b/spec/workers/concerns/application_worker_spec.rb @@ -24,4 +24,35 @@ describe ApplicationWorker do expect(worker.queue).to eq('some_queue') end end + + describe '.bulk_perform_async' do + it 'enqueues jobs in bulk' do + Sidekiq::Testing.fake! do + worker.bulk_perform_async([['Foo', [1]], ['Foo', [2]]]) + + expect(worker.jobs.count).to eq 2 + expect(worker.jobs).to all(include('enqueued_at')) + end + end + end + + describe '.bulk_perform_in' do + context 'when delay is valid' do + it 'correctly schedules jobs' do + Sidekiq::Testing.fake! do + worker.bulk_perform_in(1.minute, [['Foo', [1]], ['Foo', [2]]]) + + expect(worker.jobs.count).to eq 2 + expect(worker.jobs).to all(include('at')) + end + end + end + + context 'when delay is invalid' do + it 'raises an ArgumentError exception' do + expect { worker.bulk_perform_in(-60, [['Foo']]) } + .to raise_error(ArgumentError) + end + end + end end -- cgit v1.2.1 From 92e8ad5247e2e2c8b6488327c9818e2d83f3a881 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 5 Dec 2017 12:03:35 +0100 Subject: Merge index during table creation --- db/migrate/20170918072948_create_job_artifacts.rb | 1 + ...145523_uniqueness_contraint_job_artifact_file_type.rb | 16 ---------------- db/schema.rb | 2 +- 3 files changed, 2 insertions(+), 17 deletions(-) delete mode 100644 db/migrate/20171130145523_uniqueness_contraint_job_artifact_file_type.rb diff --git a/db/migrate/20170918072948_create_job_artifacts.rb b/db/migrate/20170918072948_create_job_artifacts.rb index 304b3fce5be..95f2c6c8ce8 100644 --- a/db/migrate/20170918072948_create_job_artifacts.rb +++ b/db/migrate/20170918072948_create_job_artifacts.rb @@ -17,6 +17,7 @@ class CreateJobArtifacts < ActiveRecord::Migration t.string :file t.foreign_key :ci_builds, column: :job_id, on_delete: :cascade + t.index [:job_id, :file_type], unique: true end end end diff --git a/db/migrate/20171130145523_uniqueness_contraint_job_artifact_file_type.rb b/db/migrate/20171130145523_uniqueness_contraint_job_artifact_file_type.rb deleted file mode 100644 index 7ecbac8a81d..00000000000 --- a/db/migrate/20171130145523_uniqueness_contraint_job_artifact_file_type.rb +++ /dev/null @@ -1,16 +0,0 @@ -class UniquenessContraintJobArtifactFileType < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - # Set this constant to true if this migration requires downtime. - DOWNTIME = false - - disable_ddl_transaction! - - def up - add_concurrent_index :ci_job_artifacts, [:job_id, :file_type], unique: true - end - - def down - remove_concurrent_index :ci_job_artifacts, [:job_id, :file_type] - end -end diff --git a/db/schema.rb b/db/schema.rb index de84d861530..da3894c2076 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20171130145523) do +ActiveRecord::Schema.define(version: 20171124150326) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" -- cgit v1.2.1 From 9711b34491d5cfd6eb2bf379f43dbbcd629a572c Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 5 Dec 2017 12:06:34 +0100 Subject: Move config_source earlier --- db/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index da3894c2076..8e5bcc8b51d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -393,9 +393,9 @@ ActiveRecord::Schema.define(version: 20171124150326) do t.integer "auto_canceled_by_id" t.integer "pipeline_schedule_id" t.integer "source" + t.integer "config_source" t.boolean "protected" t.integer "failure_reason" - t.integer "config_source" end add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree -- cgit v1.2.1 From 1e9eb3ea222d68b84ac698942076c0fa4bf07e3c Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 5 Dec 2017 11:56:03 +0000 Subject: Init zenmode in snippets pages --- app/assets/javascripts/dispatcher.js | 4 ++++ app/views/projects/_md_preview.html.haml | 2 +- changelogs/unreleased/40508-snippets-zen-mode.yml | 5 +++++ spec/features/projects/snippets_spec.rb | 5 +++++ 4 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/40508-snippets-zen-mode.yml diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index a21c92f24d6..6d2907e2164 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -298,18 +298,21 @@ import ProjectVariables from './project_variables'; break; case 'projects:snippets:show': initNotes(); + new ZenMode(); break; case 'projects:snippets:new': case 'projects:snippets:edit': case 'projects:snippets:create': case 'projects:snippets:update': new GLForm($('.snippet-form'), true); + new ZenMode(); break; case 'snippets:new': case 'snippets:edit': case 'snippets:create': case 'snippets:update': new GLForm($('.snippet-form'), false); + new ZenMode(); break; case 'projects:releases:edit': new ZenMode(); @@ -546,6 +549,7 @@ import ProjectVariables from './project_variables'; new LineHighlighter(); new BlobViewer(); initNotes(); + new ZenMode(); break; case 'import:fogbugz:new_user_map': new UsersSelect(); diff --git a/app/views/projects/_md_preview.html.haml b/app/views/projects/_md_preview.html.haml index a9431cc4956..2cd5d0c60ea 100644 --- a/app/views/projects/_md_preview.html.haml +++ b/app/views/projects/_md_preview.html.haml @@ -25,7 +25,7 @@ = markdown_toolbar_button({ icon: "list-bulleted", data: { "md-tag" => "* ", "md-prepend" => true }, title: "Add a bullet list" }) = markdown_toolbar_button({ icon: "list-numbered", data: { "md-tag" => "1. ", "md-prepend" => true }, title: "Add a numbered list" }) = markdown_toolbar_button({ icon: "task-done", data: { "md-tag" => "* [ ] ", "md-prepend" => true }, title: "Add a task list" }) - %button.toolbar-btn.toolbar-fullscreen-btn.js-zen-enter.has-tooltip{ type: "button", tabindex: -1, aria: { label: "Go full screen" }, title: "Go full screen", data: { container: "body" } } + %button.toolbar-btn.toolbar-fullscreen-btn.js-zen-enter.has-tooltip{ type: "button", tabindex: -1, "aria-label": "Go full screen", title: "Go full screen", data: { container: "body" } } = sprite_icon("screen-full") .md-write-holder diff --git a/changelogs/unreleased/40508-snippets-zen-mode.yml b/changelogs/unreleased/40508-snippets-zen-mode.yml new file mode 100644 index 00000000000..c205218575b --- /dev/null +++ b/changelogs/unreleased/40508-snippets-zen-mode.yml @@ -0,0 +1,5 @@ +--- +title: Init zen mode in snippets pages +merge_request: +author: +type: fixed diff --git a/spec/features/projects/snippets_spec.rb b/spec/features/projects/snippets_spec.rb index 1cfbbb4cb62..0fa7ca9afd4 100644 --- a/spec/features/projects/snippets_spec.rb +++ b/spec/features/projects/snippets_spec.rb @@ -39,6 +39,11 @@ describe 'Project snippets', :js do expect(page).to have_selector('.atwho-view') end + + it 'should have zen mode' do + find('.js-zen-enter').click() + expect(page).to have_selector('.fullscreen') + end end end end -- cgit v1.2.1 From 49dd62ada1f88f51f426e97c0ece18ec71e39514 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20=22BKC=22=20Carlb=C3=A4cker?= Date: Tue, 28 Nov 2017 10:23:38 +0100 Subject: Migrate Gitlab::Git::Commit.shas_with_signatures - Added tests for Git::Commit.shas_with_signatures --- lib/gitlab/git/commit.rb | 16 +++++++++++----- lib/gitlab/gitaly_client/commit_service.rb | 20 ++++++++++++++++++++ spec/lib/gitlab/git/commit_spec.rb | 29 +++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index c85dcfa0475..8900e2d7afe 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -213,11 +213,17 @@ module Gitlab end def shas_with_signatures(repository, shas) - shas.select do |sha| - begin - Rugged::Commit.extract_signature(repository.rugged, sha) - rescue Rugged::OdbError - false + GitalyClient.migrate(:filter_shas_with_signatures) do |is_enabled| + if is_enabled + Gitlab::GitalyClient::CommitService.new(repository).filter_shas_with_signatures(shas) + else + shas.select do |sha| + begin + Rugged::Commit.extract_signature(repository.rugged, sha) + rescue Rugged::OdbError + false + end + end end end end diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index 34807d280e5..7985f5b5457 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -250,6 +250,26 @@ module Gitlab consume_commits_response(response) end + def filter_shas_with_signatures(shas) + request = Gitaly::FilterShasWithSignaturesRequest.new(repository: @gitaly_repo) + + enum = Enumerator.new do |y| + shas.each_slice(20) do |revs| + request.shas = GitalyClient.encode_repeated(revs) + + y.yield request + + request = Gitaly::FilterShasWithSignaturesRequest.new + end + end + + response = GitalyClient.call(@repository.storage, :commit_service, :filter_shas_with_signatures, enum) + + response.flat_map do |msg| + msg.shas.map { |sha| EncodingHelper.encode!(sha) } + end + end + private def call_commit_diff(request_params, options = {}) diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb index 9f4e3c49adc..5ed639543e0 100644 --- a/spec/lib/gitlab/git/commit_spec.rb +++ b/spec/lib/gitlab/git/commit_spec.rb @@ -278,6 +278,35 @@ describe Gitlab::Git::Commit, seed_helper: true do it { is_expected.not_to include(SeedRepo::FirstCommit::ID) } end + shared_examples '.shas_with_signatures' do + let(:signed_shas) { %w[5937ac0a7beb003549fc5fd26fc247adbce4a52e 570e7b2abdd848b95f2f578043fc23bd6f6fd24d] } + let(:unsigned_shas) { %w[19e2e9b4ef76b422ce1154af39a91323ccc57434 c642fe9b8b9f28f9225d7ea953fe14e74748d53b] } + let(:first_signed_shas) { %w[5937ac0a7beb003549fc5fd26fc247adbce4a52e c642fe9b8b9f28f9225d7ea953fe14e74748d53b] } + + it 'has 2 signed shas' do + ret = described_class.shas_with_signatures(repository, signed_shas) + expect(ret).to eq(signed_shas) + end + + it 'has 0 signed shas' do + ret = described_class.shas_with_signatures(repository, unsigned_shas) + expect(ret).to eq([]) + end + + it 'has 1 signed sha' do + ret = described_class.shas_with_signatures(repository, first_signed_shas) + expect(ret).to contain_exactly(first_signed_shas.first) + end + end + + describe '.shas_with_signatures with gitaly on' do + it_should_behave_like '.shas_with_signatures' + end + + describe '.shas_with_signatures with gitaly disabled', :disable_gitaly do + it_should_behave_like '.shas_with_signatures' + end + describe '.find_all' do shared_examples 'finding all commits' do it 'should return a return a collection of commits' do -- cgit v1.2.1 From 1efb219561a3be804c30de8e0a74cbfc85b1a4cd Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 30 Nov 2017 23:17:41 +0100 Subject: Perform SQL matching of Build&Runner tags to greatly speed-up job picking --- app/models/ci/build.rb | 19 ++++++ app/models/ci/runner.rb | 6 +- app/services/ci/register_job_service.rb | 10 +++ .../unreleased/perform-sql-matching-of-tags.yml | 5 ++ spec/models/ci/build_spec.rb | 73 ++++++++++++++++++++++ spec/services/ci/register_job_service_spec.rb | 23 +++---- 6 files changed, 120 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/perform-sql-matching-of-tags.yml diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8738e094510..d2402b55184 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -47,6 +47,25 @@ module Ci scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } scope :ref_protected, -> { where(protected: true) } + scope :matches_tag_ids, -> (tag_ids) do + matcher = ::ActsAsTaggableOn::Tagging + .where(taggable_type: CommitStatus) + .where(context: 'tags') + .where('taggable_id = ci_builds.id') + .where.not(tag_id: tag_ids).select('1') + + where("NOT EXISTS (?)", matcher) + end + + scope :with_any_tags, -> do + matcher = ::ActsAsTaggableOn::Tagging + .where(taggable_type: CommitStatus) + .where(context: 'tags') + .where('taggable_id = ci_builds.id').select('1') + + where("EXISTS (?)", matcher) + end + mount_uploader :legacy_artifacts_file, LegacyArtifactUploader, mount_on: :artifacts_file mount_uploader :legacy_artifacts_metadata, LegacyArtifactUploader, mount_on: :artifacts_metadata diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index d39610a8995..dcbb397fb78 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -112,7 +112,7 @@ module Ci def can_pick?(build) return false if self.ref_protected? && !build.protected? - assignable_for?(build.project) && accepting_tags?(build) + assignable_for?(build.project_id) && accepting_tags?(build) end def only_for?(project) @@ -171,8 +171,8 @@ module Ci end end - def assignable_for?(project) - is_shared? || projects.exists?(id: project.id) + def assignable_for?(project_id) + is_shared? || projects.exists?(id: project_id) end def accepting_tags?(build) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index b8db709211a..2ef76e03031 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -22,6 +22,16 @@ module Ci valid = true + if Feature.enabled?('ci_job_request_with_tags_matcher') + # pick builds that does not have other tags than runner's one + builds = builds.matches_tag_ids(runner.tags.ids) + + # pick builds that have at least one tag + unless runner.run_untagged? + builds = builds.with_any_tags + end + end + builds.find do |build| next unless runner.can_pick?(build) diff --git a/changelogs/unreleased/perform-sql-matching-of-tags.yml b/changelogs/unreleased/perform-sql-matching-of-tags.yml new file mode 100644 index 00000000000..39f8a867a4d --- /dev/null +++ b/changelogs/unreleased/perform-sql-matching-of-tags.yml @@ -0,0 +1,5 @@ +--- +title: Perform SQL matching of Build&Runner tags to greatly speed-up job picking +merge_request: +author: +type: performance diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 9070692abfe..26d33663dad 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1921,4 +1921,77 @@ describe Ci::Build do end end end + + describe '.matches_tag_ids' do + set(:build) { create(:ci_build, project: project, user: user) } + let(:tag_ids) { ::ActsAsTaggableOn::Tag.named_any(tag_list).ids } + + subject { described_class.where(id: build).matches_tag_ids(tag_ids) } + + before do + build.update(tag_list: build_tag_list) + end + + context 'when have different tags' do + let(:build_tag_list) { %w(A B) } + let(:tag_list) { %w(C D) } + + it "does not match a build" do + is_expected.not_to contain_exactly(build) + end + end + + context 'when have a subset of tags' do + let(:build_tag_list) { %w(A B) } + let(:tag_list) { %w(A B C D) } + + it "does match a build" do + is_expected.to contain_exactly(build) + end + end + + context 'when build does not have tags' do + let(:build_tag_list) { [] } + let(:tag_list) { %w(C D) } + + it "does match a build" do + is_expected.to contain_exactly(build) + end + end + + context 'when does not have a subset of tags' do + let(:build_tag_list) { %w(A B C) } + let(:tag_list) { %w(C D) } + + it "does not match a build" do + is_expected.not_to contain_exactly(build) + end + end + end + + describe '.matches_tags' do + set(:build) { create(:ci_build, project: project, user: user) } + + subject { described_class.where(id: build).with_any_tags } + + before do + build.update(tag_list: tag_list) + end + + context 'when does have tags' do + let(:tag_list) { %w(A B) } + + it "does match a build" do + is_expected.to contain_exactly(build) + end + end + + context 'when does not have tags' do + let(:tag_list) { [] } + + it "does not match a build" do + is_expected.not_to contain_exactly(build) + end + end + end end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 5ac30111ec9..decdd577226 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -15,16 +15,14 @@ module Ci describe '#execute' do context 'runner follow tag list' do it "picks build with the same tag" do - pending_job.tag_list = ["linux"] - pending_job.save - specific_runner.tag_list = ["linux"] + pending_job.update(tag_list: ["linux"]) + specific_runner.update(tag_list: ["linux"]) expect(execute(specific_runner)).to eq(pending_job) end it "does not pick build with different tag" do - pending_job.tag_list = ["linux"] - pending_job.save - specific_runner.tag_list = ["win32"] + pending_job.update(tag_list: ["linux"]) + specific_runner.update(tag_list: ["win32"]) expect(execute(specific_runner)).to be_falsey end @@ -33,13 +31,12 @@ module Ci end it "does not pick build with tag" do - pending_job.tag_list = ["linux"] - pending_job.save + pending_job.update(tag_list: ["linux"]) expect(execute(specific_runner)).to be_falsey end it "pick build without tag" do - specific_runner.tag_list = ["win32"] + specific_runner.update(tag_list: ["win32"]) expect(execute(specific_runner)).to eq(pending_job) end end @@ -172,7 +169,7 @@ module Ci context 'when first build is stalled' do before do - pending_job.lock_version = 10 + pending_job.update(lock_version: 0) end subject { described_class.new(specific_runner).execute } @@ -182,7 +179,7 @@ module Ci before do allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_specific_runner) - .and_return([pending_job, other_build]) + .and_return(Ci::Build.where(id: [pending_job, other_build])) end it "receives second build from the queue" do @@ -194,7 +191,7 @@ module Ci context 'when single build is in queue' do before do allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_specific_runner) - .and_return([pending_job]) + .and_return(Ci::Build.where(id: pending_job)) end it "does not receive any valid result" do @@ -205,7 +202,7 @@ module Ci context 'when there is no build in queue' do before do allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_specific_runner) - .and_return([]) + .and_return(Ci::Build.none) end it "does not receive builds but result is valid" do -- cgit v1.2.1 From 4d025f2d7e587d067007c5e8d3b594811e91067b Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Tue, 5 Dec 2017 16:40:14 +0100 Subject: Do not run qa:internal for docs only changes --- .gitlab-ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 60a2b5d5b5b..19540e4331e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -586,6 +586,7 @@ codequality: paths: [codeclimate.json] qa:internal: + <<: *except-docs stage: test variables: SETUP_DB: "false" -- cgit v1.2.1 From 02d97d46216b60568fb248f9aeb779528abd8e9c Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Tue, 5 Dec 2017 16:43:47 +0000 Subject: add Gitlab::Database.replication_slots_supported? --- lib/gitlab/database.rb | 4 ++++ spec/lib/gitlab/database_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index cd7b4c043da..96922e1a62f 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -50,6 +50,10 @@ module Gitlab postgresql? && version.to_f >= 9.3 end + def self.replication_slots_supported? + postgresql? && version.to_f >= 9.4 + end + def self.nulls_last_order(field, direction = 'ASC') order = "#{field} #{direction}" diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index fcddfad3f9f..a5657b81952 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -73,6 +73,28 @@ describe Gitlab::Database do end end + describe '.replication_slots_supported?' do + it 'returns false when using MySQL' do + allow(described_class).to receive(:postgresql?).and_return(false) + + expect(described_class.replication_slots_supported?).to eq(false) + end + + it 'returns false when using PostgreSQL 9.3' do + allow(described_class).to receive(:postgresql?).and_return(true) + allow(described_class).to receive(:version).and_return('9.3.1') + + expect(described_class.replication_slots_supported?).to eq(false) + end + + it 'returns true when using PostgreSQL 9.4.0 or newer' do + allow(described_class).to receive(:postgresql?).and_return(true) + allow(described_class).to receive(:version).and_return('9.4.0') + + expect(described_class.replication_slots_supported?).to eq(true) + end + end + describe '.nulls_last_order' do context 'when using PostgreSQL' do before do -- cgit v1.2.1