Skip to content
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

Do not fetch VCS deps when reference didn't change #6131

Merged
merged 3 commits into from
Sep 5, 2022

Conversation

maksbotan
Copy link
Contributor

Pull Request Check List

If we have root -> foo -> VCSDependency(bar, ref="sha256"), and lock file contains bar @ sha256 and user ran poetry lock --no-update, do not fetch bar repo from git: as commit hash is explicitly locked, nothing could have changed.

  • Added tests for changed code.
  • Updated documentation for changed code.

As with #6130, I'll backport this to 1.1 once merged. And I'll need guidance on adding tests.

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@radoering
Copy link
Member

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 poetry lock --no-update. I can provide a test for this later this week.

@maksbotan
Copy link
Contributor Author

maksbotan commented Aug 31, 2022

Thanks! I've followed #6204, it's great that it's merged.

I'll try to update this in a few days.

@maksbotan
Copy link
Contributor Author

@radoering i've updated my branch to current master, incorporating your refactoring changes. Thanks!

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 neersighted added area/solver Related to the dependency resolver impact/backport Requires backport to stable branch impact/changelog Requires a changelog entry backport/1.2 labels Sep 3, 2022
@maksbotan
Copy link
Contributor Author

I'd love to write a test, but I need an advice on what actually test for. Is there a way to check that provider does not actually call anything git-related (perhaps provider.get_package_from_vcs)? Maybe it can be done through mocker?

@radoering
Copy link
Member

I added a test with a slightly different focus. It checks that a locked vcs dependency referencing a branch is not updated when running poetry lock --no-update.

@maksbotan You can probably add a similar test with a locked vcs dependency referencing a hash and check via a mock that get_package_from_vcs is not called.

@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...

@maksbotan
Copy link
Contributor Author

Thanks!
What backport are talking about? I'm not sure I'm following

@radoering
Copy link
Member

The label backport/1.2 has been set but I don't think we'll backport it.

@maksbotan
Copy link
Contributor Author

Ah, I see, thanks. Without backport this will only hit 1.3, right?

@onerandomusername
Copy link
Contributor

I'd like to see this backported if the 1.3 release is going to be anything like 1.2

@neersighted
Copy link
Member

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 neersighted removed impact/backport Requires backport to stable branch backport/1.2 labels Sep 4, 2022
@maksbotan
Copy link
Contributor Author

@radoering I've added a new test, hope it's ok

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@radoering radoering merged commit dc9d84c into python-poetry:master Sep 5, 2022
@maksbotan
Copy link
Contributor Author

Thank you @radoering!

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/solver Related to the dependency resolver impact/changelog Requires a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants