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

Drop Python 2, implement Python 3, replace virtualenv with native python 3 implementation, drop EPEL parameter #292

Merged
merged 7 commits into from
Oct 5, 2020

Conversation

waipeng
Copy link
Contributor

@waipeng waipeng commented Oct 1, 2020

Pull Request (PR) description

This PR attempts to fix tests that have been failing and blocking all PR from being merged.

To support python3, we have to introduce a few new variables to support
this creation of a py3 virtualenv, and also to set wsgi python-home 1
it. Just using activate_this.py didn't work.

In test, we also choose to use a separate virtualenv
virtenv3-puppetboard because `virtenv-puppetboard' will be created as
a py2 virtualenv by the other tests.

This Pull Request (PR) fixes the following issues

Fixes #287

manifests/init.pp Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
@bastelfreak bastelfreak added bug Something isn't working needs-work not ready to merge just yet labels Oct 1, 2020
@bastelfreak
Copy link
Member

thanks for all the work! I already fixed all the linter fails in another PR: #288

I'm not sure what's the best way to bring both PRs together. Do you want to cherry-pick my commits into your branch? Otherwise I can cherry-pick your changes into mine as soon as the acceptance tests pass.

@waipeng
Copy link
Contributor Author

waipeng commented Oct 1, 2020

Thanks for the review!

Currently I'm unable to get the CentOS tests passing. This is due to this fix installs puppetdb so that puppetboard can talk to it. Without a working puppetdb, puppetboard throws a 500 errors. You will notice most of the other tests don't test for connection, maybe this is the case.

On a dockerized CentOS I am unable to get systemd to work properly to start puppetdb properly, it errors out with

systemd[1]: New main PID <PID> does not belong to service, and PID file is not owned by root. Refusing.

There are two choices:

  1. Continue trying fix CentOS and docker and systemd, or wait for someone to jump in and help
  2. Change the test to not test for connection, but test for correctness in the generated configs (not as good)

Do you have any ideas?

@bastelfreak
Copy link
Member

in the past we had strange issues with centos and systemd in docker. using an older version of the container fixed that. Can you use centos7-64{image=centos:7.6.1810}' instead of centos7-64 in the .travis.yml and check if the tests pass afterwards?

@bastelfreak
Copy link
Member

crosses fingers for the centos tests

@waipeng
Copy link
Contributor Author

waipeng commented Oct 1, 2020

thanks for all the work! I already fixed all the linter fails in another PR: #288

I noticed the modulesync change removed BEAKER_HYPERVISOR=docker so I'm going to try to see if that passes.

@bastelfreak
Copy link
Member

you can probably let the linter fix the new violations with bundle exec rake lint_fix

@bastelfreak
Copy link
Member

I noticed the modulesync change removed BEAKER_HYPERVISOR=docker so I'm going to try to see if that passes.

yes, that's now set by a gem we require, voxpupuli-acceptance: https://github.com/voxpupuli/voxpupuli-acceptance/blob/master/lib/voxpupuli/acceptance/spec_helper_acceptance.rb#L5

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I opened #293 to convert to voxpupuli-acceptance. Entirely untested, but that's probably a good base. If that works, I'd suggest to rebase on that. Also keep the PR focused and not include a full modulesync for easier review.

manifests/apache/vhost.pp Outdated Show resolved Hide resolved
@waipeng
Copy link
Contributor Author

waipeng commented Oct 1, 2020

looks like both ubuntu1604-64 and centos7-64{image=centos:7.6.1810} have problems starting services via systemd. Any ideas how to fix them?

@bastelfreak
Copy link
Member

I will rebase this on our master

@bastelfreak
Copy link
Member

crosses fingers

@bastelfreak
Copy link
Member

Hi. i did a lot of debugging to figure out why the virtualenv command isnt found during the tests. My internet just died and I will continue tomorrow

@waipeng
Copy link
Contributor Author

waipeng commented Oct 2, 2020

So I've debugged a few different docker images

  • CentOS 7 - systemd 219 - Fail at starting puppetdb
  • Ubuntu 1604 - systemd 229 - Fail at starting postgresql
  • Ubuntu 1804 - systemd 237 - Pass
  • Ubuntu 2004 - systemd 245 - Pass starting puppetdb, but fail other tests (needs work)

I think at this point in time we are debugging systemd/docker issues, not puppetboard/puppet-puppetboard issues.

I'm not sure what is the best way forward. Hoping someone with systemd and docker experience can help?

@ekohl
Copy link
Member

ekohl commented Oct 2, 2020

I would start with (at least locally) testing with vagrant_libvirt. Real VMs at least allow you to exclude failures due to Docker. There's moby/moby#38749 which causes a lot of these issues. Sadly, it's pretty much the only CI we have. I have thought about using podman (which is supposed to be a lot more friendly to systemd inside container) but haven't gotten around to playing with that.

@bastelfreak
Copy link
Member

I currently use the following command to test on centos7 with virtualbox/vagrant:

PUPPET_INSTALL_TYPE=agent BEAKER_destroy=no BEAKER_debug=true BEAKER_PUPPET_COLLECTION=puppet6 BEAKER_setfile=centos7-64{hypervisor=vagrant} BEAKER_TESTMODE=apply bundle exec rake beaker

this currently fails with:

       	Error: Could not find command 'virtualenv'

which should be virtualenv-3. I assume this is a bug in the puppet-python module. I'm currently debugging this and also considering, at least for testing, to switch to pyvenv instead. We already use that in two other modules with python3 on centos 7 so we know it's working.

@vchepkov
Copy link

vchepkov commented Oct 2, 2020

virtualenv should be retired, since python3.6 it's no longer needed

README.md Outdated Show resolved Hide resolved
manifests/apache/vhost.pp Outdated Show resolved Hide resolved
spec/acceptance/class_spec.rb Outdated Show resolved Hide resolved
templates/wsgi.py.epp Outdated Show resolved Hide resolved
waipeng and others added 7 commits October 5, 2020 13:09
To support python3, we have to introduce a few new variables to support
this creation of a py3 virtualenv, and also to set wsgi python-home [1]
it. Just using activate_this.py didn't work.

In test, we also choose to use a separate virtualenv
`virtenv3-puppetboard` because `virtenv-puppetboard' will be created as
a py2 virtualenv by the other tests.

