Commit 5c168885 authored by Fabio Pitino's avatar Fabio Pitino Committed by Rémy Coutable
Browse files

Expose arbitrary artifacts via MR widget

* Allow user to specify `artifacts:expose_as` in CI config
* Save :has_exposed_artifacts in job metadata for queries
* Find exposed artifacts in build metadata model
* Expose API endpoint for frontend to fetch data

Fix unlrelated controller specs

Use default has_exposed_artifacts NULL

Avoid using a background migration to change NULL
to false. It's not needed.

Feedback from review

* add links to issue for follow up refactoring
* preload job artifacts and metadata associations
* merge DisallowedRegexInArrayValidator into existing
ArrayOfStringsValidator
* other minor changes

Rename params to match frontend code

Feedback from review

Feedback from review
parent fae81201
...@@ -13,7 +13,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -13,7 +13,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
skip_before_action :merge_request, only: [:index, :bulk_update] skip_before_action :merge_request, only: [:index, :bulk_update]
before_action :whitelist_query_limiting, only: [:assign_related_issues, :update] before_action :whitelist_query_limiting, only: [:assign_related_issues, :update]
before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort] before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort]
before_action :authorize_test_reports!, only: [:test_reports] before_action :authorize_read_actual_head_pipeline!, only: [:test_reports, :exposed_artifacts]
before_action :set_issuables_index, only: [:index] before_action :set_issuables_index, only: [:index]
before_action :authenticate_user!, only: [:assign_related_issues] before_action :authenticate_user!, only: [:assign_related_issues]
before_action :check_user_can_push_to_source_branch!, only: [:rebase] before_action :check_user_can_push_to_source_branch!, only: [:rebase]
...@@ -115,6 +115,14 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -115,6 +115,14 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
reports_response(@merge_request.compare_test_reports) reports_response(@merge_request.compare_test_reports)
end end
   
def exposed_artifacts
if @merge_request.has_exposed_artifacts?
reports_response(@merge_request.find_exposed_artifacts)
else
head :no_content
end
end
def edit def edit
define_edit_vars define_edit_vars
end end
...@@ -357,8 +365,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -357,8 +365,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end end
end end
   
def authorize_test_reports! def authorize_read_actual_head_pipeline!
# MergeRequest#actual_head_pipeline is the pipeline accessed in MergeRequest#compare_reports.
return render_404 unless can?(current_user, :read_build, merge_request.actual_head_pipeline) return render_404 unless can?(current_user, :read_build, merge_request.actual_head_pipeline)
end end
end end
......
...@@ -118,6 +118,11 @@ module Ci ...@@ -118,6 +118,11 @@ module Ci
   
scope :eager_load_job_artifacts, -> { includes(:job_artifacts) } scope :eager_load_job_artifacts, -> { includes(:job_artifacts) }
   
scope :with_exposed_artifacts, -> do
joins(:metadata).merge(Ci::BuildMetadata.with_exposed_artifacts)
.includes(:metadata, :job_artifacts_metadata)
end
scope :with_artifacts_not_expired, ->() { with_artifacts_archive.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_artifacts_not_expired, ->() { with_artifacts_archive.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) }
scope :with_expired_artifacts, ->() { with_artifacts_archive.where('artifacts_expire_at < ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts_archive.where('artifacts_expire_at < ?', Time.now) }
scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) }
...@@ -595,6 +600,14 @@ module Ci ...@@ -595,6 +600,14 @@ module Ci
update_column(:trace, nil) update_column(:trace, nil)
end end
   
def artifacts_expose_as
options.dig(:artifacts, :expose_as)
end
def artifacts_paths
options.dig(:artifacts, :paths)
end
def needs_touch? def needs_touch?
Time.now - updated_at > 15.minutes.to_i Time.now - updated_at > 15.minutes.to_i
end end
......
...@@ -27,6 +27,7 @@ module Ci ...@@ -27,6 +27,7 @@ module Ci
   
scope :scoped_build, -> { where('ci_builds_metadata.build_id = ci_builds.id') } scope :scoped_build, -> { where('ci_builds_metadata.build_id = ci_builds.id') }
scope :with_interruptible, -> { where(interruptible: true) } scope :with_interruptible, -> { where(interruptible: true) }
scope :with_exposed_artifacts, -> { where(has_exposed_artifacts: true) }
   
