-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Appropiate logging when a LockTimeout for VCS is reached #4804
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4804 +/- ##
==========================================
- Coverage 76.39% 76.31% -0.08%
==========================================
Files 158 158
Lines 9980 9989 +9
Branches 1261 1264 +3
==========================================
- Hits 7624 7623 -1
- Misses 2016 2025 +9
- Partials 340 341 +1
|
'Lock still active: project=%s version=%s', | ||
self.project.slug, | ||
self.version.slug, | ||
) |
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 should raise VersionLockedError
here.
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.
Since this task is used only for syncing the repo, we don't need to raise the exception here. It will return False
when it fails and that's all what we need.
'Lock still active: project=%s version=%s', | ||
self.project.slug, | ||
self.version.slug, | ||
) |
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.
Same here.
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.
Here, we need to raise a proper exception on each different case.
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.
I ran into issues with self.version
and self.project
not existing because get_project
and get_version
fail because of an API issue (https://sentry.io/read-the-docs/readthedocs-org/issues/712735617/?query=is:unresolved). We can probably change that in another PR, but thought it might be worth mentioning.
Yes, I also noted those. I need to fix them. |
Believe we can ignore the codecov issues here -- it feels wrong, and I will merge. |
Related #3856