Do not fetch VCS deps when reference didn't change#6131
Do not fetch VCS deps when reference didn't change#6131radoering merged 3 commits intopython-poetry:masterfrom
Conversation
maksbotan
force-pushed
the
maksbotan/vcs-caching
branch
from
0381cdf to
4519884
Compare
radoering
left a comment
radoering
left a comment
There was a problem hiding this comment.
Although I think the change is functionally fine, I don't like it from an architectural point of view. I have thought about it for some time and finally created #6204. If this refactoring makes it to master, this PR can be adjusted accordingly.
|
Since #6204 has been merged you may rebase this PR. The required change should be much simpler now. Re test: This change also fixes that the locked hash of a vcs dependency referencing a branch is updated when running |
|
Thanks! I've followed #6204, it's great that it's merged. I'll try to update this in a few days. |
maksbotan
force-pushed
the
maksbotan/vcs-caching
branch
from
4519884 to
3612070
Compare
|
@radoering i've updated my branch to current master, incorporating your refactoring changes. Thanks! |
neersighted
left a comment
neersighted
left a comment
There was a problem hiding this comment.
LGTM, but tests are needed. The existing tests for the provider should be good guidance @maksbotan -- but if you're still stuck on tests, @radoering are you still volunteering to get this one over the finish line?
neersighted
added
area/solver
|
I'd love to write a test, but I need an advice on what actually test for. Is there a way to check that |
|
I added a test with a slightly different focus. It checks that a locked vcs dependency referencing a branch is not updated when running @maksbotan You can probably add a similar test with a locked vcs dependency referencing a hash and check via a mock that @neersighted Do you really want to backport this? Afaik it's not a regression and it would require to backport #6204 or make significant changes in the backport... |
|
Thanks! |
|
The label |
|
Ah, I see, thanks. Without backport this will only hit 1.3, right? |
|
I'd like to see this backported if the 1.3 release is going to be anything like 1.2 |
|
That's fair -- I had it in my head this was a regression, but the truth is that 1.1 exhibited this behavior as well. |
neersighted
removed
impact/backport
maksbotan
force-pushed
the
maksbotan/vcs-caching
branch
from
10dc55b to
3936b69
Compare
|
@radoering I've added a new test, hope it's ok |
|
Thank you @radoering! |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
4 participants
Pull Request Check List
If we have
root->foo->VCSDependency(bar, ref="sha256"), and lock file containsbar @ sha256and user ranpoetry lock --no-update, do not fetchbarrepo from git: as commit hash is explicitly locked, nothing could have changed.As with #6130, I'll backport this to 1.1 once merged. And I'll need guidance on adding tests.