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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3 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.3