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.

Sign up for GitHub

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

Jump to bottom

If user declares their requirements.txt in Puppet, don't skip pip installation in python::requirements #619

Merged
merged 11 commits into from
Sep 20, 2023
Merged

If user declares their requirements.txt in Puppet, don't skip pip installation in python::requirements #619

merged 11 commits into from
Sep 20, 2023

Conversation

acullenn
Copy link
Contributor

@acullenn acullenn commented Aug 19, 2021 •
edited by kenyon
Loading

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

     file { "${project_dir}/requirements.txt":
        ensure  => present,
        content => file("${module_name}/requirements.txt")
      }
    -> python::pyvenv { "${project_dir}/pyenv":
      ensure  => present,
      version => '3.6'
      }
    -> python::requirements {"${project_dir}/requirements.txt":
        virtualenv => "${project_dir}/pyenv",
      }

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 which requirements.txt must exist or be declared in advance of invoking the class:
https://forge.puppet.com/modules/puppet/python/reference#pythonrequirements

python::requirements { '/var/www/project1/requirements.txt' :
  virtualenv => '/var/www/project1',
  proxy      => 'http://proxy.domain.com:3128',
  owner      => 'appuser',
  group      => 'apps',
}

Changes

  • Always set the local subscription in python::requirements, which is used to trigger the pip installation.

This Pull Request (PR) fixes the following issues

Fixes #613

@smortex
Copy link
Member

smortex commented Aug 20, 2021

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?

@acullenn
Add declared requirements install test (#1)
f962dc9 
Add unit test for verifying requirements installation to declared venv
@acullenn
Copy link
Contributor Author

I've added a unit test for this issue, and confirmed that it's failing when my fix is not applied:(acullenn#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?

@siebrand siebrand mentioned this pull request Oct 13, 2021
Closed
@vox-pupuli-tasks
Copy link

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

@saz
Copy link
Sponsor Contributor

saz commented Jul 25, 2022

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

@siebrand
Copy link

Is there any way to get this patch going again?

@zheng460
Copy link

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?

bastelfreak
bastelfreak reviewed Sep 12, 2023
View reviewed changes
manifests/install.pp Outdated Show resolved Hide resolved
@bastelfreak bastelfreak merged commit 9dcd35b into voxpupuli:master Sep 20, 2023
17 checks passed
@bastelfreak bastelfreak added bug Something isn't working and removed merge-conflicts labels Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zipkid zipkid zipkid left review comments

@bastelfreak bastelfreak bastelfreak approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Requirements not updated for pyvenv -- requires forceupdate
7 participants
@acullenn @smortex @saz @siebrand @zheng460 @zipkid @bastelfreak