From 91c58ed14d13dcefd7ca2834d6157a9fa630a9af Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 31 May 2017 15:59:01 +0200 Subject: FormHelper#issue_assignees_dropdown_options never has multiple assignees Only EE supports multiple issue assignees, so this CE code should not contain code to have multiple assignees. EE will override the multiple issue assignees feature by overriding this method. --- app/helpers/form_helper.rb | 16 ++++------------ .../issuable/form/_metadata_issue_assignee.html.haml | 2 +- spec/features/issues/form_spec.rb | 4 ++-- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/app/helpers/form_helper.rb b/app/helpers/form_helper.rb index 014fc46b130..729bc4bf329 100644 --- a/app/helpers/form_helper.rb +++ b/app/helpers/form_helper.rb @@ -16,8 +16,8 @@ module FormHelper end end - def issue_dropdown_options(issuable, has_multiple_assignees = true) - options = { + def issue_assignees_dropdown_options + { toggle_class: 'js-user-search js-assignee-search js-multiselect js-save-user-data', title: 'Select assignee', filter: true, @@ -27,8 +27,8 @@ module FormHelper first_user: current_user&.username, null_user: true, current_user: true, - project_id: issuable.project.try(:id), - field_name: "#{issuable.class.model_name.param_key}[assignee_ids][]", + project_id: @project.id, + field_name: 'issue[assignee_ids][]', default_label: 'Unassigned', 'max-select': 1, 'dropdown-header': 'Assignee', @@ -38,13 +38,5 @@ module FormHelper current_user_info: current_user.to_json(only: [:id, :name]) } } - - if has_multiple_assignees - options[:title] = 'Select assignee(s)' - options[:data][:'dropdown-header'] = 'Assignee(s)' - options[:data].delete(:'max-select') - end - - options end end diff --git a/app/views/shared/issuable/form/_metadata_issue_assignee.html.haml b/app/views/shared/issuable/form/_metadata_issue_assignee.html.haml index 77175c839a6..567cde764e2 100644 --- a/app/views/shared/issuable/form/_metadata_issue_assignee.html.haml +++ b/app/views/shared/issuable/form/_metadata_issue_assignee.html.haml @@ -7,5 +7,5 @@ - if issuable.assignees.length === 0 = hidden_field_tag "#{issuable.to_ability_name}[assignee_ids][]", 0, id: nil, data: { meta: '' } - = dropdown_tag(users_dropdown_label(issuable.assignees), options: issue_dropdown_options(issuable,false)) + = dropdown_tag(users_dropdown_label(issuable.assignees), options: issue_assignees_dropdown_options) = link_to 'Assign to me', '#', class: "assign-to-me-link #{'hide' if issuable.assignees.include?(current_user)}" diff --git a/spec/features/issues/form_spec.rb b/spec/features/issues/form_spec.rb index b369ef1ff79..58f6bd277e4 100644 --- a/spec/features/issues/form_spec.rb +++ b/spec/features/issues/form_spec.rb @@ -31,8 +31,8 @@ describe 'New/edit issue', :feature, :js do # the original method, resulting in infinite recurison when called. # This is likely a bug with helper modules included into dynamically generated view classes. # To work around this, we have to hold on to and call to the original implementation manually. - original_issue_dropdown_options = FormHelper.instance_method(:issue_dropdown_options) - allow_any_instance_of(FormHelper).to receive(:issue_dropdown_options).and_wrap_original do |original, *args| + original_issue_dropdown_options = FormHelper.instance_method(:issue_assignees_dropdown_options) + allow_any_instance_of(FormHelper).to receive(:issue_assignees_dropdown_options).and_wrap_original do |original, *args| options = original_issue_dropdown_options.bind(original.receiver).call(*args) options[:data][:per_page] = 2 -- cgit v1.2.1 From 4c2ca4c38d728af65e3608bb288ebcaaba426ad8 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 7 Jun 2017 16:20:20 +0200 Subject: [noop] Remove unused code To make the code back in line with EE. [ci skip] --- app/assets/javascripts/users_select.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/assets/javascripts/users_select.js b/app/assets/javascripts/users_select.js index ec45253e50b..828079cc03a 100644 --- a/app/assets/javascripts/users_select.js +++ b/app/assets/javascripts/users_select.js @@ -206,8 +206,6 @@ function UsersSelect(currentUser, els) { return $dropdown.glDropdown({ showMenuAbove: showMenuAbove, data: function(term, callback) { - var isAuthorFilter; - isAuthorFilter = $('.js-author-search'); return _this.users(term, options, function(users) { // GitLabDropdownFilter returns this.instance // GitLabDropdownRemote returns this.options.instance -- cgit v1.2.1 From fd50d9ab8ec977183240b28d749cf2eb25014f41 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 7 Jun 2017 14:48:55 +0200 Subject: Use FormHelper#issue_assignees_dropdown_options for Issue sidebar Avoid code duplication and limit the number of CE -> EE merge conflict by reusing `FormHelper#issue_assignees_dropdown_options` to set some assignee dropdown attributes. --- app/views/projects/boards/components/sidebar/_assignee.html.haml | 5 +++-- app/views/shared/issuable/_sidebar_assignees.html.haml | 9 +++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/views/projects/boards/components/sidebar/_assignee.html.haml b/app/views/projects/boards/components/sidebar/_assignee.html.haml index e8db868f49b..c314573bdea 100644 --- a/app/views/projects/boards/components/sidebar/_assignee.html.haml +++ b/app/views/projects/boards/components/sidebar/_assignee.html.haml @@ -19,10 +19,11 @@ ":data-name" => "assignee.name", ":data-username" => "assignee.username" } .dropdown - %button.dropdown-menu-toggle.js-user-search.js-author-search.js-multiselect.js-save-user-data.js-issue-board-sidebar{ type: "button", ref: "assigneeDropdown", data: { toggle: "dropdown", field_name: "issue[assignee_ids][]", first_user: (current_user.username if current_user), current_user: "true", project_id: @project.id, null_user: "true", multi_select: "true", 'max-select' => 1, dropdown: { header: 'Assignee' } }, + - dropdown_options = issue_assignees_dropdown_options + %button.dropdown-menu-toggle.js-user-search.js-author-search.js-multiselect.js-save-user-data.js-issue-board-sidebar{ type: 'button', ref: 'assigneeDropdown', data: { toggle: 'dropdown', field_name: 'issue[assignee_ids][]', first_user: current_user&.username, current_user: 'true', project_id: @project.id, null_user: 'true', multi_select: 'true', 'dropdown-header': dropdown_options[:data][:'dropdown-header'], 'max-select': dropdown_options[:data][:'max-select'] }, ":data-issuable-id" => "issue.id", ":data-issue-update" => "'#{namespace_project_issues_path(@project.namespace, @project)}/' + issue.id + '.json'" } - Select assignee + = dropdown_options[:title] = icon("chevron-down") .dropdown-menu.dropdown-select.dropdown-menu-user.dropdown-menu-selectable.dropdown-menu-author = dropdown_title("Assign to") diff --git a/app/views/shared/issuable/_sidebar_assignees.html.haml b/app/views/shared/issuable/_sidebar_assignees.html.haml index bcfa1dc826e..3513f2de371 100644 --- a/app/views/shared/issuable/_sidebar_assignees.html.haml +++ b/app/views/shared/issuable/_sidebar_assignees.html.haml @@ -34,19 +34,20 @@ - issuable.assignees.each do |assignee| = hidden_field_tag "#{issuable.to_ability_name}[assignee_ids][]", assignee.id, id: nil, data: { avatar_url: assignee.avatar_url, name: assignee.name, username: assignee.username } - - options = { toggle_class: 'js-user-search js-author-search', title: 'Assign to', filter: true, dropdown_class: 'dropdown-menu-user dropdown-menu-selectable dropdown-menu-author', placeholder: 'Search users', data: { first_user: (current_user.username if current_user), current_user: true, project_id: (@project.id if @project), author_id: issuable.author_id, field_name: "#{issuable.to_ability_name}[assignee_ids][]", issue_update: issuable_json_path(issuable), ability_name: issuable.to_ability_name, null_user: true } } - + - options = { toggle_class: 'js-user-search js-author-search', title: 'Assign to', filter: true, dropdown_class: 'dropdown-menu-user dropdown-menu-selectable dropdown-menu-author', placeholder: 'Search users', data: { first_user: current_user&.username, current_user: true, project_id: @project&.id, author_id: issuable.author_id, field_name: "#{issuable.to_ability_name}[assignee_ids][]", issue_update: issuable_json_path(issuable), ability_name: issuable.to_ability_name, null_user: true } } - title = 'Select assignee' - if issuable.is_a?(Issue) - unless issuable.assignees.any? = hidden_field_tag "#{issuable.to_ability_name}[assignee_ids][]", 0, id: nil + - dropdown_options = issue_assignees_dropdown_options + - title = dropdown_options[:title] - options[:toggle_class] += ' js-multiselect js-save-user-data' - data = { field_name: "#{issuable.to_ability_name}[assignee_ids][]" } - data[:multi_select] = true - data['dropdown-title'] = title - - data['dropdown-header'] = 'Assignee' - - data['max-select'] = 1 + - data['dropdown-header'] = dropdown_options[:data][:'dropdown-header'] + - data['max-select'] = dropdown_options[:data][:'max-select'] - options[:data].merge!(data) = dropdown_tag(title, options: options) -- cgit v1.2.1 From a8d4bf9724ce6dce69240d4953c7d38b7f05a7dd Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Thu, 15 Jun 2017 16:01:12 +0200 Subject: Use helper method to set filtered search input attributes The list of attributes for the filtered search input was getting long, so use a helper method to fill that hash. Also, for multiple issue assignees, a helper is more convenient because it would allow EE to override the behavior if MIA is supported. --- app/helpers/search_helper.rb | 12 ++++++++++++ app/views/shared/issuable/_search_bar.html.haml | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/helpers/search_helper.rb b/app/helpers/search_helper.rb index 9c46035057f..5ea2c72f819 100644 --- a/app/helpers/search_helper.rb +++ b/app/helpers/search_helper.rb @@ -126,6 +126,18 @@ module SearchHelper search_path(options) end + def search_filter_input_options(type) + { + id: "filtered-search-#{type}", + placeholder: 'Search or filter results...', + data: { + 'project-id' => @project.id, + 'username-params' => @users.to_json(only: [:id, :username]), + 'base-endpoint' => namespace_project_path(@project.namespace, @project) + } + } + end + # Sanitize a HTML field for search display. Most tags are stripped out and the # maximum length is set to 200 characters. def search_md_sanitize(object, field) diff --git a/app/views/shared/issuable/_search_bar.html.haml b/app/views/shared/issuable/_search_bar.html.haml index d3d290692a2..ae890567225 100644 --- a/app/views/shared/issuable/_search_bar.html.haml +++ b/app/views/shared/issuable/_search_bar.html.haml @@ -23,7 +23,7 @@ .scroll-container %ul.tokens-container.list-unstyled %li.input-token - %input.form-control.filtered-search{ id: "filtered-search-#{type.to_s}", placeholder: 'Search or filter results...', data: { 'project-id' => @project.id, 'username-params' => @users.to_json(only: [:id, :username]), 'base-endpoint' => namespace_project_path(@project.namespace, @project) } } + %input.form-control.filtered-search{ search_filter_input_options(type) } = icon('filter') #js-dropdown-hint.filtered-search-input-dropdown-menu.dropdown-menu.hint-dropdown %ul{ data: { dropdown: true } } -- cgit v1.2.1 From 132cd0092d6aea87359a9a0627ad2c53c4a91837 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 14 Jun 2017 22:08:24 +0200 Subject: Backport issuable for assignee slash commands from EE Avoid conflicts when merge CE to EE by backporting code from EE. Instead of checking in `SlashCommands::InterpretService` what the issuable the type of the issuable is, ask the issuable if it is capable to do those thing and implement it in the issuable itself. The issuable will check if it's possible and if the licensed feature is available. This should also make it easier to ever add multiple assignees to MergeRequests. --- app/models/concerns/issuable.rb | 12 ++++++++ app/services/quick_actions/interpret_service.rb | 38 +++++++++++++++++++------ 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 8e367576c9d..0a476efdaa9 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -102,6 +102,18 @@ module Issuable def locking_enabled? title_changed? || description_changed? end + + def allows_multiple_assignees? + false + end + + def has_multiple_assignees? + supports_multiple_assignees? && assignees.count > 1 + end + + def supports_multiple_assignees? + respond_to?(:assignee_ids) + end end module ClassMethods diff --git a/app/services/quick_actions/interpret_service.rb b/app/services/quick_actions/interpret_service.rb index 6816b137361..8adfd939c2e 100644 --- a/app/services/quick_actions/interpret_service.rb +++ b/app/services/quick_actions/interpret_service.rb @@ -92,9 +92,12 @@ module QuickActions desc 'Assign' explanation do |users| - "Assigns #{users.first.to_reference}." if users.any? + users = issuable.allows_multiple_assignees? ? users : users.take(1) + "Assigns #{users.map(&:to_reference).to_sentence}." + end + params do + issuable.allows_multiple_assignees? ? '@user1 @user2' : '@user' end - params '@user' condition do current_user.can?(:"admin_#{issuable.to_ability_name}", project) end @@ -104,25 +107,44 @@ module QuickActions command :assign do |users| next if users.empty? - if issuable.is_a?(Issue) + if issuable.allows_multiple_assignees? + @updates[:assignee_ids] = issuable.assignees.pluck(:id) + users.map(&:id) + elsif issuable.supports_multiple_assignees? @updates[:assignee_ids] = [users.last.id] else @updates[:assignee_id] = users.last.id end end - desc 'Remove assignee' + desc do + if issuable.allows_multiple_assignees? + 'Remove all or specific assignee(s)' + else + 'Remove assignee' + end + end explanation do - "Removes assignee #{issuable.assignees.first.to_reference}." + "Removes #{'assignee'.pluralize(issuable.assignees.size)} #{issuable.assignees.map(&:to_reference).to_sentence}." + end + params do + issuable.allows_multiple_assignees? ? '@user1 @user2' : '' end condition do issuable.persisted? && issuable.assignees.any? && current_user.can?(:"admin_#{issuable.to_ability_name}", project) end - command :unassign do - if issuable.is_a?(Issue) - @updates[:assignee_ids] = [] + command :unassign do |unassign_param = nil| + # When multiple users are assigned, all will be unassigned if multiple assignees are no longer allowed + users = extract_users(unassign_param) if issuable.allows_multiple_assignees? + + if issuable.supports_multiple_assignees? + @updates[:assignee_ids] = + if users&.any? + issuable.assignees.pluck(:id) - users.map(&:id) + else + [] + end else @updates[:assignee_id] = nil end -- cgit v1.2.1 From fcd46c1af4ceeec7813a91111dfce5e492695119 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 20 Jun 2017 15:03:25 +0200 Subject: Backport /reassign quick command The /reassign quick command works even when no multiple assignees are allowed of there isn't any assignee yet. So for consistency, it's also be backported to CE. But it functions the same as the /assign quick action. --- app/services/quick_actions/interpret_service.rb | 18 ++++++++++++++++++ spec/services/quick_actions/interpret_service_spec.rb | 18 +++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/app/services/quick_actions/interpret_service.rb b/app/services/quick_actions/interpret_service.rb index 8adfd939c2e..4ceabaf021e 100644 --- a/app/services/quick_actions/interpret_service.rb +++ b/app/services/quick_actions/interpret_service.rb @@ -150,6 +150,24 @@ module QuickActions end end + desc do + "Change assignee#{'(s)' if issuable.allows_multiple_assignees?}" + end + explanation do |users| + users = issuable.allows_multiple_assignees? ? users : users.take(1) + "Change #{'assignee'.pluralize(users.size)} to #{users.map(&:to_reference).to_sentence}." + end + params do + issuable.allows_multiple_assignees? ? '@user1 @user2' : '@user' + end + condition do + issuable.persisted? && + current_user.can?(:"admin_#{issuable.to_ability_name}", project) + end + command :reassign do |unassign_param| + @updates[:assignee_ids] = extract_users(unassign_param).map(&:id) + end + desc 'Set milestone' explanation do |milestone| "Sets the milestone to #{milestone.to_reference}." if milestone diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index c9e63efbc14..b1997e64557 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -362,7 +362,7 @@ describe QuickActions::InterpretService, services: true do it 'fetches assignee and populates assignee_id if content contains /assign' do _, updates = service.execute(content, issue) - expect(updates).to eq(assignee_ids: [developer.id]) + expect(updates[:assignee_ids]).to match_array([developer.id]) end end @@ -431,6 +431,22 @@ describe QuickActions::InterpretService, services: true do end end + context 'reassign command' do + let(:content) { '/reassign' } + + context 'Issue' do + it 'reassigns user if content contains /reassign @user' do + user = create(:user) + + issue.update(assignee_ids: [developer.id]) + + _, updates = service.execute("/reassign @#{user.username}", issue) + + expect(updates).to eq(assignee_ids: [user.id]) + end + end + end + it_behaves_like 'milestone command' do let(:content) { "/milestone %#{milestone.title}" } let(:issuable) { issue } -- cgit v1.2.1 From 451e25532ff43de8151b71ced8246f709c08bf92 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 20 Jun 2017 21:32:49 +0200 Subject: Make MergeRequest respond to assignee_ids & assignee_ids= To make it simpler to assign users to an Issuable, make MergeRequest support the attribute `assignee_ids`. --- app/models/concerns/issuable.rb | 6 +---- app/models/merge_request.rb | 10 +++++++- app/services/quick_actions/interpret_service.rb | 29 +++++++++------------- spec/models/merge_request_spec.rb | 16 ++++++++++++ .../quick_actions/interpret_service_spec.rb | 18 +++++++------- 5 files changed, 47 insertions(+), 32 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 0a476efdaa9..1bebd55a089 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -108,11 +108,7 @@ module Issuable end def has_multiple_assignees? - supports_multiple_assignees? && assignees.count > 1 - end - - def supports_multiple_assignees? - respond_to?(:assignee_ids) + assignees.count > 1 end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ea22ab53587..77da8413904 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -197,11 +197,19 @@ class MergeRequest < ActiveRecord::Base } end - # This method is needed for compatibility with issues to not mess view and other code + # These method are needed for compatibility with issues to not mess view and other code def assignees Array(assignee) end + def assignee_ids + Array(assignee_id) + end + + def assignee_ids=(ids) + write_attribute(:assignee_id, ids.last) + end + def assignee_or_author?(user) author_id == user.id || assignee_id == user.id end diff --git a/app/services/quick_actions/interpret_service.rb b/app/services/quick_actions/interpret_service.rb index 4ceabaf021e..df13976fb3b 100644 --- a/app/services/quick_actions/interpret_service.rb +++ b/app/services/quick_actions/interpret_service.rb @@ -107,13 +107,12 @@ module QuickActions command :assign do |users| next if users.empty? - if issuable.allows_multiple_assignees? - @updates[:assignee_ids] = issuable.assignees.pluck(:id) + users.map(&:id) - elsif issuable.supports_multiple_assignees? - @updates[:assignee_ids] = [users.last.id] - else - @updates[:assignee_id] = users.last.id - end + @updates[:assignee_ids] = + if issuable.allows_multiple_assignees? + issuable.assignees.pluck(:id) + users.map(&:id) + else + [users.last.id] + end end desc do @@ -138,16 +137,12 @@ module QuickActions # When multiple users are assigned, all will be unassigned if multiple assignees are no longer allowed users = extract_users(unassign_param) if issuable.allows_multiple_assignees? - if issuable.supports_multiple_assignees? - @updates[:assignee_ids] = - if users&.any? - issuable.assignees.pluck(:id) - users.map(&:id) - else - [] - end - else - @updates[:assignee_id] = nil - end + @updates[:assignee_ids] = + if users&.any? + issuable.assignees.pluck(:id) - users.map(&:id) + else + [] + end end desc do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index a56bc524a98..945189b922b 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -105,6 +105,22 @@ describe MergeRequest, models: true do end end + describe '#assignee_ids' do + it 'returns an array of the assigned user id' do + subject.assignee_id = 123 + + expect(subject.assignee_ids).to eq([123]) + end + end + + describe '#assignee_ids=' do + it 'sets assignee_id to the last id in the array' do + subject.assignee_ids = [123, 456] + + expect(subject.assignee_id).to eq(456) + end + end + describe '#assignee_or_author?' do let(:user) { create(:user) } diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index b1997e64557..35373675894 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -359,7 +359,7 @@ describe QuickActions::InterpretService, services: true do let(:content) { "/assign @#{developer.username}" } context 'Issue' do - it 'fetches assignee and populates assignee_id if content contains /assign' do + it 'fetches assignee and populates assignee_ids if content contains /assign' do _, updates = service.execute(content, issue) expect(updates[:assignee_ids]).to match_array([developer.id]) @@ -367,10 +367,10 @@ describe QuickActions::InterpretService, services: true do end context 'Merge Request' do - it 'fetches assignee and populates assignee_id if content contains /assign' do + it 'fetches assignee and populates assignee_ids if content contains /assign' do _, updates = service.execute(content, merge_request) - expect(updates).to eq(assignee_id: developer.id) + expect(updates).to eq(assignee_ids: [developer.id]) end end end @@ -383,7 +383,7 @@ describe QuickActions::InterpretService, services: true do end context 'Issue' do - it 'fetches assignee and populates assignee_id if content contains /assign' do + it 'fetches assignee and populates assignee_ids if content contains /assign' do _, updates = service.execute(content, issue) expect(updates[:assignee_ids]).to match_array([developer.id]) @@ -391,10 +391,10 @@ describe QuickActions::InterpretService, services: true do end context 'Merge Request' do - it 'fetches assignee and populates assignee_id if content contains /assign' do + it 'fetches assignee and populates assignee_ids if content contains /assign' do _, updates = service.execute(content, merge_request) - expect(updates).to eq(assignee_id: developer.id) + expect(updates).to eq(assignee_ids: [developer.id]) end end end @@ -422,11 +422,11 @@ describe QuickActions::InterpretService, services: true do end context 'Merge Request' do - it 'populates assignee_id: nil if content contains /unassign' do - merge_request.update(assignee_id: developer.id) + it 'populates assignee_ids: [] if content contains /unassign' do + merge_request.update(assignee_ids: [developer.id]) _, updates = service.execute(content, merge_request) - expect(updates).to eq(assignee_id: nil) + expect(updates).to eq(assignee_ids: []) end end end -- cgit v1.2.1 From 2194856fcf45c9556067a7c2fb6db677f9388c61 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 20 Jun 2017 22:02:41 +0200 Subject: Ensure /reassign does not assign multiple users Set the assignee to last user in the array if multiple assignees aren't allowed. Also, use `parse_params` where possible. --- app/services/quick_actions/interpret_service.rb | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/app/services/quick_actions/interpret_service.rb b/app/services/quick_actions/interpret_service.rb index df13976fb3b..e4dfe87e614 100644 --- a/app/services/quick_actions/interpret_service.rb +++ b/app/services/quick_actions/interpret_service.rb @@ -133,10 +133,11 @@ module QuickActions issuable.assignees.any? && current_user.can?(:"admin_#{issuable.to_ability_name}", project) end - command :unassign do |unassign_param = nil| + parse_params do |unassign_param| # When multiple users are assigned, all will be unassigned if multiple assignees are no longer allowed - users = extract_users(unassign_param) if issuable.allows_multiple_assignees? - + extract_users(unassign_param) if issuable.allows_multiple_assignees? + end + command :unassign do |users = nil| @updates[:assignee_ids] = if users&.any? issuable.assignees.pluck(:id) - users.map(&:id) @@ -159,8 +160,16 @@ module QuickActions issuable.persisted? && current_user.can?(:"admin_#{issuable.to_ability_name}", project) end - command :reassign do |unassign_param| - @updates[:assignee_ids] = extract_users(unassign_param).map(&:id) + parse_params do |assignee_param| + extract_users(assignee_param) + end + command :reassign do |users| + @updates[:assignee_ids] = + if issuable.allows_multiple_assignees? + users.map(&:id) + else + [users.last.id] + end end desc 'Set milestone' -- cgit v1.2.1 From 20f679d620380b5b5e662b790c76caf256867b01 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 26 Jun 2017 07:20:30 +0000 Subject: Allow unauthenticated access to the `/api/v4/users` API. - The issue filtering frontend code needs access to this API for non-logged-in users + public projects. It uses the API to fetch information for a user by username. - We don't authenticate this API anymore, but instead - if the `current_user` is not present: - Verify that the `username` parameter has been passed. This disallows an unauthenticated user from grabbing a list of all users on the instance. The `UsersFinder` class performs an exact match on the `username`, so we are guaranteed to get 0 or 1 users. - Verify that the resulting user (if any) is accessible to be viewed publicly by calling `can?(current_user, :read_user, user)` --- app/finders/users_finder.rb | 7 +++++-- lib/api/helpers.rb | 6 ++++++ lib/api/users.rb | 23 +++++++++++++++++------ spec/requests/api/users_spec.rb | 35 +++++++++++++++++++++++++++++++++-- 4 files changed, 61 insertions(+), 10 deletions(-) diff --git a/app/finders/users_finder.rb b/app/finders/users_finder.rb index dbd50d1db7c..0534317df8f 100644 --- a/app/finders/users_finder.rb +++ b/app/finders/users_finder.rb @@ -27,8 +27,11 @@ class UsersFinder users = by_search(users) users = by_blocked(users) users = by_active(users) - users = by_external_identity(users) - users = by_external(users) + + if current_user + users = by_external_identity(users) + users = by_external(users) + end users end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 2c73a6fdc4e..1322afaa64f 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -407,5 +407,11 @@ module API exception.status == 500 end + + # Does the current route match the route identified by + # `description`? + def route_matches_description?(description) + options.dig(:route_options, :description) == description + end end end diff --git a/lib/api/users.rb b/lib/api/users.rb index c10e3364382..34619c90d8b 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -4,7 +4,7 @@ module API before do allow_access_with_scope :read_user if request.get? - authenticate! + authenticate! unless route_matches_description?("Get the list of users") end resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do @@ -51,15 +51,26 @@ module API use :pagination end get do - unless can?(current_user, :read_users_list) - render_api_error!("Not authorized.", 403) - end - authenticated_as_admin! if params[:external].present? || (params[:extern_uid].present? && params[:provider].present?) users = UsersFinder.new(current_user, params).execute - entity = current_user.admin? ? Entities::UserWithAdmin : Entities::UserBasic + authorized = + if current_user + can?(current_user, :read_users_list) + else + # When `current_user` is not present, require that the `username` + # parameter is passed, to prevent an unauthenticated user from accessing + # a list of all the users on the GitLab instance. `UsersFinder` performs + # an exact match on the `username` parameter, so we are guaranteed to + # get either 0 or 1 `users` here. + params[:username].present? && + users.all? { |user| can?(current_user, :read_user, user) } + end + + render_api_error!("Not authorized.", 403) unless authorized + + entity = current_user.try(:admin?) ? Entities::UserWithAdmin : Entities::UserBasic present paginate(users), with: entity end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 18000d91795..01541901330 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -13,9 +13,40 @@ describe API::Users do describe 'GET /users' do context "when unauthenticated" do - it "returns authentication error" do + it "returns authorization error when the `username` parameter is not passed" do get api("/users") - expect(response).to have_http_status(401) + + expect(response).to have_http_status(403) + end + + it "returns the user when a valid `username` parameter is passed" do + user = create(:user) + + get api("/users"), username: user.username + + expect(response).to have_http_status(200) + expect(json_response).to be_an Array + expect(json_response.size).to eq(1) + expect(json_response[0]['id']).to eq(user.id) + expect(json_response[0]['username']).to eq(user.username) + end + + it "returns authorization error when the `username` parameter refers to an inaccessible user" do + user = create(:user) + + expect(Ability).to receive(:allowed?).with(nil, :read_user, user).and_return(false) + + get api("/users"), username: user.username + + expect(response).to have_http_status(403) + end + + it "returns an empty response when an invalid `username` parameter is passed" do + get api("/users"), username: 'invalid' + + expect(response).to have_http_status(200) + expect(json_response).to be_an Array + expect(json_response.size).to eq(0) end end -- cgit v1.2.1 From c39e4ccfb7cb76b9bdb613399aba2c2467b77751 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 26 Jun 2017 08:43:12 +0000 Subject: Add CHANGELOG entry for CE MR 12445 --- .../34141-allow-unauthenticated-access-to-the-users-api.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/34141-allow-unauthenticated-access-to-the-users-api.yml diff --git a/changelogs/unreleased/34141-allow-unauthenticated-access-to-the-users-api.yml b/changelogs/unreleased/34141-allow-unauthenticated-access-to-the-users-api.yml new file mode 100644 index 00000000000..a3ade8db214 --- /dev/null +++ b/changelogs/unreleased/34141-allow-unauthenticated-access-to-the-users-api.yml @@ -0,0 +1,4 @@ +--- +title: Allow unauthenticated access to the /api/v4/users API +merge_request: 12445 +author: -- cgit v1.2.1 From 790c740cce8487d1155607355d06f42ee2e83fac Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 29 Jun 2017 10:54:10 -0700 Subject: Increase CI retries to 4 for these examples By default it is 2 tries in CI. --- spec/lib/gitlab/health_checks/fs_shards_check_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb index 61c10d47434..c8c402b4f71 100644 --- a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb +++ b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb @@ -97,6 +97,12 @@ describe Gitlab::HealthChecks::FsShardsCheck do }.with_indifferent_access end + # Unsolved intermittent failure in CI https://gitlab.com/gitlab-org/gitlab-ce/issues/31128 + around(:each) do |example| + times_to_try = ENV['CI'] ? 4 : 1 + example.run_with_retry retry: times_to_try + end + it { is_expected.to all(have_attributes(labels: { shard: :default })) } it { is_expected.to include(an_object_having_attributes(name: :filesystem_accessible, value: 0)) } -- cgit v1.2.1 From 53c409cb07d295043c5e1cff738c84b3aa7405a8 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 29 Jun 2017 16:53:41 -0700 Subject: =?UTF-8?q?Rspec/AroundBlock=20doesn=E2=80=99t=20know=20about=20rs?= =?UTF-8?q?pec-retry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- spec/lib/gitlab/health_checks/fs_shards_check_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb index c8c402b4f71..fbacbc4a338 100644 --- a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb +++ b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb @@ -98,7 +98,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do end # Unsolved intermittent failure in CI https://gitlab.com/gitlab-org/gitlab-ce/issues/31128 - around(:each) do |example| + around(:each) do |example| # rubocop:disable RSpec/AroundBlock times_to_try = ENV['CI'] ? 4 : 1 example.run_with_retry retry: times_to_try end -- cgit v1.2.1 From 3c88a7869b87693ba8c3fb9814d39437dd569a31 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 29 Jun 2017 07:43:41 +0000 Subject: Implement review comments for !12445 from @godfat and @rymai. - Use `GlobalPolicy` to authorize the users that a non-authenticated user can fetch from `/api/v4/users`. We allow access if the `Gitlab::VisibilityLevel::PUBLIC` visibility level is not restricted. - Further, as before, `/api/v4/users` is only accessible to unauthenticated users if the `username` parameter is passed. - Turn off `authenticate!` for the `/api/v4/users` endpoint by matching on the actual route + method, rather than the description. - Change the type of `current_user` check in `UsersFinder` to be more compatible with EE. --- app/finders/users_finder.rb | 11 ++++------- app/policies/base_policy.rb | 6 ++++++ app/policies/global_policy.rb | 3 ++- app/policies/user_policy.rb | 6 ------ lib/api/helpers.rb | 4 ++-- lib/api/users.rb | 26 +++++++++++--------------- spec/requests/api/users_spec.rb | 2 +- 7 files changed, 26 insertions(+), 32 deletions(-) diff --git a/app/finders/users_finder.rb b/app/finders/users_finder.rb index 0534317df8f..07deceb827b 100644 --- a/app/finders/users_finder.rb +++ b/app/finders/users_finder.rb @@ -27,11 +27,8 @@ class UsersFinder users = by_search(users) users = by_blocked(users) users = by_active(users) - - if current_user - users = by_external_identity(users) - users = by_external(users) - end + users = by_external_identity(users) + users = by_external(users) users end @@ -63,13 +60,13 @@ class UsersFinder end def by_external_identity(users) - return users unless current_user.admin? && params[:extern_uid] && params[:provider] + return users unless current_user&.admin? && params[:extern_uid] && params[:provider] users.joins(:identities).merge(Identity.with_extern_uid(params[:provider], params[:extern_uid])) end def by_external(users) - return users = users.where.not(external: true) unless current_user.admin? + return users = users.where.not(external: true) unless current_user&.admin? return users unless params[:external] users.external diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb index 623424c63e0..261a2e780c5 100644 --- a/app/policies/base_policy.rb +++ b/app/policies/base_policy.rb @@ -1,4 +1,6 @@ class BasePolicy + include Gitlab::CurrentSettings + class RuleSet attr_reader :can_set, :cannot_set def initialize(can_set, cannot_set) @@ -124,4 +126,8 @@ class BasePolicy yield @rule_set end + + def restricted_public_level? + current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC) + end end diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb index 2683aaad981..e9be43a5037 100644 --- a/app/policies/global_policy.rb +++ b/app/policies/global_policy.rb @@ -1,9 +1,10 @@ class GlobalPolicy < BasePolicy def rules + can! :read_users_list unless restricted_public_level? + return unless @user can! :create_group if @user.can_create_group - can! :read_users_list unless @user.blocked? || @user.internal? can! :log_in unless @user.access_locked? diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 229846e368c..265c56aba53 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -1,6 +1,4 @@ class UserPolicy < BasePolicy - include Gitlab::CurrentSettings - def rules can! :read_user if @user || !restricted_public_level? @@ -12,8 +10,4 @@ class UserPolicy < BasePolicy cannot! :destroy_user if @subject.ghost? end end - - def restricted_public_level? - current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC) - end end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 1322afaa64f..a3aec8889d7 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -410,8 +410,8 @@ module API # Does the current route match the route identified by # `description`? - def route_matches_description?(description) - options.dig(:route_options, :description) == description + def request_matches_route?(method, route) + request.request_method == method && request.path == route end end end diff --git a/lib/api/users.rb b/lib/api/users.rb index 34619c90d8b..18ce58299e7 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -4,7 +4,7 @@ module API before do allow_access_with_scope :read_user if request.get? - authenticate! unless route_matches_description?("Get the list of users") + authenticate! unless request_matches_route?('GET', '/api/v4/users') end resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do @@ -55,22 +55,18 @@ module API users = UsersFinder.new(current_user, params).execute - authorized = - if current_user - can?(current_user, :read_users_list) - else - # When `current_user` is not present, require that the `username` - # parameter is passed, to prevent an unauthenticated user from accessing - # a list of all the users on the GitLab instance. `UsersFinder` performs - # an exact match on the `username` parameter, so we are guaranteed to - # get either 0 or 1 `users` here. - params[:username].present? && - users.all? { |user| can?(current_user, :read_user, user) } - end + authorized = can?(current_user, :read_users_list) + + # When `current_user` is not present, require that the `username` + # parameter is passed, to prevent an unauthenticated user from accessing + # a list of all the users on the GitLab instance. `UsersFinder` performs + # an exact match on the `username` parameter, so we are guaranteed to + # get either 0 or 1 `users` here. + authorized &&= params[:username].present? if current_user.blank? - render_api_error!("Not authorized.", 403) unless authorized + forbidden!("Not authorized to access /api/v4/users") unless authorized - entity = current_user.try(:admin?) ? Entities::UserWithAdmin : Entities::UserBasic + entity = current_user&.admin? ? Entities::UserWithAdmin : Entities::UserBasic present paginate(users), with: entity end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 01541901330..bf7ed2d3ad6 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -34,7 +34,7 @@ describe API::Users do it "returns authorization error when the `username` parameter refers to an inaccessible user" do user = create(:user) - expect(Ability).to receive(:allowed?).with(nil, :read_user, user).and_return(false) + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) get api("/users"), username: user.username -- cgit v1.2.1 From 912613c41be3790b004f65935e8380aea9e5895f Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Fri, 30 Jun 2017 09:00:07 -0700 Subject: Reduce 28 test runs to 4 14 to 2, but these shared examples are used twice. This was already done in another context further down the file. --- .../gitlab/health_checks/fs_shards_check_spec.rb | 36 ++++++++++++---------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb index fbacbc4a338..b333e162909 100644 --- a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb +++ b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb @@ -103,30 +103,34 @@ describe Gitlab::HealthChecks::FsShardsCheck do example.run_with_retry retry: times_to_try end - it { is_expected.to all(have_attributes(labels: { shard: :default })) } - - it { is_expected.to include(an_object_having_attributes(name: :filesystem_accessible, value: 0)) } - it { is_expected.to include(an_object_having_attributes(name: :filesystem_readable, value: 0)) } - it { is_expected.to include(an_object_having_attributes(name: :filesystem_writable, value: 0)) } - - it { is_expected.to include(an_object_having_attributes(name: :filesystem_access_latency, value: be >= 0)) } - it { is_expected.to include(an_object_having_attributes(name: :filesystem_read_latency, value: be >= 0)) } - it { is_expected.to include(an_object_having_attributes(name: :filesystem_write_latency, value: be >= 0)) } + it 'provides metrics' do + expect(subject).to all(have_attributes(labels: { shard: :default })) + expect(subject).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0)) + expect(subject).to include(an_object_having_attributes(name: :filesystem_readable, value: 0)) + expect(subject).to include(an_object_having_attributes(name: :filesystem_writable, value: 0)) + + expect(subject).to include(an_object_having_attributes(name: :filesystem_access_latency, value: be >= 0)) + expect(subject).to include(an_object_having_attributes(name: :filesystem_read_latency, value: be >= 0)) + expect(subject).to include(an_object_having_attributes(name: :filesystem_write_latency, value: be >= 0)) + end end context 'storage points to directory that has both read and write rights' do before do FileUtils.chmod_R(0755, tmp_dir) end - it { is_expected.to all(have_attributes(labels: { shard: :default })) } - it { is_expected.to include(an_object_having_attributes(name: :filesystem_accessible, value: 1)) } - it { is_expected.to include(an_object_having_attributes(name: :filesystem_readable, value: 1)) } - it { is_expected.to include(an_object_having_attributes(name: :filesystem_writable, value: 1)) } + it 'provides metrics' do + expect(subject).to all(have_attributes(labels: { shard: :default })) - it { is_expected.to include(an_object_having_attributes(name: :filesystem_access_latency, value: be >= 0)) } - it { is_expected.to include(an_object_having_attributes(name: :filesystem_read_latency, value: be >= 0)) } - it { is_expected.to include(an_object_having_attributes(name: :filesystem_write_latency, value: be >= 0)) } + expect(subject).to include(an_object_having_attributes(name: :filesystem_accessible, value: 1)) + expect(subject).to include(an_object_having_attributes(name: :filesystem_readable, value: 1)) + expect(subject).to include(an_object_having_attributes(name: :filesystem_writable, value: 1)) + + expect(subject).to include(an_object_having_attributes(name: :filesystem_access_latency, value: be >= 0)) + expect(subject).to include(an_object_having_attributes(name: :filesystem_read_latency, value: be >= 0)) + expect(subject).to include(an_object_having_attributes(name: :filesystem_write_latency, value: be >= 0)) + end end end end -- cgit v1.2.1 From 96e986327c4dad9248f9013f191119ffafe4a6d8 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 3 Jul 2017 05:14:00 +0000 Subject: Implement review comments for !12445 from @jneen. - Fix duplicate `prevent` declaration - Add spec for `GlobalPolicy` --- app/policies/global_policy.rb | 1 - spec/policies/global_policy_spec.rb | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 spec/policies/global_policy_spec.rb diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb index 7767d3cccd5..55eefa76d3f 100644 --- a/app/policies/global_policy.rb +++ b/app/policies/global_policy.rb @@ -18,7 +18,6 @@ class GlobalPolicy < BasePolicy prevent :receive_notifications prevent :use_quick_actions prevent :create_group - prevent :log_in end rule { default }.policy do diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb new file mode 100644 index 00000000000..bb0fa0c0e9c --- /dev/null +++ b/spec/policies/global_policy_spec.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +describe GlobalPolicy, models: true do + let(:current_user) { create(:user) } + let(:user) { create(:user) } + + subject { GlobalPolicy.new(current_user, [user]) } + + describe "reading the list of users" do + context "for a logged in user" do + it { is_expected.to be_allowed(:read_users_list) } + end + + context "for an anonymous user" do + let(:current_user) { nil } + + context "when the public level is restricted" do + before do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) + end + + it { is_expected.not_to be_allowed(:read_users_list) } + end + + context "when the public level is not restricted" do + before do + stub_application_setting(restricted_visibility_levels: []) + end + + it { is_expected.to be_allowed(:read_users_list) } + end + end + end +end -- cgit v1.2.1 From c7661f04d02316d48856c0b9693887228dc054d0 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Wed, 28 Jun 2017 14:36:04 +0200 Subject: Strings ready for translation; Pipeline charts Earlier, this was part of another MR, but that got split. Didn't pick that commit, as there were many merge conflicts. Vim macros seemed faster. --- app/views/projects/pipelines/charts.html.haml | 4 +- app/views/projects/pipelines/charts/_overall.haml | 16 ++-- .../projects/pipelines/charts/_pipeline_times.haml | 2 +- .../projects/pipelines/charts/_pipelines.haml | 12 +-- locale/en/gitlab.po | 97 ++++++++++++++++++++-- locale/gitlab.pot | 61 +++++++++++++- 6 files changed, 166 insertions(+), 26 deletions(-) diff --git a/app/views/projects/pipelines/charts.html.haml b/app/views/projects/pipelines/charts.html.haml index 8ffddfe6154..78002e8cd64 100644 --- a/app/views/projects/pipelines/charts.html.haml +++ b/app/views/projects/pipelines/charts.html.haml @@ -1,5 +1,5 @@ - @no_container = true -- page_title "Charts", "Pipelines" +- page_title _("Charts"), _("Pipelines") - content_for :page_specific_javascripts do = page_specific_javascript_bundle_tag('common_d3') = page_specific_javascript_bundle_tag('graphs') @@ -8,7 +8,7 @@ %div{ class: container_class } .sub-header-block .oneline - A collection of graphs for Continuous Integration + = _("A collection of graphs regarding Continuous Integration") #charts.ci-charts .row diff --git a/app/views/projects/pipelines/charts/_overall.haml b/app/views/projects/pipelines/charts/_overall.haml index 93083397d5b..66786c7ff59 100644 --- a/app/views/projects/pipelines/charts/_overall.haml +++ b/app/views/projects/pipelines/charts/_overall.haml @@ -1,15 +1,15 @@ -%h4 Overall stats +%h4= s_("PipelineCharts|Overall statistics") %ul %li - Total: - %strong= pluralize @counts[:total], 'pipeline' + = s_("PipelineCharts|Total:") + %strong= n_("1 pipeline", "%d pipelines", @counts[:total]) % @counts[:total] %li - Successful: - %strong= pluralize @counts[:success], 'pipeline' + = s_("PipelineCharts|Successful:") + %strong= n_("1 pipeline", "%d pipelines", @counts[:success]) % @counts[:success] %li - Failed: - %strong= pluralize @counts[:failed], 'pipeline' + = s_("PipelineCharts|Failed:") + %strong= n_("1 pipeline", "%d pipelines", @counts[:failed]) % @counts[:failed] %li - Success ratio: + = s_("PipelineCharts|Success ratio:") %strong #{success_ratio(@counts)}% diff --git a/app/views/projects/pipelines/charts/_pipeline_times.haml b/app/views/projects/pipelines/charts/_pipeline_times.haml index aee7c5492aa..1292f580a81 100644 --- a/app/views/projects/pipelines/charts/_pipeline_times.haml +++ b/app/views/projects/pipelines/charts/_pipeline_times.haml @@ -1,6 +1,6 @@ %div %p.light - Commit duration in minutes for last 30 commits + = _("Commit duration in minutes for last 30 commits") %canvas#build_timesChart{ height: 200 } diff --git a/app/views/projects/pipelines/charts/_pipelines.haml b/app/views/projects/pipelines/charts/_pipelines.haml index b6f453b9736..be884448087 100644 --- a/app/views/projects/pipelines/charts/_pipelines.haml +++ b/app/views/projects/pipelines/charts/_pipelines.haml @@ -1,29 +1,29 @@ -%h4 Pipelines charts +%h4= _("Pipelines charts") %p   %span.cgreen = icon("circle") - success + = s_("Pipeline|success")   %span.cgray = icon("circle") - all + = s_("Pipeline|all") .prepend-top-default %p.light - Jobs for last week + = _("Jobs for last week") (#{date_from_to(Date.today - 7.days, Date.today)}) %canvas#weekChart{ height: 200 } .prepend-top-default %p.light - Jobs for last month + = _("Jobs for last month") (#{date_from_to(Date.today - 30.days, Date.today)}) %canvas#monthChart{ height: 200 } .prepend-top-default %p.light - Jobs for last year + = _("Jobs for last year") %canvas#yearChart.padded{ height: 250 } - [:week, :month, :year].each do |scope| diff --git a/locale/en/gitlab.po b/locale/en/gitlab.po index afb8fb3176f..bda3fc09e85 100644 --- a/locale/en/gitlab.po +++ b/locale/en/gitlab.po @@ -17,9 +17,27 @@ msgstr "" "Plural-Forms: nplurals=2; plural=n != 1;\n" "\n" +msgid "%d additional commit has been omitted to prevent performance issues." +msgid_plural "%d additional commits have been omitted to prevent performance issues." +msgstr[0] "" +msgstr[1] "" + +msgid "%d commit" +msgid_plural "%d commits" +msgstr[0] "" +msgstr[1] "" + msgid "%{commit_author_link} committed %{commit_timeago}" msgstr "" +msgid "1 pipeline" +msgid_plural "%d pipelines" +msgstr[0] "" +msgstr[1] "" + +msgid "A collection of graphs regarding Continuous Integration" +msgstr "" + msgid "About auto deploy" msgstr "" @@ -61,9 +79,24 @@ msgstr[1] "" msgid "Branch %{branch_name} was created. To set up auto deploy, choose a GitLab CI Yaml template and commit your changes. %{link_to_autodeploy_doc}" msgstr "" +msgid "BranchSwitcherPlaceholder|Search branches" +msgstr "" + +msgid "BranchSwitcherTitle|Switch branch" +msgstr "" + msgid "Branches" msgstr "" +msgid "Browse Directory" +msgstr "" + +msgid "Browse File" +msgstr "" + +msgid "Browse Files" +msgstr "" + msgid "Browse files" msgstr "" @@ -159,6 +192,9 @@ msgid_plural "Commits" msgstr[0] "" msgstr[1] "" +msgid "Commit duration in minutes for last 30 commits" +msgstr "" + msgid "Commit message" msgstr "" @@ -171,6 +207,9 @@ msgstr "" msgid "Commits" msgstr "" +msgid "Commits feed" +msgstr "" + msgid "Commits|History" msgstr "" @@ -195,6 +234,9 @@ msgstr "" msgid "Create New Directory" msgstr "" +msgid "Create a personal access token on your account to pull or push via %{protocol}." +msgstr "" + msgid "Create directory" msgstr "" @@ -213,6 +255,9 @@ msgstr "" msgid "CreateTag|Tag" msgstr "" +msgid "CreateTokenToCloneLink|create a personal access token" +msgstr "" + msgid "Cron Timezone" msgstr "" @@ -323,6 +368,9 @@ msgstr "" msgid "Files" msgstr "" +msgid "Filter by commit message" +msgstr "" + msgid "Find by path" msgstr "" @@ -370,6 +418,15 @@ msgstr "" msgid "Introducing Cycle Analytics" msgstr "" +msgid "Jobs for last month" +msgstr "" + +msgid "Jobs for last week" +msgstr "" + +msgid "Jobs for last year" +msgstr "" + msgid "LFSStatus|Disabled" msgstr "" @@ -535,6 +592,21 @@ msgstr "" msgid "Pipeline Schedules" msgstr "" +msgid "PipelineCharts|Failed:" +msgstr "" + +msgid "PipelineCharts|Overall statistics" +msgstr "" + +msgid "PipelineCharts|Success ratio:" +msgstr "" + +msgid "PipelineCharts|Successful:" +msgstr "" + +msgid "PipelineCharts|Total:" +msgstr "" + msgid "PipelineSchedules|Activated" msgstr "" @@ -565,6 +637,18 @@ msgstr "" msgid "PipelineSheduleIntervalPattern|Custom" msgstr "" +msgid "Pipelines" +msgstr "" + +msgid "Pipelines charts" +msgstr "" + +msgid "Pipeline|all" +msgstr "" + +msgid "Pipeline|success" +msgstr "" + msgid "Pipeline|with stage" msgstr "" @@ -688,7 +772,7 @@ msgstr "" msgid "Select target branch" msgstr "" -msgid "Set a password on your account to pull or push via %{protocol}" +msgid "Set a password on your account to pull or push via %{protocol}." msgstr "" msgid "Set up CI" @@ -714,10 +798,7 @@ msgstr "" msgid "StarProject|Star" msgstr "" -msgid "Start a %{new_merge_request} with these changes" -msgstr "" - -msgid "Start a new merge request with these changes" +msgid "Start a %{new_merge_request} with these changes" msgstr "" msgid "Switch branch/tag" @@ -948,9 +1029,15 @@ msgstr "" msgid "Upload file" msgstr "" +msgid "UploadLink|click to upload" +msgstr "" + msgid "Use your global notification setting" msgstr "" +msgid "View open merge request" +msgstr "" + msgid "VisibilityLevel|Internal" msgstr "" diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 07f9efeb495..9f1caeddaa7 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8,8 +8,8 @@ msgid "" msgstr "" "Project-Id-Version: gitlab 1.0.0\n" "Report-Msgid-Bugs-To: \n" -"POT-Creation-Date: 2017-06-19 15:50-0500\n" -"PO-Revision-Date: 2017-06-19 15:50-0500\n" +"POT-Creation-Date: 2017-06-28 13:32+0200\n" +"PO-Revision-Date: 2017-06-28 13:32+0200\n" "Last-Translator: FULL NAME \n" "Language-Team: LANGUAGE \n" "Language: \n" @@ -18,7 +18,7 @@ msgstr "" "Content-Transfer-Encoding: 8bit\n" "Plural-Forms: nplurals=INTEGER; plural=EXPRESSION;\n" -msgid "%d additional commit have been omitted to prevent performance issues." +msgid "%d additional commit has been omitted to prevent performance issues." msgid_plural "%d additional commits have been omitted to prevent performance issues." msgstr[0] "" msgstr[1] "" @@ -31,6 +31,14 @@ msgstr[1] "" msgid "%{commit_author_link} committed %{commit_timeago}" msgstr "" +msgid "1 pipeline" +msgid_plural "%d pipelines" +msgstr[0] "" +msgstr[1] "" + +msgid "A collection of graphs regarding Continuous Integration" +msgstr "" + msgid "About auto deploy" msgstr "" @@ -185,6 +193,9 @@ msgid_plural "Commits" msgstr[0] "" msgstr[1] "" +msgid "Commit duration in minutes for last 30 commits" +msgstr "" + msgid "Commit message" msgstr "" @@ -224,6 +235,9 @@ msgstr "" msgid "Create New Directory" msgstr "" +msgid "Create a personal access token on your account to pull or push via %{protocol}." +msgstr "" + msgid "Create directory" msgstr "" @@ -242,6 +256,9 @@ msgstr "" msgid "CreateTag|Tag" msgstr "" +msgid "CreateTokenToCloneLink|create a personal access token" +msgstr "" + msgid "Cron Timezone" msgstr "" @@ -402,6 +419,15 @@ msgstr "" msgid "Introducing Cycle Analytics" msgstr "" +msgid "Jobs for last month" +msgstr "" + +msgid "Jobs for last week" +msgstr "" + +msgid "Jobs for last year" +msgstr "" + msgid "LFSStatus|Disabled" msgstr "" @@ -567,6 +593,21 @@ msgstr "" msgid "Pipeline Schedules" msgstr "" +msgid "PipelineCharts|Failed:" +msgstr "" + +msgid "PipelineCharts|Overall statistics" +msgstr "" + +msgid "PipelineCharts|Success ratio:" +msgstr "" + +msgid "PipelineCharts|Successful:" +msgstr "" + +msgid "PipelineCharts|Total:" +msgstr "" + msgid "PipelineSchedules|Activated" msgstr "" @@ -597,6 +638,18 @@ msgstr "" msgid "PipelineSheduleIntervalPattern|Custom" msgstr "" +msgid "Pipelines" +msgstr "" + +msgid "Pipelines charts" +msgstr "" + +msgid "Pipeline|all" +msgstr "" + +msgid "Pipeline|success" +msgstr "" + msgid "Pipeline|with stage" msgstr "" @@ -720,7 +773,7 @@ msgstr "" msgid "Select target branch" msgstr "" -msgid "Set a password on your account to pull or push via %{protocol}" +msgid "Set a password on your account to pull or push via %{protocol}." msgstr "" msgid "Set up CI" -- cgit v1.2.1 From 512254ef7aeb876913a659a2ae5a88bf9fa43016 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Wed, 14 Jun 2017 16:12:27 +0200 Subject: Add documentation for extended docker configuration options for CI --- doc/ci/docker/using_docker_images.md | 176 +++++++++++++++++++++++++++++++++++ 1 file changed, 176 insertions(+) diff --git a/doc/ci/docker/using_docker_images.md b/doc/ci/docker/using_docker_images.md index be4dea55c20..fee8f3fb9e7 100644 --- a/doc/ci/docker/using_docker_images.md +++ b/doc/ci/docker/using_docker_images.md @@ -125,6 +125,178 @@ test:2.2: - bundle exec rake spec ``` +Starting with GitLab 9.4 and GitLab Runner 9.4 you can also pass an extended +configuration options for image and services: + +```yaml +image: + name: ruby:2.2 + entrypoint: /bin/bash + +services: +- name: my-postgres:9.4 + alias: db-postgres + entrypoint: /usr/local/bin/db-postgres + command: start + +before_script: +- bundle install + +test: + script: + - bundle exec rake spec +``` + +Read [Extended docker configuration options](#extended-docker-configuration-options) to +check what options you can use with `image` and `services` definitions. + +## Extended docker configuration options + +> **Note: This feature is available with GitLab and GitLab Runner 9.4 or higher** + +While configuring `image` or `services` entry you can use both string or a map of +options. If you will use a string, then it must be the full name of the image to use +(including registry part if you wan't to download the image from a non-default +registry). If you will use a map of options, then it must contain at least the `name` +option, which is the same name of image as used for a string setting. + +For example, following definitions are equal: + +```yaml +image: "registry.example.com/my/image:latest" +services: +- postgresql:9.4 +- redis:latest +``` + +```yaml +image: + name: "registry.example.com/my/image:latest" +services: +- name: postgresql:9.4 +- name: redis:latest +``` + +### Available settings for `image` + +| Setting | Required | Description | +|------------|----------|-------------| +| name | yes | Full name of the image that should be used. It should contain the registry part if needed. | +| entrypoint | no | Command or script that should be executed as container's entrypoint. It will be translated to Docker's `--entrypoint` option while creating container. | + +### Available settings for `services` entry + +| Setting | Required | Description | +|------------|----------|-------------| +| name | yes | Full name of the image that should be used. It should contain the registry part if needed. | +| entrypoint | no | Command or script that should be used as container's entrypoint. It will be translated to Docker's `--entrypoint` option while creating container. | +| command | no | Command or script and eventual arguments that should be used as container's command. It will be translated to arguments passed to Docker after image's name. | +| alias | no | Additional alias, that can be used to access service from job's container. Read [Accessing the services](#accessing-the-services) for more information. | + +### Example usages + +You can use this feature for different purposes: + +#### Starting multiple services from the same image + +Before adding this feature following configuration will not work properly: + +```yaml +services: +- mysql:latest +- mysql:latest +``` + +Runner would start two containers using the `mysql:latest` image, but both +of them would be added to the job's container with the `mysql` alias. This +would end with one of services not being accessible. + +Thanks to extended docker configuration we can change above configuration +to a following version: + +```yaml +services: +- name: mysql:latest + alias: mysql-1 +- name: mysql:latest + aliase: mysql-2 +``` + +Runner will still start two containers using the `mysql:latest` image, +but now each of them will be also accessible with the alias configured +in `.gitlab-ci.yml` file. + +#### Setting a command for the service + +Let's assume we have a `super/sql:latest` image with some SQL database +inside. We would like to use it as a service for your job. The problem +is that this image doesn't start the database process while starting +the container. It have all installed and configured inside but user needs +to manually use `/usr/bin/super-sql run` as command to start the database. + +Previously we would need to create our own image based on the `super/sql:latest` +image, add the default command, and then use it in job's configuration, e.g.: + +```Dockerfile +# my-super-sql:latest image's Dockerfile +FROM super/sql:latest +CMD ["/usr/bin/super-sql", "run"] +``` + +```yaml +# .gitlab-ci.yml + +services: +- my-super-sql:latest +``` + +With docker extended docker configuration we can now do this simple, setting +only proper configuration in `.gitlab-ci.yml`, e.g: + +```yaml +# .gitlab-ci.yml + +services: +- name: super/sql:latest + command: /usr/bin/super-sql run +``` + +#### Overriding entrypoint of job's image + +Let's assume we have a `super/sql:experimental` image with some SQL database +inside. We would like to use it as base for our job, because we wan't to +execute some tests with this database binary. The problem is that this image +is configured with `/usr/bin/super-sql run` as entrypoint. That means, that +when starting container without additional options it will run database's +process, while Runner expects that image will have no entrypoint or at least +will start with a shell as entrypoint. + +Previously we would need to create our own image based on the +`super/sql:experimental` image, set the entrypoint to a shell, and then use +it in job's configuration, e.g.: + +```Dockerfile +# my-super-sql:experimental image's Dockerfile +FROM super/sql:experimental +ENTRYPOINT ["/bin/sh"] +``` + +```yaml +# .gitlab-ci.yml + +image: my-super-sql:experimental +``` + +With docker extended docker configuration we can now do this simple, setting +only proper configuration in `.gitlab-ci.yml`, e.g: + +```yaml +# .gitlab-ci.yml + +image: + name: super/sql:experimental + entrypoint: /bin/sh + ## Define image and services in `config.toml` Look for the `[runners.docker]` section: @@ -231,6 +403,10 @@ rules: 2. Slash (`/`) is replaced with double underscores (`__`) - primary alias 3. Slash (`/`) is replaced with dash (`-`) - secondary alias, requires GitLab Runner v1.1.0 or newer +Starting with 9.4 You can also set additional alias using the `alias` setting of `services` +configuration entry. Please read [Extended docker configuration options](#extended-docker-configuration-options) +for more information. + ## Configuring services Many services accept environment variables which allow you to easily change -- cgit v1.2.1 From da2f003ccfe85893c6b405190a1bfd9f176ffea8 Mon Sep 17 00:00:00 2001 From: Tomasz Maczukin Date: Fri, 30 Jun 2017 16:04:01 +0200 Subject: Update syntax description to new syntax introduced in !12536. --- doc/ci/docker/using_docker_images.md | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/doc/ci/docker/using_docker_images.md b/doc/ci/docker/using_docker_images.md index fee8f3fb9e7..c5a70d83201 100644 --- a/doc/ci/docker/using_docker_images.md +++ b/doc/ci/docker/using_docker_images.md @@ -131,13 +131,13 @@ configuration options for image and services: ```yaml image: name: ruby:2.2 - entrypoint: /bin/bash + entrypoint: ["/bin/bash"] services: - name: my-postgres:9.4 alias: db-postgres - entrypoint: /usr/local/bin/db-postgres - command: start + entrypoint: ["/usr/local/bin/db-postgres"] + command: ["start"] before_script: - bundle install @@ -182,15 +182,15 @@ services: | Setting | Required | Description | |------------|----------|-------------| | name | yes | Full name of the image that should be used. It should contain the registry part if needed. | -| entrypoint | no | Command or script that should be executed as container's entrypoint. It will be translated to Docker's `--entrypoint` option while creating container. | +| entrypoint | no | Command or script that should be executed as container's entrypoint. It will be translated to Docker's `--entrypoint` option while creating container. The syntax is similar to `Dockerfile`'s `ENTRYPOINT` directive, where each shell token is a separate string in the array. | ### Available settings for `services` entry | Setting | Required | Description | |------------|----------|-------------| | name | yes | Full name of the image that should be used. It should contain the registry part if needed. | -| entrypoint | no | Command or script that should be used as container's entrypoint. It will be translated to Docker's `--entrypoint` option while creating container. | -| command | no | Command or script and eventual arguments that should be used as container's command. It will be translated to arguments passed to Docker after image's name. | +| entrypoint | no | Command or script that should be used as container's entrypoint. It will be translated to Docker's `--entrypoint` option while creating container. The syntax is similar to `Dockerfile`'s `ENTRYPOINT` directive, where each shell token is a separate string in the array. | +| command | no | Command or script and eventual arguments that should be used as container's command. It will be translated to arguments passed to Docker after image's name. The syntax is similar to `Dockerfile`'s `CMD` directive, where each shell token is a separate string in the array. | | alias | no | Additional alias, that can be used to access service from job's container. Read [Accessing the services](#accessing-the-services) for more information. | ### Example usages @@ -250,7 +250,7 @@ services: - my-super-sql:latest ``` -With docker extended docker configuration we can now do this simple, setting +With extended docker configuration we can now do this simple, setting only proper configuration in `.gitlab-ci.yml`, e.g: ```yaml @@ -258,13 +258,15 @@ only proper configuration in `.gitlab-ci.yml`, e.g: services: - name: super/sql:latest - command: /usr/bin/super-sql run + command: ["/usr/bin/super-sql", "run"] ``` +As you can see the syntax of `command` entry is similar to Dockerfile's `CMD`. + #### Overriding entrypoint of job's image Let's assume we have a `super/sql:experimental` image with some SQL database -inside. We would like to use it as base for our job, because we wan't to +inside. We would like to use it as base for our job, because we want to execute some tests with this database binary. The problem is that this image is configured with `/usr/bin/super-sql run` as entrypoint. That means, that when starting container without additional options it will run database's @@ -295,7 +297,10 @@ only proper configuration in `.gitlab-ci.yml`, e.g: image: name: super/sql:experimental - entrypoint: /bin/sh + entrypoint: ["/bin/sh"] +``` + +As you can see the syntax of `entrypoint` entry is similar to Dockerfile's `ENTRYPOINT`. ## Define image and services in `config.toml` -- cgit v1.2.1 From 469ea5eab4b3331a50e54afb6fd1c59cb7296137 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Mon, 3 Jul 2017 15:22:47 +0200 Subject: Refactor and copyedit "Using Docker images" docs --- doc/ci/docker/using_docker_images.md | 285 ++++++++++++++++++----------------- 1 file changed, 150 insertions(+), 135 deletions(-) diff --git a/doc/ci/docker/using_docker_images.md b/doc/ci/docker/using_docker_images.md index c5a70d83201..d3433594eb7 100644 --- a/doc/ci/docker/using_docker_images.md +++ b/doc/ci/docker/using_docker_images.md @@ -1,4 +1,4 @@ -# Using Docker Images +# Using Docker images GitLab CI in conjunction with [GitLab Runner](../runners/README.md) can use [Docker Engine](https://www.docker.com/) to test and build any application. @@ -17,14 +17,16 @@ can also run on your workstation. The added benefit is that you can test all the commands that we will explore later from your shell, rather than having to test them on a dedicated CI server. -## Register docker runner +## Register Docker Runner -To use GitLab Runner with docker you need to register a new runner to use the -`docker` executor: +To use GitLab Runner with Docker you need to [register a new Runner][register] +to use the `docker` executor. + +A one-line example can be seen below: ```bash -gitlab-ci-multi-runner register \ - --url "https://gitlab.com/" \ +sudo gitlab-runner register \ + --url "https://gitlab.example.com/" \ --registration-token "PROJECT_REGISTRATION_TOKEN" \ --description "docker-ruby-2.1" \ --executor "docker" \ @@ -33,26 +35,26 @@ gitlab-ci-multi-runner register \ --docker-mysql latest ``` -The registered runner will use the `ruby:2.1` docker image and will run two +The registered runner will use the `ruby:2.1` Docker image and will run two services, `postgres:latest` and `mysql:latest`, both of which will be accessible during the build process. ## What is an image -The `image` keyword is the name of the docker image the docker executor -will run to perform the CI tasks. +The `image` keyword is the name of the Docker image the Docker executor +will run to perform the CI tasks. -By default the executor will only pull images from [Docker Hub][hub], +By default, the executor will only pull images from [Docker Hub][hub], but this can be configured in the `gitlab-runner/config.toml` by setting -the [docker pull policy][] to allow using local images. +the [Docker pull policy][] to allow using local images. For more information about images and Docker Hub please read the [Docker Fundamentals][] documentation. ## What is a service -The `services` keyword defines just another docker image that is run during -your job and is linked to the docker image that the `image` keyword defines. +The `services` keyword defines just another Docker image that is run during +your job and is linked to the Docker image that the `image` keyword defines. This allows you to access the service image during build time. The service image can run any application, but the most common use case is to @@ -60,6 +62,11 @@ run a database container, eg. `mysql`. It's easier and faster to use an existing image and run it as an additional container than install `mysql` every time the project is built. +You are not limited to have only database services. You can add as many +services you need to `.gitlab-ci.yml` or manually modify `config.toml`. +Any image found at [Docker Hub][hub] or your private Container Registry can be +used as a service. + You can see some widely used services examples in the relevant documentation of [CI services examples](../services/README.md). @@ -73,22 +80,49 @@ then be used to create a container that is linked to the job container. The service container for MySQL will be accessible under the hostname `mysql`. So, in order to access your database service you have to connect to the host -named `mysql` instead of a socket or `localhost`. +named `mysql` instead of a socket or `localhost`. Read more in [accessing the +services](#accessing-the-services). + +### Accessing the services + +Let's say that you need a Wordpress instance to test some API integration with +your application. -## Overwrite image and services +You can then use for example the [tutum/wordpress][] image in your +`.gitlab-ci.yml`: -See [How to use other images as services](#how-to-use-other-images-as-services). +```yaml +services: +- tutum/wordpress:latest +``` -## How to use other images as services +If you don't [specify a service alias](#available-settings-for-services-entry), +when the job is run, `tutum/wordpress` will be started and you will have +access to it from your build container under two hostnames to choose from: -You are not limited to have only database services. You can add as many -services you need to `.gitlab-ci.yml` or manually modify `config.toml`. -Any image found at [Docker Hub][hub] can be used as a service. +- `tutum-wordpress` +- `tutum__wordpress` -## Define image and services from `.gitlab-ci.yml` +>**Note:** +Hostnames with underscores are not RFC valid and may cause problems in 3rd party +applications. + +The default aliases for the service's hostname are created from its image name +following these rules: + +- Everything after the colon (`:`) is stripped +- Slash (`/`) is replaced with double underscores (`__`) and the primary alias + is created +- Slash (`/`) is replaced with a single dash (`-`) and the secondary alias is + created (requires GitLab Runner v1.1.0 or higher) + +To override the default behavior, you can +[specify a service alias](#available-settings-for-services-entry). + +## Define `image` and `services` from `.gitlab-ci.yml` You can simply define an image that will be used for all jobs and a list of -services that you want to use during build time. +services that you want to use during build time: ```yaml image: ruby:2.2 @@ -125,8 +159,8 @@ test:2.2: - bundle exec rake spec ``` -Starting with GitLab 9.4 and GitLab Runner 9.4 you can also pass an extended -configuration options for image and services: +Or you can pass some [extended configuration options](#extended-docker-configuration-options) +for `image` and `services`: ```yaml image: @@ -147,59 +181,70 @@ test: - bundle exec rake spec ``` -Read [Extended docker configuration options](#extended-docker-configuration-options) to -check what options you can use with `image` and `services` definitions. +## Extended Docker configuration options -## Extended docker configuration options +> **Note:** +This feature requires GitLab 9.4 and GitLab Runner 9.4 or higher. -> **Note: This feature is available with GitLab and GitLab Runner 9.4 or higher** +When configuring the `image` or `services` entries, you can use a string or a map as +options: -While configuring `image` or `services` entry you can use both string or a map of -options. If you will use a string, then it must be the full name of the image to use -(including registry part if you wan't to download the image from a non-default -registry). If you will use a map of options, then it must contain at least the `name` -option, which is the same name of image as used for a string setting. +- when using a string as an option, it must be the full name of the image to use + (including the Registry part if you want to download the image from a Registry + other than Docker Hub) +- when using a map as an option, then it must contain at least the `name` + option, which is the same name of the image as used for the string setting -For example, following definitions are equal: +For example, the following two definitions are equal: -```yaml -image: "registry.example.com/my/image:latest" -services: -- postgresql:9.4 -- redis:latest -``` +1. Using a string as an option to `image` and `services`: -```yaml -image: - name: "registry.example.com/my/image:latest" -services: -- name: postgresql:9.4 -- name: redis:latest -``` + ```yaml + image: "registry.example.com/my/image:latest" + + services: + - postgresql:9.4 + - redis:latest + ``` + +1. Using a map as an option to `image` and `services`. The use of `image:name` is + required: + + ```yaml + image: + name: "registry.example.com/my/image:latest" + + services: + - name: postgresql:9.4 + - name: redis:latest + ``` ### Available settings for `image` +> **Note:** +This feature requires GitLab 9.4 and GitLab Runner 9.4 or higher. + | Setting | Required | Description | |------------|----------|-------------| -| name | yes | Full name of the image that should be used. It should contain the registry part if needed. | -| entrypoint | no | Command or script that should be executed as container's entrypoint. It will be translated to Docker's `--entrypoint` option while creating container. The syntax is similar to `Dockerfile`'s `ENTRYPOINT` directive, where each shell token is a separate string in the array. | +| `name` | yes, when used with any other option | Full name of the image that should be used. It should contain the Registry part if needed. | +| `entrypoint` | no | Command or script that should be executed as the container's entrypoint. It will be translated to Docker's `--entrypoint` option while creating the container. The syntax is similar to [`Dockerfile`'s `ENTRYPOINT`][entrypoint] directive, where each shell token is a separate string in the array. | + +### Available settings for `services` -### Available settings for `services` entry +> **Note:** +This feature requires GitLab 9.4 and GitLab Runner 9.4 or higher. | Setting | Required | Description | |------------|----------|-------------| -| name | yes | Full name of the image that should be used. It should contain the registry part if needed. | -| entrypoint | no | Command or script that should be used as container's entrypoint. It will be translated to Docker's `--entrypoint` option while creating container. The syntax is similar to `Dockerfile`'s `ENTRYPOINT` directive, where each shell token is a separate string in the array. | -| command | no | Command or script and eventual arguments that should be used as container's command. It will be translated to arguments passed to Docker after image's name. The syntax is similar to `Dockerfile`'s `CMD` directive, where each shell token is a separate string in the array. | -| alias | no | Additional alias, that can be used to access service from job's container. Read [Accessing the services](#accessing-the-services) for more information. | - -### Example usages - -You can use this feature for different purposes: +| `name` | yes, when used with any other option | Full name of the image that should be used. It should contain the Registry part if needed. | +| `entrypoint` | no | Command or script that should be executed as the container's entrypoint. It will be translated to Docker's `--entrypoint` option while creating the container. The syntax is similar to [`Dockerfile`'s `ENTRYPOINT`][entrypoint] directive, where each shell token is a separate string in the array. | +| `command` | no | Command or script that should be used as the container's command. It will be translated to arguments passed to Docker after the image's name. The syntax is similar to [`Dockerfile`'s `CMD`][cmd] directive, where each shell token is a separate string in the array. | +| `alias` | no | Additional alias that can be used to access the service from the job's container. Read [Accessing the services](#accessing-the-services) for more information. | -#### Starting multiple services from the same image +### Starting multiple services from the same image -Before adding this feature following configuration will not work properly: +Before the new extended Docker configuration options, the following configuration +would not work properly: ```yaml services: @@ -207,38 +252,41 @@ services: - mysql:latest ``` -Runner would start two containers using the `mysql:latest` image, but both -of them would be added to the job's container with the `mysql` alias. This -would end with one of services not being accessible. +The Runner would start two containers using the `mysql:latest` image, but both +of them would be added to the job's container with the `mysql` alias based on +the [default hostname naming](#accessing-the-services). This would end with one +of the services not being accessible. -Thanks to extended docker configuration we can change above configuration -to a following version: +After the new extended Docker configuration options, the above example would +look like: ```yaml services: - name: mysql:latest alias: mysql-1 - name: mysql:latest - aliase: mysql-2 + alias: mysql-2 ``` -Runner will still start two containers using the `mysql:latest` image, -but now each of them will be also accessible with the alias configured +The Runner will still start two containers using the `mysql:latest` image, +but now each of them will also be accessible with the alias configured in `.gitlab-ci.yml` file. -#### Setting a command for the service +### Setting a command for the service -Let's assume we have a `super/sql:latest` image with some SQL database -inside. We would like to use it as a service for your job. The problem -is that this image doesn't start the database process while starting -the container. It have all installed and configured inside but user needs -to manually use `/usr/bin/super-sql run` as command to start the database. +Let's assume you have a `super/sql:latest` image with some SQL database +inside it and you would like to use it as a service for your job. Let's also +assume that this image doesn't start the database process while starting +the container and the user needs to manually use `/usr/bin/super-sql run` as +a command to start the database. -Previously we would need to create our own image based on the `super/sql:latest` -image, add the default command, and then use it in job's configuration, e.g.: +Before the new extended Docker configuration options, you would need to create +your own image based on the `super/sql:latest` image, add the default command, +and then use it in job's configuration, like: ```Dockerfile # my-super-sql:latest image's Dockerfile + FROM super/sql:latest CMD ["/usr/bin/super-sql", "run"] ``` @@ -250,8 +298,8 @@ services: - my-super-sql:latest ``` -With extended docker configuration we can now do this simple, setting -only proper configuration in `.gitlab-ci.yml`, e.g: +After the new extended Docker configuration options, you can now simply +set a `command` in `.gitlab-ci.yml`, like: ```yaml # .gitlab-ci.yml @@ -261,24 +309,29 @@ services: command: ["/usr/bin/super-sql", "run"] ``` -As you can see the syntax of `command` entry is similar to Dockerfile's `CMD`. +As you can see, the syntax of `command` is similar to [Dockerfile's `CMD`][cmd]. -#### Overriding entrypoint of job's image +### Overriding the entrypoint of an image -Let's assume we have a `super/sql:experimental` image with some SQL database -inside. We would like to use it as base for our job, because we want to -execute some tests with this database binary. The problem is that this image -is configured with `/usr/bin/super-sql run` as entrypoint. That means, that -when starting container without additional options it will run database's -process, while Runner expects that image will have no entrypoint or at least -will start with a shell as entrypoint. +Let's assume you have a `super/sql:experimental` image with some SQL database +inside it and you would like to use it as a base image for your job because you +want to execute some tests with this database binary. Let's also assume that +this image is configured with `/usr/bin/super-sql run` as an entrypoint. That +means, that when starting the container without additional options, it will run +the database's process, while Runner expects that the image will have no +entrypoint or at least will start with a shell as its entrypoint. Previously we would need to create our own image based on the `super/sql:experimental` image, set the entrypoint to a shell, and then use it in job's configuration, e.g.: +Before the new extended Docker configuration options, you would need to create +your own image based on the `super/sql:experimental` image, set the entrypoint +to a shell and then use it in job's configuration, like: + ```Dockerfile # my-super-sql:experimental image's Dockerfile + FROM super/sql:experimental ENTRYPOINT ["/bin/sh"] ``` @@ -289,8 +342,8 @@ ENTRYPOINT ["/bin/sh"] image: my-super-sql:experimental ``` -With docker extended docker configuration we can now do this simple, setting -only proper configuration in `.gitlab-ci.yml`, e.g: +After the new extended Docker configuration options, you can now simply +set an `entrypoint` in `.gitlab-ci.yml`, like: ```yaml # .gitlab-ci.yml @@ -300,7 +353,8 @@ image: entrypoint: ["/bin/sh"] ``` -As you can see the syntax of `entrypoint` entry is similar to Dockerfile's `ENTRYPOINT`. +As you can see the syntax of `entrypoint` is similar to +[Dockerfile's `ENTRYPOINT`][entrypoint]. ## Define image and services in `config.toml` @@ -315,7 +369,7 @@ Look for the `[runners.docker]` section: The image and services defined this way will be added to all job run by that runner. -## Define an image from a private Docker registry +## Define an image from a private Container Registry > **Notes:** - This feature requires GitLab Runner **1.8** or higher @@ -370,48 +424,6 @@ To configure access for `registry.example.com`, follow these steps: You can add configuration for as many registries as you want, adding more registries to the `"auths"` hash as described above. -## Accessing the services - -Let's say that you need a Wordpress instance to test some API integration with -your application. - -You can then use for example the [tutum/wordpress][] image in your -`.gitlab-ci.yml`: - -```yaml -services: -- tutum/wordpress:latest -``` - -When the job is run, `tutum/wordpress` will be started and you will have -access to it from your build container under the hostnames `tutum-wordpress` -(requires GitLab Runner v1.1.0 or newer) and `tutum__wordpress`. - -When using a private registry, the image name also includes a hostname and port -of the registry. - -```yaml -services: -- docker.example.com:5000/wordpress:latest -``` - -The service hostname will also include the registry hostname. Service will be -available under hostnames `docker.example.com-wordpress` (requires GitLab Runner v1.1.0 or newer) -and `docker.example.com__wordpress`. - -*Note: hostname with underscores is not RFC valid and may cause problems in 3rd party applications.* - -The alias hostnames for the service are made from the image name following these -rules: - -1. Everything after `:` is stripped -2. Slash (`/`) is replaced with double underscores (`__`) - primary alias -3. Slash (`/`) is replaced with dash (`-`) - secondary alias, requires GitLab Runner v1.1.0 or newer - -Starting with 9.4 You can also set additional alias using the `alias` setting of `services` -configuration entry. Please read [Extended docker configuration options](#extended-docker-configuration-options) -for more information. - ## Configuring services Many services accept environment variables which allow you to easily change @@ -438,7 +450,7 @@ See the specific documentation for ## How Docker integration works -Below is a high level overview of the steps performed by docker during job +Below is a high level overview of the steps performed by Docker during job time. 1. Create any service container: `mysql`, `postgresql`, `mongodb`, `redis`. @@ -455,7 +467,7 @@ time. ## How to debug a job locally *Note: The following commands are run without root privileges. You should be -able to run docker with your regular user account.* +able to run Docker with your regular user account.* First start with creating a file named `build_script`: @@ -515,3 +527,6 @@ creation. [mysql-hub]: https://hub.docker.com/r/_/mysql/ [runner-priv-reg]: http://docs.gitlab.com/runner/configuration/advanced-configuration.html#using-a-private-container-registry [secret variable]: ../variables/README.md#secret-variables +[entrypoint]: https://docs.docker.com/engine/reference/builder/#entrypoint +[cmd]: https://docs.docker.com/engine/reference/builder/#cmd +[register]: https://docs.gitlab.com/runner/register/ -- cgit v1.2.1 From d1488268b2e31b8f3549c6e1e46955619535cd98 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 4 Jul 2017 12:19:48 +0000 Subject: Simplify authentication logic in the v4 users API for !12445. - Rather than using an explicit check to turn off authentication for the `/users` endpoint, simply call `authenticate_non_get!`. - All `GET` endpoints we wish to restrict already call `authenticated_as_admin!`, and so remain inacessible to anonymous users. - This _does_ open up the `/users/:id` endpoint to anonymous access. It contains the same access check that `/users` users, and so is safe for use here. - More context: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12445#note_34031323 --- lib/api/helpers.rb | 6 ------ lib/api/users.rb | 9 ++++++++- spec/requests/api/users_spec.rb | 20 +++++++++++++++++--- 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a3aec8889d7..2c73a6fdc4e 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -407,11 +407,5 @@ module API exception.status == 500 end - - # Does the current route match the route identified by - # `description`? - def request_matches_route?(method, route) - request.request_method == method && request.path == route - end end end diff --git a/lib/api/users.rb b/lib/api/users.rb index bad4d76b428..5b9d9a71be4 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -4,10 +4,13 @@ module API before do allow_access_with_scope :read_user if request.get? - authenticate! unless request_matches_route?('GET', '/api/v4/users') end resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do + before do + authenticate_non_get! + end + helpers do def find_user(params) id = params[:user_id] || params[:id] @@ -405,6 +408,10 @@ module API end resource :user do + before do + authenticate! + end + desc 'Get the currently authenticated user' do success Entities::UserPublic end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index aa95ae8c7cc..8640c16203e 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -169,6 +169,7 @@ describe API::Users do describe "GET /users/:id" do it "returns a user by id" do get api("/users/#{user.id}", user) + expect(response).to have_http_status(200) expect(json_response['username']).to eq(user.username) end @@ -179,9 +180,22 @@ describe API::Users do expect(json_response['is_admin']).to be_nil end - it "returns a 401 if unauthenticated" do - get api("/users/9998") - expect(response).to have_http_status(401) + context 'for an anonymous user' do + it "returns a user by id" do + get api("/users/#{user.id}") + + expect(response).to have_http_status(200) + expect(json_response['username']).to eq(user.username) + end + + it "returns a 404 if the target user is present but inaccessible" do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(nil, :read_user, user).and_return(false) + + get api("/users/#{user.id}") + + expect(response).to have_http_status(404) + end end it "returns a 404 error if user id not found" do -- cgit v1.2.1 From 2210a71b17d41ca0f1b5b4e619aa7fa8f41ad5cd Mon Sep 17 00:00:00 2001 From: Diego Souza Date: Tue, 4 Jul 2017 15:18:51 +0000 Subject: Remove group modal like remove project modal. Closes #33130 --- app/helpers/groups_helper.rb | 5 +++++ app/views/groups/edit.html.haml | 15 +++++++++------ changelogs/unreleased/33130-remove-group-modal.yml | 4 ++++ spec/features/groups_spec.rb | 12 +++++++++--- 4 files changed, 27 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/33130-remove-group-modal.yml diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index af0b3e9c5bc..8cd61f738e1 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -58,6 +58,11 @@ module GroupsHelper IssuesFinder.new(current_user, group_id: group.id).execute end + def remove_group_message(group) + _("You are going to remove %{group_name}.\nRemoved groups CANNOT be restored!\nAre you ABSOLUTELY sure?") % + { group_name: group.name } + end + private def group_title_link(group, hidable: false) diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml index 7d5add3cc1c..9ebb3894c55 100644 --- a/app/views/groups/edit.html.haml +++ b/app/views/groups/edit.html.haml @@ -45,10 +45,13 @@ .panel.panel-danger .panel-heading Remove group .panel-body - %p - Removing group will cause all child projects and resources to be removed. - %br - %strong Removed group can not be restored! + = form_tag(@group, method: :delete) do + %p + Removing group will cause all child projects and resources to be removed. + %br + %strong Removed group can not be restored! - .form-actions - = link_to 'Remove group', @group, data: {confirm: 'Removed group can not be restored! Are you sure?'}, method: :delete, class: "btn btn-remove" + .form-actions + = button_to 'Remove group', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_group_message(@group) } + += render 'shared/confirm_modal', phrase: @group.path diff --git a/changelogs/unreleased/33130-remove-group-modal.yml b/changelogs/unreleased/33130-remove-group-modal.yml new file mode 100644 index 00000000000..4672d41ded5 --- /dev/null +++ b/changelogs/unreleased/33130-remove-group-modal.yml @@ -0,0 +1,4 @@ +--- +title: "Remove group modal like remove project modal (requires typing + confirmation)" +merge_request: 12569 +author: Diego Souza diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb index ecacca00a61..c1dc7be7088 100644 --- a/spec/features/groups_spec.rb +++ b/spec/features/groups_spec.rb @@ -135,7 +135,7 @@ feature 'Group', feature: true do expect(page).not_to have_content('secret-group') end - describe 'group edit' do + describe 'group edit', js: true do let(:group) { create(:group) } let(:path) { edit_group_path(group) } let(:new_name) { 'new-name' } @@ -157,8 +157,8 @@ feature 'Group', feature: true do end it 'removes group' do - click_link 'Remove group' - + expect { remove_with_confirm('Remove group', group.path) }.to change {Group.count}.by(-1) + expect(group.members.all.count).to be_zero expect(page).to have_content "scheduled for deletion" end end @@ -212,4 +212,10 @@ feature 'Group', feature: true do expect(page).to have_content(nested_group.name) end end + + def remove_with_confirm(button_text, confirm_with) + click_button button_text + fill_in 'confirm_name_input', with: confirm_with + click_button 'Confirm' + end end -- cgit v1.2.1 From 26ac691a688cb569a7345d8f31a406d467240bb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Chojnacki?= Date: Tue, 4 Jul 2017 15:28:34 +0000 Subject: Instrument Unicorn with Ruby exporter --- Gemfile | 1 + Gemfile.lock | 5 +- config/gitlab.yml.example | 4 + config/initializers/1_settings.rb | 6 + config/initializers/8_metrics.rb | 9 +- lib/gitlab/metrics/base_sampler.rb | 94 +++++++++++++ lib/gitlab/metrics/connection_rack_middleware.rb | 45 +++++++ lib/gitlab/metrics/influx_sampler.rb | 101 ++++++++++++++ lib/gitlab/metrics/prometheus.rb | 4 +- lib/gitlab/metrics/sampler.rb | 133 ------------------ lib/gitlab/metrics/unicorn_sampler.rb | 48 +++++++ spec/initializers/8_metrics_spec.rb | 10 +- .../metrics/connection_rack_middleware_spec.rb | 88 ++++++++++++ spec/lib/gitlab/metrics/influx_sampler_spec.rb | 150 +++++++++++++++++++++ spec/lib/gitlab/metrics/sampler_spec.rb | 150 --------------------- spec/lib/gitlab/metrics/unicorn_sampler_spec.rb | 108 +++++++++++++++ 16 files changed, 667 insertions(+), 289 deletions(-) create mode 100644 lib/gitlab/metrics/base_sampler.rb create mode 100644 lib/gitlab/metrics/connection_rack_middleware.rb create mode 100644 lib/gitlab/metrics/influx_sampler.rb delete mode 100644 lib/gitlab/metrics/sampler.rb create mode 100644 lib/gitlab/metrics/unicorn_sampler.rb create mode 100644 spec/lib/gitlab/metrics/connection_rack_middleware_spec.rb create mode 100644 spec/lib/gitlab/metrics/influx_sampler_spec.rb delete mode 100644 spec/lib/gitlab/metrics/sampler_spec.rb create mode 100644 spec/lib/gitlab/metrics/unicorn_sampler_spec.rb diff --git a/Gemfile b/Gemfile index 71d2eb1557c..323827ed01b 100644 --- a/Gemfile +++ b/Gemfile @@ -285,6 +285,7 @@ group :metrics do # Prometheus gem 'prometheus-client-mmap', '~>0.7.0.beta5' + gem 'raindrops', '~> 0.18' end group :development do diff --git a/Gemfile.lock b/Gemfile.lock index f4ddd30da1b..e6ea006356c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -599,7 +599,7 @@ GEM premailer-rails (1.9.7) actionmailer (>= 3, < 6) premailer (~> 1.7, >= 1.7.9) - prometheus-client-mmap (0.7.0.beta5) + prometheus-client-mmap (0.7.0.beta7) mmap2 (~> 2.2.6) pry (0.10.4) coderay (~> 1.1.0) @@ -658,7 +658,7 @@ GEM thor (>= 0.18.1, < 2.0) rainbow (2.2.2) rake - raindrops (0.17.0) + raindrops (0.18.0) rake (10.5.0) rblineprof (0.3.6) debugger-ruby_core_source (~> 1.3) @@ -1062,6 +1062,7 @@ DEPENDENCIES rails-deprecated_sanitizer (~> 1.0.3) rails-i18n (~> 4.0.9) rainbow (~> 2.2) + raindrops (~> 0.18) rblineprof (~> 0.3.6) rdoc (~> 4.2) recaptcha (~> 3.0) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 43a8c0078ca..4b81fd90f59 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -543,6 +543,10 @@ production: &base # enabled: true # host: localhost # port: 3808 + prometheus: + # Time between sampling of unicorn socket metrics, in seconds + # unicorn_sampler_interval: 10 + # # 5. Extra customization diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 8ddf8e4d2e4..cb11d2c34f4 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -494,6 +494,12 @@ Settings.webpack.dev_server['enabled'] ||= false Settings.webpack.dev_server['host'] ||= 'localhost' Settings.webpack.dev_server['port'] ||= 3808 +# +# Prometheus metrics settings +# +Settings['prometheus'] ||= Settingslogic.new({}) +Settings.prometheus['unicorn_sampler_interval'] ||= 10 + # # Testing settings # diff --git a/config/initializers/8_metrics.rb b/config/initializers/8_metrics.rb index a0a63ddf8f0..d56fd7a6cfa 100644 --- a/config/initializers/8_metrics.rb +++ b/config/initializers/8_metrics.rb @@ -119,6 +119,13 @@ def instrument_classes(instrumentation) end # rubocop:enable Metrics/AbcSize +Gitlab::Metrics::UnicornSampler.initialize_instance(Settings.prometheus.unicorn_sampler_interval).start + +Gitlab::Application.configure do |config| + # 0 should be Sentry to catch errors in this middleware + config.middleware.insert(1, Gitlab::Metrics::ConnectionRackMiddleware) +end + if Gitlab::Metrics.enabled? require 'pathname' require 'influxdb' @@ -175,7 +182,7 @@ if Gitlab::Metrics.enabled? GC::Profiler.enable - Gitlab::Metrics::Sampler.new.start + Gitlab::Metrics::InfluxSampler.initialize_instance.start module TrackNewRedisConnections def connect(*args) diff --git a/lib/gitlab/metrics/base_sampler.rb b/lib/gitlab/metrics/base_sampler.rb new file mode 100644 index 00000000000..219accfc029 --- /dev/null +++ b/lib/gitlab/metrics/base_sampler.rb @@ -0,0 +1,94 @@ +require 'logger' +module Gitlab + module Metrics + class BaseSampler + def self.initialize_instance(*args) + raise "#{name} singleton instance already initialized" if @instance + @instance = new(*args) + at_exit(&@instance.method(:stop)) + @instance + end + + def self.instance + @instance + end + + attr_reader :running + + # interval - The sampling interval in seconds. + def initialize(interval) + interval_half = interval.to_f / 2 + + @interval = interval + @interval_steps = (-interval_half..interval_half).step(0.1).to_a + + @mutex = Mutex.new + end + + def enabled? + true + end + + def start + return unless enabled? + + @mutex.synchronize do + return if running + @running = true + + @thread = Thread.new do + sleep(sleep_interval) + + while running + safe_sample + + sleep(sleep_interval) + end + end + end + end + + def stop + @mutex.synchronize do + return unless running + + @running = false + + if @thread + @thread.wakeup if @thread.alive? + @thread.join + @thread = nil + end + end + end + + def safe_sample + sample + rescue => e + Rails.logger.warn("#{self.class}: #{e}, stopping") + stop + end + + def sample + raise NotImplementedError + end + + # Returns the sleep interval with a random adjustment. + # + # The random adjustment is put in place to ensure we: + # + # 1. Don't generate samples at the exact same interval every time (thus + # potentially missing anything that happens in between samples). + # 2. Don't sample data at the same interval two times in a row. + def sleep_interval + while step = @interval_steps.sample + if step != @last_step + @last_step = step + + return @interval + @last_step + end + end + end + end + end +end diff --git a/lib/gitlab/metrics/connection_rack_middleware.rb b/lib/gitlab/metrics/connection_rack_middleware.rb new file mode 100644 index 00000000000..b3da360be8f --- /dev/null +++ b/lib/gitlab/metrics/connection_rack_middleware.rb @@ -0,0 +1,45 @@ +module Gitlab + module Metrics + class ConnectionRackMiddleware + def initialize(app) + @app = app + end + + def self.rack_request_count + @rack_request_count ||= Gitlab::Metrics.counter(:rack_request, 'Rack request count') + end + + def self.rack_response_count + @rack_response_count ||= Gitlab::Metrics.counter(:rack_response, 'Rack response count') + end + + def self.rack_uncaught_errors_count + @rack_uncaught_errors_count ||= Gitlab::Metrics.counter(:rack_uncaught_errors, 'Rack connections handling uncaught errors count') + end + + def self.rack_execution_time + @rack_execution_time ||= Gitlab::Metrics.histogram(:rack_execution_time, 'Rack connection handling execution time', + {}, [0.05, 0.1, 0.25, 0.5, 0.7, 1, 1.5, 2, 2.5, 3, 5, 7, 10]) + end + + def call(env) + method = env['REQUEST_METHOD'].downcase + started = Time.now.to_f + begin + ConnectionRackMiddleware.rack_request_count.increment(method: method) + + status, headers, body = @app.call(env) + + ConnectionRackMiddleware.rack_response_count.increment(method: method, status: status) + [status, headers, body] + rescue + ConnectionRackMiddleware.rack_uncaught_errors_count.increment + raise + ensure + elapsed = Time.now.to_f - started + ConnectionRackMiddleware.rack_execution_time.observe({}, elapsed) + end + end + end + end +end diff --git a/lib/gitlab/metrics/influx_sampler.rb b/lib/gitlab/metrics/influx_sampler.rb new file mode 100644 index 00000000000..6db1dd755b7 --- /dev/null +++ b/lib/gitlab/metrics/influx_sampler.rb @@ -0,0 +1,101 @@ +module Gitlab + module Metrics + # Class that sends certain metrics to InfluxDB at a specific interval. + # + # This class is used to gather statistics that can't be directly associated + # with a transaction such as system memory usage, garbage collection + # statistics, etc. + class InfluxSampler < BaseSampler + # interval - The sampling interval in seconds. + def initialize(interval = Metrics.settings[:sample_interval]) + super(interval) + @last_step = nil + + @metrics = [] + + @last_minor_gc = Delta.new(GC.stat[:minor_gc_count]) + @last_major_gc = Delta.new(GC.stat[:major_gc_count]) + + if Gitlab::Metrics.mri? + require 'allocations' + + Allocations.start + end + end + + def sample + sample_memory_usage + sample_file_descriptors + sample_objects + sample_gc + + flush + ensure + GC::Profiler.clear + @metrics.clear + end + + def flush + Metrics.submit_metrics(@metrics.map(&:to_hash)) + end + + def sample_memory_usage + add_metric('memory_usage', value: System.memory_usage) + end + + def sample_file_descriptors + add_metric('file_descriptors', value: System.file_descriptor_count) + end + + if Metrics.mri? + def sample_objects + sample = Allocations.to_hash + counts = sample.each_with_object({}) do |(klass, count), hash| + name = klass.name + + next unless name + + hash[name] = count + end + + # Symbols aren't allocated so we'll need to add those manually. + counts['Symbol'] = Symbol.all_symbols.length + + counts.each do |name, count| + add_metric('object_counts', { count: count }, type: name) + end + end + else + def sample_objects + end + end + + def sample_gc + time = GC::Profiler.total_time * 1000.0 + stats = GC.stat.merge(total_time: time) + + # We want the difference of GC runs compared to the last sample, not the + # total amount since the process started. + stats[:minor_gc_count] = + @last_minor_gc.compared_with(stats[:minor_gc_count]) + + stats[:major_gc_count] = + @last_major_gc.compared_with(stats[:major_gc_count]) + + stats[:count] = stats[:minor_gc_count] + stats[:major_gc_count] + + add_metric('gc_statistics', stats) + end + + def add_metric(series, values, tags = {}) + prefix = sidekiq? ? 'sidekiq_' : 'rails_' + + @metrics << Metric.new("#{prefix}#{series}", values, tags) + end + + def sidekiq? + Sidekiq.server? + end + end + end +end diff --git a/lib/gitlab/metrics/prometheus.rb b/lib/gitlab/metrics/prometheus.rb index 9d314a56e58..fb7bbc7cfc7 100644 --- a/lib/gitlab/metrics/prometheus.rb +++ b/lib/gitlab/metrics/prometheus.rb @@ -29,8 +29,8 @@ module Gitlab provide_metric(name) || registry.summary(name, docstring, base_labels) end - def gauge(name, docstring, base_labels = {}) - provide_metric(name) || registry.gauge(name, docstring, base_labels) + def gauge(name, docstring, base_labels = {}, multiprocess_mode = :all) + provide_metric(name) || registry.gauge(name, docstring, base_labels, multiprocess_mode) end def histogram(name, docstring, base_labels = {}, buckets = ::Prometheus::Client::Histogram::DEFAULT_BUCKETS) diff --git a/lib/gitlab/metrics/sampler.rb b/lib/gitlab/metrics/sampler.rb deleted file mode 100644 index 0000450d9bb..00000000000 --- a/lib/gitlab/metrics/sampler.rb +++ /dev/null @@ -1,133 +0,0 @@ -module Gitlab - module Metrics - # Class that sends certain metrics to InfluxDB at a specific interval. - # - # This class is used to gather statistics that can't be directly associated - # with a transaction such as system memory usage, garbage collection - # statistics, etc. - class Sampler - # interval - The sampling interval in seconds. - def initialize(interval = Metrics.settings[:sample_interval]) - interval_half = interval.to_f / 2 - - @interval = interval - @interval_steps = (-interval_half..interval_half).step(0.1).to_a - @last_step = nil - - @metrics = [] - - @last_minor_gc = Delta.new(GC.stat[:minor_gc_count]) - @last_major_gc = Delta.new(GC.stat[:major_gc_count]) - - if Gitlab::Metrics.mri? - require 'allocations' - - Allocations.start - end - end - - def start - Thread.new do - Thread.current.abort_on_exception = true - - loop do - sleep(sleep_interval) - - sample - end - end - end - - def sample - sample_memory_usage - sample_file_descriptors - sample_objects - sample_gc - - flush - ensure - GC::Profiler.clear - @metrics.clear - end - - def flush - Metrics.submit_metrics(@metrics.map(&:to_hash)) - end - - def sample_memory_usage - add_metric('memory_usage', value: System.memory_usage) - end - - def sample_file_descriptors - add_metric('file_descriptors', value: System.file_descriptor_count) - end - - if Metrics.mri? - def sample_objects - sample = Allocations.to_hash - counts = sample.each_with_object({}) do |(klass, count), hash| - name = klass.name - - next unless name - - hash[name] = count - end - - # Symbols aren't allocated so we'll need to add those manually. - counts['Symbol'] = Symbol.all_symbols.length - - counts.each do |name, count| - add_metric('object_counts', { count: count }, type: name) - end - end - else - def sample_objects - end - end - - def sample_gc - time = GC::Profiler.total_time * 1000.0 - stats = GC.stat.merge(total_time: time) - - # We want the difference of GC runs compared to the last sample, not the - # total amount since the process started. - stats[:minor_gc_count] = - @last_minor_gc.compared_with(stats[:minor_gc_count]) - - stats[:major_gc_count] = - @last_major_gc.compared_with(stats[:major_gc_count]) - - stats[:count] = stats[:minor_gc_count] + stats[:major_gc_count] - - add_metric('gc_statistics', stats) - end - - def add_metric(series, values, tags = {}) - prefix = sidekiq? ? 'sidekiq_' : 'rails_' - - @metrics << Metric.new("#{prefix}#{series}", values, tags) - end - - def sidekiq? - Sidekiq.server? - end - - # Returns the sleep interval with a random adjustment. - # - # The random adjustment is put in place to ensure we: - # - # 1. Don't generate samples at the exact same interval every time (thus - # potentially missing anything that happens in between samples). - # 2. Don't sample data at the same interval two times in a row. - def sleep_interval - while step = @interval_steps.sample - if step != @last_step - @last_step = step - - return @interval + @last_step - end - end - end - end - end -end diff --git a/lib/gitlab/metrics/unicorn_sampler.rb b/lib/gitlab/metrics/unicorn_sampler.rb new file mode 100644 index 00000000000..f6987252039 --- /dev/null +++ b/lib/gitlab/metrics/unicorn_sampler.rb @@ -0,0 +1,48 @@ +module Gitlab + module Metrics + class UnicornSampler < BaseSampler + def initialize(interval) + super(interval) + end + + def unicorn_active_connections + @unicorn_active_connections ||= Gitlab::Metrics.gauge(:unicorn_active_connections, 'Unicorn active connections', {}, :max) + end + + def unicorn_queued_connections + @unicorn_queued_connections ||= Gitlab::Metrics.gauge(:unicorn_queued_connections, 'Unicorn queued connections', {}, :max) + end + + def enabled? + # Raindrops::Linux.tcp_listener_stats is only present on Linux + unicorn_with_listeners? && Raindrops::Linux.respond_to?(:tcp_listener_stats) + end + + def sample + Raindrops::Linux.tcp_listener_stats(tcp_listeners).each do |addr, stats| + unicorn_active_connections.set({ type: 'tcp', address: addr }, stats.active) + unicorn_queued_connections.set({ type: 'tcp', address: addr }, stats.queued) + end + + Raindrops::Linux.unix_listener_stats(unix_listeners).each do |addr, stats| + unicorn_active_connections.set({ type: 'unix', address: addr }, stats.active) + unicorn_queued_connections.set({ type: 'unix', address: addr }, stats.queued) + end + end + + private + + def tcp_listeners + @tcp_listeners ||= Unicorn.listener_names.grep(%r{\A[^/]+:\d+\z}) + end + + def unix_listeners + @unix_listeners ||= Unicorn.listener_names - tcp_listeners + end + + def unicorn_with_listeners? + defined?(Unicorn) && Unicorn.listener_names.any? + end + end + end +end diff --git a/spec/initializers/8_metrics_spec.rb b/spec/initializers/8_metrics_spec.rb index a507d7f7f2b..d4189f902fd 100644 --- a/spec/initializers/8_metrics_spec.rb +++ b/spec/initializers/8_metrics_spec.rb @@ -1,17 +1,25 @@ require 'spec_helper' -require_relative '../../config/initializers/8_metrics' describe 'instrument_classes', lib: true do let(:config) { double(:config) } + let(:unicorn_sampler) { double(:unicorn_sampler) } + let(:influx_sampler) { double(:influx_sampler) } + before do allow(config).to receive(:instrument_method) allow(config).to receive(:instrument_methods) allow(config).to receive(:instrument_instance_method) allow(config).to receive(:instrument_instance_methods) + allow(Gitlab::Metrics::UnicornSampler).to receive(:initialize_instance).and_return(unicorn_sampler) + allow(Gitlab::Metrics::InfluxSampler).to receive(:initialize_instance).and_return(influx_sampler) + allow(unicorn_sampler).to receive(:start) + allow(influx_sampler).to receive(:start) + allow(Gitlab::Application).to receive(:configure) end it 'can autoload and instrument all files' do + require_relative '../../config/initializers/8_metrics' expect { instrument_classes(config) }.not_to raise_error end end diff --git a/spec/lib/gitlab/metrics/connection_rack_middleware_spec.rb b/spec/lib/gitlab/metrics/connection_rack_middleware_spec.rb new file mode 100644 index 00000000000..94251af305f --- /dev/null +++ b/spec/lib/gitlab/metrics/connection_rack_middleware_spec.rb @@ -0,0 +1,88 @@ +require 'spec_helper' + +describe Gitlab::Metrics::ConnectionRackMiddleware do + let(:app) { double('app') } + subject { described_class.new(app) } + + around do |example| + Timecop.freeze { example.run } + end + + describe '#call' do + let(:status) { 100 } + let(:env) { { 'REQUEST_METHOD' => 'GET' } } + let(:stack_result) { [status, {}, 'body'] } + + before do + allow(app).to receive(:call).and_return(stack_result) + end + + context '@app.call succeeds with 200' do + before do + allow(app).to receive(:call).and_return([200, nil, nil]) + end + + it 'increments response count with status label' do + expect(described_class).to receive_message_chain(:rack_response_count, :increment).with(include(status: 200, method: 'get')) + + subject.call(env) + end + + it 'increments requests count' do + expect(described_class).to receive_message_chain(:rack_request_count, :increment).with(method: 'get') + + subject.call(env) + end + + it 'measures execution time' do + execution_time = 10 + allow(app).to receive(:call) do |*args| + Timecop.freeze(execution_time.seconds) + end + + expect(described_class).to receive_message_chain(:rack_execution_time, :observe).with({}, execution_time) + + subject.call(env) + end + end + + context '@app.call throws exception' do + let(:rack_response_count) { double('rack_response_count') } + + before do + allow(app).to receive(:call).and_raise(StandardError) + allow(described_class).to receive(:rack_response_count).and_return(rack_response_count) + end + + it 'increments exceptions count' do + expect(described_class).to receive_message_chain(:rack_uncaught_errors_count, :increment) + + expect { subject.call(env) }.to raise_error(StandardError) + end + + it 'increments requests count' do + expect(described_class).to receive_message_chain(:rack_request_count, :increment).with(method: 'get') + + expect { subject.call(env) }.to raise_error(StandardError) + end + + it "does't increment response count" do + expect(described_class.rack_response_count).not_to receive(:increment) + + expect { subject.call(env) }.to raise_error(StandardError) + end + + it 'measures execution time' do + execution_time = 10 + allow(app).to receive(:call) do |*args| + Timecop.freeze(execution_time.seconds) + raise StandardError + end + + expect(described_class).to receive_message_chain(:rack_execution_time, :observe).with({}, execution_time) + + expect { subject.call(env) }.to raise_error(StandardError) + end + end + end +end diff --git a/spec/lib/gitlab/metrics/influx_sampler_spec.rb b/spec/lib/gitlab/metrics/influx_sampler_spec.rb new file mode 100644 index 00000000000..0bc68d64276 --- /dev/null +++ b/spec/lib/gitlab/metrics/influx_sampler_spec.rb @@ -0,0 +1,150 @@ +require 'spec_helper' + +describe Gitlab::Metrics::InfluxSampler do + let(:sampler) { described_class.new(5) } + + after do + Allocations.stop if Gitlab::Metrics.mri? + end + + describe '#start' do + it 'runs once and gathers a sample at a given interval' do + expect(sampler).to receive(:sleep).with(a_kind_of(Numeric)).twice + expect(sampler).to receive(:sample).once + expect(sampler).to receive(:running).and_return(false, true, false) + + sampler.start.join + end + end + + describe '#sample' do + it 'samples various statistics' do + expect(sampler).to receive(:sample_memory_usage) + expect(sampler).to receive(:sample_file_descriptors) + expect(sampler).to receive(:sample_objects) + expect(sampler).to receive(:sample_gc) + expect(sampler).to receive(:flush) + + sampler.sample + end + + it 'clears any GC profiles' do + expect(sampler).to receive(:flush) + expect(GC::Profiler).to receive(:clear) + + sampler.sample + end + end + + describe '#flush' do + it 'schedules the metrics using Sidekiq' do + expect(Gitlab::Metrics).to receive(:submit_metrics) + .with([an_instance_of(Hash)]) + + sampler.sample_memory_usage + sampler.flush + end + end + + describe '#sample_memory_usage' do + it 'adds a metric containing the memory usage' do + expect(Gitlab::Metrics::System).to receive(:memory_usage) + .and_return(9000) + + expect(sampler).to receive(:add_metric) + .with(/memory_usage/, value: 9000) + .and_call_original + + sampler.sample_memory_usage + end + end + + describe '#sample_file_descriptors' do + it 'adds a metric containing the amount of open file descriptors' do + expect(Gitlab::Metrics::System).to receive(:file_descriptor_count) + .and_return(4) + + expect(sampler).to receive(:add_metric) + .with(/file_descriptors/, value: 4) + .and_call_original + + sampler.sample_file_descriptors + end + end + + if Gitlab::Metrics.mri? + describe '#sample_objects' do + it 'adds a metric containing the amount of allocated objects' do + expect(sampler).to receive(:add_metric) + .with(/object_counts/, an_instance_of(Hash), an_instance_of(Hash)) + .at_least(:once) + .and_call_original + + sampler.sample_objects + end + + it 'ignores classes without a name' do + expect(Allocations).to receive(:to_hash).and_return({ Class.new => 4 }) + + expect(sampler).not_to receive(:add_metric) + .with('object_counts', an_instance_of(Hash), type: nil) + + sampler.sample_objects + end + end + end + + describe '#sample_gc' do + it 'adds a metric containing garbage collection statistics' do + expect(GC::Profiler).to receive(:total_time).and_return(0.24) + + expect(sampler).to receive(:add_metric) + .with(/gc_statistics/, an_instance_of(Hash)) + .and_call_original + + sampler.sample_gc + end + end + + describe '#add_metric' do + it 'prefixes the series name for a Rails process' do + expect(sampler).to receive(:sidekiq?).and_return(false) + + expect(Gitlab::Metrics::Metric).to receive(:new) + .with('rails_cats', { value: 10 }, {}) + .and_call_original + + sampler.add_metric('cats', value: 10) + end + + it 'prefixes the series name for a Sidekiq process' do + expect(sampler).to receive(:sidekiq?).and_return(true) + + expect(Gitlab::Metrics::Metric).to receive(:new) + .with('sidekiq_cats', { value: 10 }, {}) + .and_call_original + + sampler.add_metric('cats', value: 10) + end + end + + describe '#sleep_interval' do + it 'returns a Numeric' do + expect(sampler.sleep_interval).to be_a_kind_of(Numeric) + end + + # Testing random behaviour is very hard, so treat this test as a basic smoke + # test instead of a very accurate behaviour/unit test. + it 'does not return the same interval twice in a row' do + last = nil + + 100.times do + interval = sampler.sleep_interval + + expect(interval).not_to eq(last) + + last = interval + end + end + end +end diff --git a/spec/lib/gitlab/metrics/sampler_spec.rb b/spec/lib/gitlab/metrics/sampler_spec.rb deleted file mode 100644 index d07ce6f81af..00000000000 --- a/spec/lib/gitlab/metrics/sampler_spec.rb +++ /dev/null @@ -1,150 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Metrics::Sampler do - let(:sampler) { described_class.new(5) } - - after do - Allocations.stop if Gitlab::Metrics.mri? - end - - describe '#start' do - it 'gathers a sample at a given interval' do - expect(sampler).to receive(:sleep).with(a_kind_of(Numeric)) - expect(sampler).to receive(:sample) - expect(sampler).to receive(:loop).and_yield - - sampler.start.join - end - end - - describe '#sample' do - it 'samples various statistics' do - expect(sampler).to receive(:sample_memory_usage) - expect(sampler).to receive(:sample_file_descriptors) - expect(sampler).to receive(:sample_objects) - expect(sampler).to receive(:sample_gc) - expect(sampler).to receive(:flush) - - sampler.sample - end - - it 'clears any GC profiles' do - expect(sampler).to receive(:flush) - expect(GC::Profiler).to receive(:clear) - - sampler.sample - end - end - - describe '#flush' do - it 'schedules the metrics using Sidekiq' do - expect(Gitlab::Metrics).to receive(:submit_metrics) - .with([an_instance_of(Hash)]) - - sampler.sample_memory_usage - sampler.flush - end - end - - describe '#sample_memory_usage' do - it 'adds a metric containing the memory usage' do - expect(Gitlab::Metrics::System).to receive(:memory_usage) - .and_return(9000) - - expect(sampler).to receive(:add_metric) - .with(/memory_usage/, value: 9000) - .and_call_original - - sampler.sample_memory_usage - end - end - - describe '#sample_file_descriptors' do - it 'adds a metric containing the amount of open file descriptors' do - expect(Gitlab::Metrics::System).to receive(:file_descriptor_count) - .and_return(4) - - expect(sampler).to receive(:add_metric) - .with(/file_descriptors/, value: 4) - .and_call_original - - sampler.sample_file_descriptors - end - end - - if Gitlab::Metrics.mri? - describe '#sample_objects' do - it 'adds a metric containing the amount of allocated objects' do - expect(sampler).to receive(:add_metric) - .with(/object_counts/, an_instance_of(Hash), an_instance_of(Hash)) - .at_least(:once) - .and_call_original - - sampler.sample_objects - end - - it 'ignores classes without a name' do - expect(Allocations).to receive(:to_hash).and_return({ Class.new => 4 }) - - expect(sampler).not_to receive(:add_metric) - .with('object_counts', an_instance_of(Hash), type: nil) - - sampler.sample_objects - end - end - end - - describe '#sample_gc' do - it 'adds a metric containing garbage collection statistics' do - expect(GC::Profiler).to receive(:total_time).and_return(0.24) - - expect(sampler).to receive(:add_metric) - .with(/gc_statistics/, an_instance_of(Hash)) - .and_call_original - - sampler.sample_gc - end - end - - describe '#add_metric' do - it 'prefixes the series name for a Rails process' do - expect(sampler).to receive(:sidekiq?).and_return(false) - - expect(Gitlab::Metrics::Metric).to receive(:new) - .with('rails_cats', { value: 10 }, {}) - .and_call_original - - sampler.add_metric('cats', value: 10) - end - - it 'prefixes the series name for a Sidekiq process' do - expect(sampler).to receive(:sidekiq?).and_return(true) - - expect(Gitlab::Metrics::Metric).to receive(:new) - .with('sidekiq_cats', { value: 10 }, {}) - .and_call_original - - sampler.add_metric('cats', value: 10) - end - end - - describe '#sleep_interval' do - it 'returns a Numeric' do - expect(sampler.sleep_interval).to be_a_kind_of(Numeric) - end - - # Testing random behaviour is very hard, so treat this test as a basic smoke - # test instead of a very accurate behaviour/unit test. - it 'does not return the same interval twice in a row' do - last = nil - - 100.times do - interval = sampler.sleep_interval - - expect(interval).not_to eq(last) - - last = interval - end - end - end -end diff --git a/spec/lib/gitlab/metrics/unicorn_sampler_spec.rb b/spec/lib/gitlab/metrics/unicorn_sampler_spec.rb new file mode 100644 index 00000000000..dc0d1f2e940 --- /dev/null +++ b/spec/lib/gitlab/metrics/unicorn_sampler_spec.rb @@ -0,0 +1,108 @@ +require 'spec_helper' + +describe Gitlab::Metrics::UnicornSampler do + subject { described_class.new(1.second) } + + describe '#sample' do + let(:unicorn) { double('unicorn') } + let(:raindrops) { double('raindrops') } + let(:stats) { double('stats') } + + before do + stub_const('Unicorn', unicorn) + stub_const('Raindrops::Linux', raindrops) + allow(raindrops).to receive(:unix_listener_stats).and_return({}) + allow(raindrops).to receive(:tcp_listener_stats).and_return({}) + end + + context 'unicorn listens on unix sockets' do + let(:socket_address) { '/some/sock' } + let(:sockets) { [socket_address] } + + before do + allow(unicorn).to receive(:listener_names).and_return(sockets) + end + + it 'samples socket data' do + expect(raindrops).to receive(:unix_listener_stats).with(sockets) + + subject.sample + end + + context 'stats collected' do + before do + allow(stats).to receive(:active).and_return('active') + allow(stats).to receive(:queued).and_return('queued') + allow(raindrops).to receive(:unix_listener_stats).and_return({ socket_address => stats }) + end + + it 'updates metrics type unix and with addr' do + labels = { type: 'unix', address: socket_address } + + expect(subject).to receive_message_chain(:unicorn_active_connections, :set).with(labels, 'active') + expect(subject).to receive_message_chain(:unicorn_queued_connections, :set).with(labels, 'queued') + + subject.sample + end + end + end + + context 'unicorn listens on tcp sockets' do + let(:tcp_socket_address) { '0.0.0.0:8080' } + let(:tcp_sockets) { [tcp_socket_address] } + + before do + allow(unicorn).to receive(:listener_names).and_return(tcp_sockets) + end + + it 'samples socket data' do + expect(raindrops).to receive(:tcp_listener_stats).with(tcp_sockets) + + subject.sample + end + + context 'stats collected' do + before do + allow(stats).to receive(:active).and_return('active') + allow(stats).to receive(:queued).and_return('queued') + allow(raindrops).to receive(:tcp_listener_stats).and_return({ tcp_socket_address => stats }) + end + + it 'updates metrics type unix and with addr' do + labels = { type: 'tcp', address: tcp_socket_address } + + expect(subject).to receive_message_chain(:unicorn_active_connections, :set).with(labels, 'active') + expect(subject).to receive_message_chain(:unicorn_queued_connections, :set).with(labels, 'queued') + + subject.sample + end + end + end + end + + describe '#start' do + context 'when enabled' do + before do + allow(subject).to receive(:enabled?).and_return(true) + end + + it 'creates new thread' do + expect(Thread).to receive(:new) + + subject.start + end + end + + context 'when disabled' do + before do + allow(subject).to receive(:enabled?).and_return(false) + end + + it "doesn't create new thread" do + expect(Thread).not_to receive(:new) + + subject.start + end + end + end +end -- cgit v1.2.1