diff options
| -rw-r--r-- | app/models/project_services/bamboo_service.rb | 32 | ||||
| -rw-r--r-- | app/views/projects/deploy_keys/_index.html.haml | 2 | ||||
| -rw-r--r-- | changelogs/unreleased/24680-support-bamboo-api-polymorphism.yml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/an-opentracing-render-tracing.yml | 5 | ||||
| -rw-r--r-- | config/initializers/tracing.rb | 1 | ||||
| -rw-r--r-- | doc/user/permissions.md | 7 | ||||
| -rw-r--r-- | lib/gitlab/tracing/rails/action_view_subscriber.rb | 75 | ||||
| -rw-r--r-- | lib/gitlab/tracing/rails/active_record_subscriber.rb | 27 | ||||
| -rw-r--r-- | lib/gitlab/tracing/rails/rails_common.rb | 24 | ||||
| -rw-r--r-- | spec/lib/gitlab/tracing/rails/action_view_subscriber_spec.rb | 147 | ||||
| -rw-r--r-- | spec/lib/gitlab/tracing/rails/active_record_subscriber_spec.rb | 8 | ||||
| -rw-r--r-- | spec/models/project_services/bamboo_service_spec.rb | 26 |
12 files changed, 332 insertions, 27 deletions
diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb index a252052200a..71f5607dbdb 100644 --- a/app/models/project_services/bamboo_service.rb +++ b/app/models/project_services/bamboo_service.rb @@ -80,19 +80,27 @@ class BambooService < CiService private - def get_build_result_index - # When Bamboo returns multiple results for a given changeset, arbitrarily assume the most relevant result to be the last one. - -1 + def get_build_result(response) + return if response.code != 200 + + # May be nil if no result, a single result hash, or an array if multiple results for a given changeset. + result = response.dig('results', 'results', 'result') + + # In case of multiple results, arbitrarily assume the last one is the most relevant. + return result.last if result.is_a?(Array) + + result end def read_build_page(response) + result = get_build_result(response) key = - if response.code != 200 || response.dig('results', 'results', 'size') == '0' + if result.blank? # If actual build link can't be determined, send user to build summary page. build_key else # If actual build link is available, go to build result page. - response.dig('results', 'results', 'result', get_build_result_index, 'planResultKey', 'key') + result.dig('planResultKey', 'key') end build_url("browse/#{key}") @@ -101,11 +109,15 @@ class BambooService < CiService def read_commit_status(response) return :error unless response.code == 200 || response.code == 404 - status = if response.code == 404 || response.dig('results', 'results', 'size') == '0' - 'Pending' - else - response.dig('results', 'results', 'result', get_build_result_index, 'buildState') - end + result = get_build_result(response) + status = + if result.blank? + 'Pending' + else + result.dig('buildState') + end + + return :error unless status.present? if status.include?('Success') 'success' diff --git a/app/views/projects/deploy_keys/_index.html.haml b/app/views/projects/deploy_keys/_index.html.haml index 062aa423bde..24d665761cc 100644 --- a/app/views/projects/deploy_keys/_index.html.haml +++ b/app/views/projects/deploy_keys/_index.html.haml @@ -3,7 +3,7 @@ .settings-header %h4 Deploy Keys - %button.btn.js-settings-toggle.qa-expand-deploy-keys{ type: 'button' } + %button.btn.js-settings-toggle{ type: 'button' } = expanded ? 'Collapse' : 'Expand' %p Deploy keys allow read-only or read-write (if enabled) access to your repository. Deploy keys can be used for CI, staging or production servers. You can create a deploy key or add an existing one. diff --git a/changelogs/unreleased/24680-support-bamboo-api-polymorphism.yml b/changelogs/unreleased/24680-support-bamboo-api-polymorphism.yml new file mode 100644 index 00000000000..5117195cd0c --- /dev/null +++ b/changelogs/unreleased/24680-support-bamboo-api-polymorphism.yml @@ -0,0 +1,5 @@ +--- +title: "Support bamboo api polymorphism" +merge_request: 24680 +author: Alex Lossent +type: fixed
\ No newline at end of file diff --git a/changelogs/unreleased/an-opentracing-render-tracing.yml b/changelogs/unreleased/an-opentracing-render-tracing.yml new file mode 100644 index 00000000000..6ff7f1f3cf2 --- /dev/null +++ b/changelogs/unreleased/an-opentracing-render-tracing.yml @@ -0,0 +1,5 @@ +--- +title: Add OpenTracing instrumentation for Action View Render events +merge_request: 24728 +author: +type: other diff --git a/config/initializers/tracing.rb b/config/initializers/tracing.rb index d5bef8edb43..ddd91150c90 100644 --- a/config/initializers/tracing.rb +++ b/config/initializers/tracing.rb @@ -25,6 +25,7 @@ if Gitlab::Tracing.enabled? # Instrument Rails Gitlab::Tracing::Rails::ActiveRecordSubscriber.instrument + Gitlab::Tracing::Rails::ActionViewSubscriber.instrument # In multi-processed clustered architectures (puma, unicorn) don't # start tracing until the worker processes are spawned. This works diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 0c358390046..019652b2408 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -159,6 +159,13 @@ Confidential issues can be accessed by reporters and higher permission levels, as well as by guest users that create a confidential issue. To learn more, read through the documentation on [permissions and access to confidential issues](project/issues/confidential_issues.md#permissions-and-access-to-confidential-issues). +### Releases permissions + +[Project Releases](project/releases/index.md) can be read by all project +members (Reporters, Developers, Maintainers, Owners) **except Guests**. +Releases can be created, updated, or deleted via [Releases APIs](../api/releases/index.md) +by project Developers, Maintainers, and Owners. + ## Group members permissions NOTE: **Note:** diff --git a/lib/gitlab/tracing/rails/action_view_subscriber.rb b/lib/gitlab/tracing/rails/action_view_subscriber.rb new file mode 100644 index 00000000000..88816e1fb32 --- /dev/null +++ b/lib/gitlab/tracing/rails/action_view_subscriber.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +module Gitlab + module Tracing + module Rails + class ActionViewSubscriber + include RailsCommon + + COMPONENT_TAG = 'ActionView' + RENDER_TEMPLATE_NOTIFICATION_TOPIC = 'render_template.action_view' + RENDER_COLLECTION_NOTIFICATION_TOPIC = 'render_collection.action_view' + RENDER_PARTIAL_NOTIFICATION_TOPIC = 'render_partial.action_view' + + # Instruments Rails ActionView events for opentracing. + # Returns a lambda, which, when called will unsubscribe from the notifications + def self.instrument + subscriber = new + + subscriptions = [ + ActiveSupport::Notifications.subscribe(RENDER_TEMPLATE_NOTIFICATION_TOPIC) do |_, start, finish, _, payload| + subscriber.notify_render_template(start, finish, payload) + end, + ActiveSupport::Notifications.subscribe(RENDER_COLLECTION_NOTIFICATION_TOPIC) do |_, start, finish, _, payload| + subscriber.notify_render_collection(start, finish, payload) + end, + ActiveSupport::Notifications.subscribe(RENDER_PARTIAL_NOTIFICATION_TOPIC) do |_, start, finish, _, payload| + subscriber.notify_render_partial(start, finish, payload) + end + ] + + create_unsubscriber subscriptions + end + + # For more information on the payloads: https://guides.rubyonrails.org/active_support_instrumentation.html + def notify_render_template(start, finish, payload) + generate_span_for_notification("render_template", start, finish, payload, tags_for_render_template(payload)) + end + + def notify_render_collection(start, finish, payload) + generate_span_for_notification("render_collection", start, finish, payload, tags_for_render_collection(payload)) + end + + def notify_render_partial(start, finish, payload) + generate_span_for_notification("render_partial", start, finish, payload, tags_for_render_partial(payload)) + end + + private + + def tags_for_render_template(payload) + { + 'component' => COMPONENT_TAG, + 'template.id' => payload[:identifier], + 'template.layout' => payload[:layout] + } + end + + def tags_for_render_collection(payload) + { + 'component' => COMPONENT_TAG, + 'template.id' => payload[:identifier], + 'template.count' => payload[:count] || 0, + 'template.cache.hits' => payload[:cache_hits] || 0 + } + end + + def tags_for_render_partial(payload) + { + 'component' => COMPONENT_TAG, + 'template.id' => payload[:identifier] + } + end + end + end + end +end diff --git a/lib/gitlab/tracing/rails/active_record_subscriber.rb b/lib/gitlab/tracing/rails/active_record_subscriber.rb index 214eac47e14..32f5658e57e 100644 --- a/lib/gitlab/tracing/rails/active_record_subscriber.rb +++ b/lib/gitlab/tracing/rails/active_record_subscriber.rb @@ -4,24 +4,37 @@ module Gitlab module Tracing module Rails class ActiveRecordSubscriber - include Gitlab::Tracing::Common + include RailsCommon ACTIVE_RECORD_NOTIFICATION_TOPIC = 'sql.active_record' - DEFAULT_OPERATION_NAME = "sqlquery" + OPERATION_NAME_PREFIX = 'active_record:' + DEFAULT_OPERATION_NAME = 'sqlquery' + # Instruments Rails ActiveRecord events for opentracing. + # Returns a lambda, which, when called will unsubscribe from the notifications def self.instrument subscriber = new - ActiveSupport::Notifications.subscribe(ACTIVE_RECORD_NOTIFICATION_TOPIC) do |_, start, finish, _, payload| + subscription = ActiveSupport::Notifications.subscribe(ACTIVE_RECORD_NOTIFICATION_TOPIC) do |_, start, finish, _, payload| subscriber.notify(start, finish, payload) end + + create_unsubscriber [subscription] end # For more information on the payloads: https://guides.rubyonrails.org/active_support_instrumentation.html def notify(start, finish, payload) - operation_name = payload[:name].presence || DEFAULT_OPERATION_NAME - exception = payload[:exception] - tags = { + generate_span_for_notification(notification_name(payload), start, finish, payload, tags_for_notification(payload)) + end + + private + + def notification_name(payload) + OPERATION_NAME_PREFIX + (payload[:name].presence || DEFAULT_OPERATION_NAME) + end + + def tags_for_notification(payload) + { 'component' => 'ActiveRecord', 'span.kind' => 'client', 'db.type' => 'sql', @@ -29,8 +42,6 @@ module Gitlab 'db.cached' => payload[:cached] || false, 'db.statement' => payload[:sql] } - - postnotify_span("active_record:#{operation_name}", start, finish, tags: tags, exception: exception) end end end diff --git a/lib/gitlab/tracing/rails/rails_common.rb b/lib/gitlab/tracing/rails/rails_common.rb new file mode 100644 index 00000000000..88e914f62f8 --- /dev/null +++ b/lib/gitlab/tracing/rails/rails_common.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Gitlab + module Tracing + module Rails + module RailsCommon + extend ActiveSupport::Concern + include Gitlab::Tracing::Common + + class_methods do + def create_unsubscriber(subscriptions) + -> { subscriptions.each { |subscriber| ActiveSupport::Notifications.unsubscribe(subscriber) } } + end + end + + def generate_span_for_notification(operation_name, start, finish, payload, tags) + exception = payload[:exception] + + postnotify_span(operation_name, start, finish, tags: tags, exception: exception) + end + end + end + end +end diff --git a/spec/lib/gitlab/tracing/rails/action_view_subscriber_spec.rb b/spec/lib/gitlab/tracing/rails/action_view_subscriber_spec.rb new file mode 100644 index 00000000000..c9d1a06b3e6 --- /dev/null +++ b/spec/lib/gitlab/tracing/rails/action_view_subscriber_spec.rb @@ -0,0 +1,147 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'rspec-parameterized' + +describe Gitlab::Tracing::Rails::ActionViewSubscriber do + using RSpec::Parameterized::TableSyntax + + shared_examples 'an actionview notification' do + it 'should notify the tracer when the hash contains null values' do + expect(subject).to receive(:postnotify_span).with(notification_name, start, finish, tags: expected_tags, exception: exception) + + subject.public_send(notify_method, start, finish, payload) + end + + it 'should notify the tracer when the payload is missing values' do + expect(subject).to receive(:postnotify_span).with(notification_name, start, finish, tags: expected_tags, exception: exception) + + subject.public_send(notify_method, start, finish, payload.compact) + end + + it 'should not throw exceptions when with the default tracer' do + expect { subject.public_send(notify_method, start, finish, payload) }.not_to raise_error + end + end + + describe '.instrument' do + it 'is unsubscribeable' do + unsubscribe = described_class.instrument + + expect(unsubscribe).not_to be_nil + expect { unsubscribe.call }.not_to raise_error + end + end + + describe '#notify_render_template' do + subject { described_class.new } + let(:start) { Time.now } + let(:finish) { Time.now } + let(:notification_name) { 'render_template' } + let(:notify_method) { :notify_render_template } + + where(:identifier, :layout, :exception) do + nil | nil | nil + "" | nil | nil + "show.haml" | nil | nil + nil | "" | nil + nil | "layout.haml" | nil + nil | nil | StandardError.new + end + + with_them do + let(:payload) do + { + exception: exception, + identifier: identifier, + layout: layout + } + end + + let(:expected_tags) do + { + 'component' => 'ActionView', + 'template.id' => identifier, + 'template.layout' => layout + } + end + + it_behaves_like 'an actionview notification' + end + end + + describe '#notify_render_collection' do + subject { described_class.new } + let(:start) { Time.now } + let(:finish) { Time.now } + let(:notification_name) { 'render_collection' } + let(:notify_method) { :notify_render_collection } + + where( + :identifier, :count, :expected_count, :cache_hits, :expected_cache_hits, :exception) do + nil | nil | 0 | nil | 0 | nil + "" | nil | 0 | nil | 0 | nil + "show.haml" | nil | 0 | nil | 0 | nil + nil | 0 | 0 | nil | 0 | nil + nil | 1 | 1 | nil | 0 | nil + nil | nil | 0 | 0 | 0 | nil + nil | nil | 0 | 1 | 1 | nil + nil | nil | 0 | nil | 0 | StandardError.new + end + + with_them do + let(:payload) do + { + exception: exception, + identifier: identifier, + count: count, + cache_hits: cache_hits + } + end + + let(:expected_tags) do + { + 'component' => 'ActionView', + 'template.id' => identifier, + 'template.count' => expected_count, + 'template.cache.hits' => expected_cache_hits + } + end + + it_behaves_like 'an actionview notification' + end + end + + describe '#notify_render_partial' do + subject { described_class.new } + let(:start) { Time.now } + let(:finish) { Time.now } + let(:notification_name) { 'render_partial' } + let(:notify_method) { :notify_render_partial } + + where(:identifier, :exception) do + nil | nil + "" | nil + "show.haml" | nil + nil | StandardError.new + end + + with_them do + let(:payload) do + { + exception: exception, + identifier: identifier + } + end + + let(:expected_tags) do + { + 'component' => 'ActionView', + 'template.id' => identifier + } + end + + it_behaves_like 'an actionview notification' + end + end +end diff --git a/spec/lib/gitlab/tracing/rails/active_record_subscriber_spec.rb b/spec/lib/gitlab/tracing/rails/active_record_subscriber_spec.rb index 5eb5c044f84..3d066843148 100644 --- a/spec/lib/gitlab/tracing/rails/active_record_subscriber_spec.rb +++ b/spec/lib/gitlab/tracing/rails/active_record_subscriber_spec.rb @@ -7,11 +7,11 @@ describe Gitlab::Tracing::Rails::ActiveRecordSubscriber do using RSpec::Parameterized::TableSyntax describe '.instrument' do - it 'is unsubscribable' do - subscription = described_class.instrument + it 'is unsubscribeable' do + unsubscribe = described_class.instrument - expect(subscription).not_to be_nil - expect { ActiveSupport::Notifications.unsubscribe(subscription) }.not_to raise_error + expect(unsubscribe).not_to be_nil + expect { unsubscribe.call }.not_to raise_error end end diff --git a/spec/models/project_services/bamboo_service_spec.rb b/spec/models/project_services/bamboo_service_spec.rb index ee84fa95f0e..b880d90d28f 100644 --- a/spec/models/project_services/bamboo_service_spec.rb +++ b/spec/models/project_services/bamboo_service_spec.rb @@ -144,7 +144,7 @@ describe BambooService, :use_clean_rails_memory_store_caching do end end - describe '#calculate_reactive_cache' do + shared_examples 'reactive cache calculation' do context '#build_page' do subject { service.calculate_reactive_cache('123', 'unused')[:build_page] } @@ -155,7 +155,7 @@ describe BambooService, :use_clean_rails_memory_store_caching do end it 'returns a specific URL when response has no results' do - stub_request(body: bamboo_response(size: 0)) + stub_request(body: %q({"results":{"results":{"size":"0"}}})) is_expected.to eq('http://gitlab.com/bamboo/browse/foo') end @@ -224,6 +224,24 @@ describe BambooService, :use_clean_rails_memory_store_caching do end end + describe '#calculate_reactive_cache' do + context 'when Bamboo API returns single result' do + let(:bamboo_response_template) do + %q({"results":{"results":{"size":"1","result":{"buildState":"%{build_state}","planResultKey":{"key":"42"}}}}}) + end + + it_behaves_like 'reactive cache calculation' + end + + context 'when Bamboo API returns an array of results and we only consider the last one' do + let(:bamboo_response_template) do + %q({"results":{"results":{"size":"2","result":[{"buildState":"%{build_state}","planResultKey":{"key":"41"}},{"buildState":"%{build_state}","planResultKey":{"key":"42"}}]}}}) + end + + it_behaves_like 'reactive cache calculation' + end + end + def stub_update_and_build_request(status: 200, body: nil) bamboo_full_url = 'http://gitlab.com/bamboo/updateAndBuild.action?buildKey=foo&os_authType=basic' @@ -244,8 +262,8 @@ describe BambooService, :use_clean_rails_memory_store_caching do ).with(basic_auth: %w(mic password)) end - def bamboo_response(result_key: 42, build_state: 'success', size: 1) + def bamboo_response(build_state: 'success') # reference: https://docs.atlassian.com/atlassian-bamboo/REST/6.2.5/#d2e786 - %Q({"results":{"results":{"size":"#{size}","result":[{"buildState":"#{build_state}","planResultKey":{"key":"#{result_key}"}}]}}}) + bamboo_response_template % { build_state: build_state } end end |