-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Raise exception in failed checkout #5035
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Raise exception in failed checkout #5035
Conversation
Failure is bc we don't have tests for bzr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Left some comments to consider.
@@ -47,6 +47,10 @@ class RepositoryError(BuildEnvironmentError): | |||
'You can not have two versions with the name latest or stable.' | |||
) | |||
|
|||
FAILED_TO_CHECKOUT = _( | |||
'Failed to checkout revision: {}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this exception log to Sentry? It should not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. Actually I'm not sure, I haven't seen a RepositoryError
in sentry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some,
https://sentry.io/read-the-docs/readthedocs-org/issues/713027767/?query=RepositoryError
There are some places where it's handled
and there other where it's not:
I suppose that this is the final solution, #3856
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to merge this, and if it logs, we can handle it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good change. 👍 We can adjust later if we hit issues with it.
@@ -47,6 +47,10 @@ class RepositoryError(BuildEnvironmentError): | |||
'You can not have two versions with the name latest or stable.' | |||
) | |||
|
|||
FAILED_TO_CHECKOUT = _( | |||
'Failed to checkout revision: {}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to merge this, and if it logs, we can handle it later.
Closes #5033