diff options
| author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2016-05-05 11:44:03 +0200 |
|---|---|---|
| committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2016-05-05 11:44:03 +0200 |
| commit | 3592a59715aa254e26cf1769635d337387984bcc (patch) | |
| tree | 01747a2c4fe6770bbe32d0ed4dd61d8e2cb28f3d | |
| parent | ccd9c4be00a1e783b6d8c821aaec2b975e4a3176 (diff) | |
| parent | c583433862558550c8caedb03217880568dcdefc (diff) | |
| download | gitlab-ce-3592a59715aa254e26cf1769635d337387984bcc.tar.gz | |
Merge branch 'master' into group-navigation-redesign
64 files changed, 752 insertions, 227 deletions
diff --git a/.rubocop.yml b/.rubocop.yml index 83ed6c38678..9f179efa3ce 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -953,10 +953,9 @@ Performance/DoubleStartEndWith: Performance/EndWith: Enabled: false -# TODO: Enable LstripRstrip Cop. # Use `strip` instead of `lstrip.rstrip`. Performance/LstripRstrip: - Enabled: false + Enabled: true # TODO: Enable RangeInclude Cop. # Use `Range#cover?` instead of `Range#include?`. diff --git a/CHANGELOG b/CHANGELOG index b6527780bbf..e989e622b97 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,7 +2,9 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.8.0 (unreleased) - Project#open_branches has been cleaned up and no longer loads entire records into memory. + - Log to application.log when an admin starts and stops impersonating a user - Make build status canceled if any of the jobs was canceled and none failed + - Sanitize repo paths in new project error message - Remove future dates from contribution calendar graph. - Support e-mail notifications for comments on project snippets - Use ActionDispatch Remote IP for Akismet checking @@ -10,17 +12,26 @@ v 8.8.0 (unreleased) - Add 'l' shortcut to open Label dropdown on issuables and 'i' to create new issue on a project - Updated search UI - Display informative message when new milestone is created - - Replace Devise Async with Devise ActiveJob integration. !3902 (Connor Shea) - Allow "NEWS" and "CHANGES" as alternative names for CHANGELOG. !3768 (Connor Shea) - Added button to toggle whitespaces changes on diff view - Backport GitLab Enterprise support from EE + - Create tags using Rugged for performance reasons. !3745 - Files over 5MB can only be viewed in their raw form, files over 1MB without highlighting !3718 - Add support for supressing text diffs using .gitattributes on the default branch (Matt Oakes) - Added multiple colors for labels in dropdowns when dups happen. + - Improve description for the Two-factor Authentication sign-in screen. (Connor Shea) + - API support for the 'since' and 'until' operators on commit requests (Paco Guzman) + - Fix Gravatar hint in user profile when Gravatar is disabled. !3988 (Artem Sidorenko) + - Expire repository exists? and has_visible_content? caches after a push if necessary -v 8.7.2 (unreleased) +v 8.7.3 + - Emails, Gitlab::Email::Message, Gitlab::Diff, and Premailer::Adapter::Nokogiri are now instrumented + - Merge request widget displays TeamCity build state and code coverage correctly again. + +v 8.7.2 - The "New Branch" button is now loaded asynchronously - Fix error 500 when trying to create a wiki page + - Updated spacing between notification label and button v 8.7.1 - Throttle the update of `project.last_activity_at` to 1 minute. !3848 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 084c2d616a9..9fe4cf7b0f6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -142,6 +142,16 @@ code snippet right after your description in a new line: `~"feature proposal"`. Please keep feature proposals as small and simple as possible, complex ones might be edited to make them small and simple. +You are encouraged to use the template below for feature proposals. + +``` +## Description including problem, use cases, benefits, and/or goals + +## Proposal + +## Links / references +``` + For changes in the interface, it can be helpful to create a mockup first. If you want to create something yourself, consider opening an issue first to discuss whether it is interesting to include this in GitLab. @@ -349,7 +359,7 @@ on your merge request feel free to mention one of the Merge Marshalls in the Please ensure that your merge request meets the contribution acceptance criteria. When having your code reviewed and when reviewing merge requests please take the -[Thoughtbot code review guide] into account. +[code review guidelines](doc/development/code_review.md) into account. ### Merge request description format @@ -523,4 +533,3 @@ available at [http://contributor-covenant.org/version/1/1/0/](http://contributor [gitlab-design]: https://gitlab.com/gitlab-org/gitlab-design [free Antetype viewer (Mac OSX only)]: https://itunes.apple.com/us/app/antetype-viewer/id824152298?mt=12 [`gitlab1.atype` file]: https://gitlab.com/gitlab-org/gitlab-design/tree/master/gitlab1.atype/ -[Thoughtbot code review guide]: https://github.com/thoughtbot/guides/tree/master/code-review @@ -20,6 +20,7 @@ gem "pg", '~> 0.18.2', group: :postgres # Authentication libraries gem 'devise', '~> 3.5.4' gem 'doorkeeper', '~> 3.1' +gem 'devise-async', '~> 0.9.0' gem 'omniauth', '~> 1.3.1' gem 'omniauth-auth0', '~> 1.4.1' gem 'omniauth-azure-oauth2', '~> 0.0.6' @@ -269,7 +270,7 @@ group :development, :test do gem 'database_cleaner', '~> 1.4.0' gem 'factory_girl_rails', '~> 4.6.0' - gem 'rspec-rails', '~> 3.3.0' + gem 'rspec-rails', '~> 3.4.0' gem 'rspec-retry' gem 'spinach-rails', '~> 0.2.1' gem 'spinach-rerun-reporter', '~> 0.0.2' diff --git a/Gemfile.lock b/Gemfile.lock index b1e954e0884..2d20a30e677 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -164,6 +164,8 @@ GEM responders thread_safe (~> 0.1) warden (~> 1.2.3) + devise-async (0.9.0) + devise (~> 3.2) devise-two-factor (2.0.1) activesupport attr_encrypted (~> 1.3.2) @@ -351,7 +353,7 @@ GEM posix-spawn (~> 0.3) gitlab_emoji (0.3.1) gemojione (~> 2.2, >= 2.2.1) - gitlab_git (10.0.1) + gitlab_git (10.0.2) activesupport (~> 4.0) charlock_holmes (~> 0.7.3) github-linguist (~> 4.7.0) @@ -662,29 +664,29 @@ GEM chunky_png rqrcode-rails3 (0.1.7) rqrcode (>= 0.4.2) - rspec (3.3.0) - rspec-core (~> 3.3.0) - rspec-expectations (~> 3.3.0) - rspec-mocks (~> 3.3.0) - rspec-core (3.3.2) - rspec-support (~> 3.3.0) - rspec-expectations (3.3.1) + rspec (3.4.0) + rspec-core (~> 3.4.0) + rspec-expectations (~> 3.4.0) + rspec-mocks (~> 3.4.0) + rspec-core (3.4.4) + rspec-support (~> 3.4.0) + rspec-expectations (3.4.0) diff-lcs (>= 1.2.0, < 2.0) - rspec-support (~> 3.3.0) - rspec-mocks (3.3.2) + rspec-support (~> 3.4.0) + rspec-mocks (3.4.1) diff-lcs (>= 1.2.0, < 2.0) - rspec-support (~> 3.3.0) - rspec-rails (3.3.3) + rspec-support (~> 3.4.0) + rspec-rails (3.4.2) actionpack (>= 3.0, < 4.3) activesupport (>= 3.0, < 4.3) railties (>= 3.0, < 4.3) - rspec-core (~> 3.3.0) - rspec-expectations (~> 3.3.0) - rspec-mocks (~> 3.3.0) - rspec-support (~> 3.3.0) + rspec-core (~> 3.4.0) + rspec-expectations (~> 3.4.0) + rspec-mocks (~> 3.4.0) + rspec-support (~> 3.4.0) rspec-retry (0.4.5) rspec-core - rspec-support (3.3.0) + rspec-support (3.4.1) rubocop (0.38.0) parser (>= 2.3.0.6, < 3.0) powerpack (~> 0.1) @@ -920,6 +922,7 @@ DEPENDENCIES database_cleaner (~> 1.4.0) default_value_for (~> 3.0.0) devise (~> 3.5.4) + devise-async (~> 0.9.0) devise-two-factor (~> 2.0.0) diffy (~> 3.0.3) doorkeeper (~> 3.1) @@ -1011,7 +1014,7 @@ DEPENDENCIES responders (~> 2.0) rouge (~> 1.10.1) rqrcode-rails3 (~> 0.1.7) - rspec-rails (~> 3.3.0) + rspec-rails (~> 3.4.0) rspec-retry rubocop (~> 0.38.0) ruby-fogbugz (~> 0.2.1) @@ -1058,4 +1061,4 @@ DEPENDENCIES wikicloth (= 0.8.1) BUNDLED WITH - 1.11.2 + 1.12.1 diff --git a/README.md b/README.md index afa60116ebb..c1a29c3bb1e 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # GitLab -[](https://ci.gitlab.com/projects/1?ref=master) +[](https://gitlab.com/gitlab-org/gitlab-ce/commits/master) [](https://semaphoreci.com/gitlabhq/gitlabhq) [](https://codeclimate.com/github/gitlabhq/gitlabhq) [](https://coveralls.io/r/gitlabhq/gitlabhq?branch=master) @@ -80,7 +80,7 @@ There are a lot of [third-party applications integrating with GitLab](https://ab ## GitLab release cycle -For more information about the release process see the [release documentation](http://doc.gitlab.com/ce/release/). +For more information about the release process see the [release documentation](https://gitlab.com/gitlab-org/release-tools/blob/master/README.md). ## Upgrading diff --git a/app/assets/javascripts/merge_request_widget.js.coffee b/app/assets/javascripts/merge_request_widget.js.coffee index 065626beeb8..17a5a057a94 100644 --- a/app/assets/javascripts/merge_request_widget.js.coffee +++ b/app/assets/javascripts/merge_request_widget.js.coffee @@ -68,20 +68,18 @@ class @MergeRequestWidget $.getJSON @opts.ci_status_url, (data) => @readyForCICheck = true - if @firstCICheck - @firstCICheck = false - @opts.ci_status = data.status - - if @opts.ci_status is '' - @opts.ci_status = data.status + if data.status is '' return - if data.status isnt @opts.ci_status and data.status? + if @firstCiCheck || data.status isnt @opts.ci_status and data.status? + @opts.ci_status = data.status @showCIStatus data.status if data.coverage @showCICoverage data.coverage - if showNotification + # The first check should only update the UI, a notification + # should only be displayed on status changes + if showNotification and not @firstCiCheck status = @ciLabelForStatus(data.status) if status is "preparing" @@ -104,8 +102,7 @@ class @MergeRequestWidget @close() Turbolinks.visit _this.opts.builds_path ) - - @opts.ci_status = data.status + @firstCiCheck = false showCIStatus: (state) -> $('.ci_widget').hide() diff --git a/app/assets/stylesheets/framework/tables.scss b/app/assets/stylesheets/framework/tables.scss index 75b770ae5a2..b42075c98d0 100644 --- a/app/assets/stylesheets/framework/tables.scss +++ b/app/assets/stylesheets/framework/tables.scss @@ -32,13 +32,11 @@ table { th { background-color: $background-color; font-weight: normal; - font-size: 15px; - border-bottom: 1px solid $border-color; + border-bottom: none; } td { border-color: $table-border-color; - border-bottom: 1px solid $border-color; } } } diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 42772ff5c33..399ab914ba8 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -12,7 +12,7 @@ $gutter_inner_width: 258px; */ $border-color: #e5e5e5; $focus-border-color: #3aabf0; -$table-border-color: #eef0f2; +$table-border-color: #ececec; $background-color: #fafafa; /* diff --git a/app/assets/stylesheets/pages/milestone.scss b/app/assets/stylesheets/pages/milestone.scss index d0e72a4422c..b94f524b513 100644 --- a/app/assets/stylesheets/pages/milestone.scss +++ b/app/assets/stylesheets/pages/milestone.scss @@ -28,7 +28,7 @@ li.milestone { // Issue title span a { - color: rgba(0,0,0,0.64); + color: $gl-text-color; } } } @@ -51,7 +51,7 @@ li.milestone { margin-top: 7px; .issuable-number { - color: rgba(0,0,0,0.44); + color: $gl-placeholder-color; margin-right: 5px; } .avatar { diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 9619d65db85..50ca755bcb6 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -114,10 +114,6 @@ ul.notes { word-break: keep-all; } } - - a { - word-break: break-all; - } } .note-header { diff --git a/app/assets/stylesheets/pages/tree.scss b/app/assets/stylesheets/pages/tree.scss index 25b5e95583e..a84fc2e0318 100644 --- a/app/assets/stylesheets/pages/tree.scss +++ b/app/assets/stylesheets/pages/tree.scss @@ -16,7 +16,7 @@ tr { > td, > th { - line-height: 26px; + line-height: 23px; } &:hover { diff --git a/app/controllers/admin/impersonations_controller.rb b/app/controllers/admin/impersonations_controller.rb index 2db824c87ef..8be35f00a77 100644 --- a/app/controllers/admin/impersonations_controller.rb +++ b/app/controllers/admin/impersonations_controller.rb @@ -7,6 +7,8 @@ class Admin::ImpersonationsController < Admin::ApplicationController warden.set_user(impersonator, scope: :user) + Gitlab::AppLogger.info("User #{original_user.username} has stopped impersonating #{impersonator.username}") + session[:impersonator_id] = nil redirect_to admin_user_path(original_user) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index b8976fa09a9..f2f654c7bcd 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -41,6 +41,8 @@ class Admin::UsersController < Admin::ApplicationController warden.set_user(user, scope: :user) + Gitlab::AppLogger.info("User #{current_user.username} has started impersonating #{user.username}") + flash[:alert] = "You are now impersonating #{user.username}" redirect_to root_path diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1c53b0b21a3..17b3f49aed1 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -117,7 +117,7 @@ class ApplicationController < ActionController::Base end def after_sign_out_path_for(resource) - current_application_settings.after_sign_out_path || new_user_session_path + current_application_settings.after_sign_out_path.presence || new_user_session_path end def abilities diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index 1420b96840c..a52c614b259 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -15,7 +15,7 @@ class Projects::CommitsController < Projects::ApplicationController if search.present? @repository.find_commits_by_message(search, @ref, @path, @limit, @offset).compact else - @repository.commits(@ref, @path, @limit, @offset) + @repository.commits(@ref, path: @path, limit: @limit, offset: @offset) end @note_counts = project.notes.where(commit_id: @commits.map(&:id)). diff --git a/app/controllers/projects/graphs_controller.rb b/app/controllers/projects/graphs_controller.rb index d13ea9f34b6..092ef32e6e3 100644 --- a/app/controllers/projects/graphs_controller.rb +++ b/app/controllers/projects/graphs_controller.rb @@ -17,7 +17,7 @@ class Projects::GraphsController < Projects::ApplicationController end def commits - @commits = @project.repository.commits(@ref, nil, 2000, 0, true) + @commits = @project.repository.commits(@ref, limit: 2000, skip_merges: true) @commits_graph = Gitlab::Graphs::Commits.new(@commits) @commits_per_week_days = @commits_graph.commits_per_week_days @commits_per_time = @commits_graph.commits_per_time @@ -55,7 +55,7 @@ class Projects::GraphsController < Projects::ApplicationController private def fetch_graph - @commits = @project.repository.commits(@ref, nil, 6000, 0, true) + @commits = @project.repository.commits(@ref, limit: 6000, skip_merges: true) @log = [] @commits.each do |commit| diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 059b88e2253..352bff19383 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -8,6 +8,13 @@ class RegistrationsController < Devise::RegistrationsController def create if !Gitlab::Recaptcha.load_configurations! || verify_recaptcha + # To avoid duplicate form fields on the login page, the registration form + # names fields using `new_user`, but Devise still wants the params in + # `user`. + if params["new_#{resource_name}"].present? && params[resource_name].blank? + params[resource_name] = params.delete(:"new_#{resource_name}") + end + super else flash[:alert] = "There was an error with the reCAPTCHA code below. Please re-enter the code." diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 3d5e61d2c18..f17d02a7a3f 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -200,12 +200,8 @@ module ProjectsHelper end def repository_size(project = @project) - "#{project.repository_size} MB" - rescue - # In order to prevent 500 error - # when application cannot allocate memory - # to calculate repo size - just show 'Unknown' - 'unknown' + size_in_bytes = project.repository_size * 1.megabyte + number_to_human_size(size_in_bytes, delimiter: ',', precision: 2) end def default_url_to_repo(project = @project) @@ -341,4 +337,10 @@ module ProjectsHelper ) end end + + def sanitize_repo_path(message) + return '' unless message.present? + + message.strip.gsub(Gitlab.config.gitlab_shell.repos_path.chomp('/'), "[REPOS PATH]") + end end diff --git a/app/models/repository.rb b/app/models/repository.rb index b4319297e43..7aebfe279fb 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -87,13 +87,15 @@ class Repository nil end - def commits(ref, path = nil, limit = nil, offset = nil, skip_merges = false) + def commits(ref, path: nil, limit: nil, offset: nil, skip_merges: false, after: nil, before: nil) options = { repo: raw_repository, ref: ref, path: path, limit: limit, offset: offset, + after: after, + before: before, # --follow doesn't play well with --skip. See: # https://gitlab.com/gitlab-org/gitlab-ce/issues/3574#note_3040520 follow: false, @@ -146,10 +148,20 @@ class Repository find_branch(branch_name) end - def add_tag(tag_name, ref, message = nil) - before_push_tag + def add_tag(user, tag_name, target, message = nil) + oldrev = Gitlab::Git::BLANK_SHA + ref = Gitlab::Git::TAG_REF_PREFIX + tag_name + target = commit(target).try(:id) + + return false unless target + + options = { message: message, tagger: user_to_committer(user) } if message + + GitHooksService.new.execute(user, path_to_repo, oldrev, target, ref) do + rugged.tags.create(tag_name, target, options) + end - gitlab_shell.add_tag(path_with_namespace, tag_name, ref, message) + find_tag(tag_name) end def rm_branch(user, branch_name) @@ -575,7 +587,7 @@ class Repository end def contributors - commits = self.commits(nil, nil, 2000, 0, true) + commits = self.commits(nil, limit: 2000, offset: 0, skip_merges: true) commits.group_by(&:author_email).map do |email, commits| contributor = Gitlab::Contributor.new diff --git a/app/models/user.rb b/app/models/user.rb index b6f405c6981..ab48f8f1960 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -91,7 +91,7 @@ class User < ActiveRecord::Base devise :two_factor_backupable, otp_number_of_backup_codes: 10 serialize :otp_backup_codes, JSON - devise :lockable, :recoverable, :rememberable, :trackable, + devise :lockable, :async, :recoverable, :rememberable, :trackable, :validatable, :omniauthable, :confirmable, :registerable attr_accessor :force_random_password diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb index 707c2f7ff85..9f4481a8153 100644 --- a/app/services/create_branch_service.rb +++ b/app/services/create_branch_service.rb @@ -43,9 +43,4 @@ class CreateBranchService < BaseService out[:branch] = branch out end - - def build_push_data(project, user, branch) - Gitlab::PushDataBuilder. - build(project, user, Gitlab::Git::BLANK_SHA, branch.target, "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch.name}", []) - end end diff --git a/app/services/create_tag_service.rb b/app/services/create_tag_service.rb index 55985380d31..91ed0e354d0 100644 --- a/app/services/create_tag_service.rb +++ b/app/services/create_tag_service.rb @@ -1,50 +1,30 @@ require_relative 'base_service' class CreateTagService < BaseService - def execute(tag_name, ref, message, release_description = nil) + def execute(tag_name, target, message, release_description = nil) valid_tag = Gitlab::GitRefValidator.validate(tag_name) - if valid_tag == false - return error('Tag name invalid') - end + return error('Tag name invalid') unless valid_tag repository = project.repository - existing_tag = repository.find_tag(tag_name) - if existing_tag - return error('Tag already exists') - end - message.strip! if message - repository.add_tag(tag_name, ref, message) - new_tag = repository.find_tag(tag_name) + new_tag = nil + begin + new_tag = repository.add_tag(current_user, tag_name, target, message) + rescue Rugged::TagError + return error("Tag #{tag_name} already exists") + rescue GitHooksService::PreReceiveError + return error('Tag creation was rejected by Git hook') + end if new_tag - push_data = create_push_data(project, current_user, new_tag) - EventCreateService.new.push(project, current_user, push_data) - project.execute_hooks(push_data.dup, :tag_push_hooks) - project.execute_services(push_data.dup, :tag_push_hooks) - CreateCommitBuildsService.new.execute(project, current_user, push_data) - if release_description CreateReleaseService.new(@project, @current_user). execute(tag_name, release_description) end - - success(new_tag) + success.merge(tag: new_tag) else - error('Invalid reference name') + error("Target #{target} is invalid") end end - - def success(branch) - out = super() - out[:tag] = branch - out - end - - def create_push_data(project, user, tag) - commits = [project.commit(tag.target)].compact - Gitlab::PushDataBuilder. - build(project, user, Gitlab::Git::BLANK_SHA, tag.target, "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}", commits, tag.message) - end end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index b7af80055bf..66136b62617 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -17,6 +17,7 @@ class GitPushService < BaseService # 6. Checks if the project's main language has changed # def execute + @project.repository.after_create if @project.empty_repo? @project.repository.after_push_commit(branch_name, params[:newrev]) if push_remove_branch? diff --git a/app/services/git_tag_push_service.rb b/app/services/git_tag_push_service.rb index 64271d8bc5c..7410442609d 100644 --- a/app/services/git_tag_push_service.rb +++ b/app/services/git_tag_push_service.rb @@ -2,6 +2,7 @@ class GitTagPushService < BaseService attr_accessor :push_data def execute + project.repository.after_create if project.empty_repo? project.repository.before_push_tag @push_data = build_push_data diff --git a/app/views/devise/sessions/two_factor.html.haml b/app/views/devise/sessions/two_factor.html.haml index 22b2c1a186b..c9d1e454a5e 100644 --- a/app/views/devise/sessions/two_factor.html.haml +++ b/app/views/devise/sessions/two_factor.html.haml @@ -4,7 +4,7 @@ %h3 Two-factor Authentication .login-body = form_for(resource, as: resource_name, url: session_path(resource_name), method: :post) do |f| - = f.text_field :otp_attempt, class: 'form-control', placeholder: 'Two-factor authentication code', required: true, autofocus: true - %p.help-block.hint If you've lost your phone, you may enter one of your recovery codes. + = f.text_field :otp_attempt, class: 'form-control', placeholder: 'Two-factor Authentication code', required: true, autofocus: true + %p.help-block.hint Enter the code from the two-factor app on your mobile device. If you've lost your device, you may enter one of your recovery codes. .prepend-top-20 = f.submit "Verify code", class: "btn btn-save" diff --git a/app/views/devise/shared/_signup_box.html.haml b/app/views/devise/shared/_signup_box.html.haml index e5607dacd0d..510215bb8cd 100644 --- a/app/views/devise/shared/_signup_box.html.haml +++ b/app/views/devise/shared/_signup_box.html.haml @@ -6,7 +6,7 @@ .login-heading %h3 Create an account .login-body - = form_for(resource, as: resource_name, url: registration_path(resource_name)) do |f| + = form_for(resource, as: "new_#{resource_name}", url: registration_path(resource_name)) do |f| .devise-errors = devise_error_messages! %div @@ -16,7 +16,7 @@ %div = f.email_field :email, class: "form-control middle", placeholder: "Email", required: true .form-group.append-bottom-20#password-strength - = f.password_field :password, class: "form-control bottom", id: "user_password_sign_up", placeholder: "Password", required: true + = f.password_field :password, class: "form-control bottom", placeholder: "Password", required: true %div - if current_application_settings.recaptcha_enabled = recaptcha_tags diff --git a/app/views/events/_event.html.haml b/app/views/events/_event.html.haml index 5d622582088..e4629bae0e6 100644 --- a/app/views/events/_event.html.haml +++ b/app/views/events/_event.html.haml @@ -5,7 +5,7 @@ = cache [event, current_application_settings, "v2.2"] do - if event.author - = link_to user_path(event.author.username) do + = link_to user_path(event.author) do = image_tag avatar_icon(event.author_email, 40), class: "avatar s40", alt:'' - else = image_tag avatar_icon(event.author_email, 40), class: "avatar s40", alt:'' diff --git a/app/views/profiles/show.html.haml b/app/views/profiles/show.html.haml index f59d27f7ed0..eef50d887c7 100644 --- a/app/views/profiles/show.html.haml +++ b/app/views/profiles/show.html.haml @@ -8,11 +8,11 @@ %p - if @user.avatar? You can change your avatar here - - if Gitlab.config.gravatar.enabled + - if gravatar_enabled? or remove the current avatar to revert to #{link_to Gitlab.config.gravatar.host, "http://" + Gitlab.config.gravatar.host} - else You can upload an avatar here - - if Gitlab.config.gravatar.enabled + - if gravatar_enabled? or change it at #{link_to Gitlab.config.gravatar.host, "http://" + Gitlab.config.gravatar.host} .col-lg-9 .clearfix.avatar-image.append-bottom-default diff --git a/app/views/projects/imports/new.html.haml b/app/views/projects/imports/new.html.haml index 6027fb23360..a8a8caf7280 100644 --- a/app/views/projects/imports/new.html.haml +++ b/app/views/projects/imports/new.html.haml @@ -10,7 +10,7 @@ .panel-body %pre :preserve - #{@project.import_error.try(:strip)} + #{sanitize_repo_path(@project.import_error)} = form_for @project, url: namespace_project_import_path(@project.namespace, @project), method: :post, html: { class: 'form-horizontal' } do |f| = render "shared/import_form", f: f diff --git a/app/views/projects/milestones/show.html.haml b/app/views/projects/milestones/show.html.haml index 56543ccd062..6ec84660157 100644 --- a/app/views/projects/milestones/show.html.haml +++ b/app/views/projects/milestones/show.html.haml @@ -24,15 +24,15 @@ - else = link_to 'Reopen Milestone', namespace_project_milestone_path(@project.namespace, @project, @milestone, milestone: {state_event: :activate }), method: :put, class: "btn btn-reopen btn-nr btn-grouped" - = link_to namespace_project_milestone_path(@project.namespace, @project, @milestone), data: { confirm: 'Are you sure?' }, method: :delete, class: "btn btn-grouped btn-nr" do - = icon('trash-o') - Delete - = link_to edit_namespace_project_milestone_path(@project.namespace, @project, @milestone), class: "btn btn-grouped btn-nr" do = icon('pencil-square-o') Edit -.detail-page-description.milestone-detail.second-block + = link_to namespace_project_milestone_path(@project.namespace, @project, @milestone), data: { confirm: 'Are you sure?' }, method: :delete, class: "btn btn-grouped btn-danger" do + = icon('trash-o') + Delete + +.detail-page-description.milestone-detail %h2.title = markdown escape_once(@milestone.title), pipeline: :single_line %div diff --git a/app/views/shared/milestones/_top.html.haml b/app/views/shared/milestones/_top.html.haml index cab8743a077..7ff947a51db 100644 --- a/app/views/shared/milestones/_top.html.haml +++ b/app/views/shared/milestones/_top.html.haml @@ -24,7 +24,7 @@ - else = link_to 'Reopen Milestone', group_milestone_path(group, milestone.safe_title, title: milestone.title, milestone: {state_event: :activate }), method: :put, class: "btn btn-grouped btn-reopen" -.detail-page-description.gray-content-block.second-block +.detail-page-description.milestone-detail %h2.title = markdown escape_once(milestone.title), pipeline: :single_line diff --git a/app/views/users/show.html.haml b/app/views/users/show.html.haml index 0dff27f9654..03511b0654f 100644 --- a/app/views/users/show.html.haml +++ b/app/views/users/show.html.haml @@ -98,7 +98,7 @@ #groups.tab-pane - # This tab is always loaded via AJAX - #contributed.contributed-projects.tab-pane + #contributed.tab-pane - # This tab is always loaded via AJAX #projects.tab-pane diff --git a/app/workers/repository_check/single_repository_worker.rb b/app/workers/repository_check/single_repository_worker.rb index a76729e3c74..f2d12ba5a7d 100644 --- a/app/workers/repository_check/single_repository_worker.rb +++ b/app/workers/repository_check/single_repository_worker.rb @@ -1,9 +1,9 @@ module RepositoryCheck class SingleRepositoryWorker include Sidekiq::Worker - + sidekiq_options retry: false - + def perform(project_id) project = Project.find(project_id) project.update_columns( @@ -11,20 +11,32 @@ module RepositoryCheck last_repository_check_at: Time.now, ) end - + private - + def check(project) - repositories = [project.repository] - repositories << project.wiki.repository if project.wiki_enabled? - # Use 'map do', not 'all? do', to prevent short-circuiting - repositories.map { |repository| git_fsck(repository.path_to_repo) }.all? + if !git_fsck(project.repository) + false + elsif project.wiki_enabled? + # Historically some projects never had their wiki repos initialized; + # this happens on project creation now. Let's initialize an empty repo + # if it is not already there. + begin + project.create_wiki + rescue Rugged::RepositoryError + end + + git_fsck(project.wiki.repository) + else + true + end end - - def git_fsck(path) + + def git_fsck(repository) + path = repository.path_to_repo cmd = %W(nice git --git-dir=#{path} fsck) output, status = Gitlab::Popen.popen(cmd) - + if status.zero? true else diff --git a/bin/background_jobs b/bin/background_jobs index 1f67d732949..25a578a1c49 100755 --- a/bin/background_jobs +++ b/bin/background_jobs @@ -37,7 +37,7 @@ start_no_deamonize() start_sidekiq() { - bundle exec sidekiq -q post_receive -q mailers -q archive_repo -q system_hook -q project_web_hook -q gitlab_shell -q incoming_email -q runner -q common -q default -e $RAILS_ENV -P $sidekiq_pidfile "$@" + exec bundle exec sidekiq -q post_receive -q mailers -q archive_repo -q system_hook -q project_web_hook -q gitlab_shell -q incoming_email -q runner -q common -q default -e $RAILS_ENV -P $sidekiq_pidfile "$@" } load_ok() @@ -19,12 +19,12 @@ get_unicorn_pid() start() { - $unicorn_cmd -D + exec $unicorn_cmd -D } start_foreground() { - $unicorn_cmd + exec $unicorn_cmd } stop() diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 07ce4b6d715..e682bcb976d 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -152,7 +152,6 @@ production: &base ## Gravatar ## For Libravatar see: http://doc.gitlab.com/ce/customization/libravatar.html gravatar: - enabled: true # Use user avatar image from Gravatar.com (default: true) # gravatar urls: possible placeholders: %{hash} %{size} %{email} # plain_url: "http://..." # default: http://www.gravatar.com/avatar/%{hash}?s=%{size}&d=identicon # ssl_url: "https://..." # default: https://secure.gravatar.com/avatar/%{hash}?s=%{size}&d=identicon diff --git a/config/initializers/devise_async.rb b/config/initializers/devise_async.rb new file mode 100644 index 00000000000..05a1852cdbd --- /dev/null +++ b/config/initializers/devise_async.rb @@ -0,0 +1 @@ +Devise::Async.backend = :sidekiq diff --git a/config/initializers/metrics.rb b/config/initializers/metrics.rb index 283936d0efc..b2d08d87bac 100644 --- a/config/initializers/metrics.rb +++ b/config/initializers/metrics.rb @@ -61,12 +61,30 @@ if Gitlab::Metrics.enabled? config.instrument_instance_methods(const) end - Dir[Rails.root.join('app', 'finders', '*.rb')].each do |path| - const = File.basename(path, '.rb').camelize.constantize - - config.instrument_instance_methods(const) + # Path to search => prefix to strip from constant + paths_to_instrument = { + ['app', 'finders'] => ['app', 'finders'], + ['app', 'mailers', 'emails'] => ['app', 'mailers'], + ['app', 'services', '**'] => ['app', 'services'], + ['lib', 'gitlab', 'diff'] => ['lib'], + ['lib', 'gitlab', 'email', 'message'] => ['lib'] + } + + paths_to_instrument.each do |(path, prefix)| + prefix = Rails.root.join(*prefix) + + Dir[Rails.root.join(*path + ['*.rb'])].each do |file_path| + path = Pathname.new(file_path).relative_path_from(prefix) + const = path.to_s.sub('.rb', '').camelize.constantize + + config.instrument_methods(const) + config.instrument_instance_methods(const) + end end + config.instrument_methods(Premailer::Adapter::Nokogiri) + config.instrument_instance_methods(Premailer::Adapter::Nokogiri) + [ :Blame, :Branch, :BranchCollection, :Blob, :Commit, :Diff, :Repository, :Tag, :TagCollection, :Tree @@ -97,17 +115,6 @@ if Gitlab::Metrics.enabled? config.instrument_methods(Gitlab::ReferenceExtractor) config.instrument_instance_methods(Gitlab::ReferenceExtractor) - # Instrument all service classes - services = Rails.root.join('app', 'services') - - Dir[services.join('**', '*.rb')].each do |file_path| - path = Pathname.new(file_path).relative_path_from(services) - const = path.to_s.sub('.rb', '').camelize.constantize - - config.instrument_methods(const) - config.instrument_instance_methods(const) - end - # Instrument the classes used for checking if somebody has push access. config.instrument_instance_methods(Gitlab::GitAccess) config.instrument_instance_methods(Gitlab::GitAccessWiki) diff --git a/db/fixtures/development/10_merge_requests.rb b/db/fixtures/development/10_merge_requests.rb index 0825776ffaa..87fb8e3300d 100644 --- a/db/fixtures/development/10_merge_requests.rb +++ b/db/fixtures/development/10_merge_requests.rb @@ -1,6 +1,9 @@ Gitlab::Seeder.quiet do + # Limit the number of merge requests per project to avoid long seeds + MAX_NUM_MERGE_REQUESTS = 10 + Project.all.reject(&:empty_repo?).each do |project| - branches = project.repository.branch_names + branches = project.repository.branch_names.sample(MAX_NUM_MERGE_REQUESTS * 2) branches.each do |branch_name| break if branches.size < 2 diff --git a/doc/api/commits.md b/doc/api/commits.md index 6341440c58b..57c2e1d9b87 100644 --- a/doc/api/commits.md +++ b/doc/api/commits.md @@ -12,6 +12,8 @@ GET /projects/:id/repository/commits | --------- | ---- | -------- | ----------- | | `id` | integer | yes | The ID of a project | | `ref_name` | string | no | The name of a repository branch or tag or if not given the default branch | +| `since` | string | no | Only commits after or in this date will be returned in ISO 8601 format YYYY-MM-DDTHH:MM:SSZ | +| `until` | string | no | Only commits before or in this date will be returned in ISO 8601 format YYYY-MM-DDTHH:MM:SSZ | ```bash curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/5/repository/commits" diff --git a/doc/api/notes.md b/doc/api/notes.md index 7aa1c2155bf..a6b5b1787fd 100644 --- a/doc/api/notes.md +++ b/doc/api/notes.md @@ -15,7 +15,7 @@ GET /projects/:id/issues/:issue_id/notes Parameters: - `id` (required) - The ID of a project -- `issue_id` (required) - The ID of an issue +- `issue_id` (required) - The IID of an issue (not ID) ```json [ @@ -73,7 +73,7 @@ GET /projects/:id/issues/:issue_id/notes/:note_id Parameters: - `id` (required) - The ID of a project -- `issue_id` (required) - The ID of a project issue +- `issue_id` (required) - The IID of a project issue (not ID) - `note_id` (required) - The ID of an issue note ### Create new issue note @@ -87,7 +87,7 @@ POST /projects/:id/issues/:issue_id/notes Parameters: - `id` (required) - The ID of a project -- `issue_id` (required) - The ID of an issue +- `issue_id` (required) - The IID of an issue (not ID) - `body` (required) - The content of a note - `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z @@ -102,7 +102,7 @@ PUT /projects/:id/issues/:issue_id/notes/:note_id Parameters: - `id` (required) - The ID of a project -- `issue_id` (required) - The ID of an issue +- `issue_id` (required) - The IID of an issue (not ID) - `note_id` (required) - The ID of a note - `body` (required) - The content of a note @@ -120,7 +120,7 @@ Parameters: | Attribute | Type | Required | Description | | --------- | ---- | -------- | ----------- | | `id` | integer | yes | The ID of a project | -| `issue_id` | integer | yes | The ID of an issue | +| `issue_id` | integer | yes | The IID of an issue | | `note_id` | integer | yes | The ID of a note | ```bash diff --git a/doc/ci/ssh_keys/README.md b/doc/ci/ssh_keys/README.md index 7f825e6a065..7c0fb225dac 100644 --- a/doc/ci/ssh_keys/README.md +++ b/doc/ci/ssh_keys/README.md @@ -57,7 +57,7 @@ before_script: # WARNING: Use this only with the Docker executor, if you use it with shell # you will overwrite your user's SSH config. - mkdir -p ~/.ssh - - '[[ -f /.dockerinit ]] && echo -e "Host *\n\tStrictHostKeyChecking no\n\n" > ~/.ssh/config' + - '[[ -f /.dockerenv ]] && echo -e "Host *\n\tStrictHostKeyChecking no\n\n" > ~/.ssh/config' ``` As a final step, add the _public_ key from the one you created earlier to the diff --git a/doc/development/README.md b/doc/development/README.md index 3f3ef068f96..aa7d54c01d0 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -8,6 +8,7 @@ - [How to dump production data to staging](db_dump.md) - [Instrumentation](instrumentation.md) - [Migration Style Guide](migration_style_guide.md) for creating safe migrations +- [Performance guidelines](performance.md) - [Rake tasks](rake_tasks.md) for development - [Shell commands](shell_commands.md) in the GitLab codebase - [Sidekiq debugging](sidekiq_debugging.md) diff --git a/doc/development/performance.md b/doc/development/performance.md new file mode 100644 index 00000000000..fb37b3a889c --- /dev/null +++ b/doc/development/performance.md @@ -0,0 +1,258 @@ +# Performance Guidelines + +This document describes various guidelines to follow to ensure good and +consistent performance of GitLab. + +## Workflow + +The process of solving performance problems is roughly as follows: + +1. Make sure there's an issue open somewhere (e.g., on the GitLab CE issue + tracker), create one if there isn't. See [#15607][#15607] for an example. +2. Measure the performance of the code in a production environment such as + GitLab.com (see the [Tooling](#tooling) section below). Performance should be + measured over a period of _at least_ 24 hours. +3. Add your findings based on the measurement period (screenshots of graphs, + timings, etc) to the issue mentioned in step 1. +4. Solve the problem. +5. Create a merge request, assign the "performance" label and ping the right + people (e.g. [@yorickpeterse][yorickpeterse] and [@joshfng][joshfng]). +6. Once a change has been deployed make sure to _again_ measure for at least 24 + hours to see if your changes have any impact on the production environment. +7. Repeat until you're done. + +When providing timings make sure to provide: + +* The 95th percentile +* The 99th percentile +* The mean + +When providing screenshots of graphs, make sure that both the X and Y axes and +the legend are clearly visible. If you happen to have access to GitLab.com's own +monitoring tools you should also provide a link to any relevant +graphs/dashboards. + +## Tooling + +GitLab provides two built-in tools to aid the process of improving performance: + +* [Sherlock](doc/development/profiling.md#sherlock) +* [GitLab Performance Monitoring](doc/monitoring/performance/monitoring.md) + +GitLab employees can use GitLab.com's performance monitoring systems located at +<http://performance.gitlab.net>, this requires you to log in using your +`@gitlab.com` Email address. Non-GitLab employees are advised to set up their +own InfluxDB + Grafana stack. + +## Benchmarks + +Benchmarks are almost always useless. Benchmarks usually only test small bits of +code in isolation and often only measure the best case scenario. On top of that, +benchmarks for libraries (e.g., a Gem) tend to be biased in favour of the +library. After all there's little benefit to an author publishing a benchmark +that shows they perform worse than their competitors. + +Benchmarks are only really useful when you need a rough (emphasis on "rough") +understanding of the impact of your changes. For example, if a certain method is +slow a benchmark can be used to see if the changes you're making have any impact +on the method's performance. However, even when a benchmark shows your changes +improve performance there's no guarantee the performance also improves in a +production environment. + +When writing benchmarks you should almost always use +[benchmark-ips](https://github.com/evanphx/benchmark-ips). Ruby's `Benchmark` +module that comes with the standard library is rarely useful as it runs either a +single iteration (when using `Benchmark.bm`) or two iterations (when using +`Benchmark.bmbm`). Running this few iterations means external factors (e.g. a +video streaming in the background) can very easily skew the benchmark +statistics. + +Another problem with the `Benchmark` module is that it displays timings, not +iterations. This means that if a piece of code completes in a very short period +of time it can be very difficult to compare the timings before and after a +certain change. This in turn leads to patterns such as the following: + +```ruby +Benchmark.bmbm(10) do |bench| + bench.report 'do something' do + 100.times do + ... work here ... + end + end +end +``` + +This however leads to the question: how many iterations should we run to get +meaningful statistics? + +The benchmark-ips Gem basically takes care of all this and much more, and as a +result of this should be used instead of the `Benchmark` module. + +In short: + +1. Don't trust benchmarks you find on the internet. +2. Never make claims based on just benchmarks, always measure in production to + confirm your findings. +3. X being N times faster than Y is meaningless if you don't know what impact it + will actually have on your production environment. +4. A production environment is the _only_ benchmark that always tells the truth + (unless your performance monitoring systems are not set up correctly). +5. If you must write a benchmark use the benchmark-ips Gem instead of Ruby's + `Benchmark` module. + +## Importance of Changes + +When working on performance improvements, it's important to always ask yourself +the question "How important is it to improve the performance of this piece of +code?". Not every piece of code is equally important and it would be a waste to +spend a week trying to improve something that only impacts a tiny fraction of +our users. For example, spending a week trying to squeeze 10 milliseconds out of +a method is a waste of time when you could have spent a week squeezing out 10 +seconds elsewhere. + +There is no clear set of steps that you can follow to determine if a certain +piece of code is worth optimizing. The only two things you can do are: + +1. Think about what the code does, how it's used, how many times it's called and + how much time is spent in it relative to the total execution time (e.g., the + total time spent in a web request). +2. Ask others (preferably in the form of an issue). + +Some examples of changes that aren't really important/worth the effort: + +* Replacing double quotes with single quotes. +* Replacing usage of Array with Set when the list of values is very small. +* Replacing library A with library B when both only take up 0.1% of the total + execution time. +* Calling `freeze` on every string (see [String Freezing](#string-freezing)). + +## Slow Operations & Sidekiq + +Slow operations (e.g. merging branches) or operations that are prone to errors +(using external APIs) should be performed in a Sidekiq worker instead of +directly in a web request as much as possible. This has numerous benefits such +as: + +1. An error won't prevent the request from completing. +2. The process being slow won't affect the loading time of a page. +3. In case of a failure it's easy to re-try the process (Sidekiq takes care of + this automatically). +4. By isolating the code from a web request it will hopefully be easier to test + and maintain. + +It's especially important to use Sidekiq as much as possible when dealing with +Git operations as these operations can take quite some time to complete +depending on the performance of the underlying storage system. + +## Git Operations + +Care should be taken to not run unnecessary Git operations. For example, +retrieving the list of branch names using `Repository#branch_names` can be done +without an explicit check if a repository exists or not. In other words, instead +of this: + +```ruby +if repository.exists? + repository.branch_names.each do |name| + ... + end +end +``` + +You can just write: + +```ruby +repository.branch_names.each do |name| + ... +end +``` + +## Caching + +Operations that will often return the same result should be cached using Redis, +in particular Git operations. When caching data in Redis, make sure the cache is +flushed whenever needed. For example, a cache for the list of tags should be +flushed whenever a new tag is pushed or a tag is removed. + +When adding cache expiration code for repositories, this code should be placed +in one of the before/after hooks residing in the Repository class. For example, +if a cache should be flushed after importing a repository this code should be +added to `Repository#after_import`. This ensures the cache logic stays within +the Repository class instead of leaking into other classes. + +When caching data, make sure to also memoize the result in an instance variable. +While retrieving data from Redis is much faster than raw Git operations, it still +has overhead. By caching the result in an instance variable, repeated calls to +the same method won't end up retrieving data from Redis upon every call. When +memoizing cached data in an instance variable, make sure to also reset the +instance variable when flushing the cache. An example: + + +```ruby +def first_branch + @first_branch ||= cache.fetch(:first_branch) { branches.first } +end + +def expire_first_branch_cache + cache.expire(:first_branch) + @first_branch = nil +end +``` + +## Anti-Patterns + +This is a collection of [anti-patterns][anti-pattern] that should be avoided +unless these changes have a measurable, significant and positive impact on +production environments. + +### String Freezing + +In recent Ruby versions calling `freeze` on a String leads to it being allocated +only once and re-used. For example, on Ruby 2.3 this will only allocate the +"foo" String once: + +```ruby +10.times do + 'foo'.freeze +end +``` + +Blindly adding a `.freeze` call to every String is an anti-pattern that should +be avoided unless one can prove (using production data) the call actually has a +positive impact on performance. + +This feature of Ruby wasn't really meant to make things faster directly, instead +it was meant to reduce the number of allocations. Depending on the size of the +String and how frequently it would be allocated (before the `.freeze` call was +added), this _may_ make things faster, but there's no guarantee it will. + +Another common flavour of this is to not only freeze a String, but also assign +it to a constant, for example: + +```ruby +SOME_CONSTANT = 'foo'.freeze + +9000.times do + SOME_CONSTANT +end +``` + +The only reason you should be doing this is to prevent somebody from mutating +the global String. However, since you can just re-assign constants in Ruby +there's nothing stopping somebody from doing this elsewhere in the code: + +```ruby +SOME_CONSTANT = 'bar' +``` + +### Moving Allocations to Constants + +Storing an object as a constant so you only allocate it once _may_ improve +performance, but there's no guarantee this will. Looking up constants has an +impact on runtime performance, and as such, using a constant instead of +referencing an object directly may even slow code down. + +[#15607]: https://gitlab.com/gitlab-org/gitlab-ce/issues/15607 +[yorickpeterse]: https://gitlab.com/u/yorickpeterse +[joshfng]: https://gitlab.com/u/joshfng +[anti-pattern]: https://en.wikipedia.org/wiki/Anti-pattern diff --git a/doc/install/installation.md b/doc/install/installation.md index e721e70a596..e3af3022262 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -157,22 +157,64 @@ Create a `git` user for GitLab: ## 5. Database -We recommend using a PostgreSQL database. For MySQL check [MySQL setup guide](database_mysql.md). *Note*: because we need to make use of extensions you need at least pgsql 9.1. +We recommend using a PostgreSQL database. For MySQL check the +[MySQL setup guide](database_mysql.md). - # Install the database packages - sudo apt-get install -y postgresql postgresql-client libpq-dev +> **Note**: because we need to make use of extensions you need at least pgsql 9.1. - # Create a user for GitLab +1. Install the database packages: + + ```bash + sudo apt-get install -y postgresql postgresql-client libpq-dev postgresql-contrib + ``` + +1. Create a database user for GitLab: + + ```bash sudo -u postgres psql -d template1 -c "CREATE USER git CREATEDB;" + ``` + +1. Create the GitLab production database and grant all privileges on database: - # Create the GitLab production database & grant all privileges on database + ```bash sudo -u postgres psql -d template1 -c "CREATE DATABASE gitlabhq_production OWNER git;" + ``` + +1. Create the `pg_trgm` extension (required for GitLab 8.6+): + + ```bash + sudo -u postgres psql -d template1 -c "CREATE EXTENSION IF NOT EXISTS pg_trgm;" + ``` + +1. Try connecting to the new database with the new user: - # Try connecting to the new database with the new user + ```bash sudo -u git -H psql -d gitlabhq_production + ``` + +1. Check if the `pg_trgm` extension is enabled: + + ```bash + SELECT true AS enabled + FROM pg_available_extensions + WHERE name = 'pg_trgm' + AND installed_version IS NOT NULL; + ``` + + If the extension is enabled this will produce the following output: - # Quit the database session + ``` + enabled + --------- + t + (1 row) + ``` + +1. Quit the database session: + + ```bash gitlabhq_production> \q + ``` ## 6. Redis diff --git a/doc/update/patch_versions.md b/doc/update/patch_versions.md index 60729316cde..b4283a526f3 100644 --- a/doc/update/patch_versions.md +++ b/doc/update/patch_versions.md @@ -57,10 +57,10 @@ sudo -u git -H make cd /home/git/gitlab # PostgreSQL -sudo -u git -H bundle install --without development test mysql --deployment +sudo -u git -H bundle install --without development test mysql --with postgres --deployment # MySQL -sudo -u git -H bundle install --without development test postgres --deployment +sudo -u git -H bundle install --without development test postgres --with mysql --deployment # Optional: clean up old gems sudo -u git -H bundle clean diff --git a/features/steps/project/commits/tags.rb b/features/steps/project/commits/tags.rb index eff4234a44a..912ba580efd 100644 --- a/features/steps/project/commits/tags.rb +++ b/features/steps/project/commits/tags.rb @@ -57,11 +57,11 @@ class Spinach::Features::ProjectCommitsTags < Spinach::FeatureSteps end step 'I should see new an error that tag ref is invalid' do - expect(page).to have_content 'Invalid reference name' + expect(page).to have_content 'Target foo is invalid' end step 'I should see new an error that tag already exists' do - expect(page).to have_content 'Tag already exists' + expect(page).to have_content 'Tag v1.0.0 already exists' end step "I visit tag 'v1.1.0' page" do diff --git a/features/steps/user.rb b/features/steps/user.rb index 3230234cb6d..b1d088f07f9 100644 --- a/features/steps/user.rb +++ b/features/steps/user.rb @@ -12,7 +12,7 @@ class Spinach::Features::User < Spinach::FeatureSteps user = User.find_by(name: 'John Doe') project = contributed_project - # Issue controbution + # Issue contribution issue_params = { title: 'Bug in old browser' } Issues::CreateService.new(project, user, issue_params).execute @@ -28,7 +28,7 @@ class Spinach::Features::User < Spinach::FeatureSteps end step 'I should see contributed projects' do - page.within '.contributed-projects' do + page.within '#contributed' do expect(page).to have_content(@contributed_project.name) end end diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 4544a41b1e3..93a3a5ce089 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -12,14 +12,20 @@ module API # Parameters: # id (required) - The ID of a project # ref_name (optional) - The name of a repository branch or tag, if not given the default branch is used + # since (optional) - Only commits after or in this date will be returned + # until (optional) - Only commits before or in this date will be returned # Example Request: # GET /projects/:id/repository/commits get ":id/repository/commits" do + datetime_attributes! :since, :until + page = (params[:page] || 0).to_i per_page = (params[:per_page] || 20).to_i ref = params[:ref_name] || user_project.try(:default_branch) || 'master' + after = params[:since] + before = params[:until] - commits = user_project.repository.commits(ref, nil, per_page, page * per_page) + commits = user_project.repository.commits(ref, limit: per_page, offset: page * per_page, after: after, before: before) present commits, with: Entities::RepoCommit end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 5bbf721321d..40c967453fb 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -183,6 +183,22 @@ module API Gitlab::Access.options_with_owner.values.include? level.to_i end + # Checks the occurrences of datetime attributes, each attribute if present in the params hash must be in ISO 8601 + # format (YYYY-MM-DDTHH:MM:SSZ) or a Bad Request error is invoked. + # + # Parameters: + # keys (required) - An array consisting of elements that must be parseable as dates from the params hash + def datetime_attributes!(*keys) + keys.each do |key| + begin + params[key] = Time.xmlschema(params[key]) if params[key].present? + rescue ArgumentError + message = "\"" + key.to_s + "\" must be a timestamp in ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ" + render_api_error!(message, 400) + end + end + end + def issuable_order_by if params["order_by"] == 'updated_at' 'updated_at' diff --git a/lib/gitlab/backend/shell.rb b/lib/gitlab/backend/shell.rb index 5e2fb863a8f..132f9cd1966 100644 --- a/lib/gitlab/backend/shell.rb +++ b/lib/gitlab/backend/shell.rb @@ -79,24 +79,6 @@ module Gitlab 'rm-project', "#{name}.git"]) end - # Add repository tag from passed ref - # - # path - project path with namespace - # tag_name - new tag name - # ref - HEAD for new tag - # message - optional message for tag (annotated tag) - # - # Ex. - # add_tag("gitlab/gitlab-ci", "v4.0", "master") - # add_tag("gitlab/gitlab-ci", "v4.0", "master", "message") - # - def add_tag(path, tag_name, ref, message = nil) - cmd = %W(#{gitlab_shell_path}/bin/gitlab-projects create-tag #{path}.git - #{tag_name} #{ref}) - cmd << message unless message.nil? || message.empty? - Gitlab::Utils.system_silent(cmd) - end - # Gc repository # # path - project path with namespace diff --git a/lib/gitlab/push_data_builder.rb b/lib/gitlab/push_data_builder.rb index 67622f321a6..c8f12577112 100644 --- a/lib/gitlab/push_data_builder.rb +++ b/lib/gitlab/push_data_builder.rb @@ -66,7 +66,7 @@ module Gitlab # This method provide a sample data generated with # existing project and commits to test webhooks def build_sample(project, user) - commits = project.repository.commits(project.default_branch, nil, 3) + commits = project.repository.commits(project.default_branch, limit: 3) ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{project.default_branch}" build(project, user, commits.last.id, commits.first.id, ref, commits) end diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index 4433ef2d6f1..8c38dd5b122 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -37,7 +37,7 @@ feature 'Login', feature: true do end def enter_code(code) - fill_in 'Two-factor authentication code', with: code + fill_in 'Two-factor Authentication code', with: code click_button 'Verify code' end diff --git a/spec/features/signup_spec.rb b/spec/features/signup_spec.rb index 51b754ff85c..58aabd913eb 100644 --- a/spec/features/signup_spec.rb +++ b/spec/features/signup_spec.rb @@ -7,10 +7,10 @@ feature 'Signup', feature: true do visit root_path - fill_in 'user_name', with: user.name - fill_in 'user_username', with: user.username - fill_in 'user_email', with: user.email - fill_in 'user_password_sign_up', with: user.password + fill_in 'new_user_name', with: user.name + fill_in 'new_user_username', with: user.username + fill_in 'new_user_email', with: user.email + fill_in 'new_user_password', with: user.password click_button "Sign up" expect(current_path).to eq users_almost_there_path @@ -25,10 +25,10 @@ feature 'Signup', feature: true do visit root_path - fill_in 'user_name', with: user.name - fill_in 'user_username', with: user.username - fill_in 'user_email', with: existing_user.email - fill_in 'user_password_sign_up', with: user.password + fill_in 'new_user_name', with: user.name + fill_in 'new_user_username', with: user.username + fill_in 'new_user_email', with: existing_user.email + fill_in 'new_user_password', with: user.password click_button "Sign up" expect(current_path).to eq user_registration_path @@ -42,10 +42,10 @@ feature 'Signup', feature: true do visit root_path - fill_in 'user_name', with: user.name - fill_in 'user_username', with: user.username - fill_in 'user_email', with: existing_user.email - fill_in 'user_password_sign_up', with: user.password + fill_in 'new_user_name', with: user.name + fill_in 'new_user_username', with: user.username + fill_in 'new_user_email', with: existing_user.email + fill_in 'new_user_password', with: user.password click_button "Sign up" expect(current_path).to eq user_registration_path diff --git a/spec/features/users_spec.rb b/spec/features/users_spec.rb index c1248162031..cf116040394 100644 --- a/spec/features/users_spec.rb +++ b/spec/features/users_spec.rb @@ -5,10 +5,10 @@ feature 'Users', feature: true do scenario 'GET /users/sign_in creates a new user account' do visit new_user_session_path - fill_in 'user_name', with: 'Name Surname' - fill_in 'user_username', with: 'Great' - fill_in 'user_email', with: 'name@mail.com' - fill_in 'user_password_sign_up', with: 'password1234' + fill_in 'new_user_name', with: 'Name Surname' + fill_in 'new_user_username', with: 'Great' + fill_in 'new_user_email', with: 'name@mail.com' + fill_in 'new_user_password', with: 'password1234' expect { click_button 'Sign up' }.to change { User.count }.by(1) end @@ -31,10 +31,10 @@ feature 'Users', feature: true do scenario 'Should show one error if email is already taken' do visit new_user_session_path - fill_in 'user_name', with: 'Another user name' - fill_in 'user_username', with: 'anotheruser' - fill_in 'user_email', with: user.email - fill_in 'user_password_sign_up', with: '12341234' + fill_in 'new_user_name', with: 'Another user name' + fill_in 'new_user_username', with: 'anotheruser' + fill_in 'new_user_email', with: user.email + fill_in 'new_user_password', with: '12341234' expect { click_button 'Sign up' }.to change { User.count }.by(0) expect(page).to have_text('Email has already been taken') expect(number_of_errors_on_page(page)).to be(1), 'errors on page:\n #{errors_on_page page}' diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 62389188d2c..29bcb8c5892 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -131,4 +131,13 @@ describe ProjectsHelper do end end end + + describe '#sanitized_import_error' do + it 'removes the repo path' do + repo = File.join(Gitlab.config.gitlab_shell.repos_path, '/namespace/test.git') + import_error = "Could not clone #{repo}\n" + + expect(sanitize_repo_path(import_error)).to eq('Could not clone [REPOS PATH]/namespace/test.git') + end + end end diff --git a/spec/javascripts/merge_request_widget_spec.js.coffee b/spec/javascripts/merge_request_widget_spec.js.coffee new file mode 100644 index 00000000000..c0bd8a29e43 --- /dev/null +++ b/spec/javascripts/merge_request_widget_spec.js.coffee @@ -0,0 +1,49 @@ +#= require merge_request_widget + +describe 'MergeRequestWidget', -> + + beforeEach -> + window.notifyPermissions = () -> + window.notify = () -> + @opts = { + ci_status_url:"http://sampledomain.local/ci/getstatus", + ci_status:"", + ci_message: { + normal: "Build {{status}} for \"{{title}}\"", + preparing: "{{status}} build for \"{{title}}\"" + }, + ci_title: { + preparing: "{{status}} build", + normal: "Build {{status}}" + }, + gitlab_icon:"gitlab_logo.png", + builds_path:"http://sampledomain.local/sampleBuildsPath" + } + @class = new MergeRequestWidget(@opts) + @ciStatusData = {"title":"Sample MR title","sha":"12a34bc5","status":"success","coverage":98} + + describe 'getCIStatus', -> + beforeEach -> + spyOn(jQuery, 'getJSON').and.callFake (req, cb) => + cb(@ciStatusData) + + it 'should call showCIStatus even if a notification should not be displayed', -> + spy = spyOn(@class, 'showCIStatus').and.stub() + @class.getCIStatus(false) + expect(spy).toHaveBeenCalledWith(@ciStatusData.status) + + it 'should call showCIStatus when a notification should be displayed', -> + spy = spyOn(@class, 'showCIStatus').and.stub() + @class.getCIStatus(true) + expect(spy).toHaveBeenCalledWith(@ciStatusData.status) + + it 'should call showCICoverage when the coverage rate is set', -> + spy = spyOn(@class, 'showCICoverage').and.stub() + @class.getCIStatus(false) + expect(spy).toHaveBeenCalledWith(@ciStatusData.coverage) + + it 'should not call showCICoverage when the coverage rate is not set', -> + @ciStatusData.coverage = null + spy = spyOn(@class, 'showCICoverage').and.stub() + @class.getCIStatus(false) + expect(spy).not.toHaveBeenCalled() diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 397bb5a8028..34a13f9b5c9 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -561,7 +561,7 @@ describe Repository, models: true do end describe :skip_merged_commit do - subject { repository.commits(Gitlab::Git::BRANCH_REF_PREFIX + "'test'", nil, 100, 0, true).map{ |k| k.id } } + subject { repository.commits(Gitlab::Git::BRANCH_REF_PREFIX + "'test'", limit: 100, skip_merges: true).map{ |k| k.id } } it { is_expected.not_to include('e56497bb5f03a90a51293fc6d516788730953899') } end @@ -858,13 +858,30 @@ describe Repository, models: true do end describe '#add_tag' do - it 'adds a tag' do - expect(repository).to receive(:before_push_tag) + context 'with a valid target' do + let(:user) { build_stubbed(:user) } - expect_any_instance_of(Gitlab::Shell).to receive(:add_tag). - with(repository.path_with_namespace, '8.5', 'master', 'foo') + it 'creates the tag using rugged' do + expect(repository.rugged.tags).to receive(:create). + with('8.5', repository.commit('master').id, + hash_including(message: 'foo', + tagger: hash_including(name: user.name, email: user.email))). + and_call_original - repository.add_tag('8.5', 'master', 'foo') + repository.add_tag(user, '8.5', 'master', 'foo') + end + + it 'returns a Gitlab::Git::Tag object' do + tag = repository.add_tag(user, '8.5', 'master', 'foo') + + expect(tag).to be_a(Gitlab::Git::Tag) + end + end + + context 'with an invalid target' do + it 'returns false' do + expect(repository.add_tag(user, '8.5', 'bar', 'foo')).to be false + end end end diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index e28998d51b5..cb82ca7802d 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -32,6 +32,41 @@ describe API::API, api: true do expect(response.status).to eq(401) end end + + context "since optional parameter" do + it "should return project commits since provided parameter" do + commits = project.repository.commits("master") + since = commits.second.created_at + + get api("/projects/#{project.id}/repository/commits?since=#{since.utc.iso8601}", user) + + expect(json_response.size).to eq 2 + expect(json_response.first["id"]).to eq(commits.first.id) + expect(json_response.second["id"]).to eq(commits.second.id) + end + end + + context "until optional parameter" do + it "should return project commits until provided parameter" do + commits = project.repository.commits("master") + before = commits.second.created_at + + get api("/projects/#{project.id}/repository/commits?until=#{before.utc.iso8601}", user) + + expect(json_response.size).to eq(commits.size - 1) + expect(json_response.first["id"]).to eq(commits.second.id) + expect(json_response.second["id"]).to eq(commits.third.id) + end + end + + context "invalid xmlschema date parameters" do + it "should return an invalid parameter error message" do + get api("/projects/#{project.id}/repository/commits?since=invalid-date", user) + + expect(response.status).to eq(400) + expect(json_response['message']).to include "\"since\" must be a timestamp in ISO 8601 format" + end + end end describe "GET /projects:id/repository/commits/:sha" do diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index edcb2bedbf7..12e170b232f 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -147,7 +147,7 @@ describe API::API, api: true do tag_name: 'v8.0.0', ref: 'master' expect(response.status).to eq(400) - expect(json_response['message']).to eq('Tag already exists') + expect(json_response['message']).to eq('Tag v8.0.0 already exists') end it 'should return 400 if ref name is invalid' do @@ -155,7 +155,7 @@ describe API::API, api: true do tag_name: 'mytag', ref: 'foo' expect(response.status).to eq(400) - expect(json_response['message']).to eq('Invalid reference name') + expect(json_response['message']).to eq('Target foo is invalid') end end diff --git a/spec/services/create_tag_service_spec.rb b/spec/services/create_tag_service_spec.rb new file mode 100644 index 00000000000..91f9e663b66 --- /dev/null +++ b/spec/services/create_tag_service_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +describe CreateTagService, services: true do + let(:project) { create(:project) } + let(:repository) { project.repository } + let(:user) { create(:user) } + let(:service) { described_class.new(project, user) } + + describe '#execute' do + it 'creates the tag and returns success' do + response = service.execute('v42.42.42', 'master', 'Foo') + + expect(response[:status]).to eq(:success) + expect(response[:tag]).to be_a Gitlab::Git::Tag + expect(response[:tag].name).to eq('v42.42.42') + end + + context 'when target is invalid' do + it 'returns an error' do + response = service.execute('v1.1.0', 'foo', 'Foo') + + expect(response).to eq(status: :error, + message: 'Target foo is invalid') + end + end + + context 'when tag already exists' do + it 'returns an error' do + expect(repository).to receive(:add_tag). + with(user, 'v1.1.0', 'master', 'Foo'). + and_raise(Rugged::TagError) + + response = service.execute('v1.1.0', 'master', 'Foo') + + expect(response).to eq(status: :error, + message: 'Tag v1.1.0 already exists') + end + end + + context 'when pre-receive hook fails' do + it 'returns an error' do + expect(repository).to receive(:add_tag). + with(user, 'v1.1.0', 'master', 'Foo'). + and_raise(GitHooksService::PreReceiveError) + + response = service.execute('v1.1.0', 'master', 'Foo') + + expect(response).to eq(status: :error, + message: 'Tag creation was rejected by Git hook') + end + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 596d607f2a1..576d16e7ea3 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -51,10 +51,4 @@ FactoryGirl::SyntaxRunner.class_eval do include RSpec::Mocks::ExampleMethods end -# Work around a Rails 4.2.5.1 issue -# See https://github.com/rspec/rspec-rails/issues/1532 -RSpec::Rails::ViewRendering::EmptyTemplatePathSetDecorator.class_eval do - alias_method :find_all_anywhere, :find_all -end - ActiveRecord::Migration.maintain_test_schema! diff --git a/spec/workers/repository_check/single_repository_worker_spec.rb b/spec/workers/repository_check/single_repository_worker_spec.rb index 087e4c667d8..5a03bb77ebd 100644 --- a/spec/workers/repository_check/single_repository_worker_spec.rb +++ b/spec/workers/repository_check/single_repository_worker_spec.rb @@ -12,7 +12,7 @@ describe RepositoryCheck::SingleRepositoryWorker do subject.perform(project.id) expect(project.reload.last_repository_check_failed).to eq(false) - destroy_wiki(project) + break_wiki(project) subject.perform(project.id) expect(project.reload.last_repository_check_failed).to eq(true) @@ -20,15 +20,38 @@ describe RepositoryCheck::SingleRepositoryWorker do it 'skips wikis when disabled' do project = create(:project_empty_repo, wiki_enabled: false) - # Make sure the test would fail if it checked the wiki repo - destroy_wiki(project) + # Make sure the test would fail if the wiki repo was checked + break_wiki(project) subject.perform(project.id) expect(project.reload.last_repository_check_failed).to eq(false) end - def destroy_wiki(project) - FileUtils.rm_rf(project.wiki.repository.path_to_repo) + it 'creates missing wikis' do + project = create(:project_empty_repo, wiki_enabled: true) + FileUtils.rm_rf(wiki_path(project)) + + subject.perform(project.id) + + expect(project.reload.last_repository_check_failed).to eq(false) + end + + it 'does not create a wiki if the main repo does not exist at all' do + project = create(:project_empty_repo) + FileUtils.rm_rf(project.repository.path_to_repo) + FileUtils.rm_rf(wiki_path(project)) + + subject.perform(project.id) + + expect(File.exist?(wiki_path(project))).to eq(false) + end + + def break_wiki(project) + FileUtils.rm_rf(wiki_path(project) + '/objects') + end + + def wiki_path(project) + project.wiki.repository.path_to_repo end end |