enum timeout_source: { enum timeout_source: {
unknown_timeout_source: 1, unknown_timeout_source: 1,
......
...@@ -783,6 +783,10 @@ module Ci ...@@ -783,6 +783,10 @@ module Ci
end end
end end
   
def has_exposed_artifacts?
complete? && builds.latest.with_exposed_artifacts.exists?
end
def branch_updated? def branch_updated?
strong_memoize(:branch_updated) do strong_memoize(:branch_updated) do
push_details.branch_updated? push_details.branch_updated?
......
...@@ -16,6 +16,7 @@ module Ci ...@@ -16,6 +16,7 @@ module Ci
   
delegate :timeout, to: :metadata, prefix: true, allow_nil: true delegate :timeout, to: :metadata, prefix: true, allow_nil: true
delegate :interruptible, to: :metadata, prefix: false, allow_nil: true delegate :interruptible, to: :metadata, prefix: false, allow_nil: true
delegate :has_exposed_artifacts?, to: :metadata, prefix: false, allow_nil: true
before_create :ensure_metadata before_create :ensure_metadata
end end
   
...@@ -45,6 +46,9 @@ module Ci ...@@ -45,6 +46,9 @@ module Ci
   
def options=(value) def options=(value)
write_metadata_attribute(:options, :config_options, value) write_metadata_attribute(:options, :config_options, value)
# Store presence of exposed artifacts in build metadata to make it easier to query
ensure_metadata.has_exposed_artifacts = value&.dig(:artifacts, :expose_as).present?
end end
   
def yaml_variables=(value) def yaml_variables=(value)
......
...@@ -1255,6 +1255,27 @@ class MergeRequest < ApplicationRecord ...@@ -1255,6 +1255,27 @@ class MergeRequest < ApplicationRecord
compare_reports(Ci::CompareTestReportsService) compare_reports(Ci::CompareTestReportsService)
end end
   
def has_exposed_artifacts?
return false unless Feature.enabled?(:ci_expose_arbitrary_artifacts_in_mr, default_enabled: true)
actual_head_pipeline&.has_exposed_artifacts?
end
# TODO: this method and compare_test_reports use the same
# result type, which is handled by the controller's #reports_response.
# we should minimize mistakes by isolating the common parts.
# issue: https://gitlab.com/gitlab-org/gitlab/issues/34224
def find_exposed_artifacts
unless has_exposed_artifacts?
return { status: :error, status_reason: 'This merge request does not have exposed artifacts' }
end
compare_reports(Ci::GenerateExposedArtifactsReportService)
end
# TODO: consider renaming this as with exposed artifacts we generate reports,
# not always compare
# issue: https://gitlab.com/gitlab-org/gitlab/issues/34224
def compare_reports(service_class, current_user = nil) def compare_reports(service_class, current_user = nil)
with_reactive_cache(service_class.name, current_user&.id) do |data| with_reactive_cache(service_class.name, current_user&.id) do |data|
unless service_class.new(project, current_user) unless service_class.new(project, current_user)
...@@ -1269,6 +1290,8 @@ class MergeRequest < ApplicationRecord ...@@ -1269,6 +1290,8 @@ class MergeRequest < ApplicationRecord
def calculate_reactive_cache(identifier, current_user_id = nil, *args) def calculate_reactive_cache(identifier, current_user_id = nil, *args)
service_class = identifier.constantize service_class = identifier.constantize
   
# TODO: the type check should change to something that includes exposed artifacts service
# issue: https://gitlab.com/gitlab-org/gitlab/issues/34224
raise NameError, service_class unless service_class < Ci::CompareReportsBaseService raise NameError, service_class unless service_class < Ci::CompareReportsBaseService
   
current_user = User.find_by(id: current_user_id) current_user = User.find_by(id: current_user_id)
......
...@@ -65,6 +65,12 @@ class MergeRequestPollWidgetEntity < IssuableEntity ...@@ -65,6 +65,12 @@ class MergeRequestPollWidgetEntity < IssuableEntity
end end
end end
   
expose :exposed_artifacts_path do |merge_request|
if merge_request.has_exposed_artifacts?
exposed_artifacts_project_merge_request_path(merge_request.project, merge_request, format: :json)
end
end
expose :create_issue_to_resolve_discussions_path do |merge_request| expose :create_issue_to_resolve_discussions_path do |merge_request|
presenter(merge_request).create_issue_to_resolve_discussions_path presenter(merge_request).create_issue_to_resolve_discussions_path
end end
......
# frozen_string_literal: true # frozen_string_literal: true
   
module Ci module Ci
# TODO: when using this class with exposed artifacts we see that there are
# 2 responsibilities:
# 1. reactive caching interface (same in all cases)
# 2. data generator (report comparison in most of the case but not always)
# issue: https://gitlab.com/gitlab-org/gitlab/issues/34224
class CompareReportsBaseService < ::BaseService class CompareReportsBaseService < ::BaseService
def execute(base_pipeline, head_pipeline) def execute(base_pipeline, head_pipeline)
comparer = comparer_class.new(get_report(base_pipeline), get_report(head_pipeline)) comparer = comparer_class.new(get_report(base_pipeline), get_report(head_pipeline))
......
# frozen_string_literal: true
module Ci
# This class loops through all builds with exposed artifacts and returns
# basic information about exposed artifacts for given jobs for the frontend
# to display them as custom links in the merge request.
#
# This service must be used with care.
# Looking for exposed artifacts is very slow and should be done asynchronously.
class FindExposedArtifactsService < ::BaseService
include Gitlab::Routing
MAX_EXPOSED_ARTIFACTS = 10
def for_pipeline(pipeline, limit: MAX_EXPOSED_ARTIFACTS)
results = []
pipeline.builds.latest.with_exposed_artifacts.find_each do |job|
if job_exposed_artifacts = for_job(job)
results << job_exposed_artifacts
end
break if results.size >= limit
end
results
end
def for_job(job)
return unless job.has_exposed_artifacts?
metadata_entries = first_2_metadata_entries_for_artifacts_paths(job)
return if metadata_entries.empty?
{
text: job.artifacts_expose_as,
url: path_for_entries(metadata_entries, job),
job_path: project_job_path(project, job),
job_name: job.name
}
end
private
# we don't need to fetch all artifacts entries for a job because
# it could contain many. We only need to know whether it has 1 or more
# artifacts, so fetching the first 2 would be sufficient.
def first_2_metadata_entries_for_artifacts_paths(job)
job.artifacts_paths
.lazy
.map { |path| job.artifacts_metadata_entry(path, recursive: true) }
.select { |entry| entry.exists? }
.first(2)
end
def path_for_entries(entries, job)
return if entries.empty?
if single_artifact?(entries)
file_project_job_artifacts_path(project, job, entries.first.path)
else
browse_project_job_artifacts_path(project, job)
end
end
def single_artifact?(entries)
entries.size == 1 && entries.first.file?
end
end
end
# frozen_string_literal: true
module Ci
# TODO: a couple of points with this approach:
# + reuses existing architecture and reactive caching
# - it's not a report comparison and some comparing features must be turned off.
# see CompareReportsBaseService for more notes.
# issue: https://gitlab.com/gitlab-org/gitlab/issues/34224
class GenerateExposedArtifactsReportService < CompareReportsBaseService
def execute(base_pipeline, head_pipeline)
data = FindExposedArtifactsService.new(project, current_user).for_pipeline(head_pipeline)
{
status: :parsed,
key: key(base_pipeline, head_pipeline),
data: data
}
rescue => e
Gitlab::Sentry.track_acceptable_exception(e, extra: { project_id: project.id })
{
status: :error,
key: key(base_pipeline, head_pipeline),
status_reason: _('An error occurred while fetching exposed artifacts.')
}
end
def latest?(base_pipeline, head_pipeline, data)
data&.fetch(:key, nil) == key(base_pipeline, head_pipeline)
end
end
end
---
title: Expose arbitrary job artifacts in Merge Request widget
merge_request: 18385
author:
type: added
...@@ -274,6 +274,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do ...@@ -274,6 +274,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
get :discussions, format: :json get :discussions, format: :json
post :rebase post :rebase
get :test_reports get :test_reports
get :exposed_artifacts
   
scope constraints: { format: nil }, action: :show do scope constraints: { format: nil }, action: :show do
get :commits, defaults: { tab: 'commits' } get :commits, defaults: { tab: 'commits' }
......
# frozen_string_literal: true
class AddHasExposedArtifactsToCiBuildsMetadata < ActiveRecord::Migration[5.2]
DOWNTIME = false
def up
add_column :ci_builds_metadata, :has_exposed_artifacts, :boolean
end
def down
remove_column :ci_builds_metadata, :has_exposed_artifacts
end
end
# frozen_string_literal: true
class AddIndexToCiBuildsMetadataHasExposedArtifacts < ActiveRecord::Migration[5.2]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_concurrent_index :ci_builds_metadata, [:build_id], where: "has_exposed_artifacts IS TRUE", name: 'index_ci_builds_metadata_on_build_id_and_has_exposed_artifacts'
end
def down
remove_concurrent_index_by_name :ci_builds_metadata, 'index_ci_builds_metadata_on_build_id_and_has_exposed_artifacts'
end
end
...@@ -691,7 +691,9 @@ ActiveRecord::Schema.define(version: 2019_10_16_220135) do ...@@ -691,7 +691,9 @@ ActiveRecord::Schema.define(version: 2019_10_16_220135) do
t.boolean "interruptible" t.boolean "interruptible"
t.jsonb "config_options" t.jsonb "config_options"
t.jsonb "config_variables" t.jsonb "config_variables"
t.boolean "has_exposed_artifacts"
t.index ["build_id"], name: "index_ci_builds_metadata_on_build_id", unique: true t.index ["build_id"], name: "index_ci_builds_metadata_on_build_id", unique: true
t.index ["build_id"], name: "index_ci_builds_metadata_on_build_id_and_has_exposed_artifacts", where: "(has_exposed_artifacts IS TRUE)"
t.index ["build_id"], name: "index_ci_builds_metadata_on_build_id_and_interruptible", where: "(interruptible = true)" t.index ["build_id"], name: "index_ci_builds_metadata_on_build_id_and_interruptible", where: "(interruptible = true)"
t.index ["project_id"], name: "index_ci_builds_metadata_on_project_id" t.index ["project_id"], name: "index_ci_builds_metadata_on_project_id"
end end
......
...@@ -12,7 +12,9 @@ module Gitlab ...@@ -12,7 +12,9 @@ module Gitlab
include ::Gitlab::Config::Entry::Validatable include ::Gitlab::Config::Entry::Validatable
include ::Gitlab::Config::Entry::Attributable include ::Gitlab::Config::Entry::Attributable
   
ALLOWED_KEYS = %i[name untracked paths reports when expire_in].freeze ALLOWED_KEYS = %i[name untracked paths reports when expire_in expose_as].freeze
EXPOSE_AS_REGEX = /\A\w[-\w ]*\z/.freeze
EXPOSE_AS_ERROR_MESSAGE = "can contain only letters, digits, '-', '_' and spaces"
   
attributes ALLOWED_KEYS attributes ALLOWED_KEYS
   
...@@ -21,11 +23,18 @@ module Gitlab ...@@ -21,11 +23,18 @@ module Gitlab
validations do validations do
validates :config, type: Hash validates :config, type: Hash
validates :config, allowed_keys: ALLOWED_KEYS validates :config, allowed_keys: ALLOWED_KEYS
validates :paths, presence: true, if: :expose_as_present?
   
with_options allow_nil: true do with_options allow_nil: true do
validates :name, type: String validates :name, type: String
validates :untracked, boolean: true validates :untracked, boolean: true
validates :paths, array_of_strings: true validates :paths, array_of_strings: true
validates :paths, array_of_strings: {
with: /\A[^*]*\z/,
message: "can't contain '*' when used with 'expose_as'"
}, if: :expose_as_present?
validates :expose_as, type: String, length: { maximum: 100 }, if: :expose_as_present?
validates :expose_as, format: { with: EXPOSE_AS_REGEX, message: EXPOSE_AS_ERROR_MESSAGE }, if: :expose_as_present?
validates :reports, type: Hash validates :reports, type: Hash
validates :when, validates :when,
inclusion: { in: %w[on_success on_failure always], inclusion: { in: %w[on_success on_failure always],
...@@ -41,6 +50,12 @@ module Gitlab ...@@ -41,6 +50,12 @@ module Gitlab
@config[:reports] = reports_value if @config.key?(:reports) @config[:reports] = reports_value if @config.key?(:reports)
@config @config
end end
def expose_as_present?
return false unless Feature.enabled?(:ci_expose_arbitrary_artifacts_in_mr, default_enabled: true)
!@config[:expose_as].nil?
end
end end
end end
end end
......
...@@ -61,8 +61,15 @@ module Gitlab ...@@ -61,8 +61,15 @@ module Gitlab
include LegacyValidationHelpers include LegacyValidationHelpers
   
def validate_each(record, attribute, value) def validate_each(record, attribute, value)
unless validate_array_of_strings(value) valid = validate_array_of_strings(value)
record.errors.add(attribute, 'should be an array of strings')
record.errors.add(attribute, 'should be an array of strings') unless valid
if valid && options[:with]
unless value.all? { |v| v =~ options[:with] }
message = options[:message] || 'contains elements that do not match the format'
record.errors.add(attribute, message)
end
end end
end end
end end
......
...@@ -1515,6 +1515,9 @@ msgstr "" ...@@ -1515,6 +1515,9 @@ msgstr ""
msgid "An error occurred while fetching environments." msgid "An error occurred while fetching environments."
msgstr "" msgstr ""
   
msgid "An error occurred while fetching exposed artifacts."
msgstr ""
msgid "An error occurred while fetching folder content." msgid "An error occurred while fetching folder content."
msgstr "" msgstr ""
   
......
...@@ -4,6 +4,7 @@ require 'spec_helper' ...@@ -4,6 +4,7 @@ require 'spec_helper'
   
describe Projects::MergeRequestsController do describe Projects::MergeRequestsController do
include ProjectForksHelper include ProjectForksHelper
include Gitlab::Routing
   
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:user) { project.owner } let(:user) { project.owner }
...@@ -206,7 +207,7 @@ describe Projects::MergeRequestsController do ...@@ -206,7 +207,7 @@ describe Projects::MergeRequestsController do
it 'redirects to last_page if page number is larger than number of pages' do it 'redirects to last_page if page number is larger than number of pages' do
get_merge_requests(last_page + 1) get_merge_requests(last_page + 1)
   
expect(response).to redirect_to(namespace_project_merge_requests_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope])) expect(response).to redirect_to(project_merge_requests_path(project, page: last_page, state: controller.params[:state], scope: controller.params[:scope]))
end end
   
it 'redirects to specified page' do it 'redirects to specified page' do
...@@ -227,7 +228,7 @@ describe Projects::MergeRequestsController do ...@@ -227,7 +228,7 @@ describe Projects::MergeRequestsController do
host: external_host host: external_host
} }
   
