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
fix: clean cache after trying to fetch projects from non-existing repos #2789
Conversation
You can access the deployment of this PR at https://renku-ci-rp-2789.dev.renku.ch |
10d2fc1
to
41f2492
Compare
This is ready for review now |
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.
Thanks Lorenzo! I've made two comments regarding this PR.
).output | ||
project.save() | ||
except GitCommandError as e: | ||
if "project you were looking for could not be found" in str(e): |
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.
What are other GitCommandError
errors that can happen? To me, it seems that we should always call cache.invalidate_project
when a GitCommandError
happens.
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 thought other git errors may surface there if the repository is valid but some git actions fail.
An example would be trying to checkout a non-existing reference. In that case, leaving the project in cache could be better? I'm happy to remove the condition if you think it's always better to clean up on errors.
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.
As a side note, I just tried to checkout a non-existing branch and it seems that no exception is thrown.
Shouldn't it be the case? Should I open an issue?
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 believe it's better to clean up in case of all errors instead of filtering based on error messages. I don't think it causes any problem.
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.
As a side note, I just tried to checkout a non-existing branch and it seems that no exception is thrown.
Shouldn't it be the case? Should I open an issue?
That's weird, it should always fail. I couldn't re-produce this using clone_renku_repository('https://dev.renku.ch/gitlab/mohammad.alisafaee/lego-sets.git', '/tmp/d', checkout_revision='non-existing', raise_git_except=True)
or repo.checkout('non-existing')
. Please create a bug report with the exact command.
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.
Ok, I removed that condition on the error message.
Here is the issue describing the bug and how to reproduce it: #2797
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.
Thank you!
Clean up the cache properly when cloning a remote repo fails because the repo doesn't exist or it is otherwise not reachable.
/deploy #persist
fix #2787