From 1733a9dd032f5300e215455867dc44da2c050197 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Mon, 2 Jul 2018 17:01:31 -0500 Subject: [PATCH 001/113] Backport some changes made for this spec in EE With these changes this file will have the same content on EE --- spec/requests/api/namespaces_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/requests/api/namespaces_spec.rb b/spec/requests/api/namespaces_spec.rb index 98102fcd6a7..e2000ab42e8 100644 --- a/spec/requests/api/namespaces_spec.rb +++ b/spec/requests/api/namespaces_spec.rb @@ -23,10 +23,10 @@ describe API::Namespaces do expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers - expect(group_kind_json_response.keys).to contain_exactly('id', 'kind', 'name', 'path', 'full_path', - 'parent_id', 'members_count_with_descendants') + expect(group_kind_json_response.keys).to include('id', 'kind', 'name', 'path', 'full_path', + 'parent_id', 'members_count_with_descendants') - expect(user_kind_json_response.keys).to contain_exactly('id', 'kind', 'name', 'path', 'full_path', 'parent_id') + expect(user_kind_json_response.keys).to include('id', 'kind', 'name', 'path', 'full_path', 'parent_id') end it "admin: returns an array of all namespaces" do @@ -58,8 +58,8 @@ describe API::Namespaces do owned_group_response = json_response.find { |resource| resource['id'] == group1.id } - expect(owned_group_response.keys).to contain_exactly('id', 'kind', 'name', 'path', 'full_path', - 'parent_id', 'members_count_with_descendants') + expect(owned_group_response.keys).to include('id', 'kind', 'name', 'path', 'full_path', + 'parent_id', 'members_count_with_descendants') end it "returns correct attributes when user cannot admin group" do @@ -69,7 +69,7 @@ describe API::Namespaces do guest_group_response = json_response.find { |resource| resource['id'] == group1.id } - expect(guest_group_response.keys).to contain_exactly('id', 'kind', 'name', 'path', 'full_path', 'parent_id') + expect(guest_group_response.keys).to include('id', 'kind', 'name', 'path', 'full_path', 'parent_id') end it "user: returns an array of namespaces" do From b40c468b35ecdbef8be490ec0933afb253fad0c6 Mon Sep 17 00:00:00 2001 From: Jamie Schembri Date: Fri, 6 Jul 2018 16:14:31 +0200 Subject: [PATCH 002/113] Fix #48934 - Focus on text input on danger confirmation --- app/assets/javascripts/confirm_danger_modal.js | 5 ++++- changelogs/unreleased/48934.yml | 5 +++++ spec/features/groups_spec.rb | 6 ++++++ .../projects/settings/user_transfers_a_project_spec.rb | 9 ++++++++- spec/features/projects_spec.rb | 6 ++++++ 5 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/48934.yml diff --git a/app/assets/javascripts/confirm_danger_modal.js b/app/assets/javascripts/confirm_danger_modal.js index 1638e09132b..b0c85c2572e 100644 --- a/app/assets/javascripts/confirm_danger_modal.js +++ b/app/assets/javascripts/confirm_danger_modal.js @@ -2,13 +2,16 @@ import $ from 'jquery'; import { rstrip } from './lib/utils/common_utils'; function openConfirmDangerModal($form, text) { + const $input = $('.js-confirm-danger-input'); + $input.val(''); + $('.js-confirm-text').text(text || ''); - $('.js-confirm-danger-input').val(''); $('#modal-confirm-danger').modal('show'); const confirmTextMatch = $('.js-confirm-danger-match').text(); const $submit = $('.js-confirm-danger-submit'); $submit.disable(); + $input.focus(); $('.js-confirm-danger-input').off('input').on('input', function handleInput() { const confirmText = rstrip($(this).val()); diff --git a/changelogs/unreleased/48934.yml b/changelogs/unreleased/48934.yml new file mode 100644 index 00000000000..8e2e53ed198 --- /dev/null +++ b/changelogs/unreleased/48934.yml @@ -0,0 +1,5 @@ +--- +title: Improve danger confirmation modals by focusing input field +merge_request: +author: Jamie Schembri +type: added diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb index 053e3b189c3..e62bd6f8187 100644 --- a/spec/features/groups_spec.rb +++ b/spec/features/groups_spec.rb @@ -154,6 +154,12 @@ describe 'Group' do end end + it 'focuses confirmation field on remove group' do + click_button('Remove group') + + expect(page).to have_selector '#confirm_name_input:focus' + end + it 'removes group' do expect { remove_with_confirm('Remove group', group.path) }.to change {Group.count}.by(-1) expect(group.members.all.count).to be_zero diff --git a/spec/features/projects/settings/user_transfers_a_project_spec.rb b/spec/features/projects/settings/user_transfers_a_project_spec.rb index 96b7cf1f93b..2fdbc04fa62 100644 --- a/spec/features/projects/settings/user_transfers_a_project_spec.rb +++ b/spec/features/projects/settings/user_transfers_a_project_spec.rb @@ -10,7 +10,7 @@ describe 'Projects > Settings > User transfers a project', :js do sign_in(user) end - def transfer_project(project, group) + def transfer_project(project, group, confirm: true) visit edit_project_path(project) page.within('.js-project-transfer-form') do @@ -21,6 +21,8 @@ describe 'Projects > Settings > User transfers a project', :js do click_button('Transfer project') + return unless confirm + fill_in 'confirm_name_input', with: project.name click_button 'Confirm' @@ -28,6 +30,11 @@ describe 'Projects > Settings > User transfers a project', :js do wait_for_requests end + it 'focuses on the confirmation field' do + transfer_project(project, group, confirm: false) + expect(page).to have_selector '#confirm_name_input:focus' + end + it 'allows transferring a project to a group' do old_path = project_path(project) transfer_project(project, group) diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index 8636d17f2c4..81be28cf0e3 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -155,6 +155,12 @@ describe 'Project' do visit edit_project_path(project) end + it 'focuses on the confirmation field' do + click_button 'Remove project' + + expect(page).to have_selector '#confirm_name_input:focus' + end + it 'removes a project' do expect { remove_with_confirm('Remove project', project.path) }.to change { Project.count }.by(-1) expect(page).to have_content "Project '#{project.full_name}' is in the process of being deleted." From a4e75e7a83082c190474595ed9894ec86061d37f Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 9 Jul 2018 12:50:17 +0200 Subject: [PATCH 003/113] Use Gitaly for fetches and creating bundles --- lib/gitlab/git/repository.rb | 16 +--- lib/gitlab/shell.rb | 44 +--------- spec/lib/gitlab/git/repository_spec.rb | 70 +++++++--------- spec/lib/gitlab/shell_spec.rb | 111 ++++++------------------- spec/lib/gitlab/workhorse_spec.rb | 24 +++--- 5 files changed, 76 insertions(+), 189 deletions(-) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 420790f45d0..00c136cab41 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -898,12 +898,8 @@ module Gitlab end def fetch_source_branch!(source_repository, source_branch, local_ref) - Gitlab::GitalyClient.migrate(:fetch_source_branch) do |is_enabled| - if is_enabled - gitaly_repository_client.fetch_source_branch(source_repository, source_branch, local_ref) - else - rugged_fetch_source_branch(source_repository, source_branch, local_ref) - end + wrapped_gitaly_errors do + gitaly_repository_client.fetch_source_branch(source_repository, source_branch, local_ref) end end @@ -1058,12 +1054,8 @@ module Gitlab end def bundle_to_disk(save_path) - gitaly_migrate(:bundle_to_disk) do |is_enabled| - if is_enabled - gitaly_repository_client.create_bundle(save_path) - else - run_git!(%W(bundle create #{save_path} --all)) - end + wrapped_gitaly_errors do + gitaly_repository_client.create_bundle(save_path) end true diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index e222541992a..a17cd27e82d 100644 --- a/lib/gitlab/shell.rb +++ b/lib/gitlab/shell.rb @@ -92,21 +92,13 @@ module Gitlab # Ex. # import_repository("nfs-file06", "gitlab/gitlab-ci", "https://gitlab.com/gitlab-org/gitlab-test.git") # - # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/874 def import_repository(storage, name, url) if url.start_with?('.', '/') raise Error.new("don't use disk paths with import_repository: #{url.inspect}") end relative_path = "#{name}.git" - cmd = gitaly_migrate(:import_repository, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled| - if is_enabled - GitalyGitlabProjects.new(storage, relative_path) - else - # The timeout ensures the subprocess won't hang forever - gitlab_projects(storage, relative_path) - end - end + cmd = GitalyGitlabProjects.new(storage, relative_path) success = cmd.import_project(url, git_timeout) raise Error, cmd.output unless success @@ -126,12 +118,8 @@ module Gitlab # fetch_remote(my_repo, "upstream") # def fetch_remote(repository, remote, ssh_auth: nil, forced: false, no_tags: false, prune: true) - gitaly_migrate(:fetch_remote) do |is_enabled| - if is_enabled - repository.gitaly_repository_client.fetch_remote(remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, timeout: git_timeout, prune: prune) - else - local_fetch_remote(repository.storage, repository.relative_path, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, prune: prune) - end + wrapped_gitaly_errors do + repository.gitaly_repository_client.fetch_remote(remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags, timeout: git_timeout, prune: prune) end end @@ -389,28 +377,6 @@ module Gitlab ) end - def local_fetch_remote(storage_name, repository_relative_path, remote, ssh_auth: nil, forced: false, no_tags: false, prune: true) - vars = { force: forced, tags: !no_tags, prune: prune } - - if ssh_auth&.ssh_import? - if ssh_auth.ssh_key_auth? && ssh_auth.ssh_private_key.present? - vars[:ssh_key] = ssh_auth.ssh_private_key - end - - if ssh_auth.ssh_known_hosts.present? - vars[:known_hosts] = ssh_auth.ssh_known_hosts - end - end - - cmd = gitlab_projects(storage_name, repository_relative_path) - - success = cmd.fetch_remote(remote, git_timeout, vars) - - raise Error, cmd.output unless success - - success - end - def gitlab_shell_fast_execute(cmd) output, status = gitlab_shell_fast_execute_helper(cmd) @@ -440,10 +406,6 @@ module Gitlab Gitlab.config.gitlab_shell.git_timeout end - def gitaly_migrate(method, status: Gitlab::GitalyClient::MigrationStatus::OPT_IN, &block) - wrapped_gitaly_errors { Gitlab::GitalyClient.migrate(method, status: status, &block) } - end - def wrapped_gitaly_errors yield rescue GRPC::NotFound, GRPC::BadStatus => e diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index e6268a05d44..4f8e6f29255 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1716,59 +1716,51 @@ describe Gitlab::Git::Repository, seed_helper: true do end describe '#fetch_source_branch!' do - shared_examples '#fetch_source_branch!' do - let(:local_ref) { 'refs/merge-requests/1/head' } - let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } - let(:source_repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') } + let(:local_ref) { 'refs/merge-requests/1/head' } + let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') } + let(:source_repository) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') } - after do - ensure_seeds - end + after do + ensure_seeds + end - context 'when the branch exists' do - context 'when the commit does not exist locally' do - let(:source_branch) { 'new-branch-for-fetch-source-branch' } - let(:source_rugged) { Gitlab::GitalyClient::StorageSettings.allow_disk_access { source_repository.rugged } } - let(:new_oid) { new_commit_edit_old_file(source_rugged).oid } + context 'when the branch exists' do + context 'when the commit does not exist locally' do + let(:source_branch) { 'new-branch-for-fetch-source-branch' } + let(:source_rugged) { Gitlab::GitalyClient::StorageSettings.allow_disk_access { source_repository.rugged } } + let(:new_oid) { new_commit_edit_old_file(source_rugged).oid } - before do - source_rugged.branches.create(source_branch, new_oid) - end - - it 'writes the ref' do - expect(repository.fetch_source_branch!(source_repository, source_branch, local_ref)).to eq(true) - expect(repository.commit(local_ref).sha).to eq(new_oid) - end + before do + source_rugged.branches.create(source_branch, new_oid) end - context 'when the commit exists locally' do - let(:source_branch) { 'master' } - let(:expected_oid) { SeedRepo::LastCommit::ID } - - it 'writes the ref' do - # Sanity check: the commit should already exist - expect(repository.commit(expected_oid)).not_to be_nil - - expect(repository.fetch_source_branch!(source_repository, source_branch, local_ref)).to eq(true) - expect(repository.commit(local_ref).sha).to eq(expected_oid) - end + it 'writes the ref' do + expect(repository.fetch_source_branch!(source_repository, source_branch, local_ref)).to eq(true) + expect(repository.commit(local_ref).sha).to eq(new_oid) end end - context 'when the branch does not exist' do - let(:source_branch) { 'definitely-not-master' } + context 'when the commit exists locally' do + let(:source_branch) { 'master' } + let(:expected_oid) { SeedRepo::LastCommit::ID } - it 'does not write the ref' do - expect(repository.fetch_source_branch!(source_repository, source_branch, local_ref)).to eq(false) - expect(repository.commit(local_ref)).to be_nil + it 'writes the ref' do + # Sanity check: the commit should already exist + expect(repository.commit(expected_oid)).not_to be_nil + + expect(repository.fetch_source_branch!(source_repository, source_branch, local_ref)).to eq(true) + expect(repository.commit(local_ref).sha).to eq(expected_oid) end end end - it_behaves_like '#fetch_source_branch!' + context 'when the branch does not exist' do + let(:source_branch) { 'definitely-not-master' } - context 'without gitaly', :skip_gitaly_mock do - it_behaves_like '#fetch_source_branch!' + it 'does not write the ref' do + expect(repository.fetch_source_branch!(source_repository, source_branch, local_ref)).to eq(false) + expect(repository.commit(local_ref)).to be_nil + end end end diff --git a/spec/lib/gitlab/shell_spec.rb b/spec/lib/gitlab/shell_spec.rb index c435f988cdd..f8bf896950e 100644 --- a/spec/lib/gitlab/shell_spec.rb +++ b/spec/lib/gitlab/shell_spec.rb @@ -403,46 +403,36 @@ describe Gitlab::Shell do end describe '#create_repository' do - shared_examples '#create_repository' do - let(:repository_storage) { 'default' } - let(:repository_storage_path) do - Gitlab::GitalyClient::StorageSettings.allow_disk_access do - Gitlab.config.repositories.storages[repository_storage].legacy_disk_path - end - end - let(:repo_name) { 'project/path' } - let(:created_path) { File.join(repository_storage_path, repo_name + '.git') } - - after do - FileUtils.rm_rf(created_path) - end - - it 'creates a repository' do - expect(gitlab_shell.create_repository(repository_storage, repo_name)).to be_truthy - - expect(File.stat(created_path).mode & 0o777).to eq(0o770) - - hooks_path = File.join(created_path, 'hooks') - expect(File.lstat(hooks_path)).to be_symlink - expect(File.realpath(hooks_path)).to eq(gitlab_shell_hooks_path) - end - - it 'returns false when the command fails' do - FileUtils.mkdir_p(File.dirname(created_path)) - # This file will block the creation of the repo's .git directory. That - # should cause #create_repository to fail. - FileUtils.touch(created_path) - - expect(gitlab_shell.create_repository(repository_storage, repo_name)).to be_falsy + let(:repository_storage) { 'default' } + let(:repository_storage_path) do + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + Gitlab.config.repositories.storages[repository_storage].legacy_disk_path end end + let(:repo_name) { 'project/path' } + let(:created_path) { File.join(repository_storage_path, repo_name + '.git') } - context 'with gitaly' do - it_behaves_like '#create_repository' + after do + FileUtils.rm_rf(created_path) end - context 'without gitaly', :skip_gitaly_mock do - it_behaves_like '#create_repository' + it 'creates a repository' do + expect(gitlab_shell.create_repository(repository_storage, repo_name)).to be_truthy + + expect(File.stat(created_path).mode & 0o777).to eq(0o770) + + hooks_path = File.join(created_path, 'hooks') + expect(File.lstat(hooks_path)).to be_symlink + expect(File.realpath(hooks_path)).to eq(gitlab_shell_hooks_path) + end + + it 'returns false when the command fails' do + FileUtils.mkdir_p(File.dirname(created_path)) + # This file will block the creation of the repo's .git directory. That + # should cause #create_repository to fail. + FileUtils.touch(created_path) + + expect(gitlab_shell.create_repository(repository_storage, repo_name)).to be_falsy end end @@ -513,22 +503,12 @@ describe Gitlab::Shell do end end - shared_examples 'fetch_remote' do |gitaly_on| + describe '#fetch_remote' do def fetch_remote(ssh_auth = nil, prune = true) gitlab_shell.fetch_remote(repository.raw_repository, 'remote-name', ssh_auth: ssh_auth, prune: prune) end - def expect_gitlab_projects(fail = false, options = {}) - expect(gitlab_projects).to receive(:fetch_remote).with( - 'remote-name', - timeout, - options - ).and_return(!fail) - - allow(gitlab_projects).to receive(:output).and_return('error') if fail - end - - def expect_gitaly_call(fail, options = {}) + def expect_call(fail, options = {}) receive_fetch_remote = if fail receive(:fetch_remote).and_raise(GRPC::NotFound) @@ -539,16 +519,6 @@ describe Gitlab::Shell do expect_any_instance_of(Gitlab::GitalyClient::RepositoryService).to receive_fetch_remote end - if gitaly_on - def expect_call(fail, options = {}) - expect_gitaly_call(fail, options) - end - else - def expect_call(fail, options = {}) - expect_gitlab_projects(fail, options) - end - end - def build_ssh_auth(opts = {}) defaults = { ssh_import?: true, @@ -634,14 +604,6 @@ describe Gitlab::Shell do expect(fetch_remote(ssh_auth)).to be_truthy end end - end - - describe '#fetch_remote local', :skip_gitaly_mock do - it_should_behave_like 'fetch_remote', false - end - - describe '#fetch_remote gitaly' do - it_should_behave_like 'fetch_remote', true context 'gitaly call' do let(:remote_name) { 'remote-name' } @@ -683,25 +645,6 @@ describe Gitlab::Shell do end.to raise_error(Gitlab::Shell::Error, "error") end end - - context 'without gitaly', :disable_gitaly do - it 'returns true when the command succeeds' do - expect(gitlab_projects).to receive(:import_project).with(import_url, timeout) { true } - - result = gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url) - - expect(result).to be_truthy - end - - it 'raises an exception when the command fails' do - allow(gitlab_projects).to receive(:output) { 'error' } - expect(gitlab_projects).to receive(:import_project) { false } - - expect do - gitlab_shell.import_repository(project.repository_storage, project.disk_path, import_url) - end.to raise_error(Gitlab::Shell::Error, "error") - end - end end end diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb index 8fdcfe79fb5..b9e8c6c663a 100644 --- a/spec/lib/gitlab/workhorse_spec.rb +++ b/spec/lib/gitlab/workhorse_spec.rb @@ -36,22 +36,20 @@ describe Gitlab::Workhorse do allow(described_class).to receive(:git_archive_cache_disabled?).and_return(cache_disabled) end - context 'when Gitaly workhorse_archive feature is enabled' do - it 'sets the header correctly' do - key, command, params = decode_workhorse_header(subject) + it 'sets the header correctly' do + key, command, params = decode_workhorse_header(subject) - expect(key).to eq('Gitlab-Workhorse-Send-Data') - expect(command).to eq('git-archive') - expect(params).to include(gitaly_params) - end + expect(key).to eq('Gitlab-Workhorse-Send-Data') + expect(command).to eq('git-archive') + expect(params).to include(gitaly_params) + end - context 'when archive caching is disabled' do - let(:cache_disabled) { true } + context 'when archive caching is disabled' do + let(:cache_disabled) { true } - it 'tells workhorse not to use the cache' do - _, _, params = decode_workhorse_header(subject) - expect(params).to include({ 'DisableCache' => true }) - end + it 'tells workhorse not to use the cache' do + _, _, params = decode_workhorse_header(subject) + expect(params).to include({ 'DisableCache' => true }) end end From bc00803af03147452c12e9e2c7e8f0c0cba86f73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 9 Jul 2018 13:34:18 +0200 Subject: [PATCH 004/113] Access metadata directly from Object Storage Previously we would pull the file, now, we just stream-it as needed from Object Storage --- app/models/ci/build.rb | 4 +- app/uploaders/gitlab_uploader.rb | 17 ++ app/uploaders/job_artifact_uploader.rb | 8 - .../improve-metadata-access-performance.yml | 5 + lib/gitlab/ci/build/artifacts/metadata.rb | 19 +- lib/gitlab/ci/trace/http_io.rb | 197 ------------------ lib/gitlab/http_io.rb | 193 +++++++++++++++++ .../projects/jobs_controller_spec.rb | 2 +- .../ci/build/artifacts/metadata_spec.rb | 24 ++- .../lib/gitlab/{ci/trace => }/http_io_spec.rb | 2 +- spec/support/http_io/http_io_helpers.rb | 4 +- spec/uploaders/job_artifact_uploader_spec.rb | 2 +- 12 files changed, 258 insertions(+), 219 deletions(-) create mode 100644 changelogs/unreleased/improve-metadata-access-performance.yml delete mode 100644 lib/gitlab/ci/trace/http_io.rb create mode 100644 lib/gitlab/http_io.rb rename spec/lib/gitlab/{ci/trace => }/http_io_spec.rb (99%) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 19949f83351..acf555a5369 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -437,9 +437,9 @@ module Ci end def artifacts_metadata_entry(path, **options) - artifacts_metadata.use_file do |metadata_path| + artifacts_metadata.open do |metadata_stream| metadata = Gitlab::Ci::Build::Artifacts::Metadata.new( - metadata_path, + metadata_stream, path, **options) diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index 7919f126075..7636acf255c 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -71,6 +71,23 @@ class GitlabUploader < CarrierWave::Uploader::Base File.join('/', self.class.base_dir, dynamic_segment, filename) end + def open + stream = if file_storage? + File.open(path, "rb") if path + else + ::Gitlab::HttpIO.new(url, cached_size) if url + end + + return unless stream + return stream unless block_given? + + begin + yield(stream) + ensure + stream.close + end + end + private # Designed to be overridden by child uploaders that have a dynamic path diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb index 855cf2fc21c..f6af023e0f9 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -18,14 +18,6 @@ class JobArtifactUploader < GitlabUploader dynamic_segment end - def open - if file_storage? - File.open(path, "rb") if path - else - ::Gitlab::Ci::Trace::HttpIO.new(url, cached_size) if url - end - end - private def dynamic_segment diff --git a/changelogs/unreleased/improve-metadata-access-performance.yml b/changelogs/unreleased/improve-metadata-access-performance.yml new file mode 100644 index 00000000000..b16fa99dd3b --- /dev/null +++ b/changelogs/unreleased/improve-metadata-access-performance.yml @@ -0,0 +1,5 @@ +--- +title: Access metadata directly from Object Storage +merge_request: +author: +type: performance diff --git a/lib/gitlab/ci/build/artifacts/metadata.rb b/lib/gitlab/ci/build/artifacts/metadata.rb index 0bbd60d8ffe..375d8bc1ff5 100644 --- a/lib/gitlab/ci/build/artifacts/metadata.rb +++ b/lib/gitlab/ci/build/artifacts/metadata.rb @@ -7,14 +7,15 @@ module Gitlab module Artifacts class Metadata ParserError = Class.new(StandardError) + InvalidStreamError = Class.new(StandardError) VERSION_PATTERN = /^[\w\s]+(\d+\.\d+\.\d+)/ INVALID_PATH_PATTERN = %r{(^\.?\.?/)|(/\.?\.?/)} - attr_reader :file, :path, :full_version + attr_reader :stream, :path, :full_version - def initialize(file, path, **opts) - @file, @path, @opts = file, path, opts + def initialize(stream, path, **opts) + @stream, @path, @opts = stream, path, opts @full_version = read_version end @@ -103,7 +104,17 @@ module Gitlab end def gzip(&block) - Zlib::GzipReader.open(@file, &block) + raise InvalidStreamError, "Invalid stream" unless @stream + + # restart gzip reading + @stream.seek(0) + + gz = Zlib::GzipReader.new(@stream) + yield(gz) + rescue Zlib::Error => e + raise InvalidStreamError, e.message + ensure + gz&.finish end end end diff --git a/lib/gitlab/ci/trace/http_io.rb b/lib/gitlab/ci/trace/http_io.rb deleted file mode 100644 index 8788af57a67..00000000000 --- a/lib/gitlab/ci/trace/http_io.rb +++ /dev/null @@ -1,197 +0,0 @@ -## -# This class is compatible with IO class (https://ruby-doc.org/core-2.3.1/IO.html) -# source: https://gitlab.com/snippets/1685610 -module Gitlab - module Ci - class Trace - class HttpIO - BUFFER_SIZE = 128.kilobytes - - InvalidURLError = Class.new(StandardError) - FailedToGetChunkError = Class.new(StandardError) - - attr_reader :uri, :size - attr_reader :tell - attr_reader :chunk, :chunk_range - - alias_method :pos, :tell - - def initialize(url, size) - raise InvalidURLError unless ::Gitlab::UrlSanitizer.valid?(url) - - @uri = URI(url) - @size = size - @tell = 0 - end - - def close - # no-op - end - - def binmode - # no-op - end - - def binmode? - true - end - - def path - nil - end - - def url - @uri.to_s - end - - def seek(pos, where = IO::SEEK_SET) - new_pos = - case where - when IO::SEEK_END - size + pos - when IO::SEEK_SET - pos - when IO::SEEK_CUR - tell + pos - else - -1 - end - - raise 'new position is outside of file' if new_pos < 0 || new_pos > size - - @tell = new_pos - end - - def eof? - tell == size - end - - def each_line - until eof? - line = readline - break if line.nil? - - yield(line) - end - end - - def read(length = nil, outbuf = "") - out = "" - - length ||= size - tell - - until length <= 0 || eof? - data = get_chunk - break if data.empty? - - chunk_bytes = [BUFFER_SIZE - chunk_offset, length].min - chunk_data = data.byteslice(0, chunk_bytes) - - out << chunk_data - @tell += chunk_data.bytesize - length -= chunk_data.bytesize - end - - # If outbuf is passed, we put the output into the buffer. This supports IO.copy_stream functionality - if outbuf - outbuf.slice!(0, outbuf.bytesize) - outbuf << out - end - - out - end - - def readline - out = "" - - until eof? - data = get_chunk - new_line = data.index("\n") - - if !new_line.nil? - out << data[0..new_line] - @tell += new_line + 1 - break - else - out << data - @tell += data.bytesize - end - end - - out - end - - def write(data) - raise NotImplementedError - end - - def truncate(offset) - raise NotImplementedError - end - - def flush - raise NotImplementedError - end - - def present? - true - end - - private - - ## - # The below methods are not implemented in IO class - # - def in_range? - @chunk_range&.include?(tell) - end - - def get_chunk - unless in_range? - response = Net::HTTP.start(uri.hostname, uri.port, proxy_from_env: true, use_ssl: uri.scheme == 'https') do |http| - http.request(request) - end - - raise FailedToGetChunkError unless response.code == '200' || response.code == '206' - - @chunk = response.body.force_encoding(Encoding::BINARY) - @chunk_range = response.content_range - - ## - # Note: If provider does not return content_range, then we set it as we requested - # Provider: minio - # - When the file size is larger than requested Content-range, the Content-range is included in responces with Net::HTTPPartialContent 206 - # - When the file size is smaller than requested Content-range, the Content-range is included in responces with Net::HTTPPartialContent 206 - # Provider: AWS - # - When the file size is larger than requested Content-range, the Content-range is included in responces with Net::HTTPPartialContent 206 - # - When the file size is smaller than requested Content-range, the Content-range is included in responces with Net::HTTPPartialContent 206 - # Provider: GCS - # - When the file size is larger than requested Content-range, the Content-range is included in responces with Net::HTTPPartialContent 206 - # - When the file size is smaller than requested Content-range, the Content-range is included in responces with Net::HTTPOK 200 - @chunk_range ||= (chunk_start...(chunk_start + @chunk.bytesize)) - end - - @chunk[chunk_offset..BUFFER_SIZE] - end - - def request - Net::HTTP::Get.new(uri).tap do |request| - request.set_range(chunk_start, BUFFER_SIZE) - end - end - - def chunk_offset - tell % BUFFER_SIZE - end - - def chunk_start - (tell / BUFFER_SIZE) * BUFFER_SIZE - end - - def chunk_end - [chunk_start + BUFFER_SIZE, size].min - end - end - end - end -end diff --git a/lib/gitlab/http_io.rb b/lib/gitlab/http_io.rb new file mode 100644 index 00000000000..ce24817db54 --- /dev/null +++ b/lib/gitlab/http_io.rb @@ -0,0 +1,193 @@ +## +# This class is compatible with IO class (https://ruby-doc.org/core-2.3.1/IO.html) +# source: https://gitlab.com/snippets/1685610 +module Gitlab + class HttpIO + BUFFER_SIZE = 128.kilobytes + + InvalidURLError = Class.new(StandardError) + FailedToGetChunkError = Class.new(StandardError) + + attr_reader :uri, :size + attr_reader :tell + attr_reader :chunk, :chunk_range + + alias_method :pos, :tell + + def initialize(url, size) + raise InvalidURLError unless ::Gitlab::UrlSanitizer.valid?(url) + + @uri = URI(url) + @size = size + @tell = 0 + end + + def close + # no-op + end + + def binmode + # no-op + end + + def binmode? + true + end + + def path + nil + end + + def url + @uri.to_s + end + + def seek(pos, where = IO::SEEK_SET) + new_pos = + case where + when IO::SEEK_END + size + pos + when IO::SEEK_SET + pos + when IO::SEEK_CUR + tell + pos + else + -1 + end + + raise 'new position is outside of file' if new_pos < 0 || new_pos > size + + @tell = new_pos + end + + def eof? + tell == size + end + + def each_line + until eof? + line = readline + break if line.nil? + + yield(line) + end + end + + def read(length = nil, outbuf = "") + out = "" + + length ||= size - tell + + until length <= 0 || eof? + data = get_chunk + break if data.empty? + + chunk_bytes = [BUFFER_SIZE - chunk_offset, length].min + chunk_data = data.byteslice(0, chunk_bytes) + + out << chunk_data + @tell += chunk_data.bytesize + length -= chunk_data.bytesize + end + + # If outbuf is passed, we put the output into the buffer. This supports IO.copy_stream functionality + if outbuf + outbuf.slice!(0, outbuf.bytesize) + outbuf << out + end + + out + end + + def readline + out = "" + + until eof? + data = get_chunk + new_line = data.index("\n") + + if !new_line.nil? + out << data[0..new_line] + @tell += new_line + 1 + break + else + out << data + @tell += data.bytesize + end + end + + out + end + + def write(data) + raise NotImplementedError + end + + def truncate(offset) + raise NotImplementedError + end + + def flush + raise NotImplementedError + end + + def present? + true + end + + private + + ## + # The below methods are not implemented in IO class + # + def in_range? + @chunk_range&.include?(tell) + end + + def get_chunk + unless in_range? + response = Net::HTTP.start(uri.hostname, uri.port, proxy_from_env: true, use_ssl: uri.scheme == 'https') do |http| + http.request(request) + end + + raise FailedToGetChunkError unless response.code == '200' || response.code == '206' + + @chunk = response.body.force_encoding(Encoding::BINARY) + @chunk_range = response.content_range + + ## + # Note: If provider does not return content_range, then we set it as we requested + # Provider: minio + # - When the file size is larger than requested Content-range, the Content-range is included in responces with Net::HTTPPartialContent 206 + # - When the file size is smaller than requested Content-range, the Content-range is included in responces with Net::HTTPPartialContent 206 + # Provider: AWS + # - When the file size is larger than requested Content-range, the Content-range is included in responces with Net::HTTPPartialContent 206 + # - When the file size is smaller than requested Content-range, the Content-range is included in responces with Net::HTTPPartialContent 206 + # Provider: GCS + # - When the file size is larger than requested Content-range, the Content-range is included in responces with Net::HTTPPartialContent 206 + # - When the file size is smaller than requested Content-range, the Content-range is included in responces with Net::HTTPOK 200 + @chunk_range ||= (chunk_start...(chunk_start + @chunk.bytesize)) + end + + @chunk[chunk_offset..BUFFER_SIZE] + end + + def request + Net::HTTP::Get.new(uri).tap do |request| + request.set_range(chunk_start, BUFFER_SIZE) + end + end + + def chunk_offset + tell % BUFFER_SIZE + end + + def chunk_start + (tell / BUFFER_SIZE) * BUFFER_SIZE + end + + def chunk_end + [chunk_start + BUFFER_SIZE, size].min + end + end +end diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index e6332a10265..5f53a2d087a 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -245,7 +245,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do end it 'returns a trace' do - expect { get_trace }.to raise_error(Gitlab::Ci::Trace::HttpIO::FailedToGetChunkError) + expect { get_trace }.to raise_error(Gitlab::HttpIO::FailedToGetChunkError) end end end diff --git a/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb b/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb index 6a52ae01b2f..e327399d82d 100644 --- a/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb +++ b/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb @@ -2,13 +2,21 @@ require 'spec_helper' describe Gitlab::Ci::Build::Artifacts::Metadata do def metadata(path = '', **opts) - described_class.new(metadata_file_path, path, **opts) + described_class.new(metadata_file_stream, path, **opts) end let(:metadata_file_path) do Rails.root + 'spec/fixtures/ci_build_artifacts_metadata.gz' end + let(:metadata_file_stream) do + File.open(metadata_file_path) if metadata_file_path + end + + after do + metadata_file_stream&.close + end + context 'metadata file exists' do describe '#find_entries! empty string' do subject { metadata('').find_entries! } @@ -86,11 +94,21 @@ describe Gitlab::Ci::Build::Artifacts::Metadata do end context 'metadata file does not exist' do - let(:metadata_file_path) { '' } + let(:metadata_file_path) { nil } describe '#find_entries!' do it 'raises error' do - expect { metadata.find_entries! }.to raise_error(Errno::ENOENT) + expect { metadata.find_entries! }.to raise_error(described_class::InvalidStreamError, /Invalid stream/) + end + end + end + + context 'metadata file is invalid' do + let(:metadata_file_path) { Rails.root + 'spec/fixtures/ci_build_artifacts.zip' } + + describe '#find_entries!' do + it 'raises error' do + expect { metadata.find_entries! }.to raise_error(described_class::InvalidStreamError, /not in gzip format/) end end end diff --git a/spec/lib/gitlab/ci/trace/http_io_spec.rb b/spec/lib/gitlab/http_io_spec.rb similarity index 99% rename from spec/lib/gitlab/ci/trace/http_io_spec.rb rename to spec/lib/gitlab/http_io_spec.rb index 5474e2f518c..2c9a73609a9 100644 --- a/spec/lib/gitlab/ci/trace/http_io_spec.rb +++ b/spec/lib/gitlab/http_io_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Ci::Trace::HttpIO do +describe Gitlab::HttpIO do include HttpIOHelpers let(:http_io) { described_class.new(url, size) } diff --git a/spec/support/http_io/http_io_helpers.rb b/spec/support/http_io/http_io_helpers.rb index 2c68c2cd9a6..86b254aa67d 100644 --- a/spec/support/http_io/http_io_helpers.rb +++ b/spec/support/http_io/http_io_helpers.rb @@ -54,12 +54,12 @@ module HttpIOHelpers def set_smaller_buffer_size_than(file_size) blocks = (file_size / 128) new_size = (blocks / 2) * 128 - stub_const("Gitlab::Ci::Trace::HttpIO::BUFFER_SIZE", new_size) + stub_const("Gitlab::HttpIO::BUFFER_SIZE", new_size) end def set_larger_buffer_size_than(file_size) blocks = (file_size / 128) new_size = (blocks * 2) * 128 - stub_const("Gitlab::Ci::Trace::HttpIO::BUFFER_SIZE", new_size) + stub_const("Gitlab::HttpIO::BUFFER_SIZE", new_size) end end diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb index 026e4356ed6..d0b14768867 100644 --- a/spec/uploaders/job_artifact_uploader_spec.rb +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -55,7 +55,7 @@ describe JobArtifactUploader do end it 'returns http io stream' do - is_expected.to be_a(Gitlab::Ci::Trace::HttpIO) + is_expected.to be_a(Gitlab::HttpIO) end end end From 5a9f23e780222218cc8418fd859669befcaed1b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 9 Jul 2018 14:18:31 +0200 Subject: [PATCH 005/113] Fix and add specs for testing metadata entry --- app/uploaders/gitlab_uploader.rb | 4 ++ .../projects/jobs_controller_spec.rb | 7 ++- spec/lib/gitlab/http_io_spec.rb | 41 ++++++------ spec/models/ci/build_spec.rb | 54 ++++++++++++++++ spec/requests/api/jobs_spec.rb | 8 ++- spec/support/http_io/http_io_helpers.rb | 56 ++++++----------- spec/uploaders/gitlab_uploader_spec.rb | 62 +++++++++++++++++++ spec/uploaders/job_artifact_uploader_spec.rb | 37 ----------- 8 files changed, 172 insertions(+), 97 deletions(-) diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index 7636acf255c..7aa81a8c312 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -71,6 +71,10 @@ class GitlabUploader < CarrierWave::Uploader::Base File.join('/', self.class.base_dir, dynamic_segment, filename) end + def cached_size + size + end + def open stream = if file_storage? File.open(path, "rb") if path diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index 5f53a2d087a..38431fb1598 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -225,8 +225,11 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do end context 'when there are no network issues' do + let(:url) { 'http://object-storage/trace' } + let(:file) { expand_fixture_path('trace/sample_trace') } + before do - stub_remote_trace_206 + stub_remote_url_206(url, file) get_trace end @@ -241,7 +244,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do context 'when there is a network issue' do before do - stub_remote_trace_500 + stub_remote_url_500(url) end it 'returns a trace' do diff --git a/spec/lib/gitlab/http_io_spec.rb b/spec/lib/gitlab/http_io_spec.rb index 2c9a73609a9..788bddb8f59 100644 --- a/spec/lib/gitlab/http_io_spec.rb +++ b/spec/lib/gitlab/http_io_spec.rb @@ -4,8 +4,11 @@ describe Gitlab::HttpIO do include HttpIOHelpers let(:http_io) { described_class.new(url, size) } - let(:url) { remote_trace_url } - let(:size) { remote_trace_size } + + let(:url) { 'http://object-storage/trace' } + let(:file_path) { expand_fixture_path('trace/sample_trace') } + let(:file_body) { File.read(file_path).force_encoding(Encoding::BINARY) } + let(:size) { File.size(file_path) } describe '#close' do subject { http_io.close } @@ -86,10 +89,10 @@ describe Gitlab::HttpIO do describe '#each_line' do subject { http_io.each_line } - let(:string_io) { StringIO.new(remote_trace_body) } + let(:string_io) { StringIO.new(file_body) } before do - stub_remote_trace_206 + stub_remote_url_206(url, file_path) end it 'yields lines' do @@ -99,7 +102,7 @@ describe Gitlab::HttpIO do context 'when buckets on GCS' do context 'when BUFFER_SIZE is larger than file size' do before do - stub_remote_trace_200 + stub_remote_url_200(url, file_path) set_larger_buffer_size_than(size) end @@ -117,7 +120,7 @@ describe Gitlab::HttpIO do context 'when there are no network issue' do before do - stub_remote_trace_206 + stub_remote_url_206(url, file_path) end context 'when read whole size' do @@ -129,7 +132,7 @@ describe Gitlab::HttpIO do end it 'reads a trace' do - is_expected.to eq(remote_trace_body) + is_expected.to eq(file_body) end end @@ -139,7 +142,7 @@ describe Gitlab::HttpIO do end it 'reads a trace' do - is_expected.to eq(remote_trace_body) + is_expected.to eq(file_body) end end end @@ -153,7 +156,7 @@ describe Gitlab::HttpIO do end it 'reads a trace' do - is_expected.to eq(remote_trace_body[0, length]) + is_expected.to eq(file_body[0, length]) end end @@ -163,7 +166,7 @@ describe Gitlab::HttpIO do end it 'reads a trace' do - is_expected.to eq(remote_trace_body[0, length]) + is_expected.to eq(file_body[0, length]) end end end @@ -177,7 +180,7 @@ describe Gitlab::HttpIO do end it 'reads a trace' do - is_expected.to eq(remote_trace_body) + is_expected.to eq(file_body) end end @@ -187,7 +190,7 @@ describe Gitlab::HttpIO do end it 'reads a trace' do - is_expected.to eq(remote_trace_body) + is_expected.to eq(file_body) end end end @@ -221,11 +224,11 @@ describe Gitlab::HttpIO do let(:length) { nil } before do - stub_remote_trace_500 + stub_remote_url_500(url) end it 'reads a trace' do - expect { subject }.to raise_error(Gitlab::Ci::Trace::HttpIO::FailedToGetChunkError) + expect { subject }.to raise_error(Gitlab::HttpIO::FailedToGetChunkError) end end end @@ -233,15 +236,15 @@ describe Gitlab::HttpIO do describe '#readline' do subject { http_io.readline } - let(:string_io) { StringIO.new(remote_trace_body) } + let(:string_io) { StringIO.new(file_body) } before do - stub_remote_trace_206 + stub_remote_url_206(url, file_path) end shared_examples 'all line matching' do it 'reads a line' do - (0...remote_trace_body.lines.count).each do + (0...file_body.lines.count).each do expect(http_io.readline).to eq(string_io.readline) end end @@ -251,11 +254,11 @@ describe Gitlab::HttpIO do let(:length) { nil } before do - stub_remote_trace_500 + stub_remote_url_500(url) end it 'reads a trace' do - expect { subject }.to raise_error(Gitlab::Ci::Trace::HttpIO::FailedToGetChunkError) + expect { subject }.to raise_error(Gitlab::HttpIO::FailedToGetChunkError) end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 090ca159e08..bf116c07495 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2687,4 +2687,58 @@ describe Ci::Build do end end end + + describe '#artifacts_metadata_entry' do + set(:build) { create(:ci_build, project: project) } + let(:path) { 'other_artifacts_0.1.2/another-subdirectory/banana_sample.gif' } + + before do + stub_artifacts_object_storage + end + + subject { build.artifacts_metadata_entry(path) } + + context 'when using local storage' do + let!(:metadata) { create(:ci_job_artifact, :metadata, job: build) } + + context 'for existing file' do + it 'does exist' do + is_expected.to be_exists + end + end + + context 'for non-existing file' do + let(:path) { 'invalid-file' } + + it 'does not exist' do + is_expected.not_to be_exists + end + end + end + + context 'when using remote storage' do + include HttpIOHelpers + + let!(:metadata) { create(:ci_job_artifact, :remote_store, :metadata, job: build) } + let(:file_path) { expand_fixture_path('ci_build_artifacts_metadata.gz') } + + before do + stub_remote_url_206(metadata.file.url, file_path) + end + + context 'for existing file' do + it 'does exist' do + is_expected.to be_exists + end + end + + context 'for non-existing file' do + let(:path) { 'invalid-file' } + + it 'does not exist' do + is_expected.not_to be_exists + end + end + end + end end diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index 50d6f4b4d99..8a2812d1576 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -535,12 +535,14 @@ describe API::Jobs do context 'authorized user' do context 'when trace is in ObjectStorage' do let!(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) } + let(:url) { 'http://object-storage/trace' } + let(:file) { expand_fixture_path('trace/sample_trace') } before do - stub_remote_trace_206 + stub_remote_url_206(url, file_path) allow_any_instance_of(JobArtifactUploader).to receive(:file_storage?) { false } - allow_any_instance_of(JobArtifactUploader).to receive(:url) { remote_trace_url } - allow_any_instance_of(JobArtifactUploader).to receive(:size) { remote_trace_size } + allow_any_instance_of(JobArtifactUploader).to receive(:url) { url } + allow_any_instance_of(JobArtifactUploader).to receive(:size) { File.size(file) } end it 'returns specific job trace' do diff --git a/spec/support/http_io/http_io_helpers.rb b/spec/support/http_io/http_io_helpers.rb index 86b254aa67d..42144870eb5 100644 --- a/spec/support/http_io/http_io_helpers.rb +++ b/spec/support/http_io/http_io_helpers.rb @@ -1,54 +1,38 @@ module HttpIOHelpers - def stub_remote_trace_206 - WebMock.stub_request(:get, remote_trace_url) - .to_return { |request| remote_trace_response(request, 206) } + def stub_remote_url_206(url, file_path) + WebMock.stub_request(:get, url) + .to_return { |request| remote_url_response(file_path, request, 206) } end - def stub_remote_trace_200 - WebMock.stub_request(:get, remote_trace_url) - .to_return { |request| remote_trace_response(request, 200) } + def stub_remote_url_200(url, file_path) + WebMock.stub_request(:get, url) + .to_return { |request| remote_url_response(file_path, request, 200) } end - def stub_remote_trace_500 - WebMock.stub_request(:get, remote_trace_url) + def stub_remote_url_500(url) + WebMock.stub_request(:get, url) .to_return(status: [500, "Internal Server Error"]) end - def remote_trace_url - "http://trace.com/trace" - end - - def remote_trace_response(request, responce_status) + def remote_url_response(file_path, request, response_status) range = request.headers['Range'].match(/bytes=(\d+)-(\d+)/) + body = File.read(file_path).force_encoding(Encoding::BINARY) + size = body.bytesize + { - status: responce_status, - headers: remote_trace_response_headers(responce_status, range[1].to_i, range[2].to_i), - body: range_trace_body(range[1].to_i, range[2].to_i) + status: response_status, + headers: remote_url_response_headers(response_status, range[1].to_i, range[2].to_i, size), + body: body[range[1].to_i..range[2].to_i] } end - def remote_trace_response_headers(responce_status, from, to) - headers = { 'Content-Type' => 'text/plain' } - - if responce_status == 206 - headers.merge('Content-Range' => "bytes #{from}-#{to}/#{remote_trace_size}") + def remote_url_response_headers(response_status, from, to, size) + { 'Content-Type' => 'text/plain' }.tap do |headers| + if response_status == 206 + headers.merge('Content-Range' => "bytes #{from}-#{to}/#{size}") + end end - - headers - end - - def range_trace_body(from, to) - remote_trace_body[from..to] - end - - def remote_trace_body - @remote_trace_body ||= File.read(expand_fixture_path('trace/sample_trace')) - .force_encoding(Encoding::BINARY) - end - - def remote_trace_size - remote_trace_body.bytesize end def set_smaller_buffer_size_than(file_size) diff --git a/spec/uploaders/gitlab_uploader_spec.rb b/spec/uploaders/gitlab_uploader_spec.rb index 362f89424d4..44718ed1212 100644 --- a/spec/uploaders/gitlab_uploader_spec.rb +++ b/spec/uploaders/gitlab_uploader_spec.rb @@ -68,4 +68,66 @@ describe GitlabUploader do expect(subject.file.path).to match(/#{subject.cache_dir}/) end end + + describe '#open' do + context 'when trace is stored in File storage' do + context 'when file exists' do + let(:file) do + fixture_file_upload('spec/fixtures/trace/sample_trace', 'text/plain') + end + + before do + subject.store!(file) + end + + it 'returns io stream' do + expect(subject.open).to be_a(IO) + end + + it 'when passing block it yields' do + expect { |b| subject.open(&b) }.to yield_control + end + end + + context 'when file does not exist' do + it 'returns nil' do + expect(subject.open).to be_nil + end + + it 'when passing block it does not yield' do + expect { |b| subject.open(&b) }.not_to yield_control + end + end + end + + context 'when trace is stored in Object storage' do + before do + allow(subject).to receive(:file_storage?) { false } + end + + context 'when file exists' do + before do + allow(subject).to receive(:url) { 'http://object_storage.com/trace' } + end + + it 'returns http io stream' do + expect(subject.open).to be_a(Gitlab::HttpIO) + end + + it 'when passing block it yields' do + expect { |b| subject.open(&b) }.to yield_control.once + end + end + + context 'when file does not exist' do + it 'returns nil' do + expect(subject.open).to be_nil + end + + it 'when passing block it does not yield' do + expect { |b| subject.open(&b) }.not_to yield_control + end + end + end + end end diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb index d0b14768867..3ad5fe7e3b3 100644 --- a/spec/uploaders/job_artifact_uploader_spec.rb +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -23,43 +23,6 @@ describe JobArtifactUploader do store_dir: %r[\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z] end - describe '#open' do - subject { uploader.open } - - context 'when trace is stored in File storage' do - context 'when file exists' do - let(:file) do - fixture_file_upload('spec/fixtures/trace/sample_trace', 'text/plain') - end - - before do - uploader.store!(file) - end - - it 'returns io stream' do - is_expected.to be_a(IO) - end - end - - context 'when file does not exist' do - it 'returns nil' do - is_expected.to be_nil - end - end - end - - context 'when trace is stored in Object storage' do - before do - allow(uploader).to receive(:file_storage?) { false } - allow(uploader).to receive(:url) { 'http://object_storage.com/trace' } - end - - it 'returns http io stream' do - is_expected.to be_a(Gitlab::HttpIO) - end - end - end - context 'file is stored in valid local_path' do let(:file) do fixture_file_upload('spec/fixtures/ci_build_artifacts.zip', 'application/zip') From 8186bb39031b189396b57def9de202ddf65cbcbb Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 9 Jul 2018 15:40:48 +0100 Subject: [PATCH 006/113] Removes unused store in diffs mr refactor Removes double export for actions in diffs module in mr refactor --- app/assets/javascripts/diffs/store/actions.js | 13 +-- app/assets/javascripts/diffs/store/index.js | 11 --- .../javascripts/diffs/store/modules/index.js | 2 +- changelogs/unreleased/48951-clean-up.yml | 5 + .../diffs/components/changed_files_spec.js | 97 ++++++++++--------- 5 files changed, 59 insertions(+), 69 deletions(-) delete mode 100644 app/assets/javascripts/diffs/store/index.js create mode 100644 changelogs/unreleased/48951-clean-up.yml diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 5e0fd5109bb..5bfe42618c2 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -82,14 +82,5 @@ export const expandAllFiles = ({ commit }) => { commit(types.EXPAND_ALL_FILES); }; -export default { - setBaseConfig, - fetchDiffFiles, - setInlineDiffViewType, - setParallelDiffViewType, - showCommentForm, - cancelCommentForm, - loadMoreLines, - loadCollapsedDiff, - expandAllFiles, -}; +// prevent babel-plugin-rewire from generating an invalid default during karma tests +export default () => {}; diff --git a/app/assets/javascripts/diffs/store/index.js b/app/assets/javascripts/diffs/store/index.js deleted file mode 100644 index e6aa8f5b12a..00000000000 --- a/app/assets/javascripts/diffs/store/index.js +++ /dev/null @@ -1,11 +0,0 @@ -import Vue from 'vue'; -import Vuex from 'vuex'; -import diffsModule from './modules'; - -Vue.use(Vuex); - -export default new Vuex.Store({ - modules: { - diffs: diffsModule, - }, -}); diff --git a/app/assets/javascripts/diffs/store/modules/index.js b/app/assets/javascripts/diffs/store/modules/index.js index c745320d532..56c26d1ffad 100644 --- a/app/assets/javascripts/diffs/store/modules/index.js +++ b/app/assets/javascripts/diffs/store/modules/index.js @@ -1,4 +1,4 @@ -import actions from '../actions'; +import * as actions from '../actions'; import * as getters from '../getters'; import mutations from '../mutations'; import createState from './diff_state'; diff --git a/changelogs/unreleased/48951-clean-up.yml b/changelogs/unreleased/48951-clean-up.yml new file mode 100644 index 00000000000..0102cd43f96 --- /dev/null +++ b/changelogs/unreleased/48951-clean-up.yml @@ -0,0 +1,5 @@ +--- +title: Removes unused vuex code in mr refactor and removes unneeded dependencies +merge_request: 20499 +author: +type: other diff --git a/spec/javascripts/diffs/components/changed_files_spec.js b/spec/javascripts/diffs/components/changed_files_spec.js index 2d57af6137c..f737e8fa38e 100644 --- a/spec/javascripts/diffs/components/changed_files_spec.js +++ b/spec/javascripts/diffs/components/changed_files_spec.js @@ -1,12 +1,17 @@ import Vue from 'vue'; -import $ from 'jquery'; +import Vuex from 'vuex'; import { mountComponentWithStore } from 'spec/helpers'; -import store from '~/diffs/store'; -import ChangedFiles from '~/diffs/components/changed_files.vue'; +import diffsModule from '~/diffs/store/modules'; +import changedFiles from '~/diffs/components/changed_files.vue'; describe('ChangedFiles', () => { - const Component = Vue.extend(ChangedFiles); - const createComponent = props => mountComponentWithStore(Component, { props, store }); + const Component = Vue.extend(changedFiles); + const store = new Vuex.Store({ + modules: { + diffs: diffsModule, + }, + }); + let vm; beforeEach(() => { @@ -14,6 +19,7 @@ describe('ChangedFiles', () => {
`); + const props = { diffFiles: [ { @@ -26,7 +32,8 @@ describe('ChangedFiles', () => { }, ], }; - vm = createComponent(props); + + vm = mountComponentWithStore(Component, { props, store }); }); describe('with single file added', () => { @@ -40,58 +47,56 @@ describe('ChangedFiles', () => { }); }); - describe('template', () => { - describe('diff view mode buttons', () => { - let inlineButton; - let parallelButton; + describe('diff view mode buttons', () => { + let inlineButton; + let parallelButton; - beforeEach(() => { - inlineButton = vm.$el.querySelector('.js-inline-diff-button'); - parallelButton = vm.$el.querySelector('.js-parallel-diff-button'); + beforeEach(() => { + inlineButton = vm.$el.querySelector('.js-inline-diff-button'); + parallelButton = vm.$el.querySelector('.js-parallel-diff-button'); + }); + + it('should have Inline and Side-by-side buttons', () => { + expect(inlineButton).toBeDefined(); + expect(parallelButton).toBeDefined(); + }); + + it('should add active class to Inline button', done => { + vm.$store.state.diffs.diffViewType = 'inline'; + + vm.$nextTick(() => { + expect(inlineButton.classList.contains('active')).toEqual(true); + expect(parallelButton.classList.contains('active')).toEqual(false); + + done(); }); + }); - it('should have Inline and Side-by-side buttons', () => { - expect(inlineButton).toBeDefined(); - expect(parallelButton).toBeDefined(); + it('should toggle active state of buttons when diff view type changed', done => { + vm.$store.state.diffs.diffViewType = 'parallel'; + + vm.$nextTick(() => { + expect(inlineButton.classList.contains('active')).toEqual(false); + expect(parallelButton.classList.contains('active')).toEqual(true); + + done(); }); + }); - it('should add active class to Inline button', done => { - vm.$store.state.diffs.diffViewType = 'inline'; - - vm.$nextTick(() => { - expect(inlineButton.classList.contains('active')).toEqual(true); - expect(parallelButton.classList.contains('active')).toEqual(false); - - done(); - }); - }); - - it('should toggle active state of buttons when diff view type changed', done => { - vm.$store.state.diffs.diffViewType = 'parallel'; + describe('clicking them', () => { + it('should toggle the diff view type', done => { + parallelButton.click(); vm.$nextTick(() => { expect(inlineButton.classList.contains('active')).toEqual(false); expect(parallelButton.classList.contains('active')).toEqual(true); - done(); - }); - }); - - describe('clicking them', () => { - it('should toggle the diff view type', done => { - $(parallelButton).click(); + inlineButton.click(); vm.$nextTick(() => { - expect(inlineButton.classList.contains('active')).toEqual(false); - expect(parallelButton.classList.contains('active')).toEqual(true); - - $(inlineButton).click(); - - vm.$nextTick(() => { - expect(inlineButton.classList.contains('active')).toEqual(true); - expect(parallelButton.classList.contains('active')).toEqual(false); - done(); - }); + expect(inlineButton.classList.contains('active')).toEqual(true); + expect(parallelButton.classList.contains('active')).toEqual(false); + done(); }); }); }); From cb6bc902ed1f89e7f94caa86b75e756b9163af51 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 9 Jul 2018 16:13:10 +0100 Subject: [PATCH 007/113] Removes hardcoded and unused prop --- app/assets/javascripts/diffs/components/diff_file.vue | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index 060386c3ecb..a61e368249a 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -31,9 +31,6 @@ export default { }; }, computed: { - isDiscussionsExpanded() { - return true; // TODO: @fatihacet - Fix this. - }, isCollapsed() { return this.file.collapsed || false; }, @@ -131,7 +128,6 @@ export default { :diff-file="file" :collapsible="true" :expanded="!isCollapsed" - :discussions-expanded="isDiscussionsExpanded" :add-merge-request-buttons="true" class="js-file-title file-title" @toggleFile="handleToggle" From e6f3452314c73875bbf9560a84706161c25c2831 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Mon, 9 Jul 2018 13:24:29 +0100 Subject: [PATCH 008/113] Adds with_projects optional parameter to /groups/:id API endpoint --- ...-omit-projects-from-get-group-endpoint.yml | 5 ++++ doc/api/groups.md | 25 +++++++++++++++++ lib/api/groups.rb | 3 ++- spec/requests/api/groups_spec.rb | 27 ++++++++++++++++--- 4 files changed, 55 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/42415-omit-projects-from-get-group-endpoint.yml diff --git a/changelogs/unreleased/42415-omit-projects-from-get-group-endpoint.yml b/changelogs/unreleased/42415-omit-projects-from-get-group-endpoint.yml new file mode 100644 index 00000000000..cabe5216045 --- /dev/null +++ b/changelogs/unreleased/42415-omit-projects-from-get-group-endpoint.yml @@ -0,0 +1,5 @@ +--- +title: Adds with_projects optional parameter to GET /groups/:id API endpoint +merge_request: 20494 +author: +type: changed diff --git a/doc/api/groups.md b/doc/api/groups.md index 53d72509423..11de75039ee 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -210,6 +210,7 @@ Parameters: | --------- | ---- | -------- | ----------- | | `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user | | `with_custom_attributes` | boolean | no | Include [custom attributes](custom_attributes.md) in response (admins only) | +| `with_projects` | boolean | no | Include details from projects that belong to the specified group (defaults to `true`). | ```bash curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/groups/4 @@ -361,6 +362,30 @@ Example response: } ``` +When adding the parameter `with_projects=false`, projects will not be returned. + +```bash +curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/groups/4?with_projects=false +``` + +Example response: + +```json +{ + "id": 4, + "name": "Twitter", + "path": "twitter", + "description": "Aliquid qui quis dignissimos distinctio ut commodi voluptas est.", + "visibility": "public", + "avatar_url": null, + "web_url": "https://gitlab.example.com/groups/twitter", + "request_access_enabled": false, + "full_name": "Twitter", + "full_path": "twitter", + "parent_id": null +} +``` + ## New group Creates a new project group. Available only for users who can create groups. diff --git a/lib/api/groups.rb b/lib/api/groups.rb index f633dd88d06..797b04df059 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -150,12 +150,13 @@ module API end params do use :with_custom_attributes + optional :with_projects, type: Boolean, default: true, desc: 'Omit project details' end get ":id" do group = find_group!(params[:id]) options = { - with: Entities::GroupDetail, + with: params[:with_projects] ? Entities::GroupDetail : Entities::Group, current_user: current_user } diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index da23fdd7dca..5a04e699d60 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -251,14 +251,22 @@ describe API::Groups do projects end + def response_project_ids(json_response, key) + json_response[key].map do |project| + project['id'].to_i + end + end + context 'when unauthenticated' do it 'returns 404 for a private group' do get api("/groups/#{group2.id}") + expect(response).to have_gitlab_http_status(404) end it 'returns 200 for a public group' do get api("/groups/#{group1.id}") + expect(response).to have_gitlab_http_status(200) end @@ -268,7 +276,7 @@ describe API::Groups do get api("/groups/#{public_group.id}") - expect(json_response['projects'].map { |p| p['id'].to_i }) + expect(response_project_ids(json_response, 'projects')) .to contain_exactly(projects[:public].id) end @@ -278,7 +286,7 @@ describe API::Groups do get api("/groups/#{group1.id}") - expect(json_response['shared_projects'].map { |p| p['id'].to_i }) + expect(response_project_ids(json_response, 'shared_projects')) .to contain_exactly(projects[:public].id) end end @@ -309,6 +317,17 @@ describe API::Groups do expect(json_response['shared_projects'][0]['id']).to eq(project.id) end + it "returns one of user1's groups without projects when with_projects option is set to false" do + project = create(:project, namespace: group2, path: 'Foo') + create(:project_group_link, project: project, group: group1) + + get api("/groups/#{group1.id}", user1), with_projects: false + + expect(response).to have_gitlab_http_status(200) + expect(json_response['projects']).to be_nil + expect(json_response['shared_projects']).to be_nil + end + it "does not return a non existing group" do get api("/groups/1328", user1) @@ -327,7 +346,7 @@ describe API::Groups do get api("/groups/#{public_group.id}", user2) - expect(json_response['projects'].map { |p| p['id'].to_i }) + expect(response_project_ids(json_response, 'projects')) .to contain_exactly(projects[:public].id, projects[:internal].id) end @@ -337,7 +356,7 @@ describe API::Groups do get api("/groups/#{group1.id}", user2) - expect(json_response['shared_projects'].map { |p| p['id'].to_i }) + expect(response_project_ids(json_response, 'shared_projects')) .to contain_exactly(projects[:public].id, projects[:internal].id) end end From 75e1f256385ed16b6a0b5bc40e52ad169d041b41 Mon Sep 17 00:00:00 2001 From: Kaspar Emanuel Date: Sun, 8 Jul 2018 22:09:44 +0100 Subject: [PATCH 009/113] Fix API docs on unauthenticated projects return --- doc/api/projects.md | 43 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/doc/api/projects.md b/doc/api/projects.md index 1e06f6d01f3..a35c2a56992 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -34,7 +34,7 @@ There are currently three options for `merge_method` to choose from: ## List all projects Get a list of all visible projects across GitLab for the authenticated user. -When accessed without authentication, only public projects are returned. +When accessed without authentication, only public projects with "simple" fields are returned. ``` GET /projects @@ -47,7 +47,7 @@ GET /projects | `order_by` | string | no | Return projects ordered by `id`, `name`, `path`, `created_at`, `updated_at`, or `last_activity_at` fields. Default is `created_at` | | `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` | | `search` | string | no | Return list of projects matching the search criteria | -| `simple` | boolean | no | Return only the ID, URL, name, and path of each project | +| `simple` | boolean | no | Return only limited fields for each project. This is a no-op without authentication as then _only_ simple fields are returned. | | `owned` | boolean | no | Limit by projects owned by the current user | | `membership` | boolean | no | Limit by projects that the current user is a member of | | `starred` | boolean | no | Limit by projects starred by the current user | @@ -56,6 +56,41 @@ GET /projects | `with_issues_enabled` | boolean | no | Limit by enabled issues feature | | `with_merge_requests_enabled` | boolean | no | Limit by enabled merge requests feature | +When `simple=true` or the user is unauthenticated this returns something like: + +```json +[ + { + "id": 4, + "description": null, + "default_branch": "master", + "ssh_url_to_repo": "git@example.com:diaspora/diaspora-client.git", + "http_url_to_repo": "http://example.com/diaspora/diaspora-client.git", + "web_url": "http://example.com/diaspora/diaspora-client", + "readme_url": "http://example.com/diaspora/diaspora-client/blob/master/README.md", + "tag_list": [ + "example", + "disapora client" + ], + "name": "Diaspora Client", + "name_with_namespace": "Diaspora / Diaspora Client", + "path": "diaspora-client", + "path_with_namespace": "diaspora/diaspora-client", + "created_at": "2013-09-30T13:46:02Z", + "last_activity_at": "2013-09-30T13:46:02Z", + "forks_count": 0, + "avatar_url": "http://example.com/uploads/project/avatar/4/uploads/avatar.png", + "star_count": 0, + }, + { + "id": 6, + "description": null, + "default_branch": "master", +... +``` + +When the user is authenticated and `simple` is not set this returns something like: + ```json [ { @@ -235,7 +270,7 @@ GET /users/:user_id/projects | `order_by` | string | no | Return projects ordered by `id`, `name`, `path`, `created_at`, `updated_at`, or `last_activity_at` fields. Default is `created_at` | | `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` | | `search` | string | no | Return list of projects matching the search criteria | -| `simple` | boolean | no | Return only the ID, URL, name, and path of each project | +| `simple` | boolean | no | Return only limited fields for each project. This is a no-op without authentication as then _only_ simple fields are returned. | | `owned` | boolean | no | Limit by projects owned by the current user | | `membership` | boolean | no | Limit by projects that the current user is a member of | | `starred` | boolean | no | Limit by projects starred by the current user | @@ -723,7 +758,7 @@ GET /projects/:id/forks | `order_by` | string | no | Return projects ordered by `id`, `name`, `path`, `created_at`, `updated_at`, or `last_activity_at` fields. Default is `created_at` | | `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` | | `search` | string | no | Return list of projects matching the search criteria | -| `simple` | boolean | no | Return only the ID, URL, name, and path of each project | +| `simple` | boolean | no | Return only limited fields for each project. This is a no-op without authentication as then _only_ simple fields are returned. | | `owned` | boolean | no | Limit by projects owned by the current user | | `membership` | boolean | no | Limit by projects that the current user is a member of | | `starred` | boolean | no | Limit by projects starred by the current user | From 737666a3d121b1bf89861de4445f857256a47949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 9 Jul 2018 18:13:34 +0200 Subject: [PATCH 010/113] Fix specs --- app/uploaders/gitlab_uploader.rb | 11 ++++++----- spec/controllers/projects/jobs_controller_spec.rb | 11 +++++------ spec/requests/api/jobs_spec.rb | 4 ++-- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index 7aa81a8c312..719bd6ef418 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -76,11 +76,12 @@ class GitlabUploader < CarrierWave::Uploader::Base end def open - stream = if file_storage? - File.open(path, "rb") if path - else - ::Gitlab::HttpIO.new(url, cached_size) if url - end + stream = + if file_storage? + File.open(path, "rb") if path + else + ::Gitlab::HttpIO.new(url, cached_size) if url + end return unless stream return stream unless block_given? diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb index 38431fb1598..6be27126383 100644 --- a/spec/controllers/projects/jobs_controller_spec.rb +++ b/spec/controllers/projects/jobs_controller_spec.rb @@ -216,20 +216,19 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do end context 'when trace artifact is in ObjectStorage' do + let(:url) { 'http://object-storage/trace' } + let(:file_path) { expand_fixture_path('trace/sample_trace') } let!(:job) { create(:ci_build, :success, :trace_artifact, pipeline: pipeline) } before do allow_any_instance_of(JobArtifactUploader).to receive(:file_storage?) { false } - allow_any_instance_of(JobArtifactUploader).to receive(:url) { remote_trace_url } - allow_any_instance_of(JobArtifactUploader).to receive(:size) { remote_trace_size } + allow_any_instance_of(JobArtifactUploader).to receive(:url) { url } + allow_any_instance_of(JobArtifactUploader).to receive(:size) { File.size(file_path) } end context 'when there are no network issues' do - let(:url) { 'http://object-storage/trace' } - let(:file) { expand_fixture_path('trace/sample_trace') } - before do - stub_remote_url_206(url, file) + stub_remote_url_206(url, file_path) get_trace end diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index 8a2812d1576..0609166ed90 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -536,13 +536,13 @@ describe API::Jobs do context 'when trace is in ObjectStorage' do let!(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) } let(:url) { 'http://object-storage/trace' } - let(:file) { expand_fixture_path('trace/sample_trace') } + let(:file_path) { expand_fixture_path('trace/sample_trace') } before do stub_remote_url_206(url, file_path) allow_any_instance_of(JobArtifactUploader).to receive(:file_storage?) { false } allow_any_instance_of(JobArtifactUploader).to receive(:url) { url } - allow_any_instance_of(JobArtifactUploader).to receive(:size) { File.size(file) } + allow_any_instance_of(JobArtifactUploader).to receive(:size) { File.size(file_path) } end it 'returns specific job trace' do From 38d407d7a7fd07888cbfda26360cd0a1c4972ec5 Mon Sep 17 00:00:00 2001 From: Jamie Schembri Date: Mon, 9 Jul 2018 17:44:09 +0200 Subject: [PATCH 011/113] Fix #48537 - Update avatar only via the projects API --- .../48537-update-avatar-only-via-api.yml | 5 +++++ lib/api/projects.rb | 3 ++- spec/requests/api/projects_spec.rb | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/48537-update-avatar-only-via-api.yml diff --git a/changelogs/unreleased/48537-update-avatar-only-via-api.yml b/changelogs/unreleased/48537-update-avatar-only-via-api.yml new file mode 100644 index 00000000000..9b3ab946cc1 --- /dev/null +++ b/changelogs/unreleased/48537-update-avatar-only-via-api.yml @@ -0,0 +1,5 @@ +--- +title: Allow updating a project's avatar without other params +merge_request: +author: Jamie Schembri +type: fixed diff --git a/lib/api/projects.rb b/lib/api/projects.rb index b83da00502d..8273abe48c9 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -260,7 +260,8 @@ module API :snippets_enabled, :tag_list, :visibility, - :wiki_enabled + :wiki_enabled, + :avatar ] optional :name, type: String, desc: 'The name of the project' optional :default_branch, type: String, desc: 'The default branch of the project' diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index de540ba7a10..15da81b57db 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1526,6 +1526,20 @@ describe API::Projects do expect(response).to have_gitlab_http_status(400) end + + it 'updates avatar' do + project_param = { + avatar: fixture_file_upload('spec/fixtures/banana_sample.gif', + 'image/gif') + } + + put api("/projects/#{project3.id}", user), project_param + + expect(response).to have_gitlab_http_status(200) + expect(json_response['avatar_url']).to eq('http://localhost/uploads/'\ + '-/system/project/avatar/'\ + "#{project3.id}/banana_sample.gif") + end end context 'when authenticated as project master' do From b98bff13defd7d0af68cdba4a47da19e3e606659 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Fri, 6 Jul 2018 15:56:41 -0500 Subject: [PATCH 012/113] Backport some changes from EE --- lib/gitlab/git_access.rb | 20 +++++++++++++------- spec/lib/gitlab/git_access_spec.rb | 24 ++++++++++++++---------- 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index db7c29be94b..35808149b90 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -2,6 +2,8 @@ # class return an instance of `GitlabAccessStatus` module Gitlab class GitAccess + include Gitlab::Utils::StrongMemoize + UnauthorizedError = Class.new(StandardError) NotFoundError = Class.new(StandardError) ProjectCreationError = Class.new(StandardError) @@ -26,7 +28,7 @@ module Gitlab PUSH_COMMANDS = %w{ git-receive-pack }.freeze ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS - attr_reader :actor, :project, :protocol, :authentication_abilities, :namespace_path, :project_path, :redirected_path, :auth_result_type + attr_reader :actor, :project, :protocol, :authentication_abilities, :namespace_path, :project_path, :redirected_path, :auth_result_type, :changes def initialize(actor, project, protocol, authentication_abilities:, namespace_path: nil, project_path: nil, redirected_path: nil, auth_result_type: nil) @actor = actor @@ -40,6 +42,8 @@ module Gitlab end def check(cmd, changes) + @changes = changes + check_protocol! check_valid_actor! check_active_user! @@ -58,7 +62,7 @@ module Gitlab when *DOWNLOAD_COMMANDS check_download_access! when *PUSH_COMMANDS - check_push_access!(changes) + check_push_access! end true @@ -218,7 +222,7 @@ module Gitlab end end - def check_push_access!(changes) + def check_push_access! if project.repository_read_only? raise UnauthorizedError, ERROR_MESSAGES[:read_only] end @@ -235,17 +239,15 @@ module Gitlab return if changes.blank? # Allow access this is needed for EE. - check_change_access!(changes) + check_change_access! end - def check_change_access!(changes) + def check_change_access! # If there are worktrees with a HEAD pointing to a non-existent object, # calls to `git rev-list --all` will fail in git 2.15+. This should also # clear stale lock files. project.repository.clean_stale_repository_files - changes_list = Gitlab::ChangesList.new(changes) - # Iterate over all changes to find if user allowed all of them to be applied changes_list.each.with_index do |change, index| first_change = index == 0 @@ -321,6 +323,10 @@ module Gitlab protected + def changes_list + @changes_list ||= Gitlab::ChangesList.new(changes) + end + def user return @user if defined?(@user) diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index ff32025253a..6d11efb42c8 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -13,14 +13,6 @@ describe Gitlab::GitAccess do let(:authentication_abilities) { %i[read_project download_code push_code] } let(:redirected_path) { nil } let(:auth_result_type) { nil } - - let(:access) do - described_class.new(actor, project, - protocol, authentication_abilities: authentication_abilities, - namespace_path: namespace_path, project_path: project_path, - redirected_path: redirected_path, auth_result_type: auth_result_type) - end - let(:changes) { '_any' } let(:push_access_check) { access.check('git-receive-pack', changes) } let(:pull_access_check) { access.check('git-upload-pack', changes) } @@ -724,10 +716,11 @@ describe Gitlab::GitAccess do end describe '#check_push_access!' do + let(:unprotected_branch) { 'unprotected_branch' } + before do merge_into_protected_branch end - let(:unprotected_branch) { 'unprotected_branch' } let(:changes) do { push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow", @@ -785,7 +778,7 @@ describe Gitlab::GitAccess do aggregate_failures do matrix.each do |action, allowed| - check = -> { access.send(:check_push_access!, changes[action]) } + check = -> { push_changes(changes[action]) } if allowed expect(&check).not_to raise_error, @@ -1152,6 +1145,17 @@ describe Gitlab::GitAccess do private + def access + described_class.new(actor, project, protocol, + authentication_abilities: authentication_abilities, + namespace_path: namespace_path, project_path: project_path, + redirected_path: redirected_path, auth_result_type: auth_result_type) + end + + def push_changes(changes) + access.check('git-receive-pack', changes) + end + def raise_unauthorized(message) raise_error(Gitlab::GitAccess::UnauthorizedError, message) end From 220f37a02bb009ae3fb3ee1d442e6a9105f1729d Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 10 Jul 2018 01:49:50 +0800 Subject: [PATCH 013/113] This test was copied from EE --- spec/mailers/notify_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index a9a45367b4a..effbfb1ba84 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -314,6 +314,17 @@ describe Notify do end end + describe 'that are new with a description' do + subject { described_class.new_merge_request_email(merge_request.assignee_id, merge_request.id) } + + it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like "an unsubscribeable thread" + + it 'contains the description' do + is_expected.to have_body_text(merge_request.description) + end + end + describe 'that have been relabeled' do subject { described_class.relabeled_merge_request_email(recipient.id, merge_request.id, %w[foo bar baz], current_user.id) } From ded205b4024109942dc50594f504f0e85e2447bd Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Mon, 9 Jul 2018 22:41:19 +0200 Subject: [PATCH 014/113] Fix mountComponent helper path in docs --- doc/development/fe_guide/vue.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index e31ee087358..219b98ac696 100644 --- a/doc/development/fe_guide/vue.md +++ b/doc/development/fe_guide/vue.md @@ -425,7 +425,7 @@ There is a helper in `spec/javascripts/helpers/vue_mount_component_helper.js` th ```javascript import Vue from 'vue'; -import mountComponent from 'helpers/vue_mount_component_helper.js' +import mountComponent from 'spec/helpers/vue_mount_component_helper' import component from 'component.vue' const Component = Vue.extend(component); From 7ce51548acc8fe27ec9325a9a9583e22edbc2822 Mon Sep 17 00:00:00 2001 From: Jasper Maes Date: Mon, 9 Jul 2018 23:07:57 +0200 Subject: [PATCH 015/113] Rails5 MySQL fix rename_column as part of cleanup_concurrent_column_type_change --- changelogs/unreleased/rails5-mysql-rename-column.yml | 5 +++++ config/initializers/active_record_table_definition.rb | 5 +++++ 2 files changed, 10 insertions(+) create mode 100644 changelogs/unreleased/rails5-mysql-rename-column.yml diff --git a/changelogs/unreleased/rails5-mysql-rename-column.yml b/changelogs/unreleased/rails5-mysql-rename-column.yml new file mode 100644 index 00000000000..cbae9250744 --- /dev/null +++ b/changelogs/unreleased/rails5-mysql-rename-column.yml @@ -0,0 +1,5 @@ +--- +title: Rails5 MySQL fix rename_column as part of cleanup_concurrent_column_type_change +merge_request: 20514 +author: Jasper Maes +type: fixed diff --git a/config/initializers/active_record_table_definition.rb b/config/initializers/active_record_table_definition.rb index 8e3a1c7a62f..a71069f27a3 100644 --- a/config/initializers/active_record_table_definition.rb +++ b/config/initializers/active_record_table_definition.rb @@ -29,6 +29,11 @@ module ActiveRecord def datetime_with_timezone(column_name, **options) column(column_name, :datetime_with_timezone, options) end + + # Disable timestamp alias to datetime + def aliased_types(name, fallback) + fallback + end end end end From d79cef3a9a5577765d975326fbf4bc1b8c5634de Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Tue, 10 Jul 2018 08:11:04 +0000 Subject: [PATCH 016/113] Support manually stopping any environment from the UI --- .../components/environment_actions.vue | 91 +- .../components/environment_external_url.vue | 51 +- .../components/environment_item.vue | 867 +++++++++--------- .../components/environment_monitoring.vue | 49 +- .../components/environment_rollback.vue | 96 +- .../components/environment_stop.vue | 114 +-- .../environment_terminal_button.vue | 53 +- .../components/environments_app.vue | 4 + .../components/stop_environment_modal.vue | 92 ++ .../folder/environments_folder_view.vue | 8 + .../environments/mixins/environments_mixin.js | 22 +- .../services/environments_service.js | 2 +- .../stylesheets/pages/environments.scss | 2 +- .../projects/environments_controller.rb | 6 +- .../projects/merge_requests_controller.rb | 2 +- app/policies/environment_policy.rb | 10 +- app/serializers/environment_entity.rb | 12 +- app/views/projects/deployments/_actions.haml | 7 +- app/views/projects/deployments/_rollback.haml | 7 +- .../environments/_external_url.html.haml | 2 +- .../projects/environments/_stop.html.haml | 5 - .../projects/environments/show.html.haml | 32 +- .../unreleased/winh-stop-all-environments.yml | 5 + lib/api/environments.rb | 3 +- locale/gitlab.pot | 37 +- .../projects/environments/environment_spec.rb | 13 +- .../environments/environments_spec.rb | 12 +- .../environments/environment_rollback_spec.js | 4 +- .../environments/environment_stop_spec.js | 12 +- spec/policies/environment_policy_spec.rb | 128 ++- spec/serializers/environment_entity_spec.rb | 3 +- .../environment_serializer_spec.rb | 10 +- 32 files changed, 1009 insertions(+), 752 deletions(-) create mode 100644 app/assets/javascripts/environments/components/stop_environment_modal.vue delete mode 100644 app/views/projects/environments/_stop.html.haml create mode 100644 changelogs/unreleased/winh-stop-all-environments.yml diff --git a/app/assets/javascripts/environments/components/environment_actions.vue b/app/assets/javascripts/environments/components/environment_actions.vue index e3652fe739e..63d83e307ee 100644 --- a/app/assets/javascripts/environments/components/environment_actions.vue +++ b/app/assets/javascripts/environments/components/environment_actions.vue @@ -1,50 +1,50 @@