From 771f14b96e058649ca5db7ce6c99e38108d4abec Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 2 Feb 2016 14:09:55 +0100 Subject: [PATCH 01/37] First version of "git archive" headers --- .../projects/repositories_controller.rb | 4 ++- app/services/archive_repository_service.rb | 23 ---------------- lib/api/repositories.rb | 7 ++--- lib/gitlab/workhorse.rb | 27 ++++++++++++++++--- .../projects/repositories_controller_spec.rb | 11 +++----- spec/lib/gitlab/workhorse_spec.rb | 18 +++++++++++++ .../archive_repository_service_spec.rb | 25 ----------------- 7 files changed, 49 insertions(+), 66 deletions(-) delete mode 100644 app/services/archive_repository_service.rb create mode 100644 spec/lib/gitlab/workhorse_spec.rb delete mode 100644 spec/services/archive_repository_service_spec.rb diff --git a/app/controllers/projects/repositories_controller.rb b/app/controllers/projects/repositories_controller.rb index ba9aea1c165..5c7614cfbaf 100644 --- a/app/controllers/projects/repositories_controller.rb +++ b/app/controllers/projects/repositories_controller.rb @@ -11,7 +11,9 @@ class Projects::RepositoriesController < Projects::ApplicationController end def archive - render json: ArchiveRepositoryService.new(@project, params[:ref], params[:format]).execute + RepositoryArchiveCacheWorker.perform_async + headers.store(*Gitlab::Workhorse.send_git_archive(@project, params[:ref], params[:format])) + head :ok rescue => ex logger.error("#{self.class.name}: #{ex}") return git_not_found! diff --git a/app/services/archive_repository_service.rb b/app/services/archive_repository_service.rb deleted file mode 100644 index 2160bf13e6d..00000000000 --- a/app/services/archive_repository_service.rb +++ /dev/null @@ -1,23 +0,0 @@ -class ArchiveRepositoryService - attr_reader :project, :ref, :format - - def initialize(project, ref, format) - format ||= 'tar.gz' - @project, @ref, @format = project, ref, format.downcase - end - - def execute(options = {}) - RepositoryArchiveCacheWorker.perform_async - - metadata = project.repository.archive_metadata(ref, storage_path, format) - raise "Repository or ref not found" if metadata.empty? - - metadata - end - - private - - def storage_path - Gitlab.config.gitlab.repository_downloads_path - end -end diff --git a/lib/api/repositories.rb b/lib/api/repositories.rb index c95d2d2001d..0178289f57f 100644 --- a/lib/api/repositories.rb +++ b/lib/api/repositories.rb @@ -98,11 +98,8 @@ module API authorize! :download_code, user_project begin - ArchiveRepositoryService.new( - user_project, - params[:sha], - params[:format] - ).execute + RepositoryArchiveCacheWorker.perform_async + header *Gitlab::Workhorse.send_git_archive(@project, params[:ref], params[:format]) rescue not_found!('File') end diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index a23120a4176..2f3e57156b6 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -4,18 +4,37 @@ require 'json' module Gitlab class Workhorse class << self + SEND_DATA_HEADER = 'Gitlab-Workhorse-Send-Data' + def send_git_blob(repository, blob) - params_hash = { + params = { 'RepoPath' => repository.path_to_repo, 'BlobId' => blob.id, } - params = Base64.urlsafe_encode64(JSON.dump(params_hash)) [ - 'Gitlab-Workhorse-Send-Data', - "git-blob:#{params}", + SEND_DATA_HEADER, + "git-blob:#{encode(params)}", ] end + + def send_git_archive(project, ref, format) + format ||= 'tar.gz' + format.downcase! + params = project.repository.archive_metadata(ref, Gitlab.config.gitlab.repository_downloads_path, format) + raise "Repository or ref not found" if params.empty? + + [ + SEND_DATA_HEADER, + "git-archive:#{encode(params)}", + ] + end + + protected + + def encode(hash) + Base64.urlsafe_encode64(JSON.dump(hash)) + end end end end diff --git a/spec/controllers/projects/repositories_controller_spec.rb b/spec/controllers/projects/repositories_controller_spec.rb index 18a30033ed8..09ec4f18f9d 100644 --- a/spec/controllers/projects/repositories_controller_spec.rb +++ b/spec/controllers/projects/repositories_controller_spec.rb @@ -8,15 +8,10 @@ describe Projects::RepositoriesController do before do sign_in(user) project.team << [user, :developer] - - allow(ArchiveRepositoryService).to receive(:new).and_return(service) end - let(:service) { ArchiveRepositoryService.new(project, "master", "zip") } - - it "executes ArchiveRepositoryService" do - expect(ArchiveRepositoryService).to receive(:new).with(project, "master", "zip") - expect(service).to receive(:execute) + it "uses Gitlab::Workhorse" do + expect(Gitlab::Workhorse).to receive(:send_git_archive).with(project, "master", "zip") get :archive, namespace_id: project.namespace.path, project_id: project.path, ref: "master", format: "zip" end @@ -24,7 +19,7 @@ describe Projects::RepositoriesController do context "when the service raises an error" do before do - allow(service).to receive(:execute).and_raise("Archive failed") + allow(Gitlab::Workhorse).to receive(:send_git_archive).and_raise("Archive failed") end it "renders Not Found" do diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb new file mode 100644 index 00000000000..d940bf05061 --- /dev/null +++ b/spec/lib/gitlab/workhorse_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +describe Gitlab::Workhorse, lib: true do + let(:project) { create(:project) } + let(:subject) { Gitlab::Workhorse } + + describe "#send_git_archive" do + context "when the repository doesn't have an archive file path" do + before do + allow(project.repository).to receive(:archive_metadata).and_return(Hash.new) + end + + it "raises an error" do + expect { subject.send_git_archive(project, "master", "zip") }.to raise_error(RuntimeError) + end + end + end +end diff --git a/spec/services/archive_repository_service_spec.rb b/spec/services/archive_repository_service_spec.rb deleted file mode 100644 index bd871605c66..00000000000 --- a/spec/services/archive_repository_service_spec.rb +++ /dev/null @@ -1,25 +0,0 @@ -require 'spec_helper' - -describe ArchiveRepositoryService, services: true do - let(:project) { create(:project) } - subject { ArchiveRepositoryService.new(project, "master", "zip") } - - describe "#execute" do - it "cleans old archives" do - expect(RepositoryArchiveCacheWorker).to receive(:perform_async) - - subject.execute(timeout: 0.0) - end - - context "when the repository doesn't have an archive file path" do - before do - allow(project.repository).to receive(:archive_metadata).and_return(Hash.new) - end - - it "raises an error" do - expect { subject.execute(timeout: 0.0) }.to raise_error(RuntimeError) - end - end - - end -end From d04556009c9cb42420de45ad99ce342b38865456 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=20S=C3=B5mermaa?= Date: Fri, 29 Jan 2016 10:44:25 +0000 Subject: [PATCH 02/37] Add example of creating build artifacts only for release tags. --- doc/ci/yaml/README.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 4d280297dbb..d5f18831c18 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -424,6 +424,28 @@ artifacts: - binaries/ ``` +You may want to create artifacts only for tagged releases to avoid filling the +build server storage with temporary build artifacts. + +Create artifacts only for tags (`default-job` will not create artifacts): + +```yaml +default-job: + script: + - mvn test -U + except: + - tags + +release-job: + script: + - mvn package -U + artifacts: + paths: + - target/*.war + only: + - tags +``` + The artifacts will be send after a successful build success to GitLab, and will be accessible in the GitLab UI to download. From 150bee38b4e1754686821a6a2761d6f0bb6e69f6 Mon Sep 17 00:00:00 2001 From: Mart Somermaa Date: Wed, 3 Feb 2016 12:45:07 +0200 Subject: [PATCH 03/37] Grammar cleanup in yaml/README.md --- doc/ci/yaml/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index d5f18831c18..75156d3c311 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -446,8 +446,8 @@ release-job: - tags ``` -The artifacts will be send after a successful build success to GitLab, and will -be accessible in the GitLab UI to download. +The artifacts will be sent to GitLab after a successful build and will +be available for download in the GitLab UI. ### cache From 77d2aeabec4601a1d9dda93c8fff0c7d1051ba1f Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 11 Feb 2016 12:50:27 +0100 Subject: [PATCH 04/37] attempt to reduce code complexity on GitPushService#execute --- app/services/git_push_service.rb | 121 ++++++++++++++----------- app/workers/post_receive.rb | 2 +- spec/services/git_push_service_spec.rb | 82 +++++++++++------ 3 files changed, 124 insertions(+), 81 deletions(-) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index e3bf14966c8..491322f5ff5 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -3,6 +3,15 @@ class GitPushService include Gitlab::CurrentSettings include Gitlab::Access + def initialize(project, user, oldrev, newrev, ref) + # TODO: Consider passing an options hash instead https://github.com/bbatsov/ruby-style-guide#too-many-params + @project = project + @user = user + @oldrev = oldrev + @newrev = newrev + @ref = ref + end + # This method will be called after each git update # and only if the provided user and project is present in GitLab. # @@ -15,67 +24,67 @@ class GitPushService # 4. Executes the project's web hooks # 5. Executes the project's services # - def execute(project, user, oldrev, newrev, ref) - @project, @user = project, user - - branch_name = Gitlab::Git.ref_name(ref) - - project.repository.expire_cache(branch_name) - - if push_remove_branch?(ref, newrev) - project.repository.expire_has_visible_content_cache + def execute + @project.repository.expire_cache(branch_name) + if push_remove_branch? + @project.repository.expire_has_visible_content_cache @push_commits = [] - elsif push_to_new_branch?(ref, oldrev) - project.repository.expire_has_visible_content_cache + elsif push_to_new_branch? + @project.repository.expire_has_visible_content_cache # Re-find the pushed commits. - if is_default_branch?(ref) + if is_default_branch? # Initial push to the default branch. Take the full history of that branch as "newly pushed". - @push_commits = project.repository.commits(newrev) - - # Ensure HEAD points to the default branch in case it is not master - project.change_head(branch_name) - - # Set protection on the default branch if configured - if (current_application_settings.default_branch_protection != PROTECTION_NONE) - developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false - project.protected_branches.create({ name: project.default_branch, developers_can_push: developers_can_push }) - end + process_default_branch else # Use the pushed commits that aren't reachable by the default branch # as a heuristic. This may include more commits than are actually pushed, but - # that shouldn't matter because we check for existing cross-references later. - @push_commits = project.repository.commits_between(project.default_branch, newrev) + # that shouldn't matter because we check for existing cross-@references later. + @push_commits = @project.repository.commits_between(@project.default_branch, @newrev) # don't process commits for the initial push to the default branch - process_commit_messages(ref) + process_commit_messages end - elsif push_to_existing_branch?(ref, oldrev) + elsif push_to_existing_branch? # Collect data for this git push - @push_commits = project.repository.commits_between(oldrev, newrev) - process_commit_messages(ref) + @push_commits = @project.repository.commits_between(@oldrev, @newrev) + process_commit_messages end - # Update merge requests that may be affected by this push. A new branch # could cause the last commit of a merge request to change. - project.update_merge_requests(oldrev, newrev, ref, @user) - - @push_data = build_push_data(oldrev, newrev, ref) - - EventCreateService.new.push(project, user, @push_data) - project.execute_hooks(@push_data.dup, :push_hooks) - project.execute_services(@push_data.dup, :push_hooks) - CreateCommitBuildsService.new.execute(project, @user, @push_data) - ProjectCacheWorker.perform_async(project.id) + update_merge_requests end protected + def update_merge_requests + @project.update_merge_requests(@oldrev, @newrev, @ref, @user) + + EventCreateService.new.push(@project, @user, build_push_data) + @project.execute_hooks(build_push_data.dup, :push_hooks) + @project.execute_services(build_push_data.dup, :push_hooks) + CreateCommitBuildsService.new.execute(@project, @user, build_push_data) + ProjectCacheWorker.perform_async(@project.id) + end + + def process_default_branch + @push_commits = project.repository.commits(@newrev) + + # Ensure HEAD points to the default branch in case it is not master + project.change_head(branch_name) + + # Set protection on the default branch if configured + if (current_application_settings.default_branch_protection != PROTECTION_NONE) + developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false + @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push }) + end + end + # Extract any GFM references from the pushed commit messages. If the configured issue-closing regex is matched, # close the referenced Issue. Create cross-reference Notes corresponding to any other referenced Mentionables. - def process_commit_messages(ref) - is_default_branch = is_default_branch?(ref) + def process_commit_messages + is_default_branch = is_default_branch? authors = Hash.new do |hash, commit| email = commit.author_email @@ -104,34 +113,38 @@ class GitPushService end end - def build_push_data(oldrev, newrev, ref) - Gitlab::PushDataBuilder. - build(project, user, oldrev, newrev, ref, push_commits) + def build_push_data + @push_data ||= Gitlab::PushDataBuilder. + build(@project, @user, @oldrev, @newrev, @ref, push_commits) end - def push_to_existing_branch?(ref, oldrev) + def push_to_existing_branch? # Return if this is not a push to a branch (e.g. new commits) - Gitlab::Git.branch_ref?(ref) && !Gitlab::Git.blank_ref?(oldrev) + Gitlab::Git.branch_ref?(@ref) && !Gitlab::Git.blank_ref?(@oldrev) end - def push_to_new_branch?(ref, oldrev) - Gitlab::Git.branch_ref?(ref) && Gitlab::Git.blank_ref?(oldrev) + def push_to_new_branch? + Gitlab::Git.branch_ref?(@ref) && Gitlab::Git.blank_ref?(@oldrev) end - def push_remove_branch?(ref, newrev) - Gitlab::Git.branch_ref?(ref) && Gitlab::Git.blank_ref?(newrev) + def push_remove_branch? + Gitlab::Git.branch_ref?(@ref) && Gitlab::Git.blank_ref?(@newrev) end - def push_to_branch?(ref) - Gitlab::Git.branch_ref?(ref) + def push_to_branch? + Gitlab::Git.branch_ref?(@ref) end - def is_default_branch?(ref) - Gitlab::Git.branch_ref?(ref) && - (Gitlab::Git.ref_name(ref) == project.default_branch || project.default_branch.nil?) + def is_default_branch? + Gitlab::Git.branch_ref?(@ref) && + (Gitlab::Git.ref_name(@ref) == project.default_branch || project.default_branch.nil?) end def commit_user(commit) commit.author || user end + + def branch_name + @_branch_name ||= Gitlab::Git.ref_name(@ref) + end end diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 994b8e8ed38..80081ac5f49 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -38,7 +38,7 @@ class PostReceive if Gitlab::Git.tag_ref?(ref) GitTagPushService.new.execute(project, @user, oldrev, newrev, ref) else - GitPushService.new.execute(project, @user, oldrev, newrev, ref) + GitPushService.new(project, @user, oldrev, newrev, ref).execute end end end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index eb3a5fe43f5..ad18f48ff91 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -5,7 +5,6 @@ describe GitPushService, services: true do let(:user) { create :user } let(:project) { create :project } - let(:service) { GitPushService.new } before do @blankrev = Gitlab::Git::BLANK_SHA @@ -17,7 +16,8 @@ describe GitPushService, services: true do describe 'Push branches' do context 'new branch' do subject do - service.execute(project, user, @blankrev, @newrev, @ref) + service = described_class.new(project, user, @blankrev, @newrev, @ref) + service.execute end it { is_expected.to be_truthy } @@ -37,7 +37,8 @@ describe GitPushService, services: true do context 'existing branch' do subject do - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute end it { is_expected.to be_truthy } @@ -51,7 +52,8 @@ describe GitPushService, services: true do context 'rm branch' do subject do - service.execute(project, user, @oldrev, @blankrev, @ref) + service = described_class.new(project, user, @oldrev, @blankrev, @ref) + service.execute end it { is_expected.to be_truthy } @@ -72,7 +74,8 @@ describe GitPushService, services: true do describe "Git Push Data" do before do - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute @push_data = service.push_data @commit = project.commit(@newrev) end @@ -134,20 +137,23 @@ describe GitPushService, services: true do describe "Push Event" do before do - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute @event = Event.last + @push_data = service.push_data end it { expect(@event).not_to be_nil } it { expect(@event.project).to eq(project) } it { expect(@event.action).to eq(Event::PUSHED) } - it { expect(@event.data).to eq(service.push_data) } + it { expect(@event.data).to eq(@push_data) } context "Updates merge requests" do it "when pushing a new branch for the first time" do expect(project).to receive(:update_merge_requests). with(@blankrev, 'newrev', 'refs/heads/master', user) - service.execute(project, user, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service.execute end end end @@ -158,7 +164,8 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false }) - service.execute(project, user, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service.execute end it "when pushing a branch for the first time with default branch protection disabled" do @@ -167,7 +174,8 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).not_to receive(:create) - service.execute(project, user, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service.execute end it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do @@ -176,12 +184,14 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true }) - service.execute(project, user, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service.execute end it "when pushing new commits to existing branch" do expect(project).to receive(:execute_hooks) - service.execute(project, user, 'oldrev', 'newrev', 'refs/heads/master') + service = described_class.new(project, user, 'oldrev', 'newrev', 'refs/heads/master') + service.execute end end end @@ -204,7 +214,9 @@ describe GitPushService, services: true do it "creates a note if a pushed commit mentions an issue" do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + + service.execute end it "only creates a cross-reference note if one doesn't already exist" do @@ -212,7 +224,9 @@ describe GitPushService, services: true do expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author) - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + + service.execute end it "defaults to the pushing user if the commit's author is not known" do @@ -222,7 +236,9 @@ describe GitPushService, services: true do ) expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user) - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + + service.execute end it "finds references in the first push to a non-default branch" do @@ -231,7 +247,9 @@ describe GitPushService, services: true do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - service.execute(project, user, @blankrev, @newrev, 'refs/heads/other') + service = described_class.new(project, user, @blankrev, @newrev, 'refs/heads/other') + + service.execute end end @@ -255,18 +273,21 @@ describe GitPushService, services: true do context "to default branches" do it "closes issues" do - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute expect(Issue.find(issue.id)).to be_closed end it "adds a note indicating that the issue is now closed" do expect(SystemNoteService).to receive(:change_status).with(issue, project, commit_author, "closed", closing_commit) - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute end it "doesn't create additional cross-reference notes" do expect(SystemNoteService).not_to receive(:cross_reference) - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute end it "doesn't close issues when external issue tracker is in use" do @@ -274,7 +295,8 @@ describe GitPushService, services: true do # The push still shouldn't create cross-reference notes. expect do - service.execute(project, user, @oldrev, @newrev, 'refs/heads/hurf') + service = described_class.new(project, user, @oldrev, @newrev, 'refs/heads/hurf') + service.execute end.not_to change { Note.where(project_id: project.id, system: true).count } end end @@ -287,11 +309,13 @@ describe GitPushService, services: true do it "creates cross-reference notes" do expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author) - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute end it "doesn't close issues" do - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute expect(Issue.find(issue.id)).to be_opened end end @@ -328,7 +352,8 @@ describe GitPushService, services: true do let(:message) { "this is some work.\n\nrelated to JIRA-1" } it "should initiate one api call to jira server to mention the issue" do - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + service.execute expect(WebMock).to have_requested(:post, jira_api_comment_url).with( body: /mentioned this issue in/ @@ -346,7 +371,9 @@ describe GitPushService, services: true do } }.to_json - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + + service.execute expect(WebMock).to have_requested(:post, jira_api_transition_url).with( body: transition_body ).once @@ -357,7 +384,9 @@ describe GitPushService, services: true do body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]." }.to_json - service.execute(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, @oldrev, @newrev, @ref) + + service.execute expect(WebMock).to have_requested(:post, jira_api_comment_url).with( body: comment_body ).once @@ -376,7 +405,8 @@ describe GitPushService, services: true do end it 'push to first branch updates HEAD' do - service.execute(project, user, @blankrev, @newrev, new_ref) + service = described_class.new(project, user, @blankrev, @newrev, new_ref) + service.execute end end end From 34a6f83d3e79670774e916e0b38016a74ae9dff1 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 11 Feb 2016 18:10:14 +0100 Subject: [PATCH 05/37] Fix API --- lib/api/repositories.rb | 2 +- lib/gitlab/workhorse.rb | 4 ++-- spec/requests/api/repositories_spec.rb | 13 ++++++++++--- spec/support/workhorse_helpers.rb | 16 ++++++++++++++++ 4 files changed, 29 insertions(+), 6 deletions(-) create mode 100644 spec/support/workhorse_helpers.rb diff --git a/lib/api/repositories.rb b/lib/api/repositories.rb index 0178289f57f..0d0f0d4616d 100644 --- a/lib/api/repositories.rb +++ b/lib/api/repositories.rb @@ -99,7 +99,7 @@ module API begin RepositoryArchiveCacheWorker.perform_async - header *Gitlab::Workhorse.send_git_archive(@project, params[:ref], params[:format]) + header *Gitlab::Workhorse.send_git_archive(user_project, params[:sha], params[:format]) rescue not_found!('File') end diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index 2f3e57156b6..c3ddd4c2680 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -3,9 +3,9 @@ require 'json' module Gitlab class Workhorse - class << self - SEND_DATA_HEADER = 'Gitlab-Workhorse-Send-Data' + SEND_DATA_HEADER = 'Gitlab-Workhorse-Send-Data' + class << self def send_git_blob(repository, blob) params = { 'RepoPath' => repository.path_to_repo, diff --git a/spec/requests/api/repositories_spec.rb b/spec/requests/api/repositories_spec.rb index 4911cdd9da6..0ae63b0afec 100644 --- a/spec/requests/api/repositories_spec.rb +++ b/spec/requests/api/repositories_spec.rb @@ -4,6 +4,7 @@ require 'mime/types' describe API::API, api: true do include ApiHelpers include RepoHelpers + include WorkhorseHelpers let(:user) { create(:user) } let(:user2) { create(:user) } @@ -91,21 +92,27 @@ describe API::API, api: true do get api("/projects/#{project.id}/repository/archive", user) repo_name = project.repository.name.gsub("\.git", "") expect(response.status).to eq(200) - expect(json_response['ArchivePath']).to match(/#{repo_name}\-[^\.]+\.tar.gz/) + type, params = workhorse_send_data + expect(type).to eq('git-archive') + expect(params['ArchivePath']).to match(/#{repo_name}\-[^\.]+\.tar.gz/) end it "should get the archive.zip" do get api("/projects/#{project.id}/repository/archive.zip", user) repo_name = project.repository.name.gsub("\.git", "") expect(response.status).to eq(200) - expect(json_response['ArchivePath']).to match(/#{repo_name}\-[^\.]+\.zip/) + type, params = workhorse_send_data + expect(type).to eq('git-archive') + expect(params['ArchivePath']).to match(/#{repo_name}\-[^\.]+\.zip/) end it "should get the archive.tar.bz2" do get api("/projects/#{project.id}/repository/archive.tar.bz2", user) repo_name = project.repository.name.gsub("\.git", "") expect(response.status).to eq(200) - expect(json_response['ArchivePath']).to match(/#{repo_name}\-[^\.]+\.tar.bz2/) + type, params = workhorse_send_data + expect(type).to eq('git-archive') + expect(params['ArchivePath']).to match(/#{repo_name}\-[^\.]+\.tar.bz2/) end it "should return 404 for invalid sha" do diff --git a/spec/support/workhorse_helpers.rb b/spec/support/workhorse_helpers.rb new file mode 100644 index 00000000000..c5c5d4c63d1 --- /dev/null +++ b/spec/support/workhorse_helpers.rb @@ -0,0 +1,16 @@ +module WorkhorseHelpers + extend self + + def workhorse_send_data + @_workhorse_send_data ||= begin + header = response.headers[Gitlab::Workhorse::SEND_DATA_HEADER] + split_header = header.split(':') + type = split_header.shift + header = split_header.join(':') + [ + type, + JSON.parse(Base64.urlsafe_decode64(header)), + ] + end + end +end \ No newline at end of file From 83a81ffc5df04de74b8a93737ddc54d26981baef Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 12 Feb 2016 09:16:19 +0100 Subject: [PATCH 06/37] typo --- app/services/git_push_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 491322f5ff5..5362dd401be 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -40,7 +40,7 @@ class GitPushService else # Use the pushed commits that aren't reachable by the default branch # as a heuristic. This may include more commits than are actually pushed, but - # that shouldn't matter because we check for existing cross-@references later. + # that shouldn't matter because we check for existing cross-references later. @push_commits = @project.repository.commits_between(@project.default_branch, @newrev) # don't process commits for the initial push to the default branch From b877afc3711a5d7223244b244965730d4e80ddc2 Mon Sep 17 00:00:00 2001 From: Prayag Verma Date: Fri, 12 Feb 2016 19:02:35 +0530 Subject: [PATCH 07/37] Fix a typo [ci skip] Remove extra `make` --- doc/development/migration_style_guide.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 4fa1961fde9..a6077779c08 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -13,7 +13,7 @@ or inconsistencies and guard for that. Try to make as little assumptions as poss about the state of the database. Please don't depend on GitLab specific code since it can change in future versions. -If needed copy-paste GitLab code into the migration to make make it forward compatible. +If needed copy-paste GitLab code into the migration to make it forward compatible. ## Comments in the migration From 19a3d42cb3262a135451e0a557366d4477ac81fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 15 Feb 2016 11:51:26 +0100 Subject: [PATCH 08/37] Re-add section about NGINX config and init script updates in 8.4->8.5 update doc These sections were removed but: - even if the NGINX config wasn't modified, it might be in future updates so it's better to always have it instead of having to remember to add it depending on the changes - the init script update section must be there since it's a safe command line that should be run on every update [ci skip] --- doc/update/8.4-to-8.5.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/doc/update/8.4-to-8.5.md b/doc/update/8.4-to-8.5.md index 42b26439848..408a17ac348 100644 --- a/doc/update/8.4-to-8.5.md +++ b/doc/update/8.4-to-8.5.md @@ -82,6 +82,32 @@ There are new configuration options available for [`gitlab.yml`](config/gitlab.y git diff origin/8-4-stable:config/gitlab.yml.example origin/8-5-stable:config/gitlab.yml.example ``` +#### Nginx configuration + +Ensure you're still up-to-date with the latest NGINX configuration changes: + +```sh +# For HTTPS configurations +git diff origin/8-4-stable:lib/support/nginx/gitlab-ssl origin/8-5-stable:lib/support/nginx/gitlab-ssl + +# For HTTP configurations +git diff origin/8-4-stable:lib/support/nginx/gitlab origin/8-5-stable:lib/support/nginx/gitlab +``` + +If you are using Apache instead of NGINX please see the updated [Apache templates]. +Also note that because Apache does not support upstreams behind Unix sockets you +will need to let gitlab-workhorse listen on a TCP port. You can do this +via [/etc/default/gitlab]. + +[Apache templates]: https://gitlab.com/gitlab-org/gitlab-recipes/tree/master/web-server/apache +[/etc/default/gitlab]: https://gitlab.com/gitlab-org/gitlab-ce/blob/8-5-stable/lib/support/init.d/gitlab.default.example#L37 + +#### Init script + +Ensure you're still up-to-date with the latest init script changes: + + sudo cp lib/support/init.d/gitlab /etc/init.d/gitlab + ### 8. Start application sudo service gitlab start From 20e79f714a4b98526d5eeffc82c0de89c8369c4e Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 15 Feb 2016 15:51:00 +0100 Subject: [PATCH 09/37] refactored GitPushService and updated spec --- app/services/git_push_service.rb | 47 ++++++++++--------------- app/workers/post_receive.rb | 7 +++- spec/services/git_push_service_spec.rb | 48 +++++++++++++------------- 3 files changed, 49 insertions(+), 53 deletions(-) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 5362dd401be..b203065e708 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -1,19 +1,10 @@ -class GitPushService - attr_accessor :project, :user, :push_data, :push_commits +class GitPushService < BaseService + attr_accessor :push_data, :push_commits include Gitlab::CurrentSettings include Gitlab::Access - def initialize(project, user, oldrev, newrev, ref) - # TODO: Consider passing an options hash instead https://github.com/bbatsov/ruby-style-guide#too-many-params - @project = project - @user = user - @oldrev = oldrev - @newrev = newrev - @ref = ref - end - # This method will be called after each git update - # and only if the provided user and project is present in GitLab. + # and only if the provided user and project are present in GitLab. # # All callbacks for post receive action should be placed here. # @@ -41,14 +32,14 @@ class GitPushService # Use the pushed commits that aren't reachable by the default branch # as a heuristic. This may include more commits than are actually pushed, but # that shouldn't matter because we check for existing cross-references later. - @push_commits = @project.repository.commits_between(@project.default_branch, @newrev) + @push_commits = @project.repository.commits_between(@project.default_branch, params[:newrev]) # don't process commits for the initial push to the default branch process_commit_messages end elsif push_to_existing_branch? # Collect data for this git push - @push_commits = @project.repository.commits_between(@oldrev, @newrev) + @push_commits = @project.repository.commits_between(params[:oldrev], params[:newrev]) process_commit_messages end # Update merge requests that may be affected by this push. A new branch @@ -59,17 +50,17 @@ class GitPushService protected def update_merge_requests - @project.update_merge_requests(@oldrev, @newrev, @ref, @user) + @project.update_merge_requests(params[:oldrev], params[:newrev], params[:ref], current_user) - EventCreateService.new.push(@project, @user, build_push_data) + EventCreateService.new.push(@project, current_user, build_push_data) @project.execute_hooks(build_push_data.dup, :push_hooks) @project.execute_services(build_push_data.dup, :push_hooks) - CreateCommitBuildsService.new.execute(@project, @user, build_push_data) + CreateCommitBuildsService.new.execute(@project, current_user, build_push_data) ProjectCacheWorker.perform_async(@project.id) end def process_default_branch - @push_commits = project.repository.commits(@newrev) + @push_commits = project.repository.commits(params[:newrev]) # Ensure HEAD points to the default branch in case it is not master project.change_head(branch_name) @@ -103,7 +94,7 @@ class GitPushService # Close issues if these commits were pushed to the project's default branch and the commit message matches the # closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to # a different branch. - closed_issues = commit.closes_issues(user) + closed_issues = commit.closes_issues(current_user) closed_issues.each do |issue| Issues::CloseService.new(project, authors[commit], {}).execute(issue, commit) end @@ -115,36 +106,36 @@ class GitPushService def build_push_data @push_data ||= Gitlab::PushDataBuilder. - build(@project, @user, @oldrev, @newrev, @ref, push_commits) + build(@project, current_user, params[:oldrev], params[:newrev], params[:ref], push_commits) end def push_to_existing_branch? # Return if this is not a push to a branch (e.g. new commits) - Gitlab::Git.branch_ref?(@ref) && !Gitlab::Git.blank_ref?(@oldrev) + Gitlab::Git.branch_ref?(params[:ref]) && !Gitlab::Git.blank_ref?(params[:oldrev]) end def push_to_new_branch? - Gitlab::Git.branch_ref?(@ref) && Gitlab::Git.blank_ref?(@oldrev) + Gitlab::Git.branch_ref?(params[:ref]) && Gitlab::Git.blank_ref?(params[:oldrev]) end def push_remove_branch? - Gitlab::Git.branch_ref?(@ref) && Gitlab::Git.blank_ref?(@newrev) + Gitlab::Git.branch_ref?(params[:ref]) && Gitlab::Git.blank_ref?(params[:newrev]) end def push_to_branch? - Gitlab::Git.branch_ref?(@ref) + Gitlab::Git.branch_ref?(params[:ref]) end def is_default_branch? - Gitlab::Git.branch_ref?(@ref) && - (Gitlab::Git.ref_name(@ref) == project.default_branch || project.default_branch.nil?) + Gitlab::Git.branch_ref?(params[:ref]) && + (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?) end def commit_user(commit) - commit.author || user + commit.author || current_user end def branch_name - @_branch_name ||= Gitlab::Git.ref_name(@ref) + @_branch_name ||= Gitlab::Git.ref_name(params[:ref]) end end diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 80081ac5f49..9e7934b84d8 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -38,7 +38,12 @@ class PostReceive if Gitlab::Git.tag_ref?(ref) GitTagPushService.new.execute(project, @user, oldrev, newrev, ref) else - GitPushService.new(project, @user, oldrev, newrev, ref).execute + GitPushService.new(project, @user, + { + oldrev: oldrev, + newrev: newrev, + ref: ref + }).execute end end end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index ad18f48ff91..f953f226e67 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -16,7 +16,7 @@ describe GitPushService, services: true do describe 'Push branches' do context 'new branch' do subject do - service = described_class.new(project, user, @blankrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: @ref }) service.execute end @@ -37,7 +37,7 @@ describe GitPushService, services: true do context 'existing branch' do subject do - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end @@ -52,7 +52,7 @@ describe GitPushService, services: true do context 'rm branch' do subject do - service = described_class.new(project, user, @oldrev, @blankrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @blankrev, ref: @ref }) service.execute end @@ -74,7 +74,7 @@ describe GitPushService, services: true do describe "Git Push Data" do before do - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute @push_data = service.push_data @commit = project.commit(@newrev) @@ -137,7 +137,7 @@ describe GitPushService, services: true do describe "Push Event" do before do - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute @event = Event.last @push_data = service.push_data @@ -152,7 +152,7 @@ describe GitPushService, services: true do it "when pushing a new branch for the first time" do expect(project).to receive(:update_merge_requests). with(@blankrev, 'newrev', 'refs/heads/master', user) - service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) service.execute end end @@ -164,7 +164,7 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false }) - service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) service.execute end @@ -174,7 +174,7 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).not_to receive(:create) - service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) service.execute end @@ -184,13 +184,13 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true }) - service = described_class.new(project, user, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) service.execute end it "when pushing new commits to existing branch" do expect(project).to receive(:execute_hooks) - service = described_class.new(project, user, 'oldrev', 'newrev', 'refs/heads/master') + service = described_class.new(project, user, { oldrev: 'oldrev', newrev: 'newrev', ref: 'refs/heads/master' }) service.execute end end @@ -214,7 +214,7 @@ describe GitPushService, services: true do it "creates a note if a pushed commit mentions an issue" do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end @@ -224,7 +224,7 @@ describe GitPushService, services: true do expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author) - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end @@ -236,7 +236,7 @@ describe GitPushService, services: true do ) expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user) - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end @@ -247,7 +247,7 @@ describe GitPushService, services: true do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - service = described_class.new(project, user, @blankrev, @newrev, 'refs/heads/other') + service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: 'refs/heads/other' }) service.execute end @@ -273,20 +273,20 @@ describe GitPushService, services: true do context "to default branches" do it "closes issues" do - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute expect(Issue.find(issue.id)).to be_closed end it "adds a note indicating that the issue is now closed" do expect(SystemNoteService).to receive(:change_status).with(issue, project, commit_author, "closed", closing_commit) - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end it "doesn't create additional cross-reference notes" do expect(SystemNoteService).not_to receive(:cross_reference) - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end @@ -295,7 +295,7 @@ describe GitPushService, services: true do # The push still shouldn't create cross-reference notes. expect do - service = described_class.new(project, user, @oldrev, @newrev, 'refs/heads/hurf') + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: 'refs/heads/hurf' }) service.execute end.not_to change { Note.where(project_id: project.id, system: true).count } end @@ -309,12 +309,12 @@ describe GitPushService, services: true do it "creates cross-reference notes" do expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author) - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end it "doesn't close issues" do - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute expect(Issue.find(issue.id)).to be_opened end @@ -352,7 +352,7 @@ describe GitPushService, services: true do let(:message) { "this is some work.\n\nrelated to JIRA-1" } it "should initiate one api call to jira server to mention the issue" do - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute expect(WebMock).to have_requested(:post, jira_api_comment_url).with( @@ -371,7 +371,7 @@ describe GitPushService, services: true do } }.to_json - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute expect(WebMock).to have_requested(:post, jira_api_transition_url).with( @@ -384,7 +384,7 @@ describe GitPushService, services: true do body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]." }.to_json - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute expect(WebMock).to have_requested(:post, jira_api_comment_url).with( @@ -405,7 +405,7 @@ describe GitPushService, services: true do end it 'push to first branch updates HEAD' do - service = described_class.new(project, user, @blankrev, @newrev, new_ref) + service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: new_ref }) service.execute end end From 789aef7f26597f026859b2ddd29fab1120ce8abe Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 15 Feb 2016 16:06:45 -0500 Subject: [PATCH 10/37] Handle nil commits in Gitlab::PushDataBuilder.build Closes #13469 --- lib/gitlab/push_data_builder.rb | 2 ++ spec/lib/gitlab/push_data_builder_spec.rb | 21 ++++++++++++--------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/push_data_builder.rb b/lib/gitlab/push_data_builder.rb index 4f9cdef3869..efcb23c0c6d 100644 --- a/lib/gitlab/push_data_builder.rb +++ b/lib/gitlab/push_data_builder.rb @@ -22,6 +22,8 @@ module Gitlab # } # def build(project, user, oldrev, newrev, ref, commits = [], message = nil) + commits = Array(commits) + # Total commits count commits_count = commits.size diff --git a/spec/lib/gitlab/push_data_builder_spec.rb b/spec/lib/gitlab/push_data_builder_spec.rb index 3ef61685398..5ec9a84c5ab 100644 --- a/spec/lib/gitlab/push_data_builder_spec.rb +++ b/spec/lib/gitlab/push_data_builder_spec.rb @@ -1,12 +1,12 @@ require 'spec_helper' -describe 'Gitlab::PushDataBuilder', lib: true do +describe Gitlab::PushDataBuilder, lib: true do let(:project) { create(:project) } let(:user) { create(:user) } - describe :build_sample do - let(:data) { Gitlab::PushDataBuilder.build_sample(project, user) } + describe '.build_sample' do + let(:data) { described_class.build_sample(project, user) } it { expect(data).to be_a(Hash) } it { expect(data[:before]).to eq('6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') } @@ -22,13 +22,11 @@ describe 'Gitlab::PushDataBuilder', lib: true do it { expect(data[:commits].first[:removed]).to eq([]) } end - describe :build do + describe '.build' do let(:data) do - Gitlab::PushDataBuilder.build(project, - user, - Gitlab::Git::BLANK_SHA, - '8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b', - 'refs/tags/v1.1.0') + described_class.build(project, user, Gitlab::Git::BLANK_SHA, + '8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b', + 'refs/tags/v1.1.0') end it { expect(data).to be_a(Hash) } @@ -38,5 +36,10 @@ describe 'Gitlab::PushDataBuilder', lib: true do it { expect(data[:ref]).to eq('refs/tags/v1.1.0') } it { expect(data[:commits]).to be_empty } it { expect(data[:total_commits_count]).to be_zero } + + it 'does not raise an error when given nil commits' do + expect { described_class.build(spy, spy, spy, spy, spy, nil) }. + not_to raise_error + end end end From 15126fc70ffaca07b6e6e487a7d3a4f0318ee882 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 16 Feb 2016 17:01:25 +0100 Subject: [PATCH 11/37] Fix SVG blob rendering --- app/views/projects/blob/_image.html.haml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/views/projects/blob/_image.html.haml b/app/views/projects/blob/_image.html.haml index 51fa91b08e4..6955b7086b9 100644 --- a/app/views/projects/blob/_image.html.haml +++ b/app/views/projects/blob/_image.html.haml @@ -1,2 +1,8 @@ .file-content.image_file - %img{ src: namespace_project_raw_path(@project.namespace, @project, @id)} + - if blob_svg?(blob) + - # We need to scrub SVG but we cannot do so in the RawController + - blob.load_all_data!(@repository) + - blob = sanitize_svg(blob) + %img{ src: "data:#{blob.mime_type};base64,#{Base64.encode64(blob.data)}"} + - else + %img{ src: namespace_project_raw_path(@project.namespace, @project, @id)} From 24fa458615f4c77eedd8176f905d51a2df87515a Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Tue, 16 Feb 2016 17:29:45 +0100 Subject: [PATCH 12/37] Use /raw/ requests for image diffs --- app/views/projects/diffs/_image.html.haml | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/views/projects/diffs/_image.html.haml b/app/views/projects/diffs/_image.html.haml index 4fcf7ea0b26..e3699c6cab4 100644 --- a/app/views/projects/diffs/_image.html.haml +++ b/app/views/projects/diffs/_image.html.haml @@ -1,19 +1,19 @@ - diff = diff_file.diff -- file.load_all_data!(@project.repository) +- file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(@commit.id, diff.new_path)) +- old_file_raw_path = namespace_project_raw_path(@project.namespace, @project, tree_join(@commit.parent_id, diff.old_path)) - if diff.renamed_file || diff.new_file || diff.deleted_file .image %span.wrap .frame{class: image_diff_class(diff)} - %img{src: "data:#{file.mime_type};base64,#{Base64.encode64(file.data)}"} + %img{src: file_raw_path} %p.image-info= "#{number_to_human_size file.size}" - else - - old_file.load_all_data!(@project.repository) .image %div.two-up.view %span.wrap .frame.deleted %a{href: namespace_project_blob_path(@project.namespace, @project, tree_join(@commit.parent_id, diff.old_path))} - %img{src: "data:#{old_file.mime_type};base64,#{Base64.encode64(old_file.data)}"} + %img{src: old_file_raw_path} %p.image-info.hide %span.meta-filesize= "#{number_to_human_size old_file.size}" | @@ -25,7 +25,7 @@ %span.wrap .frame.added %a{href: namespace_project_blob_path(@project.namespace, @project, tree_join(@commit.id, diff.new_path))} - %img{src: "data:#{file.mime_type};base64,#{Base64.encode64(file.data)}"} + %img{src: file_raw_path} %p.image-info.hide %span.meta-filesize= "#{number_to_human_size file.size}" | @@ -38,10 +38,10 @@ %div.swipe.view.hide .swipe-frame .frame.deleted - %img{src: "data:#{old_file.mime_type};base64,#{Base64.encode64(old_file.data)}"} + %img{src: old_file_raw_path} .swipe-wrap .frame.added - %img{src: "data:#{file.mime_type};base64,#{Base64.encode64(file.data)}"} + %img{src: file_raw_path} %span.swipe-bar %span.top-handle %span.bottom-handle @@ -49,9 +49,9 @@ %div.onion-skin.view.hide .onion-skin-frame .frame.deleted - %img{src: "data:#{old_file.mime_type};base64,#{Base64.encode64(old_file.data)}"} + %img{src: old_file_raw_path} .frame.added - %img{src: "data:#{file.mime_type};base64,#{Base64.encode64(file.data)}"} + %img{src: file_raw_path} .controls .transparent .drag-track From 250d2fed998734e1c884cd0a07c592cd3b272a03 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 16 Feb 2016 15:57:05 -0500 Subject: [PATCH 13/37] Fix labels for git clone/git fetch Project CI setting --- app/views/projects/edit.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index fdcb6987471..042f660077e 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -119,13 +119,13 @@ .col-sm-offset-2.col-sm-10 %p Get recent application code using the following command: .radio - = f.label :build_allow_git_fetch do + = f.label :build_allow_git_fetch_false do = f.radio_button :build_allow_git_fetch, 'false' %strong git clone %br %span.descr Slower but makes sure you have a clean dir before every build .radio - = f.label :build_allow_git_fetch do + = f.label :build_allow_git_fetch_true do = f.radio_button :build_allow_git_fetch, 'true' %strong git fetch %br From 594d21cdca107460e080b7884bed49e2634cc275 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 16 Feb 2016 18:14:47 -0500 Subject: [PATCH 14/37] Bump unicorn to `~> 4.9.0` --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index a1c57af4bac..6dcc9b0dd80 100644 --- a/Gemfile +++ b/Gemfile @@ -112,7 +112,7 @@ gem 'diffy', '~> 3.0.3' # Application server group :unicorn do - gem "unicorn", '~> 4.8.2' + gem "unicorn", '~> 4.9.0' gem 'unicorn-worker-killer', '~> 0.4.2' end diff --git a/Gemfile.lock b/Gemfile.lock index 718285e1665..1ca4b7b96b4 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -833,7 +833,7 @@ GEM unf (0.1.4) unf_ext unf_ext (0.0.7.1) - unicorn (4.8.3) + unicorn (4.9.0) kgio (~> 2.6) rack raindrops (~> 0.7) @@ -1034,7 +1034,7 @@ DEPENDENCIES uglifier (~> 2.7.2) underscore-rails (~> 1.8.0) unf (~> 0.1.4) - unicorn (~> 4.8.2) + unicorn (~> 4.9.0) unicorn-worker-killer (~> 0.4.2) version_sorter (~> 2.0.0) virtus (~> 1.0.1) From c569f8b4800424704691925f9e39b415b5eca33c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 17 Feb 2016 10:17:03 +0100 Subject: [PATCH 15/37] Fix a bug preventing from doing subsequent edits in any Issuable sidebar --- app/views/projects/issues/update.js.haml | 2 +- app/views/projects/merge_requests/update.js.haml | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/projects/issues/update.js.haml b/app/views/projects/issues/update.js.haml index a54733883b4..986d8c220db 100644 --- a/app/views/projects/issues/update.js.haml +++ b/app/views/projects/issues/update.js.haml @@ -1,3 +1,3 @@ $('aside.right-sidebar')[0].outerHTML = "#{escape_javascript(render 'shared/issuable/sidebar', issuable: @issue)}"; $('aside.right-sidebar').effect('highlight'); -new Issue(); \ No newline at end of file +new IssuableContext(); diff --git a/app/views/projects/merge_requests/update.js.haml b/app/views/projects/merge_requests/update.js.haml index ce5157d69a2..9cce5660e1c 100644 --- a/app/views/projects/merge_requests/update.js.haml +++ b/app/views/projects/merge_requests/update.js.haml @@ -1,3 +1,3 @@ -$('aside.right-sidebar')[0].outerHTML= "#{escape_javascript(render 'shared/issuable/sidebar', issuable: @merge_request)}"; -$('aside.right-sidebar').effect('highlight') -merge_request = new MergeRequest(); +$('aside.right-sidebar')[0].outerHTML = "#{escape_javascript(render 'shared/issuable/sidebar', issuable: @merge_request)}"; +$('aside.right-sidebar').effect('highlight'); +new IssuableContext(); From 5255a54df9778b107734a84acb85230d62d3cff7 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 17 Feb 2016 10:42:59 +0100 Subject: [PATCH 16/37] refactored some stuff based on MR feedback --- app/services/git_push_service.rb | 6 +- app/workers/post_receive.rb | 7 +-- spec/services/git_push_service_spec.rb | 84 +++++++++----------------- 3 files changed, 34 insertions(+), 63 deletions(-) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index b203065e708..a1711d234ff 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -106,7 +106,7 @@ class GitPushService < BaseService def build_push_data @push_data ||= Gitlab::PushDataBuilder. - build(@project, current_user, params[:oldrev], params[:newrev], params[:ref], push_commits) + build(@project, current_user, params[:oldrev], params[:newrev], params[:ref], push_commits) end def push_to_existing_branch? @@ -128,7 +128,7 @@ class GitPushService < BaseService def is_default_branch? Gitlab::Git.branch_ref?(params[:ref]) && - (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?) + (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?) end def commit_user(commit) @@ -136,6 +136,6 @@ class GitPushService < BaseService end def branch_name - @_branch_name ||= Gitlab::Git.ref_name(params[:ref]) + @branch_name ||= Gitlab::Git.ref_name(params[:ref]) end end diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 9e7934b84d8..14d7813412e 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -38,12 +38,7 @@ class PostReceive if Gitlab::Git.tag_ref?(ref) GitTagPushService.new.execute(project, @user, oldrev, newrev, ref) else - GitPushService.new(project, @user, - { - oldrev: oldrev, - newrev: newrev, - ref: ref - }).execute + GitPushService.new(project, @user, oldrev: oldrev, newrev: newrev, ref: ref).execute end end end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index f953f226e67..48e796b6946 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -16,8 +16,7 @@ describe GitPushService, services: true do describe 'Push branches' do context 'new branch' do subject do - service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @blankrev, @newrev, @ref ) end it { is_expected.to be_truthy } @@ -37,8 +36,7 @@ describe GitPushService, services: true do context 'existing branch' do subject do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it { is_expected.to be_truthy } @@ -52,8 +50,7 @@ describe GitPushService, services: true do context 'rm branch' do subject do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @blankrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @blankrev, @ref ) end it { is_expected.to be_truthy } @@ -74,8 +71,7 @@ describe GitPushService, services: true do describe "Git Push Data" do before do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + service = execute_service(project, user, @oldrev, @newrev, @ref ) @push_data = service.push_data @commit = project.commit(@newrev) end @@ -137,8 +133,7 @@ describe GitPushService, services: true do describe "Push Event" do before do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + service = execute_service(project, user, @oldrev, @newrev, @ref ) @event = Event.last @push_data = service.push_data end @@ -152,8 +147,7 @@ describe GitPushService, services: true do it "when pushing a new branch for the first time" do expect(project).to receive(:update_merge_requests). with(@blankrev, 'newrev', 'refs/heads/master', user) - service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) - service.execute + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end end end @@ -164,8 +158,7 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false }) - service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) - service.execute + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end it "when pushing a branch for the first time with default branch protection disabled" do @@ -174,8 +167,7 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).not_to receive(:create) - service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) - service.execute + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do @@ -184,14 +176,12 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true }) - service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) - service.execute + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end it "when pushing new commits to existing branch" do expect(project).to receive(:execute_hooks) - service = described_class.new(project, user, { oldrev: 'oldrev', newrev: 'newrev', ref: 'refs/heads/master' }) - service.execute + execute_service(project, user, 'oldrev', 'newrev', 'refs/heads/master' ) end end end @@ -214,9 +204,7 @@ describe GitPushService, services: true do it "creates a note if a pushed commit mentions an issue" do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "only creates a cross-reference note if one doesn't already exist" do @@ -224,9 +212,7 @@ describe GitPushService, services: true do expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "defaults to the pushing user if the commit's author is not known" do @@ -236,9 +222,7 @@ describe GitPushService, services: true do ) expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "finds references in the first push to a non-default branch" do @@ -247,9 +231,7 @@ describe GitPushService, services: true do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: 'refs/heads/other' }) - - service.execute + execute_service(project, user, @blankrev, @newrev, 'refs/heads/other' ) end end @@ -273,21 +255,18 @@ describe GitPushService, services: true do context "to default branches" do it "closes issues" do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) expect(Issue.find(issue.id)).to be_closed end it "adds a note indicating that the issue is now closed" do expect(SystemNoteService).to receive(:change_status).with(issue, project, commit_author, "closed", closing_commit) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "doesn't create additional cross-reference notes" do expect(SystemNoteService).not_to receive(:cross_reference) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "doesn't close issues when external issue tracker is in use" do @@ -295,8 +274,7 @@ describe GitPushService, services: true do # The push still shouldn't create cross-reference notes. expect do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: 'refs/heads/hurf' }) - service.execute + execute_service(project, user, @oldrev, @newrev, 'refs/heads/hurf' ) end.not_to change { Note.where(project_id: project.id, system: true).count } end end @@ -309,13 +287,11 @@ describe GitPushService, services: true do it "creates cross-reference notes" do expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "doesn't close issues" do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) expect(Issue.find(issue.id)).to be_opened end end @@ -352,8 +328,7 @@ describe GitPushService, services: true do let(:message) { "this is some work.\n\nrelated to JIRA-1" } it "should initiate one api call to jira server to mention the issue" do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) expect(WebMock).to have_requested(:post, jira_api_comment_url).with( body: /mentioned this issue in/ @@ -371,9 +346,7 @@ describe GitPushService, services: true do } }.to_json - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) expect(WebMock).to have_requested(:post, jira_api_transition_url).with( body: transition_body ).once @@ -384,9 +357,7 @@ describe GitPushService, services: true do body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]." }.to_json - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) expect(WebMock).to have_requested(:post, jira_api_comment_url).with( body: comment_body ).once @@ -405,8 +376,13 @@ describe GitPushService, services: true do end it 'push to first branch updates HEAD' do - service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: new_ref }) - service.execute + execute_service(project, user, @blankrev, @newrev, new_ref ) end end + + def execute_service(project, user, oldrev, newrev, ref) + service = described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref ) + service.execute + service + end end From 016367c640bf10781e6a5cbabe138257b2a8242e Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 17 Feb 2016 11:08:10 +0100 Subject: [PATCH 17/37] No use to sanitize partial blob data --- app/views/projects/blob/_blob.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/blob/_blob.html.haml b/app/views/projects/blob/_blob.html.haml index 2c5b8dc4356..f3bfe0a18b0 100644 --- a/app/views/projects/blob/_blob.html.haml +++ b/app/views/projects/blob/_blob.html.haml @@ -36,7 +36,7 @@ = render "download", blob: blob - elsif blob.text? - if blob_svg?(blob) - = render "image", blob: sanitize_svg(blob) + = render "image", blob: blob - else = render "text", blob: blob - elsif blob.image? From a87fe2a47220fafdf1eb322f253ea061fe907f95 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 17 Feb 2016 11:08:47 +0100 Subject: [PATCH 18/37] =?UTF-8?q?Fixes=20requested=20by=20R=C3=A9my?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/views/projects/blob/_image.html.haml | 7 ++++--- app/views/projects/diffs/_image.html.haml | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/views/projects/blob/_image.html.haml b/app/views/projects/blob/_image.html.haml index 6955b7086b9..113dba5d832 100644 --- a/app/views/projects/blob/_image.html.haml +++ b/app/views/projects/blob/_image.html.haml @@ -1,8 +1,9 @@ .file-content.image_file - if blob_svg?(blob) - - # We need to scrub SVG but we cannot do so in the RawController + - # We need to scrub SVG but we cannot do so in the RawController: it would + - # be wrong/strange if RawController modified the data. - blob.load_all_data!(@repository) - blob = sanitize_svg(blob) - %img{ src: "data:#{blob.mime_type};base64,#{Base64.encode64(blob.data)}"} + %img{src: "data:#{blob.mime_type};base64,#{Base64.encode64(blob.data)}"} - else - %img{ src: namespace_project_raw_path(@project.namespace, @project, @id)} + %img{src: namespace_project_raw_path(@project.namespace, @project, @id)} diff --git a/app/views/projects/diffs/_image.html.haml b/app/views/projects/diffs/_image.html.haml index e3699c6cab4..752e92e2e6b 100644 --- a/app/views/projects/diffs/_image.html.haml +++ b/app/views/projects/diffs/_image.html.haml @@ -5,7 +5,7 @@ .image %span.wrap .frame{class: image_diff_class(diff)} - %img{src: file_raw_path} + %img{src: diff.deleted_file ? old_file_raw_path : file_raw_path} %p.image-info= "#{number_to_human_size file.size}" - else .image From a9e0301c230a81242d476f30d7089565919214b3 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 16 Feb 2016 17:31:37 +0100 Subject: [PATCH 19/37] Expire caches after forking/importing a repository This ensures the caches for Repository#empty? and Repository#has_visible_content? are flushed after a repository has been imported or forked. Fixes gitlab-org/gitlab-ce#13505 --- app/models/repository.rb | 8 ++++++++ app/workers/repository_fork_worker.rb | 1 + app/workers/repository_import_worker.rb | 1 + spec/models/repository_spec.rb | 11 +++++++++++ spec/workers/repository_fork_worker_spec.rb | 12 ++++++++++++ spec/workers/repository_import_worker_spec.rb | 19 +++++++++++++++++++ 6 files changed, 52 insertions(+) create mode 100644 spec/workers/repository_import_worker_spec.rb diff --git a/app/models/repository.rb b/app/models/repository.rb index ba275fd9803..5696504e7ec 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -258,6 +258,14 @@ class Repository @root_ref = nil end + # Expires the cache(s) used to determine if a repository is empty or not. + def expire_emptiness_caches + cache.expire(:empty?) + @empty = nil + + expire_has_visible_content_cache + end + def expire_has_visible_content_cache cache.expire(:has_visible_content?) @has_visible_content = nil diff --git a/app/workers/repository_fork_worker.rb b/app/workers/repository_fork_worker.rb index 2f991c52339..2572b9d6d98 100644 --- a/app/workers/repository_fork_worker.rb +++ b/app/workers/repository_fork_worker.rb @@ -27,6 +27,7 @@ class RepositoryForkWorker return end + project.repository.expire_emptiness_caches project.import_finish end end diff --git a/app/workers/repository_import_worker.rb b/app/workers/repository_import_worker.rb index e295a9ddd14..0b6f746e118 100644 --- a/app/workers/repository_import_worker.rb +++ b/app/workers/repository_import_worker.rb @@ -18,6 +18,7 @@ class RepositoryImportWorker return end + project.repository.expire_emptiness_caches project.import_finish end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index e1ee43e64db..2cd0606a61d 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -355,6 +355,17 @@ describe Repository, models: true do end end + describe '#expire_emptiness_caches' do + let(:cache) { repository.send(:cache) } + + it 'expires the caches' do + expect(cache).to receive(:expire).with(:empty?) + expect(repository).to receive(:expire_has_visible_content_cache) + + repository.expire_emptiness_caches + end + end + describe :skip_merged_commit do subject { repository.commits(Gitlab::Git::BRANCH_REF_PREFIX + "'test'", nil, 100, 0, true).map{ |k| k.id } } diff --git a/spec/workers/repository_fork_worker_spec.rb b/spec/workers/repository_fork_worker_spec.rb index dae31992620..172537474ee 100644 --- a/spec/workers/repository_fork_worker_spec.rb +++ b/spec/workers/repository_fork_worker_spec.rb @@ -19,6 +19,18 @@ describe RepositoryForkWorker do fork_project.namespace.path) end + it 'flushes the empty caches' do + expect_any_instance_of(Gitlab::Shell).to receive(:fork_repository). + with(project.path_with_namespace, fork_project.namespace.path). + and_return(true) + + expect_any_instance_of(Repository).to receive(:expire_emptiness_caches). + and_call_original + + subject.perform(project.id, project.path_with_namespace, + fork_project.namespace.path) + end + it "handles bad fork" do expect_any_instance_of(Gitlab::Shell).to receive(:fork_repository).and_return(false) subject.perform( diff --git a/spec/workers/repository_import_worker_spec.rb b/spec/workers/repository_import_worker_spec.rb new file mode 100644 index 00000000000..6739063543b --- /dev/null +++ b/spec/workers/repository_import_worker_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe RepositoryImportWorker do + let(:project) { create(:project) } + + subject { described_class.new } + + describe '#perform' do + it 'imports a project' do + expect_any_instance_of(Projects::ImportService).to receive(:execute). + and_return({ status: :ok }) + + expect_any_instance_of(Repository).to receive(:expire_emptiness_caches) + expect_any_instance_of(Project).to receive(:import_finish) + + subject.perform(project.id) + end + end +end From b1203108b0eda20f87c75004f035e9e7a51f1124 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 16 Feb 2016 23:11:56 +0100 Subject: [PATCH 20/37] Flush all repository caches when deleting a repo This ensures that _all_ caches (including any caches normally only flushed under certain conditions) are flushed whenever a project is removed. Because cache keys are based on project namespaces (excluding IDs) not doing so could result in a newly created project re-using old caches (if the project was re-created using the same name). --- app/models/repository.rb | 9 +++++++++ app/services/projects/destroy_service.rb | 14 +++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 5696504e7ec..1c33a7f9679 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -238,6 +238,15 @@ class Repository expire_branch_cache(branch_name) end + # Expires _all_ caches, including those that would normally only be expired + # under specific conditions. + def expire_all_caches! + expire_cache + expire_root_ref_cache + expire_emptiness_caches + expire_has_visible_content_cache + end + def expire_branch_cache(branch_name = nil) # When we push to the root branch we have to flush the cache for all other # branches as their statistics are based on the commits relative to the diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 294157b4f0e..f4dcb142850 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -16,11 +16,15 @@ module Projects return false unless can?(current_user, :remove_project, project) project.team.truncate - project.repository.expire_cache unless project.empty_repo? repo_path = project.path_with_namespace wiki_path = repo_path + '.wiki' + # Flush the cache for both repositories. This has to be done _before_ + # removing the physical repositories as some expiration code depends on + # Git data (e.g. a list of branch names). + flush_caches(project, wiki_path) + Project.transaction do project.destroy! @@ -70,5 +74,13 @@ module Projects def removal_path(path) "#{path}+#{project.id}#{DELETED_FLAG}" end + + def flush_caches(project, wiki_path) + project.repository.expire_all_caches! if project.repository.exists? + + wiki_repo = Repository.new(wiki_path, project) + + wiki_repo.expire_all_caches! if wiki_repo.exists? + end end end From 29bfdf88eab813fd9d5946dcb0126c9dc0d4c83b Mon Sep 17 00:00:00 2001 From: Jacob Schatz Date: Wed, 17 Feb 2016 06:37:21 -0500 Subject: [PATCH 21/37] Fix double scrollbar issues --- .../stylesheets/framework/gitlab-theme.scss | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/app/assets/stylesheets/framework/gitlab-theme.scss b/app/assets/stylesheets/framework/gitlab-theme.scss index 8d9a0aae568..0f68582e447 100644 --- a/app/assets/stylesheets/framework/gitlab-theme.scss +++ b/app/assets/stylesheets/framework/gitlab-theme.scss @@ -118,3 +118,19 @@ body { @include gitlab-theme(#9988CC, $theme-violet, #443366, #332255); } } + +::-webkit-scrollbar{ + width: 3px; +} + +::-webkit-scrollbar-thumb{ + background-color:$theme-charcoal; border-radius: 0; +} + +::-webkit-scrollbar-thumb:hover{ + background-color:$theme-charcoal; +} + +::-webkit-scrollbar-track{ + background-color:#FFF; +} \ No newline at end of file From e9e01bcd960868db39e616c2a254d493097d712f Mon Sep 17 00:00:00 2001 From: Mark Riedesel Date: Thu, 11 Feb 2016 08:52:37 -0600 Subject: [PATCH 22/37] Fix 500 error when comparing by tags --- app/models/repository.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/repository.rb b/app/models/repository.rb index 7f0047a002e..a5787a05277 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -611,6 +611,8 @@ class Repository end def merge_base(first_commit_id, second_commit_id) + first_commit_id = commit(first_commit_id).try(:id) || first_commit_id + second_commit_id = commit(second_commit_id).try(:id) || second_commit_id rugged.merge_base(first_commit_id, second_commit_id) rescue Rugged::ReferenceError nil From 3d9f8ab3b5cf62b426b1c84f30bf41941a3b0fe3 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 17 Feb 2016 14:20:49 +0100 Subject: [PATCH 23/37] Use gitlab-workhorse 0.6.5 --- GITLAB_WORKHORSE_VERSION | 2 +- doc/install/installation.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/GITLAB_WORKHORSE_VERSION b/GITLAB_WORKHORSE_VERSION index d2b13eb644d..ef5e4454454 100644 --- a/GITLAB_WORKHORSE_VERSION +++ b/GITLAB_WORKHORSE_VERSION @@ -1 +1 @@ -0.6.4 +0.6.5 diff --git a/doc/install/installation.md b/doc/install/installation.md index 7ae73450afb..7f16e6ff5c4 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -353,7 +353,7 @@ GitLab Shell is an SSH access and repository management software developed speci cd /home/git sudo -u git -H git clone https://gitlab.com/gitlab-org/gitlab-workhorse.git cd gitlab-workhorse - sudo -u git -H git checkout 0.6.4 + sudo -u git -H git checkout 0.6.5 sudo -u git -H make ### Initialize Database and Activate Advanced Features From 9823d00e0b13224ae9e820e7d3f9fade69201e99 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 17 Feb 2016 11:32:02 -0200 Subject: [PATCH 24/37] Add ability to see and sort on vote count from Issues and MR lists --- CHANGELOG | 1 + app/assets/stylesheets/pages/issues.scss | 2 +- .../stylesheets/pages/merge_requests.scss | 4 +- app/helpers/sorting_helper.rb | 18 ++++++ app/models/concerns/issuable.rb | 25 ++++++++ app/views/projects/issues/_issue.html.haml | 19 +++++++ .../merge_requests/_merge_request.html.haml | 19 +++++++ app/views/shared/_sort_dropdown.html.haml | 4 ++ features/project/issues/issues.feature | 10 ++++ features/project/merge_requests.feature | 11 ++++ features/steps/project/issues/issues.rb | 57 +++++++++++++++++++ features/steps/project/merge_requests.rb | 50 ++++++++++++++++ features/steps/shared/issuable.rb | 16 ++++++ spec/factories/notes.rb | 14 +++++ 14 files changed, 247 insertions(+), 3 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 74bc366d203..08a9b8df4a0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -62,6 +62,7 @@ v 8.5.0 (unreleased) - Replaces "Create merge request" link with one to the "Merge Request" when one exists - Fix CI builds badge, add a new link to builds badge, deprecate the old one - Fix broken link to project in build notification emails + - Ability to see and sort on vote count from Issues and MR lists v 8.4.4 - Update omniauth-saml gem to 1.4.2 diff --git a/app/assets/stylesheets/pages/issues.scss b/app/assets/stylesheets/pages/issues.scss index 8694bd654a7..1cc853dd4f5 100644 --- a/app/assets/stylesheets/pages/issues.scss +++ b/app/assets/stylesheets/pages/issues.scss @@ -24,7 +24,7 @@ display: inline-block; } - .issue-no-comments { + .issue-no-comments, .issue-no-votes { opacity: 0.5; } } diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index f033ff15f88..6b497cd56ed 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -163,7 +163,7 @@ display: inline-block; } - .merge-request-no-comments { + .merge-request-no-comments, .merge-request-no-votes { opacity: 0.5; } } @@ -236,4 +236,4 @@ } } } -} \ No newline at end of file +} diff --git a/app/helpers/sorting_helper.rb b/app/helpers/sorting_helper.rb index 241179b0212..f9026b887da 100644 --- a/app/helpers/sorting_helper.rb +++ b/app/helpers/sorting_helper.rb @@ -11,6 +11,8 @@ module SortingHelper sort_value_largest_repo => sort_title_largest_repo, sort_value_recently_signin => sort_title_recently_signin, sort_value_oldest_signin => sort_title_oldest_signin, + sort_value_downvotes => sort_title_downvotes, + sort_value_upvotes => sort_title_upvotes } end @@ -54,6 +56,14 @@ module SortingHelper 'Oldest sign in' end + def sort_title_downvotes + 'Least popular' + end + + def sort_title_upvotes + 'Most popular' + end + def sort_value_oldest_updated 'updated_asc' end @@ -93,4 +103,12 @@ module SortingHelper def sort_value_oldest_signin 'oldest_sign_in' end + + def sort_value_downvotes + 'downvotes_desc' + end + + def sort_value_upvotes + 'upvotes_desc' + end end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 5136d0196a5..e5f089fb8a0 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -69,10 +69,35 @@ module Issuable case method.to_s when 'milestone_due_asc' then order_milestone_due_asc when 'milestone_due_desc' then order_milestone_due_desc + when 'downvotes_desc' then order_downvotes_desc + when 'upvotes_desc' then order_upvotes_desc else order_by(method) end end + + def order_downvotes_desc + order_votes_desc('thumbsdown') + end + + def order_upvotes_desc + order_votes_desc('thumbsup') + end + + def order_votes_desc(award_emoji_name) + issuable_table = self.arel_table + note_table = Note.arel_table + + join_clause = issuable_table.join(note_table, Arel::Nodes::OuterJoin).on( + note_table[:noteable_id].eq(issuable_table[:id]).and( + note_table[:noteable_type].eq(self.name).and( + note_table[:is_award].eq(true).and(note_table[:note].eq(award_emoji_name)) + ) + ) + ).join_sources + + joins(join_clause).group(issuable_table[:id]).reorder("COUNT(notes.id) DESC") + end end def today? diff --git a/app/views/projects/issues/_issue.html.haml b/app/views/projects/issues/_issue.html.haml index f9cf4910df3..5b0edcfa978 100644 --- a/app/views/projects/issues/_issue.html.haml +++ b/app/views/projects/issues/_issue.html.haml @@ -15,6 +15,25 @@ %li = link_to_member(@project, issue.assignee, name: false, title: "Assigned to :name") + - upvotes, downvotes = issue.upvotes, issue.downvotes + - if upvotes > 0 || downvotes > 0 + %li + = icon('thumbs-up') + = upvotes + - else + %li{ class: 'issue-no-votes' } + = icon('thumbs-up') + = upvotes + + - if upvotes > 0 || downvotes > 0 + %li + = icon('thumbs-down') + = downvotes + - else + %li{ class: 'issue-no-votes' } + = icon('thumbs-down') + = downvotes + - note_count = issue.notes.user.count - if note_count > 0 %li diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index e25bf917b43..b230b3a0110 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/app/views/projects/merge_requests/_merge_request.html.haml @@ -24,6 +24,25 @@ %li = link_to_member(merge_request.source_project, merge_request.assignee, name: false, title: "Assigned to :name") + - upvotes, downvotes = merge_request.upvotes, merge_request.downvotes + - if upvotes > 0 || downvotes > 0 + %li + = icon('thumbs-up') + = upvotes + - else + %li{ class: 'merge-request-no-votes' } + = icon('thumbs-up') + = upvotes + + - if upvotes > 0 || downvotes > 0 + %li + = icon('thumbs-down') + = downvotes + - else + %li{ class: 'merge-request-no-votes' } + = icon('thumbs-down') + = downvotes + - note_count = merge_request.mr_and_commit_notes.user.count - if note_count > 0 %li diff --git a/app/views/shared/_sort_dropdown.html.haml b/app/views/shared/_sort_dropdown.html.haml index f09ab25276d..e3a6a5a68b6 100644 --- a/app/views/shared/_sort_dropdown.html.haml +++ b/app/views/shared/_sort_dropdown.html.haml @@ -20,3 +20,7 @@ = sort_title_milestone_soon = link_to page_filter_path(sort: sort_value_milestone_later) do = sort_title_milestone_later + = link_to page_filter_path(sort: sort_value_upvotes) do + = sort_title_upvotes + = link_to page_filter_path(sort: sort_value_downvotes) do + = sort_title_downvotes diff --git a/features/project/issues/issues.feature b/features/project/issues/issues.feature index ca2399d85a9..89af58dcef3 100644 --- a/features/project/issues/issues.feature +++ b/features/project/issues/issues.feature @@ -88,6 +88,16 @@ Feature: Project Issues And I visit dashboard merge requests page Then The list should be sorted by "Oldest updated" + @javascript + Scenario: Sort issues by upvotes/downvotes + Given project "Shop" have "Bugfix" open issue + And issue "Release 0.4" have 2 upvotes and 1 downvote + And issue "Tweet control" have 1 upvote and 2 downvotes + And I sort the list by "Most popular" + Then The list should be sorted by "Most popular" + And I sort the list by "Least popular" + Then The list should be sorted by "Least popular" + @javascript Scenario: I search issue Given I fill in issue search with "Re" diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 5995e787961..495f25f28e7 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -107,6 +107,17 @@ Feature: Project Merge Requests And I visit dashboard merge requests page Then The list should be sorted by "Oldest updated" + @javascript + Scenario: Sort merge requests by upvotes/downvotes + Given project "Shop" have "Bug NS-05" open merge request with diffs inside + And project "Shop" have "Bug NS-06" open merge request + And merge request "Bug NS-04" have 2 upvotes and 1 downvote + And merge request "Bug NS-06" have 1 upvote and 2 downvotes + And I sort the list by "Most popular" + Then The list should be sorted by "Most popular" + And I sort the list by "Least popular" + Then The list should be sorted by "Least popular" + @javascript Scenario: Visiting Merge Requests after commenting on diffs Given project "Shop" have "Bug NS-05" open merge request with diffs inside diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb index 09a89e99831..54d64bacc52 100644 --- a/features/steps/project/issues/issues.rb +++ b/features/steps/project/issues/issues.rb @@ -174,6 +174,13 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps author: project.users.first) end + step 'project "Shop" have "Bugfix" open issue' do + create(:issue, + title: "Bugfix", + project: project, + author: project.users.first) + end + step 'project "Shop" have "Release 0.3" closed issue' do create(:closed_issue, title: "Release 0.3", @@ -181,6 +188,56 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps author: project.users.first) end + step 'issue "Release 0.4" have 2 upvotes and 1 downvote' do + issue = Issue.find_by(title: 'Release 0.4') + create_list(:upvote_note, 2, project: project, noteable: issue) + create(:downvote_note, project: project, noteable: issue) + end + + step 'issue "Tweet control" have 1 upvote and 2 downvotes' do + issue = Issue.find_by(title: 'Tweet control') + create(:upvote_note, project: project, noteable: issue) + create_list(:downvote_note, 2, project: project, noteable: issue) + end + + step 'The list should be sorted by "Least popular"' do + page.within '.issues-list' do + page.within 'li.issue:nth-child(1)' do + expect(page).to have_content 'Tweet control' + expect(page).to have_content '1 2' + end + + page.within 'li.issue:nth-child(2)' do + expect(page).to have_content 'Release 0.4' + expect(page).to have_content '2 1' + end + + page.within 'li.issue:nth-child(3)' do + expect(page).to have_content 'Bugfix' + expect(page).to have_content '0 0' + end + end + end + + step 'The list should be sorted by "Most popular"' do + page.within '.issues-list' do + page.within 'li.issue:nth-child(1)' do + expect(page).to have_content 'Release 0.4' + expect(page).to have_content '2 1' + end + + page.within 'li.issue:nth-child(2)' do + expect(page).to have_content 'Tweet control' + expect(page).to have_content '1 2' + end + + page.within 'li.issue:nth-child(3)' do + expect(page).to have_content 'Bugfix' + expect(page).to have_content '0 0' + end + end + end + step 'empty project "Empty Project"' do create :empty_project, name: 'Empty Project', namespace: @user.namespace end diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index 337893e6209..2160d8645fd 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -138,6 +138,56 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps author: project.users.first) end + step 'merge request "Bug NS-04" have 2 upvotes and 1 downvote' do + merge_request = MergeRequest.find_by(title: 'Bug NS-04') + create_list(:upvote_note, 2, project: project, noteable: merge_request) + create(:downvote_note, project: project, noteable: merge_request) + end + + step 'merge request "Bug NS-06" have 1 upvote and 2 downvotes' do + merge_request = MergeRequest.find_by(title: 'Bug NS-06') + create(:upvote_note, project: project, noteable: merge_request) + create_list(:downvote_note, 2, project: project, noteable: merge_request) + end + + step 'The list should be sorted by "Least popular"' do + page.within '.mr-list' do + page.within 'li.merge-request:nth-child(1)' do + expect(page).to have_content 'Bug NS-06' + expect(page).to have_content '1 2' + end + + page.within 'li.merge-request:nth-child(2)' do + expect(page).to have_content 'Bug NS-04' + expect(page).to have_content '2 1' + end + + page.within 'li.merge-request:nth-child(3)' do + expect(page).to have_content 'Bug NS-05' + expect(page).to have_content '0 0' + end + end + end + + step 'The list should be sorted by "Most popular"' do + page.within '.mr-list' do + page.within 'li.merge-request:nth-child(1)' do + expect(page).to have_content 'Bug NS-04' + expect(page).to have_content '2 1' + end + + page.within 'li.merge-request:nth-child(2)' do + expect(page).to have_content 'Bug NS-06' + expect(page).to have_content '1 2' + end + + page.within 'li.merge-request:nth-child(3)' do + expect(page).to have_content 'Bug NS-05' + expect(page).to have_content '0 0' + end + end + end + step 'I click on the Changes tab' do page.within '.merge-request-tabs' do click_link 'Changes' diff --git a/features/steps/shared/issuable.rb b/features/steps/shared/issuable.rb index 2117feaedb8..ae10c6069a9 100644 --- a/features/steps/shared/issuable.rb +++ b/features/steps/shared/issuable.rb @@ -113,6 +113,22 @@ module SharedIssuable end end + step 'I sort the list by "Least popular"' do + find('button.dropdown-toggle.btn').click + + page.within('ul.dropdown-menu.dropdown-menu-align-right li') do + click_link 'Least popular' + end + end + + step 'I sort the list by "Most popular"' do + find('button.dropdown-toggle.btn').click + + page.within('ul.dropdown-menu.dropdown-menu-align-right li') do + click_link 'Most popular' + end + end + step 'The list should be sorted by "Oldest updated"' do page.within('div.dropdown.inline.prepend-left-10') do expect(page.find('button.dropdown-toggle.btn')).to have_content('Oldest updated') diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index 35a20adeef3..32c202891d8 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -34,6 +34,8 @@ FactoryGirl.define do factory :note_on_merge_request_diff, traits: [:on_merge_request, :on_diff] factory :note_on_project_snippet, traits: [:on_project_snippet] factory :system_note, traits: [:system] + factory :downvote_note, traits: [:award, :downvote] + factory :upvote_note, traits: [:award, :upvote] trait :on_commit do project @@ -65,6 +67,18 @@ FactoryGirl.define do system true end + trait :award do + is_award true + end + + trait :downvote do + note "thumbsdown" + end + + trait :upvote do + note "thumbsup" + end + trait :with_attachment do attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "`/png") } end From 16e116bc167b4c6a4e1efccc9fa429163fc28c10 Mon Sep 17 00:00:00 2001 From: Jacob Schatz Date: Wed, 17 Feb 2016 09:02:34 -0500 Subject: [PATCH 25/37] Correct icon for milestones. --- app/views/shared/issuable/_sidebar.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index 2aada5c9952..a45775f36b5 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -47,7 +47,7 @@ .block.milestone .sidebar-collapsed-icon - = icon('balance-scale') + = icon('clock-o') %span - if issuable.milestone = issuable.milestone.title From 3d3ac87af991319d6960a963c0975c83bf7e4108 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 17 Feb 2016 15:05:44 +0100 Subject: [PATCH 26/37] Redirect /import to project page if no importing at all and repo exists Fixes #13367. --- app/controllers/projects/imports_controller.rb | 12 +++++++++--- app/models/project.rb | 4 ++++ spec/controllers/projects/imports_controller_spec.rb | 12 ++++++++++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/app/controllers/projects/imports_controller.rb b/app/controllers/projects/imports_controller.rb index 07f355c35b1..196996f1752 100644 --- a/app/controllers/projects/imports_controller.rb +++ b/app/controllers/projects/imports_controller.rb @@ -3,6 +3,7 @@ class Projects::ImportsController < Projects::ApplicationController before_action :authorize_admin_project! before_action :require_no_repo, only: [:new, :create] before_action :redirect_if_progress, only: [:new, :create] + before_action :redirect_if_no_import, only: :show def new end @@ -63,14 +64,19 @@ class Projects::ImportsController < Projects::ApplicationController def require_no_repo if @project.repository_exists? - redirect_to(namespace_project_path(@project.namespace, @project)) + redirect_to namespace_project_path(@project.namespace, @project) end end def redirect_if_progress if @project.import_in_progress? - redirect_to namespace_project_import_path(@project.namespace, @project) && - return + redirect_to namespace_project_import_path(@project.namespace, @project) + end + end + + def redirect_if_no_import + if @project.repository_exists? && @project.no_import? + redirect_to namespace_project_path(@project.namespace, @project) end end end diff --git a/app/models/project.rb b/app/models/project.rb index a43878ebcad..95ad88c76ae 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -382,6 +382,10 @@ class Project < ActiveRecord::Base external_import? || forked? end + def no_import? + import_status == 'none' + end + def external_import? import_url.present? end diff --git a/spec/controllers/projects/imports_controller_spec.rb b/spec/controllers/projects/imports_controller_spec.rb index 85d1d1e0524..0147bd2b953 100644 --- a/spec/controllers/projects/imports_controller_spec.rb +++ b/spec/controllers/projects/imports_controller_spec.rb @@ -104,6 +104,18 @@ describe Projects::ImportsController do end end end + + context 'when import never happened' do + before do + project.update_attribute(:import_status, :none) + end + + it 'redirects to namespace_project_path' do + get :show, namespace_id: project.namespace.to_param, project_id: project.to_param + + expect(response).to redirect_to namespace_project_path(project.namespace, project) + end + end end end end From b3bd7c1999a1b2a662c029a608bcf25d50e9ee82 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 17 Feb 2016 15:22:33 +0100 Subject: [PATCH 27/37] Add newline --- spec/support/workhorse_helpers.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/support/workhorse_helpers.rb b/spec/support/workhorse_helpers.rb index c5c5d4c63d1..107b6e30924 100644 --- a/spec/support/workhorse_helpers.rb +++ b/spec/support/workhorse_helpers.rb @@ -13,4 +13,4 @@ module WorkhorseHelpers ] end end -end \ No newline at end of file +end From 42c2064a36f41f83501d9c8183fe93733da7ede6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 17 Feb 2016 16:31:14 +0100 Subject: [PATCH 28/37] Sort line notes used in parallel diff by created_at Fixes #13464. --- app/helpers/diff_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index f9bacc8ba45..6a3ab3ea40a 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -69,7 +69,7 @@ module DiffHelper end def line_comments - @line_comments ||= @line_notes.select(&:active?).group_by(&:line_code) + @line_comments ||= @line_notes.select(&:active?).sort_by(&:created_at).group_by(&:line_code) end def organize_comments(type_left, type_right, line_code_left, line_code_right) From c110c9bd7f2fabb5c9397291f1f13c6accab70e6 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 17 Feb 2016 18:29:43 +0100 Subject: [PATCH 29/37] refactored spec --- spec/services/git_push_service_spec.rb | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 48e796b6946..994585fb32c 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -14,10 +14,17 @@ describe GitPushService, services: true do end describe 'Push branches' do + + let(:oldrev) { @oldrev } + let(:newrev) { @newrev } + + subject do + execute_service(project, user, oldrev, newrev, @ref ) + end + context 'new branch' do - subject do - execute_service(project, user, @blankrev, @newrev, @ref ) - end + + let(:oldrev) { @blankrev } it { is_expected.to be_truthy } @@ -35,9 +42,6 @@ describe GitPushService, services: true do end context 'existing branch' do - subject do - execute_service(project, user, @oldrev, @newrev, @ref ) - end it { is_expected.to be_truthy } @@ -49,9 +53,8 @@ describe GitPushService, services: true do end context 'rm branch' do - subject do - execute_service(project, user, @oldrev, @blankrev, @ref ) - end + + let(:newrev) { @blankrev } it { is_expected.to be_truthy } From e5f9c4460f8c888e0a4372e31f22e61b96c2cbe6 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Wed, 17 Feb 2016 20:34:54 +0200 Subject: [PATCH 30/37] Remove remaining sqlite method call --- lib/tasks/gitlab/check.rake | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/tasks/gitlab/check.rake b/lib/tasks/gitlab/check.rake index 54d95cd62a5..81099cb8ba9 100644 --- a/lib/tasks/gitlab/check.rake +++ b/lib/tasks/gitlab/check.rake @@ -16,7 +16,6 @@ namespace :gitlab do check_git_config check_database_config_exists - check_database_is_not_sqlite check_migrations_are_up check_orphaned_group_members check_gitlab_config_exists From 2770de961711076d1e8374bd943382eb14c90ad5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 17 Feb 2016 21:15:13 -0500 Subject: [PATCH 31/37] Reopened MRs should also be considered as open. --- app/models/merge_request.rb | 2 +- .../merge_requests_controller_spec.rb | 34 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 1be8061e53d..ea2b0e075f6 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -137,7 +137,7 @@ class MergeRequest < ActiveRecord::Base scope :by_milestone, ->(milestone) { where(milestone_id: milestone) } scope :in_projects, ->(project_ids) { where("source_project_id in (:project_ids) OR target_project_id in (:project_ids)", project_ids: project_ids) } scope :of_projects, ->(ids) { where(target_project_id: ids) } - scope :opened, -> { with_state(:opened) } + scope :opened, -> { with_states(:opened, :reopened) } scope :merged, -> { with_state(:merged) } scope :closed, -> { with_state(:closed) } scope :closed_and_merged, -> { with_states(:closed, :merged) } diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 9450a389d81..e82fe26c7a6 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -123,6 +123,40 @@ describe Projects::MergeRequestsController do end end + describe 'GET #index' do + def get_merge_requests + get :index, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + state: 'opened' + end + + context 'when filtering by opened state' do + + context 'with opened merge requests' do + it 'should list those merge requests' do + get_merge_requests + + expect(assigns(:merge_requests)).to include(merge_request) + end + end + + context 'with reopened merge requests' do + before do + merge_request.close! + merge_request.reopen! + end + + it 'should list those merge requests' do + get_merge_requests + + expect(assigns(:merge_requests)).to include(merge_request) + end + end + + end + end + describe 'GET diffs' do def go(format: 'html') get :diffs, From 364072b7d0232a5e306a305ca0bb40b03da5b630 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 18 Feb 2016 10:29:20 +0100 Subject: [PATCH 32/37] Return a builds array in builds create service --- app/services/ci/create_builds_service.rb | 1 + .../services/ci/create_builds_service_spec.rb | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 spec/services/ci/create_builds_service_spec.rb diff --git a/app/services/ci/create_builds_service.rb b/app/services/ci/create_builds_service.rb index ad901f2da5d..002f7ba1278 100644 --- a/app/services/ci/create_builds_service.rb +++ b/app/services/ci/create_builds_service.rb @@ -34,6 +34,7 @@ module Ci build = commit.builds.create!(build_attrs) build.execute_hooks + build end end end diff --git a/spec/services/ci/create_builds_service_spec.rb b/spec/services/ci/create_builds_service_spec.rb new file mode 100644 index 00000000000..9bf72f45766 --- /dev/null +++ b/spec/services/ci/create_builds_service_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +describe Ci::CreateBuildsService, services: true do + let(:commit) { create(:ci_commit) } + let(:user) { create(:user) } + + describe '#execute' do + subject do + described_class.new.execute(commit, stage, 'master', nil, user, nil, status) + end + + context 'stubbed .gitlab-ci.yml' do + let(:stage) { 'test' } + let(:status) { 'success' } + + it { is_expected.to be_an_instance_of Array } + it { is_expected.to all(be_an_instance_of Ci::Build) } + end + end +end From 0ae7c954537663aa2a574133e42d05eafe5db6fb Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 18 Feb 2016 10:52:57 +0100 Subject: [PATCH 33/37] Add specs for build status helper class --- spec/factories/ci/builds.rb | 8 ++++++++ spec/lib/ci/status_spec.rb | 17 +++++++++++++++++ spec/services/ci/create_builds_service_spec.rb | 14 +++++++++++--- 3 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 spec/lib/ci/status_spec.rb diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index c1b6ecd329a..46a211fa60e 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -16,6 +16,14 @@ FactoryGirl.define do commit factory: :ci_commit + trait :success do + status 'success' + end + + trait :failed do + status 'failed' + end + trait :canceled do status 'canceled' end diff --git a/spec/lib/ci/status_spec.rb b/spec/lib/ci/status_spec.rb new file mode 100644 index 00000000000..aa59bdff1af --- /dev/null +++ b/spec/lib/ci/status_spec.rb @@ -0,0 +1,17 @@ +require 'spec_helper' + +describe Ci::Status do + describe '.get_status' do + subject { described_class.get_status(builds) } + + context 'all builds successful' do + let(:builds) { Array.new(2) { create(:ci_build, :success) } } + it { is_expected.to eq 'success' } + end + + context 'at least one build failed' do + let(:builds) { [create(:ci_build, :success), create(:ci_build, :failed)] } + it { is_expected.to eq 'failed' } + end + end +end diff --git a/spec/services/ci/create_builds_service_spec.rb b/spec/services/ci/create_builds_service_spec.rb index 9bf72f45766..1fca3628686 100644 --- a/spec/services/ci/create_builds_service_spec.rb +++ b/spec/services/ci/create_builds_service_spec.rb @@ -5,16 +5,24 @@ describe Ci::CreateBuildsService, services: true do let(:user) { create(:user) } describe '#execute' do + # Using stubbed .gitlab-ci.yml created in commit factory + # + subject do - described_class.new.execute(commit, stage, 'master', nil, user, nil, status) + described_class.new.execute(commit, 'test', 'master', nil, user, nil, status) end - context 'stubbed .gitlab-ci.yml' do - let(:stage) { 'test' } + context 'next builds available' do let(:status) { 'success' } it { is_expected.to be_an_instance_of Array } it { is_expected.to all(be_an_instance_of Ci::Build) } end + + context 'builds skipped' do + let(:status) { 'skipped' } + + it { is_expected.to be_empty } + end end end From 8114786665a59526a988ce346393243996769aff Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 18 Feb 2016 11:47:35 +0100 Subject: [PATCH 34/37] Fix builds scheduler when first build is allowed to fail Before this fix when there was only one relevant, previous build and it failed, but was allowed to fail, entire build had been marked as skipped. Closes #3192 --- lib/ci/status.rb | 4 +--- spec/factories/ci/builds.rb | 12 ++++++++++++ spec/lib/ci/status_spec.rb | 20 ++++++++++++++++++++ 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/lib/ci/status.rb b/lib/ci/status.rb index c02b3b8f3e4..3fb1fe29494 100644 --- a/lib/ci/status.rb +++ b/lib/ci/status.rb @@ -1,11 +1,9 @@ module Ci class Status def self.get_status(statuses) - statuses.reject! { |status| status.try(&:allow_failure?) } - if statuses.none? 'skipped' - elsif statuses.all?(&:success?) + elsif statuses.all? { |status| status.success? || status.ignored? } 'success' elsif statuses.all?(&:pending?) 'pending' diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 46a211fa60e..1243647a78d 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -28,6 +28,18 @@ FactoryGirl.define do status 'canceled' end + trait :running do + status 'running' + end + + trait :pending do + status 'pending' + end + + trait :allowed_to_fail do + allow_failure true + end + after(:build) do |build, evaluator| build.project = build.commit.project end diff --git a/spec/lib/ci/status_spec.rb b/spec/lib/ci/status_spec.rb index aa59bdff1af..cc4199dc7b6 100644 --- a/spec/lib/ci/status_spec.rb +++ b/spec/lib/ci/status_spec.rb @@ -13,5 +13,25 @@ describe Ci::Status do let(:builds) { [create(:ci_build, :success), create(:ci_build, :failed)] } it { is_expected.to eq 'failed' } end + + context 'at least one running' do + let(:builds) { [create(:ci_build, :success), create(:ci_build, :running)] } + it { is_expected.to eq 'running' } + end + + context 'at least one pending' do + let(:builds) { [create(:ci_build, :success), create(:ci_build, :pending)] } + it { is_expected.to eq 'running' } + end + + context 'build success and failed but allowed to fail' do + let(:builds) { [create(:ci_build, :success), create(:ci_build, :failed, :allowed_to_fail)] } + it { is_expected.to eq 'success' } + end + + context 'one build failed but allowed to fail' do + let(:builds) { [create(:ci_build, :failed, :allowed_to_fail)] } + it { is_expected.to eq 'success' } + end end end From 52d5eefd1fbb0af68c541b3d23397501f665dfa0 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 18 Feb 2016 12:05:40 +0100 Subject: [PATCH 35/37] Add Changelog entry for builds scheduler fix --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 08a9b8df4a0..b0d08d97226 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -63,6 +63,7 @@ v 8.5.0 (unreleased) - Fix CI builds badge, add a new link to builds badge, deprecate the old one - Fix broken link to project in build notification emails - Ability to see and sort on vote count from Issues and MR lists + - Fix builds scheduler when first build in stage was allowed to fail v 8.4.4 - Update omniauth-saml gem to 1.4.2 From bee26f9c4ef3c3cb1b1d3eb88500220823a7d7fb Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 18 Feb 2016 14:08:50 +0100 Subject: [PATCH 36/37] Add specs covering bug in build allowed to fail case --- spec/models/ci/commit_spec.rb | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/spec/models/ci/commit_spec.rb b/spec/models/ci/commit_spec.rb index dfc0cc3be1c..4dc309a4255 100644 --- a/spec/models/ci/commit_spec.rb +++ b/spec/models/ci/commit_spec.rb @@ -247,6 +247,35 @@ describe Ci::Commit, models: true do end end + + context 'custom stage with first job allowed to fail' do + let(:yaml) do + { + stages: ['clean', 'test'], + clean_job: { + stage: 'clean', + allow_failure: true, + script: 'BUILD', + }, + test_job: { + stage: 'test', + script: 'TEST', + }, + } + end + + before do + stub_ci_commit_yaml_file(YAML.dump(yaml)) + create_builds + end + + it 'properly schedules builds' do + expect(commit.builds.pluck(:status)).to contain_exactly('pending') + commit.builds.running_or_pending.each(&:drop) + expect(commit.builds.pluck(:status)).to contain_exactly('pending', 'failed') + end + end + context 'properly creates builds when "when" is defined' do let(:yaml) do { From 71795b9ae8b21e796be73090bf96f66b953ad138 Mon Sep 17 00:00:00 2001 From: Jacob Schatz Date: Thu, 18 Feb 2016 10:55:30 -0500 Subject: [PATCH 37/37] Uses cross browser niceScroll to do scrolling on sidebar. --- .../javascripts/issuable_context.js.coffee | 2 ++ .../stylesheets/framework/gitlab-theme.scss | 16 ---------------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/issuable_context.js.coffee b/app/assets/javascripts/issuable_context.js.coffee index d17b1123418..e52b73f94f6 100644 --- a/app/assets/javascripts/issuable_context.js.coffee +++ b/app/assets/javascripts/issuable_context.js.coffee @@ -15,3 +15,5 @@ class @IssuableContext block.find('.selectbox').show() block.find('.value').hide() block.find('.js-select2').select2("open") + + $(".right-sidebar").niceScroll() diff --git a/app/assets/stylesheets/framework/gitlab-theme.scss b/app/assets/stylesheets/framework/gitlab-theme.scss index 0f68582e447..12cef6f8ea1 100644 --- a/app/assets/stylesheets/framework/gitlab-theme.scss +++ b/app/assets/stylesheets/framework/gitlab-theme.scss @@ -117,20 +117,4 @@ body { &.ui_violet { @include gitlab-theme(#9988CC, $theme-violet, #443366, #332255); } -} - -::-webkit-scrollbar{ - width: 3px; -} - -::-webkit-scrollbar-thumb{ - background-color:$theme-charcoal; border-radius: 0; -} - -::-webkit-scrollbar-thumb:hover{ - background-color:$theme-charcoal; -} - -::-webkit-scrollbar-track{ - background-color:#FFF; } \ No newline at end of file