diff options
24 files changed, 272 insertions, 128 deletions
diff --git a/app/assets/javascripts/pipelines/components/pipelines_actions.vue b/app/assets/javascripts/pipelines/components/pipelines_actions.vue index 01dfe51cc17..c4c63a52358 100644 --- a/app/assets/javascripts/pipelines/components/pipelines_actions.vue +++ b/app/assets/javascripts/pipelines/components/pipelines_actions.vue @@ -69,8 +69,7 @@ @click="onClickAction(action.path)" :class="{ disabled: isActionDisabled(action) }" :disabled="isActionDisabled(action)"> - <span v-html="playIconSvg"></span> - <span>{{action.name}}</span> + {{action.name}} </button> </li> </ul> diff --git a/app/assets/javascripts/pipelines/components/pipelines_artifacts.vue b/app/assets/javascripts/pipelines/components/pipelines_artifacts.vue index b19bd509a00..751a20991af 100644 --- a/app/assets/javascripts/pipelines/components/pipelines_artifacts.vue +++ b/app/assets/javascripts/pipelines/components/pipelines_artifacts.vue @@ -39,11 +39,7 @@ rel="nofollow" download :href="artifact.path"> - <i - class="fa fa-download" - aria-hidden="true"> - </i> - <span>Download {{artifact.name}} artifacts</span> + Download {{artifact.name}} artifacts </a> </li> </ul> diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index e8efe8fab27..82bceddf1f0 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -125,7 +125,7 @@ module GroupsHelper end def default_help - s_("GroupSettings|This setting will be applied to all subgroups unless overridden by a group owner.") + s_("GroupSettings|This setting will be applied to all subgroups unless overridden by a group owner. Groups that already have access to the project will continue to have access unless removed manually.") end def ancestor_locked_but_you_can_override(group) diff --git a/app/helpers/sorting_helper.rb b/app/helpers/sorting_helper.rb index b408ec0c6a4..c4a73bedbcd 100644 --- a/app/helpers/sorting_helper.rb +++ b/app/helpers/sorting_helper.rb @@ -79,111 +79,111 @@ module SortingHelper end def sort_title_priority - 'Priority' + s_('SortOptions|Priority') end def sort_title_label_priority - 'Label priority' + s_('SortOptions|Label priority') end def sort_title_oldest_updated - 'Oldest updated' + s_('SortOptions|Oldest updated') end def sort_title_recently_updated - 'Last updated' + s_('SortOptions|Last updated') end def sort_title_oldest_activity - 'Oldest updated' + s_('SortOptions|Oldest updated') end def sort_title_latest_activity - 'Last updated' + s_('SortOptions|Last updated') end def sort_title_oldest_created - 'Oldest created' + s_('SortOptions|Oldest created') end def sort_title_recently_created - 'Last created' + s_('SortOptions|Last created') end def sort_title_milestone_soon - 'Milestone due soon' + s_('SortOptions|Milestone due soon') end def sort_title_milestone_later - 'Milestone due later' + s_('SortOptions|Milestone due later') end def sort_title_due_date_soon - 'Due soon' + s_('SortOptions|Due soon') end def sort_title_due_date_later - 'Due later' + s_('SortOptions|Due later') end def sort_title_start_date_soon - 'Start soon' + s_('SortOptions|Start soon') end def sort_title_start_date_later - 'Start later' + s_('SortOptions|Start later') end def sort_title_name - 'Name' + s_('SortOptions|Name') end def sort_title_largest_repo - 'Largest repository' + s_('SortOptions|Largest repository') end def sort_title_largest_group - 'Largest group' + s_('SortOptions|Largest group') end def sort_title_recently_signin - 'Recent sign in' + s_('SortOptions|Recent sign in') end def sort_title_oldest_signin - 'Oldest sign in' + s_('SortOptions|Oldest sign in') end def sort_title_downvotes - 'Least popular' + s_('SortOptions|Least popular') end def sort_title_upvotes - 'Most popular' + s_('SortOptions|Most popular') end def sort_title_last_joined - 'Last joined' + s_('SortOptions|Last joined') end def sort_title_oldest_joined - 'Oldest joined' + s_('SortOptions|Oldest joined') end def sort_title_access_level_asc - 'Access level, ascending' + s_('SortOptions|Access level, ascending') end def sort_title_access_level_desc - 'Access level, descending' + s_('SortOptions|Access level, descending') end def sort_title_name_asc - 'Name, ascending' + s_('SortOptions|Name, ascending') end def sort_title_name_desc - 'Name, descending' + s_('SortOptions|Name, descending') end def sort_value_last_joined diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml index 05c1d2b383c..49101d1efa4 100644 --- a/app/views/projects/branches/_branch.html.haml +++ b/app/views/projects/branches/_branch.html.haml @@ -13,20 +13,23 @@ - if branch.name == @repository.root_ref %span.label.label-primary default - elsif @repository.merged_to_root_ref? branch.name - %span.label.label-info.has-tooltip{ title: "Merged into #{@repository.root_ref}" } - merged + %span.label.label-info.has-tooltip{ title: s_('Branches|Merged into %{default_branch}') % { default_branch: @repository.root_ref } } + = s_('Branches|merged') - if protected_branch?(@project, branch) %span.label.label-success - protected + = s_('Branches|protected') .controls.hidden-xs< - if merge_project && create_mr_button?(@repository.root_ref, branch.name) = link_to create_mr_path(@repository.root_ref, branch.name), class: 'btn btn-default' do - Merge request + = _('Merge request') - if branch.name != @repository.root_ref - = link_to project_compare_index_path(@project, from: @repository.root_ref, to: branch.name), class: "btn btn-default #{'prepend-left-10' unless merge_project}", method: :post, title: "Compare" do - Compare + = link_to project_compare_index_path(@project, from: @repository.root_ref, to: branch.name), + class: "btn btn-default #{'prepend-left-10' unless merge_project}", + method: :post, + title: s_('Branches|Compare') do + = s_('Branches|Compare') = render 'projects/buttons/download', project: @project, ref: branch.name, pipeline: @refs_pipelines[branch.name] @@ -34,12 +37,12 @@ - if branch.name == @project.repository.root_ref %button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip disabled", disabled: true, - title: "The default branch cannot be deleted" } + title: s_('Branches|The default branch cannot be deleted') } = icon("trash-o") - elsif protected_branch?(@project, branch) - if can?(current_user, :delete_protected_branch, @project) %button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip", - title: "Delete protected branch", + title: s_('Branches|Delete protected branch'), data: { toggle: "modal", target: "#modal-delete-branch", delete_path: project_branch_path(@project, branch.name), @@ -49,20 +52,22 @@ - else %button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip disabled", disabled: true, - title: "Only a project master or owner can delete a protected branch" } + title: s_('Branches|Only a project master or owner can delete a protected branch') } = icon("trash-o") - else = link_to project_branch_path(@project, branch.name), class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip", - title: "Delete branch", + title: s_('Branches|Delete branch'), method: :delete, - data: { confirm: "Deleting the '#{branch.name}' branch cannot be undone. Are you sure?" }, + data: { confirm: s_("Branches|Deleting the '%{branch_name}' branch cannot be undone. Are you sure?") % { branch_name: branch.name } }, remote: true, - "aria-label" => "Delete branch" do + 'aria-label' => s_('Branches|Delete branch') do = icon("trash-o") - if branch.name != @repository.root_ref - .divergence-graph{ title: "#{number_commits_behind} commits behind #{@repository.root_ref}, #{number_commits_ahead} commits ahead" } + .divergence-graph{ title: s_('%{number_commits_behind} commits behind %{default_branch}, %{number_commits_ahead} commits ahead') % { number_commits_behind: number_commits_behind, + default_branch: @repository.root_ref, + number_commits_ahead: number_commits_ahead } } .graph-side .bar.bar-behind{ style: "width: #{number_commits_behind * bar_graph_width_factor}%" } %span.count.count-behind= number_commits_behind @@ -76,4 +81,4 @@ = render 'projects/branches/commit', commit: commit, project: @project - else %p - Cant find HEAD commit for this branch + = s_('Branches|Cant find HEAD commit for this branch') diff --git a/app/views/projects/branches/_delete_protected_modal.html.haml b/app/views/projects/branches/_delete_protected_modal.html.haml index f00a0ee6925..e0008e322a0 100644 --- a/app/views/projects/branches/_delete_protected_modal.html.haml +++ b/app/views/projects/branches/_delete_protected_modal.html.haml @@ -4,36 +4,38 @@ .modal-header %button.close{ data: { dismiss: 'modal' } } × %h3.page-title - Delete protected branch - = surround "'", "'?" do + - title_branch_name = capture do %span.js-branch-name.ref-name>[branch name] + = s_("Branches|Delete protected branch '%{branch_name}'?").html_safe % { branch_name: title_branch_name } .modal-body %p - You’re about to permanently delete the protected branch - = succeed '.' do - %strong.js-branch-name.ref-name [branch name] + - branch_name = capture do + %strong.js-branch-name.ref-name>[branch name] + = s_('Branches|You’re about to permanently delete the protected branch %{branch_name}.').html_safe % { branch_name: branch_name } %p.js-not-merged - default_branch = capture do %span.ref-name= @repository.root_ref - = s_("Branches|This branch hasn’t been merged into %{default_branch}.").html_safe % { default_branch: default_branch } - = s_("Branches|To avoid data loss, consider merging this branch before deleting it.") + = s_('Branches|This branch hasn’t been merged into %{default_branch}.').html_safe % { default_branch: default_branch } + = s_('Branches|To avoid data loss, consider merging this branch before deleting it.') %p - Once you confirm and press - = succeed ',' do - %strong Delete protected branch - it cannot be undone or recovered. + - delete_protected_branch = capture do + %strong + = s_('Branches|Delete protected branch') + = s_('Branches|Once you confirm and press %{delete_protected_branch}, it cannot be undone or recovered.').html_safe % { delete_protected_branch: delete_protected_branch } %p - %strong To confirm, type - %kbd.js-branch-name [branch name] + - branch_name_confirmation = capture do + %kbd.js-branch-name [branch name] + %strong + = s_('Branches|To confirm, type %{branch_name_confirmation}:').html_safe % { branch_name_confirmation: branch_name_confirmation } .form-group = text_field_tag 'delete_branch_input', '', class: 'form-control js-delete-branch-input' .modal-footer %button.btn{ data: { dismiss: 'modal' } } Cancel - = link_to 'Delete protected branch', '', + = link_to s_('Branches|Delete protected branch'), '', class: "btn btn-danger js-delete-branch", - title: 'Delete branch', + title: s_('Branches|Delete branch'), method: :delete, - "aria-label" => "Delete" + 'aria-label' => s_('Branches|Delete branch') diff --git a/app/views/projects/branches/index.html.haml b/app/views/projects/branches/index.html.haml index 73583c6bbc2..ea6e7e9db6c 100644 --- a/app/views/projects/branches/index.html.haml +++ b/app/views/projects/branches/index.html.haml @@ -1,17 +1,17 @@ - @no_container = true -- page_title "Branches" +- page_title _('Branches') = render "projects/commits/head" %div{ class: container_class } .top-area.adjust - if can?(current_user, :admin_project, @project) .nav-text - Protected branches can be managed in - = link_to 'project settings', project_protected_branches_path(@project) + - project_settings_link = link_to s_('Branches|project settings'), project_protected_branches_path(@project) + = s_('Branches|Protected branches can be managed in %{project_settings_link}').html_safe % { project_settings_link: project_settings_link } .nav-controls = form_tag(filter_branches_path, method: :get) do - = search_field_tag :search, params[:search], { placeholder: 'Filter by branch name', id: 'branch-search', class: 'form-control search-text-input input-short', spellcheck: false } + = search_field_tag :search, params[:search], { placeholder: s_('Branches|Filter by branch name'), id: 'branch-search', class: 'form-control search-text-input input-short', spellcheck: false } .dropdown.inline> %button.dropdown-menu-toggle{ type: 'button', 'data-toggle' => 'dropdown' } @@ -20,16 +20,21 @@ = icon('chevron-down') %ul.dropdown-menu.dropdown-menu-align-right.dropdown-menu-selectable %li.dropdown-header - Sort by + = s_('Branches|Sort by') - branches_sort_options_hash.each do |value, title| %li = link_to title, filter_branches_path(sort: value), class: ("is-active" if @sort == value) - if can? current_user, :push_code, @project - = link_to project_merged_branches_path(@project), class: 'btn btn-inverted btn-remove has-tooltip', title: "Delete all branches that are merged into '#{@project.repository.root_ref}'", method: :delete, data: { confirm: "Deleting the merged branches cannot be undone. Are you sure?", container: 'body' } do - Delete merged branches + = link_to project_merged_branches_path(@project), + class: 'btn btn-inverted btn-remove has-tooltip', + title: s_("Branches|Delete all branches that are merged into '%{default_branch}'") % { default_branch: @project.repository.root_ref }, + method: :delete, + data: { confirm: s_('Branches|Deleting the merged branches cannot be undone. Are you sure?'), + container: 'body' } do + = s_('Branches|Delete merged branches') = link_to new_project_branch_path(@project), class: 'btn btn-create' do - New branch + = s_('Branches|New branch') - if @branches.any? %ul.content-list.all-branches @@ -37,6 +42,7 @@ = render "projects/branches/branch", branch: branch = paginate @branches, theme: 'gitlab' - else - .nothing-here-block No branches to show + .nothing-here-block + = s_('Branches|No branches to show') = render 'projects/branches/delete_protected_modal' diff --git a/app/views/projects/buttons/_download.html.haml b/app/views/projects/buttons/_download.html.haml index 9d85e027ac9..fa6f6d0b588 100644 --- a/app/views/projects/buttons/_download.html.haml +++ b/app/views/projects/buttons/_download.html.haml @@ -11,19 +11,15 @@ #{ _('Source code') } %li = link_to archive_project_repository_path(project, ref: ref, format: 'zip'), rel: 'nofollow', download: '' do - %i.fa.fa-download %span= _('Download zip') %li = link_to archive_project_repository_path(project, ref: ref, format: 'tar.gz'), rel: 'nofollow', download: '' do - %i.fa.fa-download %span= _('Download tar.gz') %li = link_to archive_project_repository_path(project, ref: ref, format: 'tar.bz2'), rel: 'nofollow', download: '' do - %i.fa.fa-download %span= _('Download tar.bz2') %li = link_to archive_project_repository_path(project, ref: ref, format: 'tar'), rel: 'nofollow', download: '' do - %i.fa.fa-download %span= _('Download tar') - if pipeline && pipeline.latest_builds_with_artifacts.any? @@ -36,6 +32,5 @@ - pipeline.latest_builds_with_artifacts.each do |job| %li = link_to latest_succeeded_project_artifacts_path(project, "#{ref}/download", job: job.name), rel: 'nofollow', download: '' do - %i.fa.fa-download %span #{s_('DownloadArtifacts|Download')} '#{job.name}' diff --git a/app/views/projects/buttons/_dropdown.html.haml b/app/views/projects/buttons/_dropdown.html.haml index b04d6a1fa5e..2589c53beae 100644 --- a/app/views/projects/buttons/_dropdown.html.haml +++ b/app/views/projects/buttons/_dropdown.html.haml @@ -11,19 +11,16 @@ - if can_create_issue %li = link_to new_project_issue_path(@project) do - = icon('exclamation-circle fw') #{ _('New issue') } - if merge_project %li = link_to project_new_merge_request_path(merge_project) do - = icon('tasks fw') #{ _('New merge request') } - if can_create_snippet %li = link_to new_project_snippet_path(@project) do - = icon('file-text-o fw') #{ _('New snippet') } - if can_create_issue || merge_project || can_create_snippet @@ -32,20 +29,16 @@ - if can?(current_user, :push_code, @project) %li = link_to project_new_blob_path(@project, @project.default_branch || 'master') do - = icon('file fw') #{ _('New file') } %li = link_to new_project_branch_path(@project) do - = icon('code-fork fw') #{ _('New branch') } %li = link_to new_project_tag_path(@project) do - = icon('tags fw') #{ _('New tag') } - elsif current_user && current_user.already_forked?(@project) %li = link_to project_new_blob_path(@project, @project.default_branch || 'master') do - = icon('file fw') #{ _('New file') } - elsif can?(current_user, :fork_project, @project) %li @@ -55,5 +48,4 @@ - fork_path = project_forks_path(@project, namespace_key: current_user.namespace.id, continue: continue_params) = link_to fork_path, method: :post do - = icon('file fw') #{ _('New file') } diff --git a/app/views/projects/tree/_old_tree_header.html.haml b/app/views/projects/tree/_old_tree_header.html.haml index 13705ca303b..3a43dde8052 100644 --- a/app/views/projects/tree/_old_tree_header.html.haml +++ b/app/views/projects/tree/_old_tree_header.html.haml @@ -20,15 +20,12 @@ - if can_edit_tree? %li = link_to project_new_blob_path(@project, @id) do - = icon('pencil fw') #{ _('New file') } %li = link_to '#modal-upload-blob', { 'data-target' => '#modal-upload-blob', 'data-toggle' => 'modal' } do - = icon('file fw') #{ _('Upload file') } %li = link_to '#modal-create-new-dir', { 'data-target' => '#modal-create-new-dir', 'data-toggle' => 'modal' } do - = icon('folder fw') #{ _('New directory') } - elsif can?(current_user, :fork_project, @project) %li @@ -38,7 +35,6 @@ - fork_path = project_forks_path(@project, namespace_key: current_user.namespace.id, continue: continue_params) = link_to fork_path, method: :post do - = icon('pencil fw') #{ _('New file') } %li - continue_params = { to: request.fullpath, @@ -47,7 +43,6 @@ - fork_path = project_forks_path(@project, namespace_key: current_user.namespace.id, continue: continue_params) = link_to fork_path, method: :post do - = icon('file fw') #{ _('Upload file') } %li - continue_params = { to: request.fullpath, @@ -56,15 +51,12 @@ - fork_path = project_forks_path(@project, namespace_key: current_user.namespace.id, continue: continue_params) = link_to fork_path, method: :post do - = icon('folder fw') #{ _('New directory') } %li.divider %li = link_to new_project_branch_path(@project) do - = icon('code-fork fw') #{ _('New branch') } %li = link_to new_project_tag_path(@project) do - = icon('tags fw') #{ _('New tag') } diff --git a/app/views/shared/boards/_show.html.haml b/app/views/shared/boards/_show.html.haml index 2e5e7911981..722890a01e5 100644 --- a/app/views/shared/boards/_show.html.haml +++ b/app/views/shared/boards/_show.html.haml @@ -31,10 +31,11 @@ ":board-id" => "boardId", ":key" => "_uid" } = render "shared/boards/components/sidebar" - %board-add-issues-modal{ "new-issue-path" => new_project_issue_path(@project), - "milestone-path" => milestones_filter_dropdown_path, - "label-path" => labels_filter_path, - "empty-state-svg" => image_path('illustrations/issues.svg'), - ":issue-link-base" => "issueLinkBase", - ":root-path" => "rootPath", - ":project-id" => @project.try(:id) } + - if @project + %board-add-issues-modal{ "new-issue-path" => new_project_issue_path(@project), + "milestone-path" => milestones_filter_dropdown_path, + "label-path" => labels_filter_path, + "empty-state-svg" => image_path('illustrations/issues.svg'), + ":issue-link-base" => "issueLinkBase", + ":root-path" => "rootPath", + ":project-id" => @project.id } diff --git a/changelogs/unreleased/36549-circuit-breaker-handles-missing-storages.yml b/changelogs/unreleased/36549-circuit-breaker-handles-missing-storages.yml new file mode 100644 index 00000000000..f5ccb163d98 --- /dev/null +++ b/changelogs/unreleased/36549-circuit-breaker-handles-missing-storages.yml @@ -0,0 +1,5 @@ +--- +title: Allow the git circuit breaker to correctly handle missing repository storages +merge_request: 14417 +author: +type: fixed diff --git a/doc/development/ux_guide/basics.md b/doc/development/ux_guide/basics.md index dcd5f677f25..e215026bcca 100644 --- a/doc/development/ux_guide/basics.md +++ b/doc/development/ux_guide/basics.md @@ -33,7 +33,7 @@ This is the typeface used for code blocks and references to commits, branches, a ## Icons -GitLab has a strong, unique personality. When you look at any screen, you should know immediately know that it is GitLab. +GitLab has a strong, unique personality. When you look at any screen, you should know immediately that it is GitLab. Iconography is a powerful visual cue to the user and is a great way for us to reflect our particular sense of style. - **Standard size:** 16px * 16px diff --git a/lib/gitlab/data_builder/push.rb b/lib/gitlab/data_builder/push.rb index 4ab5b3455a5..31a46a738c3 100644 --- a/lib/gitlab/data_builder/push.rb +++ b/lib/gitlab/data_builder/push.rb @@ -64,8 +64,11 @@ module Gitlab # For performance purposes maximum 20 latest commits # will be passed as post receive hook data. - commit_attrs = commits_limited.map do |commit| - commit.hook_attrs(with_changed_files: true) + # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/38259 + commit_attrs = Gitlab::GitalyClient.allow_n_plus_1_calls do + commits_limited.map do |commit| + commit.hook_attrs(with_changed_files: true) + end end type = Gitlab::Git.tag_ref?(ref) ? 'tag_push' : 'push' diff --git a/lib/gitlab/git/storage.rb b/lib/gitlab/git/storage.rb index e28be4b8a38..08e6c29abad 100644 --- a/lib/gitlab/git/storage.rb +++ b/lib/gitlab/git/storage.rb @@ -11,6 +11,7 @@ module Gitlab end CircuitOpen = Class.new(Inaccessible) + Misconfiguration = Class.new(Inaccessible) REDIS_KEY_PREFIX = 'storage_accessible:'.freeze diff --git a/lib/gitlab/git/storage/circuit_breaker.rb b/lib/gitlab/git/storage/circuit_breaker.rb index 9ea9367d4b7..1eaa2d83fb6 100644 --- a/lib/gitlab/git/storage/circuit_breaker.rb +++ b/lib/gitlab/git/storage/circuit_breaker.rb @@ -28,14 +28,26 @@ module Gitlab def self.for_storage(storage) cached_circuitbreakers = RequestStore.fetch(:circuitbreaker_cache) do Hash.new do |hash, storage_name| - hash[storage_name] = new(storage_name) + hash[storage_name] = build(storage_name) end end cached_circuitbreakers[storage] end - def initialize(storage, hostname = Gitlab::Environment.hostname) + def self.build(storage, hostname = Gitlab::Environment.hostname) + config = Gitlab.config.repositories.storages[storage] + + if !config.present? + NullCircuitBreaker.new(storage, hostname, error: Misconfiguration.new("Storage '#{storage}' is not configured")) + elsif !config['path'].present? + NullCircuitBreaker.new(storage, hostname, error: Misconfiguration.new("Path for storage '#{storage}' is not configured")) + else + new(storage, hostname) + end + end + + def initialize(storage, hostname) @storage = storage @hostname = hostname @@ -64,6 +76,10 @@ module Gitlab recent_failure || too_many_failures end + def failure_info + @failure_info ||= get_failure_info + end + # Memoizing the `storage_available` call means we only do it once per # request when the storage is available. # @@ -121,10 +137,12 @@ module Gitlab end end - def failure_info - @failure_info ||= get_failure_info + def cache_key + @cache_key ||= "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}#{storage}:#{hostname}" end + private + def get_failure_info last_failure, failure_count = Gitlab::Git::Storage.redis.with do |redis| redis.hmget(cache_key, :last_failure, :failure_count) @@ -134,10 +152,6 @@ module Gitlab FailureInfo.new(last_failure, failure_count.to_i) end - - def cache_key - @cache_key ||= "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}#{storage}:#{hostname}" - end end end end diff --git a/lib/gitlab/git/storage/health.rb b/lib/gitlab/git/storage/health.rb index 2d723147f4f..1564e94b7f7 100644 --- a/lib/gitlab/git/storage/health.rb +++ b/lib/gitlab/git/storage/health.rb @@ -78,7 +78,7 @@ module Gitlab def failing_circuit_breakers @failing_circuit_breakers ||= failing_on_hosts.map do |hostname| - CircuitBreaker.new(storage_name, hostname) + CircuitBreaker.build(storage_name, hostname) end end diff --git a/lib/gitlab/git/storage/null_circuit_breaker.rb b/lib/gitlab/git/storage/null_circuit_breaker.rb new file mode 100644 index 00000000000..297c043d054 --- /dev/null +++ b/lib/gitlab/git/storage/null_circuit_breaker.rb @@ -0,0 +1,47 @@ +module Gitlab + module Git + module Storage + class NullCircuitBreaker + # These will have actual values + attr_reader :storage, + :hostname + + # These will always have nil values + attr_reader :storage_path, + :failure_wait_time, + :failure_reset_time, + :storage_timeout + + def initialize(storage, hostname, error: nil) + @storage = storage + @hostname = hostname + @error = error + end + + def perform + @error ? raise(@error) : yield + end + + def circuit_broken? + !!@error + end + + def failure_count_threshold + 1 + end + + def last_failure + circuit_broken? ? Time.now : nil + end + + def failure_count + circuit_broken? ? 1 : 0 + end + + def failure_info + Gitlab::Git::Storage::CircuitBreaker::FailureInfo.new(last_failure, failure_count) + end + end + end + end +end diff --git a/lib/gitlab/health_checks/fs_shards_check.rb b/lib/gitlab/health_checks/fs_shards_check.rb index a1905b091b4..afaa59b1018 100644 --- a/lib/gitlab/health_checks/fs_shards_check.rb +++ b/lib/gitlab/health_checks/fs_shards_check.rb @@ -125,7 +125,7 @@ module Gitlab end def storage_circuitbreaker_test(storage_name) - Gitlab::Git::Storage::CircuitBreaker.new(storage_name).perform { "OK" } + Gitlab::Git::Storage::CircuitBreaker.build(storage_name).perform { "OK" } rescue Gitlab::Git::Storage::Inaccessible nil end diff --git a/spec/javascripts/pipelines/pipelines_artifacts_spec.js b/spec/javascripts/pipelines/pipelines_artifacts_spec.js index acb67d0ec21..a8a8e3e2cff 100644 --- a/spec/javascripts/pipelines/pipelines_artifacts_spec.js +++ b/spec/javascripts/pipelines/pipelines_artifacts_spec.js @@ -34,7 +34,7 @@ describe('Pipelines Artifacts dropdown', () => { ).toEqual(artifacts[0].path); expect( - component.$el.querySelector('.dropdown-menu li a span').textContent, + component.$el.querySelector('.dropdown-menu li a').textContent, ).toContain(artifacts[0].name); }); }); diff --git a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb index c86353abb7c..98cf7966dad 100644 --- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb +++ b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: true, broken_storage: true do let(:storage_name) { 'default' } - let(:circuit_breaker) { described_class.new(storage_name) } + let(:circuit_breaker) { described_class.new(storage_name, hostname) } let(:hostname) { Gitlab::Environment.hostname } let(:cache_key) { "storage_accessible:#{storage_name}:#{hostname}" } @@ -22,7 +22,8 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: 'failure_wait_time' => 30, 'failure_reset_time' => 1800, 'storage_timeout' => 5 - } + }, + 'nopath' => { 'path' => nil } ) end @@ -59,6 +60,14 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: expect(breaker).to be_a(described_class) expect(described_class.for_storage('default')).to eq(breaker) end + + it 'returns a broken circuit breaker for an unknown storage' do + expect(described_class.for_storage('unknown').circuit_broken?).to be_truthy + end + + it 'returns a broken circuit breaker when the path is not set' do + expect(described_class.for_storage('nopath').circuit_broken?).to be_truthy + end end describe '#initialize' do diff --git a/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb new file mode 100644 index 00000000000..0e645008c88 --- /dev/null +++ b/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb @@ -0,0 +1,77 @@ +require 'spec_helper' + +describe Gitlab::Git::Storage::NullCircuitBreaker do + let(:storage) { 'default' } + let(:hostname) { 'localhost' } + let(:error) { nil } + + subject(:breaker) { described_class.new(storage, hostname, error: error) } + + context 'with an error' do + let(:error) { Gitlab::Git::Storage::Misconfiguration.new('error') } + + describe '#perform' do + it { expect { breaker.perform { 'ok' } }.to raise_error(error) } + end + + describe '#circuit_broken?' do + it { expect(breaker.circuit_broken?).to be_truthy } + end + + describe '#last_failure' do + it { Timecop.freeze { expect(breaker.last_failure).to eq(Time.now) } } + end + + describe '#failure_count' do + it { expect(breaker.failure_count).to eq(breaker.failure_count_threshold) } + end + + describe '#failure_info' do + it { Timecop.freeze { expect(breaker.failure_info).to eq(Gitlab::Git::Storage::CircuitBreaker::FailureInfo.new(Time.now, breaker.failure_count_threshold)) } } + end + end + + context 'not broken' do + describe '#perform' do + it { expect(breaker.perform { 'ok' }).to eq('ok') } + end + + describe '#circuit_broken?' do + it { expect(breaker.circuit_broken?).to be_falsy } + end + + describe '#last_failure' do + it { expect(breaker.last_failure).to be_nil } + end + + describe '#failure_count' do + it { expect(breaker.failure_count).to eq(0) } + end + + describe '#failure_info' do + it { expect(breaker.failure_info).to eq(Gitlab::Git::Storage::CircuitBreaker::FailureInfo.new(nil, 0)) } + end + end + + describe '#failure_count_threshold' do + it { expect(breaker.failure_count_threshold).to eq(1) } + end + + it 'implements the CircuitBreaker interface' do + ours = described_class.public_instance_methods + theirs = Gitlab::Git::Storage::CircuitBreaker.public_instance_methods + + # These methods are not part of the public API, but are public to allow the + # CircuitBreaker specs to operate. They should be made private over time. + exceptions = %i[ + cache_key + check_storage_accessible! + no_failures? + storage_available? + track_storage_accessible + track_storage_inaccessible + ] + + expect(theirs - ours).to contain_exactly(*exceptions) + end +end 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 f5c9680bf59..73dd236a5c6 100644 --- a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb +++ b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb @@ -21,7 +21,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do let(:metric_class) { Gitlab::HealthChecks::Metric } let(:result_class) { Gitlab::HealthChecks::Result } - let(:repository_storages) { [:default] } + let(:repository_storages) { ['default'] } let(:tmp_dir) { Dir.mktmpdir } let(:storages_paths) do @@ -64,7 +64,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do allow(described_class).to receive(:storage_circuitbreaker_test) { true } end - it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: :default)) } + it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: 'default')) } end context 'storage points to directory that has both read and write rights' do @@ -72,7 +72,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do FileUtils.chmod_R(0755, tmp_dir) end - it { is_expected.to include(result_class.new(true, nil, shard: :default)) } + it { is_expected.to include(result_class.new(true, nil, shard: 'default')) } it 'cleans up files used for testing' do expect(described_class).to receive(:storage_write_test).with(any_args).and_call_original @@ -85,7 +85,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do allow(described_class).to receive(:storage_read_test).with(any_args).and_return(false) end - it { is_expected.to include(result_class.new(false, 'cannot read from storage', shard: :default)) } + it { is_expected.to include(result_class.new(false, 'cannot read from storage', shard: 'default')) } end context 'write test fails' do @@ -93,7 +93,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do allow(described_class).to receive(:storage_write_test).with(any_args).and_return(false) end - it { is_expected.to include(result_class.new(false, 'cannot write to storage', shard: :default)) } + it { is_expected.to include(result_class.new(false, 'cannot write to storage', shard: 'default')) } end end end @@ -109,7 +109,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do it 'provides metrics' do metrics = described_class.metrics - expect(metrics).to all(have_attributes(labels: { shard: :default })) + expect(metrics).to all(have_attributes(labels: { shard: 'default' })) expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0)) expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 0)) expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 0)) @@ -128,7 +128,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do it 'provides metrics' do metrics = described_class.metrics - expect(metrics).to all(have_attributes(labels: { shard: :default })) + expect(metrics).to all(have_attributes(labels: { shard: 'default' })) expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 1)) expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 1)) expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 1)) @@ -156,14 +156,14 @@ describe Gitlab::HealthChecks::FsShardsCheck do describe '#readiness' do subject { described_class.readiness } - it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: :default)) } + it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: 'default')) } end describe '#metrics' do it 'provides metrics' do metrics = described_class.metrics - expect(metrics).to all(have_attributes(labels: { shard: :default })) + expect(metrics).to all(have_attributes(labels: { shard: 'default' })) expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0)) expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 0)) expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 0)) diff --git a/spec/support/stub_configuration.rb b/spec/support/stub_configuration.rb index 45c10e78789..2dfb4d4a07f 100644 --- a/spec/support/stub_configuration.rb +++ b/spec/support/stub_configuration.rb @@ -42,7 +42,7 @@ module StubConfiguration # Default storage is always required messages['default'] ||= Gitlab.config.repositories.storages.default messages.each do |storage_name, storage_settings| - storage_settings['path'] ||= TestEnv.repos_path + storage_settings['path'] = TestEnv.repos_path unless storage_settings.key?('path') storage_settings['failure_count_threshold'] ||= 10 storage_settings['failure_wait_time'] ||= 30 storage_settings['failure_reset_time'] ||= 1800 |