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" + extend Gitlab::Ci::Model - enum type { - archive: 0, - metadata: 1, - trace: 2 - } + 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