Closed acullenn closed 9 months ago
Ah. I feel like I encountered this at some point, but I was not able to reproduce my failing test after refactoring so assumed I added a missing dependency somewhere…
I would have expected these changes to break tests where multiple venv share the same requirements.txt file but maybe we do not test this case in CI.
Yet, I do not feel this is the right fix since we want to trigger a new install if the requirements file is changed and not managed by Puppet (e.g. it is part of a repo managed by vcsrepo). I think we now fail here (and it seems not to not have a corresponding test).
Can you add a (failing) unit test that demonstrate the problem you want to fix so that I have a way to reproduce this and think about it?
I've added a unit test for this issue, and confirmed that it's failing when my fix is not applied:(https://github.com/acullenhms/puppet-python/pull/2)
But it succeeds when the subscription changes are in place. Interestingly though, The Puppet + Ruby 2.6/2.7 test fail when the fix exists, but succeed when it isn't. I'd be interested to know why.
This is my first Ruby +/ Puppet unit test, so if I've made a mistake or style flub please let me know!
I would have expected these changes to break tests where multiple venv share the same requirements.txt file but maybe we do not test this case in CI.
My understanding of the codebase is incomplete - what about the logic for requirements installation would make you expect this?
Dear @acullenhms, thanks for the PR!
This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?
You can find my sourcecode at voxpupuli/vox-pupuli-tasks
@smortex showed a valid reason, why local_subscribe
might be set to undefined
. I'm using vcsrepo
which clones a repo with the requirements.txt
included, which means, that there's no File[$requirements]
defined in all cases.
But: this setup won't trigger any installation, as long as forceupdate
isn't set to true, as refreshonly
won't help here (there's nothing refreshing anything - I can set this up manually on the vcsrepo
part).
There's a --dry-run
option added in pip 22.2 (which is the newest release), which might help here - in the future.
A way to check, if there are uninstalled packages would be the easiest way to resolve this issue, but as far as I know, there's no such thing.
python3 -c "import pkg_resources; pkg_resources.require(open('requirements.txt',mode='r'))" &>/dev/null
seems to work, but it fails, if there's a mismatch within the package name. E.g.
zope-interface==5.4.0 # requirements.txt
zope.interface==5.4.0 # pip freeze
Looks like, as if pip does some magic on the package names (-
vs _
, -
vs .
,...)
Is there any way to get this patch going again?
Hello, I used Puppet to manage my requirement.txt. This module won't subscribe to the requirement.txt. I have a similar fix that I would like to apply. Is there anything I can do to help get this going?
@smortex
Pull Request (PR) description
Fixes issue wherein the
python::requirements
class does not correctly install a requirements.txt file declared in Puppet to the declared venv, e.g.This appears to be due to the logic for setting the local resource subscription - it checks if the requirements file is
not
declared before setting the local subscription which ultimately gates the pip installation. This behavior runs contrary to the provided Example(s), in whichrequirements.txt
must exist or be declared in advance of invoking the class: https://forge.puppet.com/modules/puppet/python/reference#pythonrequirementsChanges
python::requirements
, which is used to trigger the pip installation.This Pull Request (PR) fixes the following issues
Fixes #613