-
Notifications
You must be signed in to change notification settings - Fork 79
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
(Git-1) use tmpdir for feedstocks instead of static directory #2872
Conversation
76d3cb8
to
0b3f203
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2872 +/- ##
==========================================
+ Coverage 77.07% 77.49% +0.41%
==========================================
Files 112 115 +3
Lines 12053 12215 +162
==========================================
+ Hits 9290 9466 +176
+ Misses 2763 2749 -14 ☔ View full report in Codecov by Sentry. |
@beckermr ready for review |
Cool. I'm away for the next few days so it will likely be a bit. |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
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 have some comments / requests / questions. Also, in general I was in the process of removing the context module and its associated dataclasses. They had carried sensitive state in the past and needing them made it a pain to debug the bot. This PR adds more things. If we want to keep them, it might be useful to add factory functions to make a context given some simple data (e.g., the feedstock name).
Are you referring to the
I think I don't really get it. What for? So that setting |
Yeah it was setting the attrs field which is a pain, but also I don't like having more than one effective api for things related to the feedstocks. Then, for example, we get code that looks up the automerge attribute either directly from attrs or from this new attribute on the context. It's confusing to have both. |
Understood. But let's get this though nonetheless then? In my imagination, the Open to refactor this further in separate PRs of course. |
What is wrong with the pre-commit CI integration? It shows a timeout for some reason |
Idk. Maybe try and retrigger with an empty commit. |
I'm seeing this in other repos though. |
It also happened in previous commits here - maybe it is an upstream issue? |
This is first step of #2812.