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

use pyenv cookbook instead of poise for python install/usage #350

Closed
wants to merge 14 commits into from
Closed

use pyenv cookbook instead of poise for python install/usage #350

wants to merge 14 commits into from

Conversation

jtschelling
Copy link

@jtschelling jtschelling commented May 29, 2020 •
edited
Loading

Description

remove poise_python usage (unmaintained) and replaces with sous-chef supported pyenv cookbook https://github.com/sous-chefs/pyenv

Issues Resolved

#349

Check List

@jtschelling jtschelling requested a review from a team May 29, 2020 17:37
jtschelling
jtschelling commented May 29, 2020
View reviewed changes
recipes/carbon.rb Outdated Show resolved Hide resolved
jtschelling
jtschelling commented May 29, 2020
View reviewed changes
resources/carbon_cache.rb
@@ -21,14 +21,10 @@
property :config, [Hash, nil], default: nil

action :create do
python_package backend_name do
backend_attributes.each { |attr, value| send(attr, value) }
Copy link
Author

@jtschelling jtschelling May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is some ruby/chef patterns that I don't really understand, please let me know what this is doing and how I can emulate this in my changes

Copy link
Member

Choose a reason for hiding this comment

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

This is “just” collecting all the attributes and sending them though. It’s a little terse, and hard to read. So I’m with you here and write something easier to read.

Copy link
Author

Choose a reason for hiding this comment

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

so its another way to set the properties here? if install_options was set in the backend_attributes would it overwrite the --no-binary=:all: property below this? keeping that in mind for how I want to fix this

jtschelling
jtschelling commented May 29, 2020
View reviewed changes
metadata.rb Outdated Show resolved Hide resolved
jtschelling
jtschelling commented May 29, 2020
View reviewed changes
attributes/uwsgi.rb
@@ -25,5 +25,4 @@
default['graphite']['uwsgi']['carbon'] = '127.0.0.1:2003'
default['graphite']['uwsgi']['listen_http'] = false
default['graphite']['uwsgi']['port'] = 8080
default['graphite']['uwsgi']['service_type'] = 'runit'
Copy link
Author

@jtschelling jtschelling May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

this is a cleanup as i was thinking about the systemd portions of the cookbook, its not used anywhere. should i remove the change?

@jtschelling
Copy link
Author

jtschelling commented May 29, 2020 •
edited
Loading

CI is failing, was there a known update to the git resource's :sync action in chef 16? i'm not familiar with the updates there, I only tested this on 14 & 15. solved the problem in the pyenv cookbook sous-chefs/pyenv#72

damacus
damacus reviewed May 30, 2020
View reviewed changes
attributes/default.rb Outdated Show resolved Hide resolved
damacus
damacus reviewed May 30, 2020
View reviewed changes
metadata.rb Show resolved Hide resolved
damacus
damacus reviewed May 30, 2020
View reviewed changes
recipes/_web_packages.rb
@@ -19,30 +19,28 @@

package Array(node['graphite']['system_packages'])

python_package 'django' do
pyenv_pip 'django' do
virtualenv node['graphite']['base_dir']
Copy link
Member

Choose a reason for hiding this comment

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

If we have a library helper we should use that in the resource instead of using node attributes.

Copy link
Author

Choose a reason for hiding this comment

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

I'm kind of unclear on what you're asking, are you saying setting the virtualenv as a library helper and then remove setting all the virtualenv properties when using the pyenv_pip resources? Or are you saying using use a library helper to get the node attribute?

@damacus
Copy link
Member

damacus commented May 30, 2020

If you can get the tests passing, I think this is almost good to go. The next stage would probably be to remove all the recipes from this cookbook!

@jtschelling
Copy link
Author

working on testing now

@xorima xorima requested a review from damacus June 18, 2020 10:55
@xorima xorima added Feature Request Enhancement to existing functionality or new functionality In Progress Maintenance Maintenance issues such as failing builds Tech Debt Will improve the maintainability of the codebase labels Jun 18, 2020
tas50
tas50 reviewed Jul 23, 2020
View reviewed changes
kitchen.yml Show resolved Hide resolved
tas50
tas50 reviewed Jul 23, 2020
View reviewed changes
metadata.rb Outdated Show resolved Hide resolved
tas50
tas50 reviewed Jul 23, 2020
View reviewed changes
resources/python.rb Outdated
default_action :install