expect(response).to redirect_to(namespace_project_merge_requests_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope])) expect(response).to redirect_to(project_merge_requests_path(project, page: last_page, state: controller.params[:state], scope: controller.params[:scope]))
end end
end end
   
...@@ -770,6 +771,189 @@ describe Projects::MergeRequestsController do ...@@ -770,6 +771,189 @@ describe Projects::MergeRequestsController do
end end
end end
   
describe 'GET exposed_artifacts' do
let(:merge_request) do
create(:merge_request,
:with_merge_request_pipeline,
target_project: project,
source_project: project)
end
let(:pipeline) do
create(:ci_pipeline,
:success,
project: merge_request.source_project,
ref: merge_request.source_branch,
sha: merge_request.diff_head_sha)
end
let!(:job) { create(:ci_build, pipeline: pipeline, options: job_options) }
let!(:job_metadata) { create(:ci_job_artifact, :metadata, job: job) }
before do
allow_any_instance_of(MergeRequest)
.to receive(:find_exposed_artifacts)
.and_return(report)
allow_any_instance_of(MergeRequest)
.to receive(:actual_head_pipeline)
.and_return(pipeline)
end
subject do
get :exposed_artifacts, params: {
namespace_id: project.namespace.to_param,
project_id: project,
id: merge_request.iid
},
format: :json
end
describe 'permissions on a public project with private CI/CD' do
let(:project) { create :project, :repository, :public, :builds_private }
let(:report) { { status: :parsed, data: [] } }
let(:job_options) { {} }
context 'while signed out' do
before do
sign_out(user)
end
it 'responds with a 404' do
subject
expect(response).to have_gitlab_http_status(404)
expect(response.body).to be_blank
end