diff options
61 files changed, 1102 insertions, 318 deletions
diff --git a/app/assets/javascripts/pipelines/components/graph/job_component.vue b/app/assets/javascripts/pipelines/components/graph/job_component.vue index 9b136573135..d501c465a96 100644 --- a/app/assets/javascripts/pipelines/components/graph/job_component.vue +++ b/app/assets/javascripts/pipelines/components/graph/job_component.vue @@ -17,6 +17,7 @@ * "text": "passed", * "label": "passed", * "group": "success", + * "tooltip": "passed", * "details_path": "/root/ci-mock/builds/4256", * "action": { * "icon": "retry", @@ -69,12 +70,12 @@ textBuilder.push(this.job.name); } - if (this.job.name && this.status.label) { + if (this.job.name && this.status.tooltip) { textBuilder.push('-'); } - if (this.status.label) { - textBuilder.push(`${this.job.status.label}`); + if (this.status.tooltip) { + textBuilder.push(`${this.job.status.tooltip}`); } return textBuilder.join(' '); @@ -100,6 +101,7 @@ :title="tooltipText" :class="cssClassJobName" data-container="body" + data-html="true" class="js-pipeline-graph-job-link" > @@ -115,6 +117,7 @@ class="js-job-component-tooltip" :title="tooltipText" :class="cssClassJobName" + data-html="true" data-container="body" > diff --git a/app/assets/stylesheets/pages/builds.scss b/app/assets/stylesheets/pages/builds.scss index 98d460339cd..7a6352e45f1 100644 --- a/app/assets/stylesheets/pages/builds.scss +++ b/app/assets/stylesheets/pages/builds.scss @@ -391,7 +391,7 @@ } &:hover { - background-color: $row-hover; + background-color: $dropdown-item-hover-bg; } .icon-retry { diff --git a/app/controllers/projects/jobs_controller.rb b/app/controllers/projects/jobs_controller.rb index ac9eb2047a0..dd12d30a085 100644 --- a/app/controllers/projects/jobs_controller.rb +++ b/app/controllers/projects/jobs_controller.rb @@ -2,7 +2,6 @@ class Projects::JobsController < Projects::ApplicationController include SendFileUpload before_action :build, except: [:index, :cancel_all] - before_action :authorize_read_build!, only: [:index, :show, :status, :raw, :trace] before_action :authorize_update_build!, @@ -45,8 +44,11 @@ class Projects::JobsController < Projects::ApplicationController end def show - @builds = @project.pipelines.find_by_sha(@build.sha).builds.order('id DESC') - @builds = @builds.where("id not in (?)", @build.id) + @builds = @project.pipelines + .find_by_sha(@build.sha) + .builds + .order('id DESC') + .present(current_user: current_user) @pipeline = @build.pipeline respond_to do |format| diff --git a/app/controllers/projects/lfs_storage_controller.rb b/app/controllers/projects/lfs_storage_controller.rb index 2515e4b9a17..ebde0df1f7b 100644 --- a/app/controllers/projects/lfs_storage_controller.rb +++ b/app/controllers/projects/lfs_storage_controller.rb @@ -31,7 +31,9 @@ class Projects::LfsStorageController < Projects::GitHttpClientController render plain: 'Unprocessable entity', status: 422 end rescue ActiveRecord::RecordInvalid - render_400 + render_lfs_forbidden + rescue UploadedFile::InvalidPathError + render_lfs_forbidden rescue ObjectStorage::RemoteStoreError render_lfs_forbidden end @@ -66,10 +68,11 @@ class Projects::LfsStorageController < Projects::GitHttpClientController end def create_file!(oid, size) - LfsObject.new(oid: oid, size: size).tap do |object| - object.file.store_workhorse_file!(params, :file) - object.save! - end + uploaded_file = UploadedFile.from_params( + params, :file, LfsObjectUploader.workhorse_local_upload_path) + return unless uploaded_file + + LfsObject.create!(oid: oid, size: size, file: uploaded_file) end def link_to_project!(object) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index df57b4f65e3..fbb95fe16df 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -7,6 +7,7 @@ module Ci belongs_to :project belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id + before_save :update_file_store before_save :set_size, if: :file_changed? scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } @@ -21,6 +22,10 @@ module Ci trace: 3 } + def update_file_store + self.file_store = file.object_store + end + def self.artifacts_size_for(project) self.where(project: project).sum(:size) end diff --git a/app/models/concerns/presentable.rb b/app/models/concerns/presentable.rb index 7b33b837004..bc4fbd19a02 100644 --- a/app/models/concerns/presentable.rb +++ b/app/models/concerns/presentable.rb @@ -1,4 +1,12 @@ module Presentable + extend ActiveSupport::Concern + + class_methods do + def present(attributes) + all.map { |klass_object| klass_object.present(attributes) } + end + end + def present(**attributes) Gitlab::View::Presenter::Factory .new(self, attributes) diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index 255475e1fe6..9afebda19be 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -15,6 +15,8 @@ module Ci def status_title if auto_canceled? "Job is redundant and is auto-canceled by Pipeline ##{auto_canceled_by_id}" + else + tooltip_for_badge end end @@ -28,5 +30,19 @@ module Ci trigger_request.user_variables end end + + def tooltip_message + "#{subject.name} - #{detailed_status.status_tooltip}" + end + + private + + def tooltip_for_badge + detailed_status.badge_tooltip.capitalize + end + + def detailed_status + @detailed_status ||= subject.detailed_status(user) + end end end diff --git a/app/serializers/status_entity.rb b/app/serializers/status_entity.rb index a7c2e21e92b..8e8bda2f9df 100644 --- a/app/serializers/status_entity.rb +++ b/app/serializers/status_entity.rb @@ -2,7 +2,7 @@ class StatusEntity < Grape::Entity include RequestAwareEntity expose :icon, :text, :label, :group - + expose :status_tooltip, as: :tooltip expose :has_details?, as: :has_details expose :details_path diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb index ef0f8acefd6..dd86753479d 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -2,6 +2,8 @@ class JobArtifactUploader < GitlabUploader extend Workhorse::UploadPath include ObjectStorage::Concern + ObjectNotReadyError = Class.new(StandardError) + storage_options Gitlab.config.artifacts def size @@ -25,6 +27,8 @@ class JobArtifactUploader < GitlabUploader private def dynamic_segment + raise ObjectNotReadyError, 'JobArtifact is not ready' unless model.id + creation_date = model.created_at.utc.strftime('%Y_%m_%d') File.join(disk_hash[0..1], disk_hash[2..3], disk_hash, diff --git a/app/uploaders/legacy_artifact_uploader.rb b/app/uploaders/legacy_artifact_uploader.rb index b726b053493..efb7893d153 100644 --- a/app/uploaders/legacy_artifact_uploader.rb +++ b/app/uploaders/legacy_artifact_uploader.rb @@ -2,6 +2,8 @@ class LegacyArtifactUploader < GitlabUploader extend Workhorse::UploadPath include ObjectStorage::Concern + ObjectNotReadyError = Class.new(StandardError) + storage_options Gitlab.config.artifacts def store_dir @@ -11,6 +13,8 @@ class LegacyArtifactUploader < GitlabUploader private def dynamic_segment + raise ObjectNotReadyError, 'Build is not ready' unless model.id + File.join(model.created_at.utc.strftime('%Y_%m'), model.project_id.to_s, model.id.to_s) end end diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 3f59ee39299..bd258e04d3f 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -156,11 +156,10 @@ module ObjectStorage end def workhorse_authorize - if options = workhorse_remote_upload_options - { RemoteObject: options } - else - { TempPath: workhorse_local_upload_path } - end + { + RemoteObject: workhorse_remote_upload_options, + TempPath: workhorse_local_upload_path + }.compact end def workhorse_local_upload_path @@ -293,16 +292,14 @@ module ObjectStorage } end - def store_workhorse_file!(params, identifier) - filename = params["#{identifier}.name"] - - if remote_object_id = params["#{identifier}.remote_id"] - store_remote_file!(remote_object_id, filename) - elsif local_path = params["#{identifier}.path"] - store_local_file!(local_path, filename) - else - raise RemoteStoreError, 'Bad file' + def cache!(new_file = sanitized_file) + # We intercept ::UploadedFile which might be stored on remote storage + # We use that for "accelerated" uploads, where we store result on remote storage + if new_file.is_a?(::UploadedFile) && new_file.remote_id + return cache_remote_file!(new_file.remote_id, new_file.original_filename) end + + super end private @@ -313,36 +310,29 @@ module ObjectStorage self.file_storage? end - def store_remote_file!(remote_object_id, filename) - raise RemoteStoreError, 'Missing filename' unless filename - + def cache_remote_file!(remote_object_id, original_filename) file_path = File.join(TMP_UPLOAD_PATH, remote_object_id) file_path = Pathname.new(file_path).cleanpath.to_s raise RemoteStoreError, 'Bad file path' unless file_path.start_with?(TMP_UPLOAD_PATH + '/') - self.object_store = Store::REMOTE - # TODO: # This should be changed to make use of `tmp/cache` mechanism # instead of using custom upload directory, # using tmp/cache makes this implementation way easier than it is today - CarrierWave::Storage::Fog::File.new(self, storage, file_path).tap do |file| + CarrierWave::Storage::Fog::File.new(self, storage_for(Store::REMOTE), file_path).tap do |file| raise RemoteStoreError, 'Missing file' unless file.exists? - self.filename = filename - self.file = storage.store!(file) - end - end - - def store_local_file!(local_path, filename) - raise RemoteStoreError, 'Missing filename' unless filename - - root_path = File.realpath(self.class.workhorse_local_upload_path) - file_path = File.realpath(local_path) - raise RemoteStoreError, 'Bad file path' unless file_path.start_with?(root_path) + # Remote stored file, we force to store on remote storage + self.object_store = Store::REMOTE - self.object_store = Store::LOCAL - self.store!(UploadedFile.new(file_path, filename)) + # TODO: + # We store file internally and force it to be considered as `cached` + # This makes CarrierWave to store file in permament location (copy/delete) + # once this object is saved, but not sooner + @cache_id = "force-to-use-cache" # rubocop:disable Gitlab/ModuleWithInstanceVariables + @file = file # rubocop:disable Gitlab/ModuleWithInstanceVariables + @filename = original_filename # rubocop:disable Gitlab/ModuleWithInstanceVariables + end end # this is a hack around CarrierWave. The #migrate method needs to be diff --git a/app/views/ci/status/_badge.html.haml b/app/views/ci/status/_badge.html.haml index 35a3563dff1..5114387984b 100644 --- a/app/views/ci/status/_badge.html.haml +++ b/app/views/ci/status/_badge.html.haml @@ -4,10 +4,10 @@ - css_classes = "ci-status ci-#{status.group} #{'has-tooltip' if title.present?}" - if link && status.has_details? - = link_to status.details_path, class: css_classes, title: title do + = link_to status.details_path, class: css_classes, title: title, data: { html: title.present? } do = sprite_icon(status.icon) = status.text - else - %span{ class: css_classes, title: title } + %span{ class: css_classes, title: title, data: { html: title.present? } } = sprite_icon(status.icon) = status.text diff --git a/app/views/ci/status/_dropdown_graph_badge.html.haml b/app/views/ci/status/_dropdown_graph_badge.html.haml index c5b4439e273..db2040110fa 100644 --- a/app/views/ci/status/_dropdown_graph_badge.html.haml +++ b/app/views/ci/status/_dropdown_graph_badge.html.haml @@ -3,14 +3,15 @@ - subject = local_assigns.fetch(:subject) - status = subject.detailed_status(current_user) - klass = "ci-status-icon ci-status-icon-#{status.group}" -- tooltip = "#{subject.name} - #{status.label}" +- tooltip = "#{subject.name} - #{status.status_tooltip}" - if status.has_details? - = link_to status.details_path, class: 'mini-pipeline-graph-dropdown-item', data: { toggle: 'tooltip', title: tooltip, container: 'body' } do + = link_to status.details_path, class: 'mini-pipeline-graph-dropdown-item', data: { toggle: 'tooltip', title: tooltip, html: true, container: 'body' } do %span{ class: klass }= sprite_icon(status.icon) %span.ci-build-text= subject.name + - else - .menu-item.mini-pipeline-graph-dropdown-item{ data: { toggle: 'tooltip', title: tooltip, container: 'body' } } + .menu-item.mini-pipeline-graph-dropdown-item{ data: { toggle: 'tooltip', html: true, title: tooltip, container: 'body' } } %span{ class: klass }= sprite_icon(status.icon) %span.ci-build-text= subject.name diff --git a/app/views/projects/jobs/_sidebar.html.haml b/app/views/projects/jobs/_sidebar.html.haml index ecf186e3dc8..0b57ebedebd 100644 --- a/app/views/projects/jobs/_sidebar.html.haml +++ b/app/views/projects/jobs/_sidebar.html.haml @@ -1,5 +1,3 @@ -- builds = @build.pipeline.builds.to_a - %aside.right-sidebar.right-sidebar-expanded.build-sidebar.js-build-sidebar.js-right-sidebar{ data: { "offset-top" => "101", "spy" => "affix" } } .sidebar-container .blocks-container @@ -91,7 +89,8 @@ - HasStatus::ORDERED_STATUSES.each do |build_status| - builds.select{|build| build.status == build_status}.each do |build| .build-job{ class: sidebar_build_class(build, @build), data: { stage: build.stage } } - = link_to project_job_path(@project, build) do + - tooltip = build.tooltip_message + = link_to(project_job_path(@project, build), data: { toggle: 'tooltip', html: true, title: tooltip, container: 'body' }) do = sprite_icon('arrow-right', size:16, css_class: 'icon-arrow-right') %span{ class: "ci-status-icon-#{build.status}" } = ci_icon_for_status(build.status) @@ -101,5 +100,4 @@ - else = build.id - if build.retried? - %span.has-tooltip{ data: { container: 'body', placement: 'bottom' }, title: 'Job was retried' } - = sprite_icon('retry', size:16, css_class: 'icon-retry') + = sprite_icon('retry', size:16, css_class: 'icon-retry') diff --git a/app/views/projects/jobs/show.html.haml b/app/views/projects/jobs/show.html.haml index fa27ded7cc2..dece4dfe167 100644 --- a/app/views/projects/jobs/show.html.haml +++ b/app/views/projects/jobs/show.html.haml @@ -107,7 +107,7 @@ illustration_size: 'svg-430', title: _('This job has not started yet'), content: _('This job is in pending state and is waiting to be picked by a runner') - = render "sidebar" + = render "sidebar", builds: @builds .js-build-options{ data: javascript_build_options } diff --git a/changelogs/unreleased/44269-show-failure-reason-on-upgrade-tooltip-of-jobs.yml b/changelogs/unreleased/44269-show-failure-reason-on-upgrade-tooltip-of-jobs.yml new file mode 100644 index 00000000000..b3ae8ca7340 --- /dev/null +++ b/changelogs/unreleased/44269-show-failure-reason-on-upgrade-tooltip-of-jobs.yml @@ -0,0 +1,5 @@ +--- +title: Display error message on job's tooltip if this one fails +merge_request: 17782 +author: +type: added diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 7bb863e2278..acf7754abe6 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -308,6 +308,7 @@ Settings.artifacts['max_size'] ||= 100 # in megabytes Settings.artifacts['object_store'] ||= Settingslogic.new({}) Settings.artifacts['object_store']['enabled'] = false if Settings.artifacts['object_store']['enabled'].nil? Settings.artifacts['object_store']['remote_directory'] ||= nil +Settings.artifacts['object_store']['direct_upload'] = false if Settings.artifacts['object_store']['direct_upload'].nil? Settings.artifacts['object_store']['background_upload'] = true if Settings.artifacts['object_store']['background_upload'].nil? Settings.artifacts['object_store']['proxy_download'] = false if Settings.artifacts['object_store']['proxy_download'].nil? # Convert upload connection settings to use string keys, to make Fog happy diff --git a/config/initializers/artifacts_direct_upload_support.rb b/config/initializers/artifacts_direct_upload_support.rb new file mode 100644 index 00000000000..d2bc35ea613 --- /dev/null +++ b/config/initializers/artifacts_direct_upload_support.rb @@ -0,0 +1,7 @@ +artifacts_object_store = Gitlab.config.artifacts.object_store + +if artifacts_object_store.enabled && + artifacts_object_store.direct_upload && + artifacts_object_store.connection&.provider.to_s != 'Google' + raise "Only 'Google' is supported as a object storage provider when 'direct_upload' of artifacts is used" +end diff --git a/doc/administration/job_artifacts.md b/doc/administration/job_artifacts.md index ac3a12930c3..896cb93e5ed 100644 --- a/doc/administration/job_artifacts.md +++ b/doc/administration/job_artifacts.md @@ -108,6 +108,7 @@ For source installations the following settings are nested under `artifacts:` an |---------|-------------|---------| | `enabled` | Enable/disable object storage | `false` | | `remote_directory` | The bucket name where Artfacts will be stored| | +| `direct_upload` | Set to true to enable direct upload of Artifacts without the need of local shared storage. Option may be removed once we decide to support only single storage for all files. Currently only `Google` provider is supported | `false` | | `background_upload` | Set to false to disable automatic upload. Option may be removed once upload is direct to S3 | `true` | | `proxy_download` | Set to true to enable proxying all files served. Option allows to reduce egress traffic as this allows clients to download directly from remote storage instead of proxying all data | `false` | | `connection` | Various connection options described below | | diff --git a/lib/api/api.rb b/lib/api/api.rb index 62ffebeacb0..073471b4c4d 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -78,6 +78,14 @@ module API rack_response({ 'message' => '404 Not found' }.to_json, 404) end + rescue_from UploadedFile::InvalidPathError do |e| + rack_response({ 'message' => e.message }.to_json, 400) + end + + rescue_from ObjectStorage::RemoteStoreError do |e| + rack_response({ 'message' => e.message }.to_json, 500) + end + # Retain 405 error rather than a 500 error for Grape 0.15.0+. # https://github.com/ruby-grape/grape/blob/a3a28f5b5dfbb2797442e006dbffd750b27f2a76/UPGRADING.md#changes-to-method-not-allowed-routes rescue_from Grape::Exceptions::MethodNotAllowed do |e| diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 61c138a7dec..a582aa0ec2c 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -389,28 +389,6 @@ module API # file helpers - def uploaded_file(field, uploads_path) - if params[field] - bad_request!("#{field} is not a file") unless params[field][:filename] - return params[field] - end - - return nil unless params["#{field}.path"] && params["#{field}.name"] - - # sanitize file paths - # this requires all paths to exist - required_attributes! %W(#{field}.path) - uploads_path = File.realpath(uploads_path) - file_path = File.realpath(params["#{field}.path"]) - bad_request!('Bad file path') unless file_path.start_with?(uploads_path) - - UploadedFile.new( - file_path, - params["#{field}.name"], - params["#{field}.type"] || 'application/octet-stream' - ) - end - def present_disk_file!(path, filename, content_type = 'application/octet-stream') filename ||= File.basename(path) header['Content-Disposition'] = "attachment; filename=#{filename}" diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 834253d8e94..60aeb69e10a 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -186,7 +186,7 @@ module API status 200 content_type Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE - Gitlab::Workhorse.artifact_upload_ok + JobArtifactUploader.workhorse_authorize end desc 'Upload artifacts for job' do @@ -201,14 +201,15 @@ module API requires :id, type: Integer, desc: %q(Job's ID) optional :token, type: String, desc: %q(Job's authentication token) optional :expire_in, type: String, desc: %q(Specify when artifacts should expire) - optional :file, type: File, desc: %q(Artifact's file) optional 'file.path', type: String, desc: %q(path to locally stored body (generated by Workhorse)) optional 'file.name', type: String, desc: %q(real filename as send in Content-Disposition (generated by Workhorse)) optional 'file.type', type: String, desc: %q(real content type as send in Content-Type (generated by Workhorse)) - optional 'file.sha256', type: String, desc: %q(sha256 checksum of the file) + optional 'file.size', type: Integer, desc: %q(real size of file (generated by Workhorse)) + optional 'file.sha256', type: String, desc: %q(sha256 checksum of the file (generated by Workhorse)) optional 'metadata.path', type: String, desc: %q(path to locally stored body (generated by Workhorse)) optional 'metadata.name', type: String, desc: %q(filename (generated by Workhorse)) - optional 'metadata.sha256', type: String, desc: %q(sha256 checksum of the file) + optional 'metadata.size', type: Integer, desc: %q(real size of metadata (generated by Workhorse)) + optional 'metadata.sha256', type: String, desc: %q(sha256 checksum of metadata (generated by Workhorse)) end post '/:id/artifacts' do not_allowed! unless Gitlab.config.artifacts.enabled @@ -217,21 +218,34 @@ module API job = authenticate_job! forbidden!('Job is not running!') unless job.running? - workhorse_upload_path = JobArtifactUploader.workhorse_upload_path - artifacts = uploaded_file(:file, workhorse_upload_path) - metadata = uploaded_file(:metadata, workhorse_upload_path) + artifacts = UploadedFile.from_params(params, :file, JobArtifactUploader.workhorse_local_upload_path) + metadata = UploadedFile.from_params(params, :metadata, JobArtifactUploader.workhorse_local_upload_path) bad_request!('Missing artifacts file!') unless artifacts file_to_large! unless artifacts.size < max_artifacts_size + bad_request!("Already uploaded") if job.job_artifacts_archive + 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, file_sha256: params['file.sha256'], expire_in: expire_in) - job.build_job_artifacts_metadata(project: job.project, file_type: :metadata, file: metadata, file_sha256: params['metadata.sha256'], expire_in: expire_in) if metadata - job.artifacts_expire_in = expire_in + job.build_job_artifacts_archive( + project: job.project, + file: artifacts, + file_type: :archive, + file_sha256: artifacts.sha256, + expire_in: expire_in) + + if metadata + job.build_job_artifacts_metadata( + project: job.project, + file: metadata, + file_type: :metadata, + file_sha256: metadata.sha256, + expire_in: expire_in) + end - if job.save + if job.update(artifacts_expire_in: expire_in) present job, with: Entities::JobRequest::Response else render_validation_error!(job) diff --git a/lib/gitlab/ci/status/build/factory.rb b/lib/gitlab/ci/status/build/factory.rb index c852d607373..20a319caf86 100644 --- a/lib/gitlab/ci/status/build/factory.rb +++ b/lib/gitlab/ci/status/build/factory.rb @@ -6,10 +6,12 @@ module Gitlab def self.extended_statuses [[Status::Build::Cancelable, Status::Build::Retryable], + [Status::Build::Failed], [Status::Build::FailedAllowed, Status::Build::Play, Status::Build::Stop], - [Status::Build::Action]] + [Status::Build::Action], + [Status::Build::Retried]] end def self.common_helpers diff --git a/lib/gitlab/ci/status/build/failed.rb b/lib/gitlab/ci/status/build/failed.rb new file mode 100644 index 00000000000..155f4fc1343 --- /dev/null +++ b/lib/gitlab/ci/status/build/failed.rb @@ -0,0 +1,40 @@ +module Gitlab + module Ci + module Status + module Build + class Failed < Status::Extended + REASONS = { + 'unknown_failure' => 'unknown failure', + 'script_failure' => 'script failure', + 'api_failure' => 'API failure', + 'stuck_or_timeout_failure' => 'stuck or timeout failure', + 'runner_system_failure' => 'runner system failure', + 'missing_dependency_failure' => 'missing dependency failure' + }.freeze + + def status_tooltip + base_message + end + + def badge_tooltip + base_message + end + + def self.matches?(build, user) + build.failed? + end + + private + + def base_message + "#{s_('CiStatusLabel|failed')} #{description}" + end + + def description + "<br> (#{REASONS[subject.failure_reason]})" + end + end + end + end + end +end diff --git a/lib/gitlab/ci/status/build/failed_allowed.rb b/lib/gitlab/ci/status/build/failed_allowed.rb index dc90f398c7e..ca0046fb1f7 100644 --- a/lib/gitlab/ci/status/build/failed_allowed.rb +++ b/lib/gitlab/ci/status/build/failed_allowed.rb @@ -4,7 +4,7 @@ module Gitlab module Build class FailedAllowed < Status::Extended def label - 'failed (allowed to fail)' + "failed #{allowed_to_fail_title}" end def icon @@ -15,9 +15,19 @@ module Gitlab 'failed_with_warnings' end + def status_tooltip + "#{@status.status_tooltip} #{allowed_to_fail_title}" + end + def self.matches?(build, user) build.failed? && build.allow_failure? end + + private + + def allowed_to_fail_title + "(allowed to fail)" + end end end end diff --git a/lib/gitlab/ci/status/build/retried.rb b/lib/gitlab/ci/status/build/retried.rb new file mode 100644 index 00000000000..6e190e4ee3c --- /dev/null +++ b/lib/gitlab/ci/status/build/retried.rb @@ -0,0 +1,17 @@ +module Gitlab + module Ci + module Status + module Build + class Retried < Status::Extended + def status_tooltip + @status.status_tooltip + " (retried)" + end + + def self.matches?(build, user) + build.retried? + end + end + end + end + end +end diff --git a/lib/gitlab/ci/status/core.rb b/lib/gitlab/ci/status/core.rb index d4fd83b93f8..daab6bb2de5 100644 --- a/lib/gitlab/ci/status/core.rb +++ b/lib/gitlab/ci/status/core.rb @@ -57,6 +57,16 @@ module Gitlab def action_title raise NotImplementedError end + + # Hint that appears on all the pipeline graph tooltips and builds on the right sidebar in Job detail view + def status_tooltip + label + end + + # Hint that appears on the build badges + def badge_tooltip + subject.status + end end end end diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index 6a0a1b65fe9..cd840bd5b01 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -125,6 +125,8 @@ excluded_attributes: - :trace - :token - :when + - :artifacts_file + - :artifacts_metadata push_event_payload: - :event_id project_badges: diff --git a/lib/gitlab/middleware/multipart.rb b/lib/gitlab/middleware/multipart.rb index d4c54049b74..a5f5d719cc1 100644 --- a/lib/gitlab/middleware/multipart.rb +++ b/lib/gitlab/middleware/multipart.rb @@ -82,7 +82,7 @@ module Gitlab end def open_file(path, name) - ::UploadedFile.new(path, name || File.basename(path), 'application/octet-stream') + ::UploadedFile.new(path, filename: name || File.basename(path), content_type: 'application/octet-stream') end end diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index b102812ec12..2faeaf16d55 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -36,10 +36,6 @@ module Gitlab } end - def artifact_upload_ok - { TempPath: JobArtifactUploader.workhorse_upload_path } - end - def send_git_blob(repository, blob) params = if Gitlab::GitalyClient.feature_enabled?(:workhorse_raw_show, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) { diff --git a/lib/uploaded_file.rb b/lib/uploaded_file.rb index 4a3c40f88eb..5dc85b2baea 100644 --- a/lib/uploaded_file.rb +++ b/lib/uploaded_file.rb @@ -1,8 +1,10 @@ require "tempfile" +require "tmpdir" require "fileutils" -# Taken from: Rack::Test::UploadedFile class UploadedFile + InvalidPathError = Class.new(StandardError) + # The filename, *not* including the path, of the "uploaded" file attr_reader :original_filename @@ -12,14 +14,46 @@ class UploadedFile # The content type of the "uploaded" file attr_accessor :content_type - def initialize(path, filename, content_type = "text/plain") - raise "#{path} file does not exist" unless ::File.exist?(path) + attr_reader :remote_id + attr_reader :sha256 + + def initialize(path, filename: nil, content_type: "application/octet-stream", sha256: nil, remote_id: nil) + raise InvalidPathError, "#{path} file does not exist" unless ::File.exist?(path) @content_type = content_type @original_filename = filename || ::File.basename(path) + @content_type = content_type + @sha256 = sha256 + @remote_id = remote_id @tempfile = File.new(path, 'rb') end + def self.from_params(params, field, upload_path) + unless params["#{field}.path"] + raise InvalidPathError, "file is invalid" if params["#{field}.remote_id"] + + return + end + + file_path = File.realpath(params["#{field}.path"]) + + unless self.allowed_path?(file_path, [upload_path, Dir.tmpdir].compact) + raise InvalidPathError, "insecure path used '#{file_path}'" + end + + UploadedFile.new(file_path, + filename: params["#{field}.name"], + content_type: params["#{field}.type"] || 'application/octet-stream', + sha256: params["#{field}.sha256"], + remote_id: params["#{field}.remote_id"]) + end + + def self.allowed_path?(file_path, paths) + paths.any? do |path| + File.exist?(path) && file_path.start_with?(File.realpath(path)) + end + end + def path @tempfile.path end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 30235a3a876..fdacbe6c3f1 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -238,5 +238,10 @@ FactoryBot.define do trait :protected do protected true end + + trait :script_failure do + failed + failure_reason 1 + end end end diff --git a/spec/features/projects/jobs/user_browses_job_spec.rb b/spec/features/projects/jobs/user_browses_job_spec.rb index 4c49cff30d4..b7eee39052a 100644 --- a/spec/features/projects/jobs/user_browses_job_spec.rb +++ b/spec/features/projects/jobs/user_browses_job_spec.rb @@ -34,4 +34,26 @@ describe 'User browses a job', :js do expect(build.project.running_or_pending_build_count).to eq(build.project.builds.running_or_pending.count(:all)) end + + context 'with a failed job' do + let!(:build) { create(:ci_build, :failed, pipeline: pipeline) } + + it 'displays the failure reason' do + within('.builds-container') do + build_link = first('.build-job > a') + expect(build_link['data-title']).to eq('test - failed <br> (unknown failure)') + end + end + end + + context 'when a failed job has been retried' do + let!(:build) { create(:ci_build, :failed, :retried, pipeline: pipeline) } + + it 'displays the failure reason and retried label' do + within('.builds-container') do + build_link = first('.build-job > a') + expect(build_link['data-title']).to eq('test - failed <br> (unknown failure) (retried)') + end + end + end end diff --git a/spec/features/projects/jobs/user_browses_jobs_spec.rb b/spec/features/projects/jobs/user_browses_jobs_spec.rb index 767777f3bf9..36ebbeadd4a 100644 --- a/spec/features/projects/jobs/user_browses_jobs_spec.rb +++ b/spec/features/projects/jobs/user_browses_jobs_spec.rb @@ -29,4 +29,15 @@ describe 'User browses jobs' do expect(ci_lint_tool_link[:href]).to end_with(ci_lint_path) end end + + context 'with a failed job' do + let!(:build) { create(:ci_build, :coverage, :failed, pipeline: pipeline) } + + it 'displays a tooltip with the failure reason' do + page.within('.ci-table') do + failed_job_link = page.find('.ci-failed') + expect(failed_job_link[:title]).to eq('Failed <br> (unknown failure)') + end + end + end end diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index 266ef693d0b..990e5c4d9df 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -115,6 +115,13 @@ describe 'Pipeline', :js do expect(page).not_to have_content('Retry job') end + + it 'should include the failure reason' do + page.within('#ci-badge-test') do + build_link = page.find('.js-pipeline-graph-job-link') + expect(build_link['data-original-title']).to eq('test - failed <br> (unknown failure)') + end + end end context 'when pipeline has manual jobs' do @@ -289,6 +296,15 @@ describe 'Pipeline', :js do it { expect(build_manual.reload).to be_pending } end + + context 'failed jobs' do + it 'displays a tooltip with the failure reason' do + page.within('.ci-table') do + failed_job_link = page.find('.ci-failed') + expect(failed_job_link[:title]).to eq('Failed <br> (unknown failure)') + end + end + end end describe 'GET /:project/pipelines/:id/failures' do diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb index 0e81c6c629a..6e63e0f0b49 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -394,6 +394,23 @@ describe 'Pipelines', :js do expect(build.reload).to be_canceled end end + + context 'for a failed pipeline' do + let!(:build) do + create(:ci_build, :failed, pipeline: pipeline, + stage: 'build', + name: 'build') + end + + it 'should display the failure reason' do + find('.js-builds-dropdown-button').click + + within('.js-builds-dropdown-list') do + build_element = page.find('.mini-pipeline-graph-dropdown-item') + expect(build_element['data-title']).to eq('build - failed <br> (unknown failure)') + end + end + end end context 'with pagination' do diff --git a/spec/initializers/artifacts_direct_upload_support_spec.rb b/spec/initializers/artifacts_direct_upload_support_spec.rb new file mode 100644 index 00000000000..bfb71da3388 --- /dev/null +++ b/spec/initializers/artifacts_direct_upload_support_spec.rb @@ -0,0 +1,71 @@ +require 'spec_helper' + +describe 'Artifacts direct upload support' do + subject do + load Rails.root.join('config/initializers/artifacts_direct_upload_support.rb') + end + + let(:connection) do + { provider: provider } + end + + before do + stub_artifacts_setting( + object_store: { + enabled: enabled, + direct_upload: direct_upload, + connection: connection + }) + end + + context 'when object storage is enabled' do + let(:enabled) { true } + + context 'when direct upload is enabled' do + let(:direct_upload) { true } + + context 'when provider is Google' do + let(:provider) { 'Google' } + + it 'succeeds' do + expect { subject }.not_to raise_error + end + end + + context 'when connection is empty' do + let(:connection) { nil } + + it 'raises an error' do + expect { subject }.to raise_error /object storage provider when 'direct_upload' of artifacts is used/ + end + end + + context 'when other provider is used' do + let(:provider) { 'AWS' } + + it 'raises an error' do + expect { subject }.to raise_error /object storage provider when 'direct_upload' of artifacts is used/ + end + end + end + + context 'when direct upload is disabled' do + let(:direct_upload) { false } + let(:provider) { 'AWS' } + + it 'succeeds' do + expect { subject }.not_to raise_error + end + end + end + + context 'when object storage is disabled' do + let(:enabled) { false } + let(:direct_upload) { false } + let(:provider) { 'AWS' } + + it 'succeeds' do + expect { subject }.not_to raise_error + end + end +end diff --git a/spec/javascripts/pipelines/graph/job_component_spec.js b/spec/javascripts/pipelines/graph/job_component_spec.js index ce181a1e515..c9677ae209a 100644 --- a/spec/javascripts/pipelines/graph/job_component_spec.js +++ b/spec/javascripts/pipelines/graph/job_component_spec.js @@ -13,6 +13,7 @@ describe('pipeline graph job component', () => { icon: 'icon_status_success', text: 'passed', label: 'passed', + tooltip: 'passed', group: 'success', details_path: '/root/ci-mock/builds/4256', has_details: true, @@ -137,6 +138,7 @@ describe('pipeline graph job component', () => { status: { icon: 'icon_status_success', label: 'success', + tooltip: 'success', }, }, }); diff --git a/spec/lib/gitlab/ci/status/build/action_spec.rb b/spec/lib/gitlab/ci/status/build/action_spec.rb index d612d29e3e0..bdec582b57b 100644 --- a/spec/lib/gitlab/ci/status/build/action_spec.rb +++ b/spec/lib/gitlab/ci/status/build/action_spec.rb @@ -53,4 +53,14 @@ describe Gitlab::Ci::Status::Build::Action do end end end + + describe '#badge_tooltip' do + let(:user) { create(:user) } + let(:build) { create(:ci_build, :non_playable) } + let(:status) { Gitlab::Ci::Status::Core.new(build, user) } + + it 'returns the status' do + expect(subject.badge_tooltip).to eq('created') + end + end end diff --git a/spec/lib/gitlab/ci/status/build/cancelable_spec.rb b/spec/lib/gitlab/ci/status/build/cancelable_spec.rb index 9cdebaa5cf2..3ef0b6817e9 100644 --- a/spec/lib/gitlab/ci/status/build/cancelable_spec.rb +++ b/spec/lib/gitlab/ci/status/build/cancelable_spec.rb @@ -40,6 +40,24 @@ describe Gitlab::Ci::Status::Build::Cancelable do end end + describe '#status_tooltip' do + it 'does not override status status_tooltip' do + expect(status).to receive(:status_tooltip) + + subject.status_tooltip + end + end + + describe '#badge_tooltip' do + let(:user) { create(:user) } + let(:build) { create(:ci_build) } + let(:status) { Gitlab::Ci::Status::Core.new(build, user) } + + it 'returns the status' do + expect(subject.badge_tooltip).to eq('pending') + end + end + describe 'action details' do let(:user) { create(:user) } let(:build) { create(:ci_build) } diff --git a/spec/lib/gitlab/ci/status/build/factory_spec.rb b/spec/lib/gitlab/ci/status/build/factory_spec.rb index d196bc6a4c2..bbfa60169a1 100644 --- a/spec/lib/gitlab/ci/status/build/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/build/factory_spec.rb @@ -48,11 +48,11 @@ describe Gitlab::Ci::Status::Build::Factory do it 'matches correct extended statuses' do expect(factory.extended_statuses) - .to eq [Gitlab::Ci::Status::Build::Retryable] + .to eq [Gitlab::Ci::Status::Build::Retryable, Gitlab::Ci::Status::Build::Failed] end - it 'fabricates a retryable build status' do - expect(status).to be_a Gitlab::Ci::Status::Build::Retryable + it 'fabricates a failed build status' do + expect(status).to be_a Gitlab::Ci::Status::Build::Failed end it 'fabricates status with correct details' do @@ -60,6 +60,7 @@ describe Gitlab::Ci::Status::Build::Factory do expect(status.icon).to eq 'status_failed' expect(status.favicon).to eq 'favicon_status_failed' expect(status.label).to eq 'failed' + expect(status.status_tooltip).to eq 'failed <br> (unknown failure)' expect(status).to have_details expect(status).to have_action end @@ -75,6 +76,7 @@ describe Gitlab::Ci::Status::Build::Factory do it 'matches correct extended statuses' do expect(factory.extended_statuses) .to eq [Gitlab::Ci::Status::Build::Retryable, + Gitlab::Ci::Status::Build::Failed, Gitlab::Ci::Status::Build::FailedAllowed] end diff --git a/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb b/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb index 99a5a7e4aca..bfaa508785e 100644 --- a/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb +++ b/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe Gitlab::Ci::Status::Build::FailedAllowed do let(:status) { double('core status') } let(:user) { double('user') } + let(:build) { create(:ci_build, :failed, :allowed_to_fail) } subject do described_class.new(status) @@ -68,6 +69,28 @@ describe Gitlab::Ci::Status::Build::FailedAllowed do end end + describe '#badge_tooltip' do + let(:user) { create(:user) } + let(:failed_status) { Gitlab::Ci::Status::Failed.new(build, user) } + let(:build_status) { Gitlab::Ci::Status::Build::Failed.new(failed_status) } + let(:status) { described_class.new(build_status) } + + it 'does override badge_tooltip' do + expect(status.badge_tooltip).to eq('failed <br> (unknown failure)') + end + end + + describe '#status_tooltip' do + let(:user) { create(:user) } + let(:failed_status) { Gitlab::Ci::Status::Failed.new(build, user) } + let(:build_status) { Gitlab::Ci::Status::Build::Failed.new(failed_status) } + let(:status) { described_class.new(build_status) } + + it 'does override status_tooltip' do + expect(status.status_tooltip).to eq 'failed <br> (unknown failure) (allowed to fail)' + end + end + describe '.matches?' do subject { described_class.matches?(build, user) } diff --git a/spec/lib/gitlab/ci/status/build/failed_spec.rb b/spec/lib/gitlab/ci/status/build/failed_spec.rb new file mode 100644 index 00000000000..cadb424ea2c --- /dev/null +++ b/spec/lib/gitlab/ci/status/build/failed_spec.rb @@ -0,0 +1,83 @@ +require 'spec_helper' + +describe Gitlab::Ci::Status::Build::Failed do + let(:build) { create(:ci_build, :script_failure) } + let(:status) { double('core status') } + let(:user) { double('user') } + + subject { described_class.new(status) } + + describe '#text' do + it 'does not override status text' do + expect(status).to receive(:text) + + subject.text + end + end + + describe '#icon' do + it 'does not override status icon' do + expect(status).to receive(:icon) + + subject.icon + end + end + + describe '#group' do + it 'does not override status group' do + expect(status).to receive(:group) + + subject.group + end + end + + describe '#favicon' do + it 'does not override status label' do + expect(status).to receive(:favicon) + + subject.favicon + end + end + + describe '#label' do + it 'does not override label' do + expect(status).to receive(:label) + + subject.label + end + end + + describe '#badge_tooltip' do + let(:user) { create(:user) } + let(:status) { Gitlab::Ci::Status::Failed.new(build, user) } + + it 'does override badge_tooltip' do + expect(subject.badge_tooltip).to eq 'failed <br> (script failure)' + end + end + + describe '#status_tooltip' do + let(:user) { create(:user) } + let(:status) { Gitlab::Ci::Status::Failed.new(build, user) } + + it 'does override status_tooltip' do + expect(subject.status_tooltip).to eq 'failed <br> (script failure)' + end + end + + describe '.matches?' do + context 'with a failed build' do + it 'returns true' do + expect(described_class.matches?(build, user)).to be_truthy + end + end + + context 'with any other type of build' do + let(:build) { create(:ci_build, :success) } + + it 'returns false' do + expect(described_class.matches?(build, user)).to be_falsy + end + end + end +end diff --git a/spec/lib/gitlab/ci/status/build/play_spec.rb b/spec/lib/gitlab/ci/status/build/play_spec.rb index 81d5f553fd1..35e47cd2526 100644 --- a/spec/lib/gitlab/ci/status/build/play_spec.rb +++ b/spec/lib/gitlab/ci/status/build/play_spec.rb @@ -14,6 +14,22 @@ describe Gitlab::Ci::Status::Build::Play do end end + describe '#status_tooltip' do + it 'does not override status status_tooltip' do + expect(status).to receive(:status_tooltip) + + subject.status_tooltip + end + end + + describe '#badge_tooltip' do + it 'does not override status badge_tooltip' do + expect(status).to receive(:badge_tooltip) + + subject.badge_tooltip + end + end + describe '#has_action?' do context 'when user is allowed to update build' do context 'when user is allowed to trigger protected action' do diff --git a/spec/lib/gitlab/ci/status/build/retried_spec.rb b/spec/lib/gitlab/ci/status/build/retried_spec.rb new file mode 100644 index 00000000000..ee9acaf1c21 --- /dev/null +++ b/spec/lib/gitlab/ci/status/build/retried_spec.rb @@ -0,0 +1,96 @@ +require 'spec_helper' + +describe Gitlab::Ci::Status::Build::Retried do + let(:build) { create(:ci_build, :retried) } + let(:status) { double('core status') } + let(:user) { double('user') } + + subject { described_class.new(status) } + + describe '#text' do + it 'does not override status text' do + expect(status).to receive(:text) + + subject.text + end + end + + describe '#icon' do + it 'does not override status icon' do + expect(status).to receive(:icon) + + subject.icon + end + end + + describe '#group' do + it 'does not override status group' do + expect(status).to receive(:group) + + subject.group + end + end + + describe '#favicon' do + it 'does not override status label' do + expect(status).to receive(:favicon) + + subject.favicon + end + end + + describe '#label' do + it 'does not override status label' do + expect(status).to receive(:label) + + subject.label + end + end + + describe '#badge_tooltip' do + let(:user) { create(:user) } + let(:build) { create(:ci_build, :retried) } + let(:status) { Gitlab::Ci::Status::Success.new(build, user) } + + it 'returns status' do + expect(status.badge_tooltip).to eq('pending') + end + end + + describe '#status_tooltip' do + let(:user) { create(:user) } + + context 'with a failed build' do + let(:build) { create(:ci_build, :failed, :retried) } + let(:failed_status) { Gitlab::Ci::Status::Failed.new(build, user) } + let(:status) { Gitlab::Ci::Status::Build::Failed.new(failed_status) } + + it 'does override status_tooltip' do + expect(subject.status_tooltip).to eq 'failed <br> (unknown failure) (retried)' + end + end + + context 'with another build' do + let(:build) { create(:ci_build, :retried) } + let(:status) { Gitlab::Ci::Status::Success.new(build, user) } + + it 'does override status_tooltip' do + expect(subject.status_tooltip).to eq 'passed (retried)' + end + end + end + + describe '.matches?' do + subject { described_class.matches?(build, user) } + + context 'with a retried build' do + it { is_expected.to be_truthy } + end + + context 'with a build that has not been retried' do + let(:build) { create(:ci_build, :success) } + + it { is_expected.to be_falsy } + end + end +end diff --git a/spec/lib/gitlab/ci/status/build/retryable_spec.rb b/spec/lib/gitlab/ci/status/build/retryable_spec.rb index 14d42e0d70f..0c5099b7da5 100644 --- a/spec/lib/gitlab/ci/status/build/retryable_spec.rb +++ b/spec/lib/gitlab/ci/status/build/retryable_spec.rb @@ -40,6 +40,24 @@ describe Gitlab::Ci::Status::Build::Retryable do end end + describe '#status_tooltip' do + it 'does not override status status_tooltip' do + expect(status).to receive(:status_tooltip) + + subject.status_tooltip + end + end + + describe '#badge_tooltip' do + let(:user) { create(:user) } + let(:build) { create(:ci_build) } + let(:status) { Gitlab::Ci::Status::Core.new(build, user) } + + it 'does return status' do + expect(status.badge_tooltip).to eq('pending') + end + end + describe 'action details' do let(:user) { create(:user) } let(:build) { create(:ci_build) } diff --git a/spec/lib/gitlab/ci/status/build/stop_spec.rb b/spec/lib/gitlab/ci/status/build/stop_spec.rb index 18e250772f0..f16fc5c9205 100644 --- a/spec/lib/gitlab/ci/status/build/stop_spec.rb +++ b/spec/lib/gitlab/ci/status/build/stop_spec.rb @@ -77,4 +77,24 @@ describe Gitlab::Ci::Status::Build::Stop do end end end + + describe '#status_tooltip' do + it 'does not override status status_tooltip' do + expect(status).to receive(:status_tooltip) + + subject.status_tooltip + end + end + + describe '#badge_tooltip' do + let(:user) { create(:user) } + let(:build) { create(:ci_build, :playable) } + let(:status) { Gitlab::Ci::Status::Core.new(build, user) } + + it 'does not override status badge_tooltip' do + expect(status).to receive(:badge_tooltip) + + subject.badge_tooltip + end + end end diff --git a/spec/lib/gitlab/ci/status/success_warning_spec.rb b/spec/lib/gitlab/ci/status/success_warning_spec.rb index 4582354e739..6d05545d1d8 100644 --- a/spec/lib/gitlab/ci/status/success_warning_spec.rb +++ b/spec/lib/gitlab/ci/status/success_warning_spec.rb @@ -1,8 +1,10 @@ require 'spec_helper' describe Gitlab::Ci::Status::SuccessWarning do + let(:status) { double('status') } + subject do - described_class.new(double('status')) + described_class.new(status) end describe '#test' do diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json index 8c1bf6f7be8..6d63749296e 100644 --- a/spec/lib/gitlab/import_export/project.json +++ b/spec/lib/gitlab/import_export/project.json @@ -6180,12 +6180,6 @@ "user_id": null, "target_url": null, "description": null, - "artifacts_file": { - "url": null - }, - "artifacts_metadata": { - "url": null - }, "erased_by_id": null, "erased_at": null, "type": "Ci::Build", @@ -6218,12 +6212,6 @@ "user_id": null, "target_url": null, "description": null, - "artifacts_file": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/72/p5_build_artifacts.zip" - }, - "artifacts_metadata": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/72/p5_build_artifacts_metadata.gz" - }, "erased_by_id": null, "erased_at": null } @@ -6292,12 +6280,6 @@ "user_id": null, "target_url": null, "description": null, - "artifacts_file": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/74/p5_build_artifacts.zip" - }, - "artifacts_metadata": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/74/p5_build_artifacts_metadata.gz" - }, "erased_by_id": null, "erased_at": null }, @@ -6327,12 +6309,6 @@ "user_id": null, "target_url": null, "description": null, - "artifacts_file": { - "url": null - }, - "artifacts_metadata": { - "url": null - }, "erased_by_id": null, "erased_at": null } @@ -6392,12 +6368,6 @@ "user_id": null, "target_url": null, "description": null, - "artifacts_file": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/76/p5_build_artifacts.zip" - }, - "artifacts_metadata": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/76/p5_build_artifacts_metadata.gz" - }, "erased_by_id": null, "erased_at": null }, @@ -6427,12 +6397,6 @@ "user_id": null, "target_url": null, "description": null, - "artifacts_file": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/75/p5_build_artifacts.zip" - }, - "artifacts_metadata": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/75/p5_build_artifacts_metadata.gz" - }, "erased_by_id": null, "erased_at": null } @@ -6492,12 +6456,6 @@ "user_id": null, "target_url": null, "description": null, - "artifacts_file": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/78/p5_build_artifacts.zip" - }, - "artifacts_metadata": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/78/p5_build_artifacts_metadata.gz" - }, "erased_by_id": null, "erased_at": null }, @@ -6527,12 +6485,6 @@ "user_id": null, "target_url": null, "description": null, - "artifacts_file": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/77/p5_build_artifacts.zip" - }, - "artifacts_metadata": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/77/p5_build_artifacts_metadata.gz" - }, "erased_by_id": null, "erased_at": null } @@ -6592,12 +6544,6 @@ "user_id": null, "target_url": null, "description": null, - "artifacts_file": { - "url": null - }, - "artifacts_metadata": { - "url": null - }, "erased_by_id": null, "erased_at": null }, @@ -6627,12 +6573,6 @@ "user_id": null, "target_url": null, "description": null, - "artifacts_file": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/80/p5_build_artifacts.zip" - }, - "artifacts_metadata": { - "url": "/Users/Test/Test/gitlab-development-kit/gitlab/shared/artifacts/2016_03/5/80/p5_build_artifacts_metadata.gz" - }, "erased_by_id": null, "erased_at": null } diff --git a/spec/lib/uploaded_file_spec.rb b/spec/lib/uploaded_file_spec.rb new file mode 100644 index 00000000000..cc99e7e8911 --- /dev/null +++ b/spec/lib/uploaded_file_spec.rb @@ -0,0 +1,116 @@ +require 'spec_helper' + +describe UploadedFile do + describe ".from_params" do + let(:temp_dir) { Dir.tmpdir } + let(:temp_file) { Tempfile.new("test", temp_dir) } + let(:upload_path) { nil } + + subject do + described_class.from_params(params, :file, upload_path) + end + + before do + FileUtils.touch(temp_file) + end + + after do + FileUtils.rm_f(temp_file) + FileUtils.rm_r(upload_path) if upload_path + end + + context 'when valid file is specified' do + context 'only local path is specified' do + let(:params) do + { 'file.path' => temp_file.path } + end + + it "succeeds" do + is_expected.not_to be_nil + end + + it "generates filename from path" do + expect(subject.original_filename).to eq(::File.basename(temp_file.path)) + end + end + + context 'all parameters are specified' do + let(:params) do + { 'file.path' => temp_file.path, + 'file.name' => 'my_file.txt', + 'file.type' => 'my/type', + 'file.sha256' => 'sha256', + 'file.remote_id' => 'remote_id' } + end + + it "succeeds" do + is_expected.not_to be_nil + end + + it "generates filename from path" do + expect(subject.original_filename).to eq('my_file.txt') + expect(subject.content_type).to eq('my/type') + expect(subject.sha256).to eq('sha256') + expect(subject.remote_id).to eq('remote_id') + end + end + end + + context 'when no params are specified' do + let(:params) do + {} + end + + it "does not return an object" do + is_expected.to be_nil + end + end + + context 'when only remote id is specified' do + let(:params) do + { 'file.remote_id' => 'remote_id' } + end + + it "raises an error" do + expect { subject }.to raise_error(UploadedFile::InvalidPathError, /file is invalid/) + end + end + + context 'when verifying allowed paths' do + let(:params) do + { 'file.path' => temp_file.path } + end + + context 'when file is stored in system temporary folder' do + let(:temp_dir) { Dir.tmpdir } + + it "succeeds" do + is_expected.not_to be_nil + end + end + + context 'when file is stored in user provided upload path' do + let(:upload_path) { Dir.mktmpdir } + let(:temp_dir) { upload_path } + + it "succeeds" do + is_expected.not_to be_nil + end + end + + context 'when file is stored outside of user provided upload path' do + let!(:generated_dir) { Dir.mktmpdir } + let!(:temp_dir) { Dir.mktmpdir } + + before do + # We overwrite default temporary path + allow(Dir).to receive(:tmpdir).and_return(generated_dir) + end + + it "raises an error" do + expect { subject }.to raise_error(UploadedFile::InvalidPathError, /insecure path used/) + end + end + end + end +end diff --git a/spec/presenters/ci/build_presenter_spec.rb b/spec/presenters/ci/build_presenter_spec.rb index 1a8001be6ab..cc16d0f156b 100644 --- a/spec/presenters/ci/build_presenter_spec.rb +++ b/spec/presenters/ci/build_presenter_spec.rb @@ -72,13 +72,44 @@ describe Ci::BuildPresenter do end end - context 'when build is not auto-canceled' do - before do - expect(build).to receive(:auto_canceled?).and_return(false) + context 'when build failed' do + let(:build) { create(:ci_build, :failed, pipeline: pipeline) } + + it 'returns the reason of failure' do + status_title = presenter.status_title + + expect(status_title).to eq('Failed <br> (unknown failure)') end + end + + context 'when build has failed && retried' do + let(:build) { create(:ci_build, :failed, :retried, pipeline: pipeline) } - it 'does not have a status title' do - expect(presenter.status_title).to be_nil + it 'does not include retried title' do + status_title = presenter.status_title + + expect(status_title).not_to include('(retried)') + expect(status_title).to eq('Failed <br> (unknown failure)') + end + end + + context 'when build has failed and is allowed to' do + let(:build) { create(:ci_build, :failed, :allowed_to_fail, pipeline: pipeline) } + + it 'returns the reason of failure' do + status_title = presenter.status_title + + expect(status_title).to eq('Failed <br> (unknown failure)') + end + end + + context 'For any other build' do + let(:build) { create(:ci_build, :success, pipeline: pipeline) } + + it 'returns the status' do + tooltip_description = presenter.status_title + + expect(tooltip_description).to eq('Success') end end end @@ -134,4 +165,56 @@ describe Ci::BuildPresenter do end end end + + describe '#tooltip_message' do + context 'When build has failed' do + let(:build) { create(:ci_build, :script_failure, pipeline: pipeline) } + + it 'returns the reason of failure' do + tooltip = subject.tooltip_message + + expect(tooltip).to eq("#{build.name} - failed <br> (script failure)") + end + end + + context 'When build has failed and retried' do + let(:build) { create(:ci_build, :script_failure, :retried, pipeline: pipeline) } + + it 'should include the reason of failure and the retried title' do + tooltip = subject.tooltip_message + + expect(tooltip).to eq("#{build.name} - failed <br> (script failure) (retried)") + end + end + + context 'When build has failed and is allowed to' do + let(:build) { create(:ci_build, :script_failure, :allowed_to_fail, pipeline: pipeline) } + + it 'should include the reason of failure' do + tooltip = subject.tooltip_message + + expect(tooltip).to eq("#{build.name} - failed <br> (script failure) (allowed to fail)") + end + end + + context 'For any other build (no retried)' do + let(:build) { create(:ci_build, :success, pipeline: pipeline) } + + it 'should include build name and status' do + tooltip = subject.tooltip_message + + expect(tooltip).to eq("#{build.name} - passed") + end + end + + context 'For any other build (retried)' do + let(:build) { create(:ci_build, :success, :retried, pipeline: pipeline) } + + it 'should include build name and status' do + tooltip = subject.tooltip_message + + expect(tooltip).to eq("#{build.name} - passed (retried)") + end + end + end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 284e8931812..847579807ae 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -951,12 +951,53 @@ describe API::Runner, :clean_gitlab_redis_shared_state do describe 'POST /api/v4/jobs/:id/artifacts/authorize' do context 'when using token as parameter' do - it 'authorizes posting artifacts to running job' do - authorize_artifacts_with_token_in_params + context 'posting artifacts to running job' do + subject do + authorize_artifacts_with_token_in_params + end - expect(response).to have_gitlab_http_status(200) - expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE) - expect(json_response['TempPath']).not_to be_nil + shared_examples 'authorizes local file' do + it 'succeeds' do + subject + + expect(response).to have_gitlab_http_status(200) + expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE) + expect(json_response['TempPath']).to eq(JobArtifactUploader.workhorse_local_upload_path) + expect(json_response['RemoteObject']).to be_nil + end + end + + context 'when using local storage' do + it_behaves_like 'authorizes local file' + end + + context 'when using remote storage' do + context 'when direct upload is enabled' do + before do + stub_artifacts_object_storage(enabled: true, direct_upload: true) + end + + it 'succeeds' do + subject + + expect(response).to have_gitlab_http_status(200) + expect(response.content_type.to_s).to eq(Gitlab::Workhorse::INTERNAL_API_CONTENT_TYPE) + expect(json_response['TempPath']).to eq(JobArtifactUploader.workhorse_local_upload_path) + expect(json_response['RemoteObject']).to have_key('ID') + expect(json_response['RemoteObject']).to have_key('GetURL') + expect(json_response['RemoteObject']).to have_key('StoreURL') + expect(json_response['RemoteObject']).to have_key('DeleteURL') + end + end + + context 'when direct upload is disabled' do + before do + stub_artifacts_object_storage(enabled: true, direct_upload: false) + end + + it_behaves_like 'authorizes local file' + end + end end it 'fails to post too large artifact' do @@ -1052,20 +1093,45 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end end - context 'when uses regular file post' do - before do - upload_artifacts(file_upload, headers_with_token, false) + context 'when uses accelerated file post' do + context 'for file stored locally' do + before do + upload_artifacts(file_upload, headers_with_token) + end + + it_behaves_like 'successful artifacts upload' end - it_behaves_like 'successful artifacts upload' - end + context 'for file stored remotelly' do + let!(:fog_connection) do + stub_artifacts_object_storage(direct_upload: true) + end - context 'when uses accelerated file post' do - before do - upload_artifacts(file_upload, headers_with_token, true) - end + before do + fog_connection.directories.get('artifacts').files.create( + key: 'tmp/upload/12312300', + body: 'content' + ) - it_behaves_like 'successful artifacts upload' + upload_artifacts(file_upload, headers_with_token, + { 'file.remote_id' => remote_id }) + end + + context 'when valid remote_id is used' do + let(:remote_id) { '12312300' } + + it_behaves_like 'successful artifacts upload' + end + + context 'when invalid remote_id is used' do + let(:remote_id) { 'invalid id' } + + it 'responds with bad request' do + expect(response).to have_gitlab_http_status(500) + expect(json_response['message']).to eq("Missing file") + end + end + end end context 'when using runners token' do @@ -1209,15 +1275,19 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end context 'when artifacts are being stored outside of tmp path' do + let(:new_tmpdir) { Dir.mktmpdir } + before do + # init before overwriting tmp dir + file_upload + # 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(JobArtifactUploader).to receive(:workhorse_upload_path).and_return(@tmpdir) + allow(Dir).to receive(:tmpdir).and_return(new_tmpdir) end after do - FileUtils.remove_entry @tmpdir + FileUtils.remove_entry(new_tmpdir) end it' "fails to post artifacts for outside of tmp path"' do @@ -1227,12 +1297,11 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end end - def upload_artifacts(file, headers = {}, accelerated = true) - params = if accelerated - { 'file.path' => file.path, 'file.name' => file.original_filename } - else - { 'file' => file } - end + def upload_artifacts(file, headers = {}, params = {}) + params = params.merge({ + 'file.path' => file.path, + 'file.name' => file.original_filename + }) post api("/jobs/#{job.id}/artifacts"), params, headers end diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 1e6bd993c08..f80abb06fca 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -1016,7 +1016,7 @@ describe 'Git LFS API and storage' do it_behaves_like 'a valid response' do it 'responds with status 200, location of lfs remote store and object details' do - expect(json_response['TempPath']).to be_nil + expect(json_response['TempPath']).to eq(LfsObjectUploader.workhorse_local_upload_path) expect(json_response['RemoteObject']).to have_key('ID') expect(json_response['RemoteObject']).to have_key('GetURL') expect(json_response['RemoteObject']).to have_key('StoreURL') @@ -1073,7 +1073,9 @@ describe 'Git LFS API and storage' do ['123123', '../../123123'].each do |remote_id| context "with invalid remote_id: #{remote_id}" do subject do - put_finalize_with_args('file.remote_id' => remote_id) + put_finalize(with_tempfile: true, args: { + 'file.remote_id' => remote_id + }) end it 'responds with status 403' do @@ -1093,9 +1095,10 @@ describe 'Git LFS API and storage' do end subject do - put_finalize_with_args( + put_finalize(with_tempfile: true, args: { 'file.remote_id' => '12312300', - 'file.name' => 'name') + 'file.name' => 'name' + }) end it 'responds with status 200' do @@ -1331,7 +1334,7 @@ describe 'Git LFS API and storage' do put "#{project.http_url_to_repo}/gitlab-lfs/objects/#{sample_oid}/#{sample_size}/authorize", nil, authorize_headers end - def put_finalize(lfs_tmp = lfs_tmp_file, with_tempfile: false) + def put_finalize(lfs_tmp = lfs_tmp_file, with_tempfile: false, args: {}) upload_path = LfsObjectUploader.workhorse_local_upload_path file_path = upload_path + '/' + lfs_tmp if lfs_tmp @@ -1340,12 +1343,12 @@ describe 'Git LFS API and storage' do FileUtils.touch(file_path) end - args = { + extra_args = { 'file.path' => file_path, 'file.name' => File.basename(file_path) - }.compact + } - put_finalize_with_args(args) + put_finalize_with_args(args.merge(extra_args).compact) end def put_finalize_with_args(args) diff --git a/spec/serializers/build_serializer_spec.rb b/spec/serializers/build_serializer_spec.rb index 9673b11c2a2..98cd15e248b 100644 --- a/spec/serializers/build_serializer_spec.rb +++ b/spec/serializers/build_serializer_spec.rb @@ -28,15 +28,31 @@ describe BuildSerializer do end describe '#represent_status' do - context 'when represents only status' do - let(:resource) { create(:ci_build) } + context 'for a failed build' do + let(:resource) { create(:ci_build, :failed) } + let(:status) { resource.detailed_status(double('user')) } + + subject { serializer.represent_status(resource) } + + it 'serializes only status' do + expect(subject[:text]).to eq(status.text) + expect(subject[:label]).to eq('failed') + expect(subject[:tooltip]).to eq('failed <br> (unknown failure)') + expect(subject[:icon]).to eq(status.icon) + expect(subject[:favicon]).to match_asset_path("/assets/ci_favicons/#{status.favicon}.ico") + end + end + + context 'for any other type of build' do + let(:resource) { create(:ci_build, :success) } let(:status) { resource.detailed_status(double('user')) } subject { serializer.represent_status(resource) } it 'serializes only status' do expect(subject[:text]).to eq(status.text) - expect(subject[:label]).to eq(status.label) + expect(subject[:label]).to eq('passed') + expect(subject[:tooltip]).to eq('passed') expect(subject[:icon]).to eq(status.icon) expect(subject[:favicon]).to match_asset_path("/assets/ci_favicons/#{status.favicon}.ico") end diff --git a/spec/serializers/job_entity_spec.rb b/spec/serializers/job_entity_spec.rb index 026360e91a3..24a6f1a2a8a 100644 --- a/spec/serializers/job_entity_spec.rb +++ b/spec/serializers/job_entity_spec.rb @@ -38,7 +38,7 @@ describe JobEntity do it 'contains details' do expect(subject).to include :status - expect(subject[:status]).to include :icon, :favicon, :text, :label + expect(subject[:status]).to include :icon, :favicon, :text, :label, :tooltip end context 'when job is retryable' do @@ -126,7 +126,29 @@ describe JobEntity do it 'contains details' do expect(subject).to include :status - expect(subject[:status]).to include :icon, :favicon, :text, :label + expect(subject[:status]).to include :icon, :favicon, :text, :label, :tooltip + end + end + + context 'when job failed' do + let(:job) { create(:ci_build, :script_failure) } + + describe 'status' do + it 'should contain the failure reason inside label' do + expect(subject[:status]).to include :icon, :favicon, :text, :label, :tooltip + expect(subject[:status][:label]).to eq('failed') + expect(subject[:status][:tooltip]).to eq('failed <br> (script failure)') + end + end + end + + context 'when job passed' do + let(:job) { create(:ci_build, :success) } + + describe 'status' do + it 'should not contain the failure reason inside label' do + expect(subject[:status][:label]).to eq('passed') + end end end end diff --git a/spec/serializers/pipeline_entity_spec.rb b/spec/serializers/pipeline_entity_spec.rb index 248552d1858..2473c561f4b 100644 --- a/spec/serializers/pipeline_entity_spec.rb +++ b/spec/serializers/pipeline_entity_spec.rb @@ -30,7 +30,7 @@ describe PipelineEntity do expect(subject).to include :details expect(subject[:details]) .to include :duration, :finished_at - expect(subject[:details][:status]).to include :icon, :favicon, :text, :label + expect(subject[:details][:status]).to include :icon, :favicon, :text, :label, :tooltip end it 'contains flags' do diff --git a/spec/serializers/stage_entity_spec.rb b/spec/serializers/stage_entity_spec.rb index 40e303f7b89..2034c7891ef 100644 --- a/spec/serializers/stage_entity_spec.rb +++ b/spec/serializers/stage_entity_spec.rb @@ -26,7 +26,7 @@ describe StageEntity do end it 'contains detailed status' do - expect(subject[:status]).to include :text, :label, :group, :icon + expect(subject[:status]).to include :text, :label, :group, :icon, :tooltip expect(subject[:status][:label]).to eq 'passed' end diff --git a/spec/serializers/status_entity_spec.rb b/spec/serializers/status_entity_spec.rb index 70402bac2e2..559475e571c 100644 --- a/spec/serializers/status_entity_spec.rb +++ b/spec/serializers/status_entity_spec.rb @@ -16,7 +16,7 @@ describe StatusEntity do subject { entity.as_json } it 'contains status details' do - expect(subject).to include :text, :icon, :favicon, :label, :group + expect(subject).to include :text, :icon, :favicon, :label, :group, :tooltip expect(subject).to include :has_details, :details_path expect(subject[:favicon]).to match_asset_path('/assets/ci_favicons/favicon_status_success.ico') end diff --git a/spec/support/stub_configuration.rb b/spec/support/stub_configuration.rb index bad1d34df3a..a75a3eaefcb 100644 --- a/spec/support/stub_configuration.rb +++ b/spec/support/stub_configuration.rb @@ -45,6 +45,10 @@ module StubConfiguration allow(Gitlab.config.lfs).to receive_messages(to_settings(messages)) end + def stub_artifacts_setting(messages) + allow(Gitlab.config.artifacts).to receive_messages(to_settings(messages)) + end + def stub_storage_settings(messages) messages.deep_stringify_keys! diff --git a/spec/uploaders/object_storage_spec.rb b/spec/uploaders/object_storage_spec.rb index 17d508d0146..16455e2517b 100644 --- a/spec/uploaders/object_storage_spec.rb +++ b/spec/uploaders/object_storage_spec.rb @@ -516,108 +516,46 @@ describe ObjectStorage do end end - describe '#store_workhorse_file!' do + describe '#cache!' do subject do - uploader.store_workhorse_file!(params, :file) + uploader.cache!(uploaded_file) end context 'when local file is used' do context 'when valid file is used' do - let(:target_path) do - File.join(uploader_class.root, uploader_class::TMP_UPLOAD_PATH) + let(:uploaded_file) do + fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') end - before do - FileUtils.mkdir_p(target_path) - end - - context 'when no filename is specified' do - let(:params) do - { "file.path" => "test/file" } - end - - it 'raises an error' do - expect { subject }.to raise_error(uploader_class::RemoteStoreError, /Missing filename/) - end - end - - context 'when invalid file is specified' do - let(:file_path) do - File.join(target_path, "..", "test.file") - end - - before do - FileUtils.touch(file_path) - end - - let(:params) do - { "file.path" => file_path, - "file.name" => "my_file.txt" } - end - - it 'raises an error' do - expect { subject }.to raise_error(uploader_class::RemoteStoreError, /Bad file path/) - end - end - - context 'when filename is specified' do - let(:params) do - { "file.path" => tmp_file, - "file.name" => "my_file.txt" } - end - - let(:tmp_file) { Tempfile.new('filename', target_path) } - - before do - FileUtils.touch(tmp_file) - end - - after do - FileUtils.rm_f(tmp_file) - end - - it 'succeeds' do - expect { subject }.not_to raise_error - - expect(uploader).to be_exists - end - - it 'proper path is being used' do - subject - - expect(uploader.path).to start_with(uploader_class.root) - expect(uploader.path).to end_with("my_file.txt") - end + it "properly caches the file" do + subject - it 'source file to not exist' do - subject - - expect(File.exist?(tmp_file.path)).to be_falsey - end + expect(uploader).to be_exists + expect(uploader.path).to start_with(uploader_class.root) + expect(uploader.filename).to eq('rails_sample.jpg') end end end context 'when remote file is used' do + let(:temp_file) { Tempfile.new("test") } + let!(:fog_connection) do stub_uploads_object_storage(uploader_class) end - context 'when valid file is used' do - context 'when no filename is specified' do - let(:params) do - { "file.remote_id" => "test/123123" } - end + before do + FileUtils.touch(temp_file) + end - it 'raises an error' do - expect { subject }.to raise_error(uploader_class::RemoteStoreError, /Missing filename/) - end - end + after do + FileUtils.rm_f(temp_file) + end + context 'when valid file is used' do context 'when invalid file is specified' do - let(:params) do - { "file.remote_id" => "../test/123123", - "file.name" => "my_file.txt" } + let(:uploaded_file) do + UploadedFile.new(temp_file.path, remote_id: "../test/123123") end it 'raises an error' do @@ -626,9 +564,8 @@ describe ObjectStorage do end context 'when non existing file is specified' do - let(:params) do - { "file.remote_id" => "test/12312300", - "file.name" => "my_file.txt" } + let(:uploaded_file) do + UploadedFile.new(temp_file.path, remote_id: "test/123123") end it 'raises an error' do @@ -636,10 +573,9 @@ describe ObjectStorage do end end - context 'when filename is specified' do - let(:params) do - { "file.remote_id" => "test/123123", - "file.name" => "my_file.txt" } + context 'when valid file is specified' do + let(:uploaded_file) do + UploadedFile.new(temp_file.path, filename: "my_file.txt", remote_id: "test/123123") end let!(:fog_file) do @@ -649,36 +585,37 @@ describe ObjectStorage do ) end - it 'succeeds' do + it 'file to be cached and remote stored' do expect { subject }.not_to raise_error expect(uploader).to be_exists - end - - it 'path to not be temporary' do - subject - + expect(uploader).to be_cached expect(uploader.path).not_to be_nil - expect(uploader.path).not_to include('tmp/upload') - expect(uploader.url).to include('/my_file.txt') + expect(uploader.path).not_to include('tmp/cache') + expect(uploader.url).not_to be_nil + expect(uploader.path).not_to include('tmp/cache') + expect(uploader.object_store).to eq(described_class::Store::REMOTE) end - it 'url is used' do - subject + context 'when file is stored' do + subject do + uploader.store!(uploaded_file) + end - expect(uploader.url).not_to be_nil - expect(uploader.url).to include('/my_file.txt') + it 'file to be remotely stored in permament location' do + subject + + expect(uploader).to be_exists + expect(uploader).not_to be_cached + expect(uploader.path).not_to be_nil + expect(uploader.path).not_to include('tmp/upload') + expect(uploader.path).not_to include('tmp/cache') + expect(uploader.url).to include('/my_file.txt') + expect(uploader.object_store).to eq(described_class::Store::REMOTE) + end end end end end - - context 'when no file is used' do - let(:params) { {} } - - it 'raises an error' do - expect { subject }.to raise_error(uploader_class::RemoteStoreError, /Bad file/) - end - end end end diff --git a/spec/views/projects/jobs/show.html.haml_spec.rb b/spec/views/projects/jobs/show.html.haml_spec.rb index 6a67da79ec5..9e692159bd0 100644 --- a/spec/views/projects/jobs/show.html.haml_spec.rb +++ b/spec/views/projects/jobs/show.html.haml_spec.rb @@ -1,8 +1,10 @@ require 'spec_helper' describe 'projects/jobs/show' do + let(:user) { create(:user) } let(:project) { create(:project, :repository) } let(:build) { create(:ci_build, pipeline: pipeline) } + let(:builds) { project.builds.present(current_user: user) } let(:pipeline) do create(:ci_pipeline, project: project, sha: project.commit.id) @@ -11,6 +13,7 @@ describe 'projects/jobs/show' do before do assign(:build, build.present) assign(:project, project) + assign(:builds, builds) allow(view).to receive(:can?).and_return(true) end |