property :pyenv_name, String, name_property: true
property :python_version, String, default: '2.7.17'
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're releasing a major version can we go to Python 3 + the latest graphite

Copy link
Contributor

Choose a reason for hiding this comment

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

@jtschelling Can you please address this?

Copy link
Author

Choose a reason for hiding this comment

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

sorry, stuck on getting python 3.8.5 working here and this unfortunately dropped in priority a little for me. troubleshooting it when I can

tas50
tas50 reviewed Jul 23, 2020
View reviewed changes
resources/python.rb Outdated Show resolved Hide resolved
tas50
tas50 reviewed Jul 23, 2020
View reviewed changes
.github/workflows/ci.yml Outdated Show resolved Hide resolved
tas50
tas50 reviewed Jul 23, 2020
View reviewed changes
kitchen.yml Outdated Show resolved Hide resolved
@kitchen-porter
Copy link
Contributor

1 Error
🚫 Please include a CHANGELOG entry.

Generated by 🚫 Danger

@ramereth
Copy link
Contributor

ramereth commented Aug 5, 2020

@jtschelling can you please fix the converge errors that seem to happening in the tests?

@ramereth ramereth mentioned this pull request Sep 3, 2020
Merged
@ramereth ramereth added the Waiting on Contributor Awaiting on the person who raised this to update label Oct 3, 2020
@ramereth ramereth linked an issue Oct 3, 2020 that may be closed by this pull request
remove unmaintained poise_python dependency #349
Open
@swalberg
Copy link

swalberg commented Oct 8, 2020

Is anyone actively working on this now? I've been updating our wrapper cookbooks and realized we're running up against the same thing. I tried running the tests but am getting a variety of errors depending on the platform. Before I dive too deep, thought I'd check that I'm not duplicating someone's effort.

@swalberg
Copy link

swalberg commented Oct 8, 2020

Played with the tests a bit, some remarks largely so I don't forget:

  • Ubuntu-2004 won't converge because a package (python-rrdtool) doesn't exist
  • Ubuntu-1804 converges but won't pass one of the tests, the root cause seems to be that the config files are being written out empty and that test is the one that looks for non-default behaviour. It also seems that run_context.resource_collection is empty so no config is available, even though the test looks like it specifies the right config.
  • Centos-7 won't converge because the test tries to install a version of Django that requires a newer version of sqlite than comes with centos-7. The cookbook lets us set the version in the attributes, but it's also set in the single_node.rb fixture to something incompatible.

@jtschelling
Copy link
Author

@swalberg I fell apart here trying to get python 3.8 working (couldn't get graphite to start), and never got back to working on it. Honestly its probably best to open a new PR for this after reading through and choosing some of my changes to include, it really got messy by the end here. I encountered the same problems that you had, in particular the run_context.resource_collection not populating and the centos 7 build issue. Unfortunately I can't really get back to working on this at the moment so you won't collide with anything I'm doing if you pick up work on this

@ramereth
Copy link
Contributor

ramereth commented Oct 8, 2020

@swalberg feel free to open a new PR since @jtschelling isn't going to get to this again soon. We can close this one and refer a new one. Thanks for taking a look!

@docwho76
Copy link

docwho76 commented Mar 2, 2021

What is needed to get this over the line and merged?

@ramereth
Copy link
Contributor

ramereth commented Mar 2, 2021

@docwho76 Someone will need to make a new PR based on this and resolve the conflicts and address any new issues that crop up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tas50 tas50 tas50 left review comments

@ramereth ramereth ramereth left review comments

@damacus damacus Awaiting requested review from damacus

Assignees
No one assigned
Labels
Feature Request Enhancement to existing functionality or new functionality In Progress Maintenance Maintenance issues such as failing builds Tech Debt Will improve the maintainability of the codebase Waiting on Contributor Awaiting on the person who raised this to update
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

remove unmaintained poise_python dependency
8 participants
@jtschelling @damacus @kitchen-porter @ramereth @swalberg @docwho76 @tas50 @xorima