[1]: https://modwsgi.readthedocs.io/en/develop/user-guides/virtual-environments.html
Puppetboard requires Python 3.6 or newer to run. CentOS 7 has that, but
without a wsgi module to setup apache. CentOS 8 support isn't yet
implemented in voxpupuli/python so we cannot support this version here.
We've no working acceptance tests and no suitable data in params.pp for
SLES.
README.md Show resolved Hide resolved
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I would change the title of the PR for the changelog. It ended up also improving docs, add a parameter for daemon opts, use python::pyvenv.

@bastelfreak bastelfreak changed the title Fix up tests for python3 Drop Python 2, implement Python 3, replace virtualenv with native python 3 implementation Oct 5, 2020
@bastelfreak bastelfreak changed the title Drop Python 2, implement Python 3, replace virtualenv with native python 3 implementation Drop Python 2, implement Python 3, replace virtualenv with native python 3 implementation, drop EPEL parameter Oct 5, 2020
@bastelfreak bastelfreak merged commit c9267e7 into voxpupuli:master Oct 5, 2020
@waipeng
Copy link
Contributor Author

waipeng commented Oct 5, 2020

Thanks @bastelfreak, @ekohl and all for your help!

Can we have a version released for this please? We are keen to use it.

@waipeng waipeng deleted the fix_test_py3 branch October 5, 2020 22:16
@bastelfreak
Copy link
Member

i want to get #302 in before we do a release

kenyon added a commit to kenyon/puppet-puppetboard that referenced this pull request Dec 18, 2023
Suse support was dropped from metadata.json in voxpupuli#292 commit
77ea67b due to lack of working tests.
@kenyon kenyon mentioned this pull request Dec 18, 2023
kenyon added a commit to kenyon/puppet-puppetboard that referenced this pull request Dec 19, 2023
Suse support was dropped from metadata.json in voxpupuli#292 commit
77ea67b due to lack of working tests.
kenyon added a commit to kenyon/puppet-puppetboard that referenced this pull request Jan 7, 2024
Suse support was dropped from metadata.json in voxpupuli#292 commit
77ea67b due to lack of working tests.
kenyon added a commit to kenyon/puppet-puppetboard that referenced this pull request Apr 4, 2024
Suse support was dropped from metadata.json in voxpupuli#292 commit
77ea67b due to lack of working tests.
kenyon added a commit to kenyon/puppet-puppetboard that referenced this pull request Apr 16, 2024
Suse support was dropped from metadata.json in voxpupuli#292 commit
77ea67b due to lack of working tests.
h0tw1r3 pushed a commit to kenyon/puppet-puppetboard that referenced this pull request May 5, 2024
Suse support was dropped from metadata.json in voxpupuli#292 commit
77ea67b due to lack of working tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lateset on ubuntu 18.04
6 participants