diff options
author | Kamil Trzcinski <ayufan@ayufan.eu> | 2017-06-01 10:52:19 +0300 |
---|---|---|
committer | Kamil Trzcinski <ayufan@ayufan.eu> | 2017-06-01 17:34:48 +0300 |
commit | 6185d12c183b539ea06ab3550b2c21045d169ca4 (patch) | |
tree | 9a5a8e50f8b0c6c64d69099d0374dc15ace808ee | |
parent | e61f38d79eb85a7c601bd146d5b8e48a8b4418e5 (diff) |
Add missing specs
-rw-r--r-- | app/controllers/projects/artifacts_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/uploads_controller.rb | 2 | ||||
-rw-r--r-- | app/uploaders/artifact_uploader.rb | 4 | ||||
-rw-r--r-- | app/uploaders/gitlab_uploader.rb | 8 | ||||
-rw-r--r-- | app/uploaders/records_uploads.rb | 4 | ||||
-rw-r--r-- | db/post_migrate/20170523083112_migrate_old_artifacts.rb | 10 | ||||
-rw-r--r-- | lib/api/helpers.rb | 10 | ||||
-rw-r--r-- | lib/api/jobs.rb | 10 | ||||
-rw-r--r-- | lib/api/runner.rb | 11 | ||||
-rw-r--r-- | lib/api/v3/builds.rb | 10 | ||||
-rw-r--r-- | lib/ci/api/builds.rb | 8 | ||||
-rw-r--r-- | spec/migrations/migrate_old_artifacts_spec.rb | 16 | ||||
-rw-r--r-- | spec/uploaders/artifact_uploader_spec.rb | 38 | ||||
-rw-r--r-- | spec/uploaders/gitlab_uploader_spec.rb | 56 |
14 files changed, 132 insertions, 57 deletions
diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb index b400670746f..ea036b1f705 100644 --- a/app/controllers/projects/artifacts_controller.rb +++ b/app/controllers/projects/artifacts_controller.rb @@ -10,7 +10,7 @@ class Projects::ArtifactsController < Projects::ApplicationController before_action :set_path_and_entry, only: [:file, :raw] def download - if artifacts_file.local_file? + if artifacts_file.file_storage? send_file artifacts_file.path, disposition: 'attachment' else redirect_to artifacts_file.url diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 7493bf38bba..eef53730291 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -80,7 +80,7 @@ class UploadsController < ApplicationController else @uploader = @model.send(upload_mount) - redirect_to @uploader.url unless @uploader.local_storage? + redirect_to @uploader.url unless @uploader.file_storage? end @uploader diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb index 668b6b8e048..3bc0408f557 100644 --- a/app/uploaders/artifact_uploader.rb +++ b/app/uploaders/artifact_uploader.rb @@ -7,6 +7,10 @@ class ArtifactUploader < GitlabUploader 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 diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index c1d9007dc71..02afddb8c6a 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -9,15 +9,11 @@ class GitlabUploader < CarrierWave::Uploader::Base delegate :base_dir, to: :class - def local_file? - local_storage? && file&.is_a?(CarrierWave::SanitizedFile) - end - - def local_storage? + def file_storage? storage.is_a?(CarrierWave::Storage::File) end - def local_cache_storage? + def file_cache_storage? cache_storage.is_a?(CarrierWave::Storage::File) end diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index be3a2caa60c..4c127f29250 100644 --- a/app/uploaders/records_uploads.rb +++ b/app/uploaders/records_uploads.rb @@ -16,7 +16,7 @@ module RecordsUploads # # Called `after :store` def record_upload(_tempfile) - return unless local_file? + return unless file_storage? return unless file.exists? Upload.record(self) @@ -26,7 +26,7 @@ module RecordsUploads # # Called `before :remove` def destroy_upload(*args) - return unless local_file? + return unless file_storage? return unless file Upload.remove_path(relative_path) diff --git a/db/post_migrate/20170523083112_migrate_old_artifacts.rb b/db/post_migrate/20170523083112_migrate_old_artifacts.rb index 3067e3d404b..f2690bd0017 100644 --- a/db/post_migrate/20170523083112_migrate_old_artifacts.rb +++ b/db/post_migrate/20170523083112_migrate_old_artifacts.rb @@ -22,7 +22,7 @@ class MigrateOldArtifacts < ActiveRecord::Migration def builds_with_artifacts Build.with_artifacts .joins('JOIN projects ON projects.id = ci_builds.project_id') - .where('ci_builds.id < ?', min_id || 0) + .where('ci_builds.id < ?', min_id) .where('projects.ci_id IS NOT NULL') .select('id', 'created_at', 'project_id', 'projects.ci_id AS ci_id') end @@ -30,18 +30,18 @@ class MigrateOldArtifacts < ActiveRecord::Migration def min_id Build.joins('JOIN projects ON projects.id = ci_builds.project_id') .where('projects.ci_id IS NULL') - .pluck('min(ci_builds.id)') + .pluck('coalesce(min(ci_builds.id), 0)') .first end class Build < ActiveRecord::Base self.table_name = 'ci_builds' - scope :with_artifacts, ->() { where.not(artifacts_file: [nil, '']) } + scope :with_artifacts, -> { where.not(artifacts_file: [nil, '']) } def migrate_artifacts! - return unless File.exists?(source_artifacts_path) - return if File.exists?(target_artifacts_path) + return unless File.exist?(source_artifacts_path) + return if File.exist?(target_artifacts_path) ensure_target_path diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index d61450f8258..81f6fc3201d 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -311,6 +311,16 @@ module API end end + def present_artifacts!(artifacts_file) + return not_found! unless artifacts_file.exists? + + if artifacts_file.file_storage? + present_file!(artifacts_file.path, artifacts_file.filename) + else + redirect_to(artifacts_file.url) + end + end + private def private_token diff --git a/lib/api/jobs.rb b/lib/api/jobs.rb index 7f94b1afdc4..8a67de10bca 100644 --- a/lib/api/jobs.rb +++ b/lib/api/jobs.rb @@ -224,16 +224,6 @@ module API find_build(id) || not_found! end - def present_artifacts!(artifacts_file) - if !artifacts_file.local_file? - redirect_to(build.artifacts_file.url) - elsif artifacts_file.exists? - present_file!(artifacts_file.path, artifacts_file.filename) - else - not_found! - end - end - def filter_builds(builds, scope) return builds if scope.nil? || scope.empty? diff --git a/lib/api/runner.rb b/lib/api/runner.rb index ddcc0429de0..3fd0536dadd 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -241,16 +241,7 @@ module API get '/:id/artifacts' do job = authenticate_job! - artifacts_file = job.artifacts_file - unless artifacts_file.local_file? - return redirect_to job.artifacts_file.url - end - - unless artifacts_file.exists? - not_found! - end - - present_file!(artifacts_file.path, artifacts_file.filename) + present_artifacts!(job.artifacts_file) end end end diff --git a/lib/api/v3/builds.rb b/lib/api/v3/builds.rb index 7ce7b43e3bd..93ad9eb26b8 100644 --- a/lib/api/v3/builds.rb +++ b/lib/api/v3/builds.rb @@ -225,16 +225,6 @@ module API find_build(id) || not_found! end - def present_artifacts!(artifacts_file) - if !artifacts_file.local_file? - redirect_to(build.artifacts_file.url) - elsif artifacts_file.exists? - present_file!(artifacts_file.path, artifacts_file.filename) - else - not_found! - end - end - def filter_builds(builds, scope) return builds if scope.nil? || scope.empty? diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index 0f2044a54b4..2285ef241d7 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -187,14 +187,14 @@ module Ci build = authenticate_build! artifacts_file = build.artifacts_file - unless artifacts_file.local_file? - return redirect_to build.artifacts_file.url - end - unless artifacts_file.exists? not_found! end + unless artifacts_file.file_storage? + return redirect_to build.artifacts_file.url + end + present_file!(artifacts_file.path, artifacts_file.filename) end diff --git a/spec/migrations/migrate_old_artifacts_spec.rb b/spec/migrations/migrate_old_artifacts_spec.rb index 9d951038dcc..50f4bbda001 100644 --- a/spec/migrations/migrate_old_artifacts_spec.rb +++ b/spec/migrations/migrate_old_artifacts_spec.rb @@ -93,17 +93,17 @@ describe MigrateOldArtifacts do FileUtils.mkdir_p(legacy_path(build)) FileUtils.copy( - Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), - File.join(legacy_path(build), "ci_build_artifacts.zip") - ) + Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), + File.join(legacy_path(build), "ci_build_artifacts.zip")) + FileUtils.copy( - Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), - File.join(legacy_path(build), "ci_build_artifacts_metadata.gz") - ) + Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), + File.join(legacy_path(build), "ci_build_artifacts_metadata.gz")) + build.update_columns( artifacts_file: 'ci_build_artifacts.zip', - artifacts_metadata: 'ci_build_artifacts_metadata.gz', - ) + artifacts_metadata: 'ci_build_artifacts_metadata.gz') + build.reload end diff --git a/spec/uploaders/artifact_uploader_spec.rb b/spec/uploaders/artifact_uploader_spec.rb new file mode 100644 index 00000000000..24e2e3a9f0e --- /dev/null +++ b/spec/uploaders/artifact_uploader_spec.rb @@ -0,0 +1,38 @@ +require 'rails_helper' + +describe ArtifactUploader do + let(: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 +end diff --git a/spec/uploaders/gitlab_uploader_spec.rb b/spec/uploaders/gitlab_uploader_spec.rb new file mode 100644 index 00000000000..78e9d9cf46c --- /dev/null +++ b/spec/uploaders/gitlab_uploader_spec.rb @@ -0,0 +1,56 @@ +require 'rails_helper' +require 'carrierwave/storage/fog' + +describe GitlabUploader do + let(:uploader_class) { Class.new(described_class) } + + subject { uploader_class.new } + + describe '#file_storage?' do + context 'when file storage is used' do + before do + uploader_class.storage(:file) + end + + it { is_expected.to be_file_storage } + end + + context 'when is remote storage' do + before do + uploader_class.storage(:fog) + end + + it { is_expected.not_to be_file_storage } + end + end + + describe '#file_cache_storage?' do + context 'when file storage is used' do + before do + uploader_class.cache_storage(:file) + end + + it { is_expected.to be_file_cache_storage } + end + + context 'when is remote storage' do + before do + uploader_class.cache_storage(:fog) + end + + it { is_expected.not_to be_file_cache_storage } + end + end + + describe '#move_to_cache' do + it 'is true' do + expect(subject.move_to_cache).to eq(true) + end + end + + describe '#move_to_store' do + it 'is true' do + expect(subject.move_to_store).to eq(true) + end + end +end |