From b29abfbdc0f923420c7e6259bbf8518a8d95ab25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Wed, 28 Oct 2015 14:30:50 +0100 Subject: [PATCH] Expect URL and path to be returned from the remote This commit expects a managed remote to return at least a URL to the repository, and optionally a path. Note that OpenProject currently only supports local repositories for Git, and thus using managed remotes with Git WILL require a path returned from the remote. For Subversion, also returning a `file://` URL is sufficient, since it can browse that. Returned external URLs must be accessible from OpenProject, since we do not receive any authentication from the remote. --- app/workers/scm/create_remote_repository_job.rb | 10 +++++++++- app/workers/scm/relocate_repository_job.rb | 9 ++++++++- app/workers/scm/remote_repository_job.rb | 5 +++-- config/locales/en.yml | 1 + .../manual/repository-integration.md | 5 +++++ extra/Apache/OpenProjectRepoman.pm | 5 ++++- .../repositories/create_repository_spec.rb | 4 +++- .../repositories/repository_settings_spec.rb | 4 +++- .../create_managed_repository_service_spec.rb | 17 +++++++++++++++-- .../delete_managed_repository_service_spec.rb | 2 +- spec/support/scm/relocate_repository.rb | 8 ++++++-- 11 files changed, 58 insertions(+), 12 deletions(-) diff --git a/app/workers/scm/create_remote_repository_job.rb b/app/workers/scm/create_remote_repository_job.rb index bbcf729782..46cd143dfe 100644 --- a/app/workers/scm/create_remote_repository_job.rb +++ b/app/workers/scm/create_remote_repository_job.rb @@ -37,6 +37,14 @@ # Until then, a synchronous process is more failsafe. class Scm::CreateRemoteRepositoryJob < Scm::RemoteRepositoryJob def perform - send(repository_request.merge(action: :create)) + response = send(repository_request.merge(action: :create)) + repository.root_url = response['path'] + repository.url = response['url'] + + unless repository.save + raise OpenProject::Scm::Exceptions::ScmError.new( + I18n.t('repositories.errors.remote_save_failed') + ) + end end end diff --git a/app/workers/scm/relocate_repository_job.rb b/app/workers/scm/relocate_repository_job.rb index ad86aaa632..48b6ba7f13 100644 --- a/app/workers/scm/relocate_repository_job.rb +++ b/app/workers/scm/relocate_repository_job.rb @@ -43,9 +43,16 @@ class Scm::RelocateRepositoryJob < Scm::RemoteRepositoryJob ## # POST to the remote managed repository a request to relocate the repository def relocate_remote - send(repository_request.merge( + response = send(repository_request.merge( action: :relocate, old_repository: repository.root_url)) + repository.root_url = response['path'] + repository.url = response['url'] + + unless repository.save + Rails.logger.error("Could not relocate the remote repository " \ + "#{repository.repository_identifier}.") + end end ## diff --git a/app/workers/scm/remote_repository_job.rb b/app/workers/scm/remote_repository_job.rb index 9e6326037d..023818fb77 100644 --- a/app/workers/scm/remote_repository_job.rb +++ b/app/workers/scm/remote_repository_job.rb @@ -39,7 +39,6 @@ class Scm::RemoteRepositoryJob attr_reader :repository - ## # Initialize the job, optionally saving the whole repository object # (use only when not serializing the job.) @@ -65,9 +64,9 @@ class Scm::RemoteRepositoryJob response = ::Net::HTTP.start(uri.hostname, uri.port) do |http| http.request(req) end + info = try_to_parse_response(response.body) unless response.is_a? ::Net::HTTPSuccess - info = try_to_parse_response(response.body) raise OpenProject::Scm::Exceptions::ScmError.new( I18n.t('repositories.errors.remote_call_failed', code: response.code, @@ -75,6 +74,8 @@ class Scm::RemoteRepositoryJob ) ) end + + info end def try_to_parse_response(body) diff --git a/config/locales/en.yml b/config/locales/en.yml index c963a5ddb8..f097e04f23 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1569,6 +1569,7 @@ en: disabled_or_unknown_vendor: "The SCM vendor %{vendor} is disabled or no longer available." remote_call_failed: "Calling the managed remote failed with message '%{message}' (Code: %{code})" remote_invalid_response: "Received an invalid response from the managed remote." + remote_save_failed: "Could not save the repository with the parameters retrieved from the remote." git: instructions: managed_url: "This is the URL of the managed (local) Git repository." diff --git a/doc/operation_guides/manual/repository-integration.md b/doc/operation_guides/manual/repository-integration.md index 8b8aa6ee57..c31aa27981 100644 --- a/doc/operation_guides/manual/repository-integration.md +++ b/doc/operation_guides/manual/repository-integration.md @@ -95,6 +95,11 @@ Upon creating and deleting repositories in the frontend, OpenProject will POST t "token": } +The endpoint is expected to return a JSON with at least a `message` property when the response is not successful (2xx). +When the response is successful, it must at least return a `url` property that contains an accessible URL, an optionally, a `path` property to access the repository locally. +Note that for Git repositories, OpenProject currently can only read them locally (i.e, through an NFS mount), so a path is mandatory here. +For Subversion, you can either return a `file:///` URL, or a local path. + Our main use-case for this feature is to reduce the complexity of permission issues around Subversion mainly in packager, for which a simple Apache wrapper script is used in `extra/Apache/OpenProjectRepoman.pm`. This functionality is very limited, but may be extended when other use cases arise. It supports notifications for creating repositories (action `create`), moving repositories (action `relocate`, when a project's identifier has changed), and deleting repositories (action `delete`). diff --git a/extra/Apache/OpenProjectRepoman.pm b/extra/Apache/OpenProjectRepoman.pm index bdaca687bc..e2b44cc81b 100644 --- a/extra/Apache/OpenProjectRepoman.pm +++ b/extra/Apache/OpenProjectRepoman.pm @@ -153,7 +153,10 @@ sub _handle_request { return { success => JSON::PP::true, message => "The action has completed sucessfully.", - repository => $target + repository => $target, + path => $target, + # This is only useful in the packager context + url => 'file://' + $target }; } diff --git a/spec/features/repositories/create_repository_spec.rb b/spec/features/repositories/create_repository_spec.rb index 3f1ec86547..bd3d99c0ea 100644 --- a/spec/features/repositories/create_repository_spec.rb +++ b/spec/features/repositories/create_repository_spec.rb @@ -215,7 +215,9 @@ describe 'Create repository', type: :feature, js: true, selenium: true do } before do - stub_request(:post, url).to_return(status: 200) + stub_request(:post, url) + .to_return(status: 200, + body: { success: true, url: 'file:///foo/bar' }.to_json) end it_behaves_like 'it can create the managed repository' diff --git a/spec/features/repositories/repository_settings_spec.rb b/spec/features/repositories/repository_settings_spec.rb index f0b06ee4b3..b15264122b 100644 --- a/spec/features/repositories/repository_settings_spec.rb +++ b/spec/features/repositories/repository_settings_spec.rb @@ -160,7 +160,9 @@ describe 'Repository Settings', type: :feature, js: true do :managed ) - stub_request(:post, url).to_return(status: 200) + stub_request(:post, url) + .to_return(status: 200, + body: { success: true, url: 'file:///foo/bar' }.to_json) repo.save! repo diff --git a/spec/services/scm/create_managed_repository_service_spec.rb b/spec/services/scm/create_managed_repository_service_spec.rb index 6a00f67b34..033e91e4a8 100644 --- a/spec/services/scm/create_managed_repository_service_spec.rb +++ b/spec/services/scm/create_managed_repository_service_spec.rb @@ -146,7 +146,7 @@ describe Scm::CreateManagedRepositoryService do } let(:repository) { - repo = Repository::Subversion.new(scm_type: :managed) + repo = FactoryGirl.build(:repository_subversion, scm_type: :managed) repo.project = project repo.configure(:managed, nil) repo @@ -158,8 +158,18 @@ describe Scm::CreateManagedRepositoryService do end context 'with a remote callback' do + let(:returned_url) { 'file:///tmp/some/url/to/repo' } + let(:root_url) { '/tmp/some/url/to/repo' } before do - stub_request(:post, url).to_return(status: 200) + stub_request(:post, url) + .to_return( + status: 200, + body: { url: returned_url, path: root_url }.to_json + ) + + # Avoid setting up a second call to the remote during save + # since we only templated the repository, not created one! + expect(repository).to receive(:save).and_return(true) end it 'calls the callback' do @@ -167,6 +177,9 @@ describe Scm::CreateManagedRepositoryService do .to receive(:new).and_call_original expect(service.call).to be true + expect(repository.root_url).to eq(root_url) + expect(repository.url).to eq(returned_url) + expect(WebMock) .to have_requested(:post, url) .with(body: hash_including(action: 'create')) diff --git a/spec/services/scm/delete_managed_repository_service_spec.rb b/spec/services/scm/delete_managed_repository_service_spec.rb index db9a3f4521..74a8aa8511 100644 --- a/spec/services/scm/delete_managed_repository_service_spec.rb +++ b/spec/services/scm/delete_managed_repository_service_spec.rb @@ -132,7 +132,7 @@ describe Scm::DeleteManagedRepositoryService do context 'with a valid remote' do before do - stub_request(:post, url).to_return(status: 200) + stub_request(:post, url).to_return(status: 200, body: {}.to_json ) end it 'calls the callback' do diff --git a/spec/support/scm/relocate_repository.rb b/spec/support/scm/relocate_repository.rb index adb2f27a1a..1b98d10350 100644 --- a/spec/support/scm/relocate_repository.rb +++ b/spec/support/scm/relocate_repository.rb @@ -46,14 +46,18 @@ shared_examples_for 'repository can be relocated' do |vendor| let(:config) { { manages: url } } let(:repository) { - stub_request(:post, url).to_return(status: 200) + stub_request(:post, url) + .to_return(status: 200, + body: { success: true, url: 'file:///foo/bar', path: '/tmp/foo/bar' }.to_json) FactoryGirl.create("repository_#{vendor}".to_sym, project: project, scm_type: :managed) } before do - stub_request(:post, url).to_return(status: 200) + stub_request(:post, url) + .to_return(status: 200, + body: { success: true, url: 'file:///new/bar', path: '/tmp/new/bar' }.to_json) end it 'sends a relocation request when project identifier is updated' do