There will be about 1h of downtime on 2020-03-02 at 13:00 UTC+1 (change of servers and upgrade to Heptapod 0.20.0 final)

Commit 9ca94ed8 authored by Georges Racinet's avatar Georges Racinet 🦑

Repository loading: returning proper error if not found

We already had good enough error responses for the case when
the *storage* is not found, but it was missing for the case
when the repository is not found in an existing storage.

This is important, because it can actually break the landing page
of some users: the Rails application does not seem to expect an
unspecified Gitaly error here, but it should recover from
NOT_FOUND.

I've been impaired by this on dev setup after
with some repository removals, but don't have anything of that
kind on hand any more, which would be more reassuring.

We could have a better separation in concerns, as the
subsequent change in `RepositoryExists()` indicates, but that's
not a blocker for the imminent release.
parent e4074022b983
Pipeline #12613 passed with stages
in 1 minute and 19 seconds
......@@ -11,7 +11,6 @@ import tempfile
from mercurial import (
archival,
scmutil,
error,
)
# TODO move that to the heptapod package
......@@ -64,8 +63,14 @@ class RepositoryServiceServicer(RepositoryServiceServicer, HGitalyServicer):
try:
self.load_repo(request.repository, context)
exists = True
except error.RepoError:
except KeyError:
exists = False
# TODO would be better to have a two-layer helper
# in servicer: load_repo() for proper gRPC error handling and
# load_repo_raw_exceptions() (name to be improved) to get the
# raw exceptions
context.set_code(StatusCode.OK)
context.set_details('')
return RepositoryExistsResponse(exists=exists)
......
......@@ -9,6 +9,7 @@ import os
from grpc import StatusCode
from mercurial import (
error,
hg,
ui as uimod,
)
......@@ -69,4 +70,9 @@ class HGitalyServicer:
logger.info("loading repo at %r", repo_path)
# ensure caller gets private copy of ui
return hg.repository(self.ui.copy(), repo_path)
try:
return hg.repository(self.ui.copy(), repo_path)
except error.RepoError as exc:
context.set_code(StatusCode.NOT_FOUND)
context.set_details(repr(exc.args))
raise KeyError('repo', repo_path)
......@@ -57,7 +57,7 @@ def test_load_repo(tmpdir):
assert context.code == grpc.StatusCode.NOT_FOUND
def test_not_found_propagation(grpc_channel):
def test_not_found_propagation(grpc_channel, server_repos_root):
# Taking a random RPC to check that the client receives the
# proper error response
repo_stub = RepositoryServiceStub(grpc_channel)
......@@ -69,3 +69,12 @@ def test_not_found_propagation(grpc_channel):
assert exc.code() == grpc.StatusCode.NOT_FOUND
assert 'dream' in exc.details()
with pytest.raises(grpc.RpcError) as exc_info:
repo_stub.HasLocalBranches(HasLocalBranchesRequest(
repository=Repository(storage_name='default',
relative_path='not_here')))
exc = exc_info.value
assert exc.code() == grpc.StatusCode.NOT_FOUND
assert 'not_here' in exc.details()
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment