feat: handle migration errors from the template #2819
Conversation
|
You can access the deployment of this PR at https://renku-ci-rp-2819.dev.renku.ch |
lorenzo-cavazzi
force-pushed
the
2769-migration-errors
branch
from
fe666ec
to
7793801
Compare
renku/core/template/template.py
Outdated
| @@ -420,6 +420,10 @@ def fetch(cls, source: Optional[str], reference: Optional[str]) -> "RepositoryTe | |||
| try: | |||
| repository = clone_repository(url=source, path=path, checkout_revision=reference, install_lfs=False) | |||
| except errors.GitError as e: | |||
| if "Cannot checkout reference" in str(e): | |||
| raise errors.TemplateMissingReferenceError( | |||
| f"Cannot find the reference {reference} in the template repository from {source}" | |||
There was a problem hiding this comment.
reference {reference} -> reference '{reference}' so that it's more visible what the reference is.
| message = f"{str(e)}; You can still manually update the template and set a difference reference." | ||
| raise errors.TemplateUpdateError(message) | ||
| except errors.InvalidTemplateError: | ||
| raise errors.TemplateUpdateError("Template cannot be fetched.") |
There was a problem hiding this comment.
What's the reason to convert these errors to TemplateUpdateError? I believe we don't need to catch them here.
There was a problem hiding this comment.
The idea was to simplify catching/handling them in the service and in the CLI. For the latter, throwing a new error results in a clean error message, while not doing it means a long and confusing stack trace is printed (see screen below).
Is there a better way to handle this? Or maybe that should be done in the cli code?
There was a problem hiding this comment.
I see. It's fine to do it here.
| @wraps(f) | ||
| def decorated_function(*args, **kwargs): | ||
| """Represents decorated function.""" | ||
| # NOTE: verify if this may better go in MigrationsCheckCtrl as try/except in to_response() |
There was a problem hiding this comment.
I don't get this comment. Is it a left-over TODO?
There was a problem hiding this comment.
Ouch! Yes, that is a leftover, thanks for spotting it!
|
|
||
|
|
||
| @handle_migration_read_errors | ||
| def handle_migration_write_errors(f): |
There was a problem hiding this comment.
Why do we need to distinguish between migration read/write errors. AFAIK, the difference is that in read we only check for migration; so, the same errors can happen in both cases.
There was a problem hiding this comment.
The write section handles a couple of extra errors, and those throw a more severe error (Program instead of Intermittent).
The point is that errors while reading are somewhat expected (e.g. template may be deleted, that's not a bug), while actually migrating the project should be invoked by the client only if the read errors didn't happen.
So a DockerfileUpdateError or a MigrationError caught on writing is likely to be a bug or severe error from which the user may not be able to recover in the UI.
/deploy renku=auto-update/renku-core-1.2.1 #persist
Description
This PR improves the UX when a project's template cannot be checked out correctly. It adds new errors and some basic indications to the user in both the CLI and the service. The 2 new cases handled properly are:
fix #2769