Page MenuHomePhabricator

Puppet tox: properly lint both Py2 and Py3 files
Open, NormalPublic

Description

The Puppet repository contains both Python2 and Python3 files, both with and without .py extension (see T144169).
The CI runs tox to lint them, but at the moment flake8 is run with Python2 and lints all the files with Python2 flake8, failing on Py3 files.

We should find a solution that allows to properly lint each file according to its shebang (when present) or something else (a specially crafted comment?) when the shebang is not present (or enforce all files to have a shebang).

We could use the approach suggested when discussing about T144169, to have CI pass the list of modified files to a custom script that will select those that are Python (by extension, shebang and other detection ways) and have it properly lint them with the right Python2/3 version accordingly.

Thoughts?

Event Timeline

Volans created this task.Jan 8 2018, 2:19 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 8 2018, 2:19 PM
Volans triaged this task as Normal priority.Jan 8 2018, 2:19 PM

In your tox.ini, you can do something like

[testenv:flake8]
commands = flake8
deps = flake8
basepython = python3

to use Python 3 for flake8. Of course, that forces everything to Python 3 (which isn't a terrible thing long term tbh).

We could use the approach suggested when discussing about T144169, to have CI pass the list of modified files to a custom script that will select those that are Python (by extension, shebang and other detection ways) and have it properly lint them with the right Python2/3 version accordingly.

This sounds like a good idea.

238482n375 set Security to Software security bug.Jun 15 2018, 8:06 AM
238482n375 added a project: Security.
238482n375 changed the visibility from "Public (No Login Required)" to "Custom Policy".
238482n375 added a subscriber: 238482n375.
This comment was removed by Volans.
Volans changed the visibility from "Custom Policy" to "Public (No Login Required)".Jun 15 2018, 8:30 AM
Volans removed a project: Security.
Volans removed a subscriber: 238482n375.
Restricted Application added a project: Security. · View Herald TranscriptJun 15 2018, 8:31 AM

Change 447793 had a related patch set uploaded (by Filippo Giunchedi; owner: Filippo Giunchedi):
[operations/puppet@production] cumin: ignore flake8 syntax until pep8 uses python 3

https://gerrit.wikimedia.org/r/447793

Change 447793 merged by Filippo Giunchedi:
[operations/puppet@production] cumin: ignore flake8 syntax until pep8 uses python 3

https://gerrit.wikimedia.org/r/447793

Bstorm added a subscriber: Bstorm.Nov 8 2018, 7:04 PM
GTirloni removed a subscriber: GTirloni.Mar 21 2019, 9:11 PM

Given that py2 EOL is at the end of 2019, I'm not sure it's worth to spend our energies to make this check smart at this point.
An alternative proposal could at some point to migrate CI to test our Python files with Python 3.4 (jessie), 3.5 (stretch) and 3.7 (buster) [optionally 3.6 too but seems unnecessary in our current infrastructure].

So, given that we pass the modified files to tox IIRC, only when touching a python file the test might fail, reminding us that we need to migrate that script to Py3. If it doesn't fail on a py2 file it just means that is not using any py2-only syntax, although that doesn't mean that it will work as is on Python3 .

From a super quick test (don't take it too seriously ;) ) running on the whole repo with Python 3.4 we have 101 files for which flake8 (3.7.7) report issues.

Also it seems that at least some files under modules/openstack/files/mitaka/admin_scripts are not our own and should probably be excluded in tox.ini. (cc @Bstorm could you check/confirm? )

I think we could do some test of the real impact of migrating to a py3 flake8. Thoughts?

+1 for the migration. Assuming it is easily reversible, we can even try to change it and see what happens, revert if it creates some issues.

This would allow me to having to change python3 scripts so they are parsable by python2 (and not using its advanced features).

Bstorm added a subscriber: Andrew.Mar 25 2019, 3:06 PM

Also it seems that at least some files under modules/openstack/files/mitaka/admin_scripts are not our own and should probably be excluded in tox.ini. (cc @Bstorm could you check/confirm? )

That's mostly scripts written or at least maintained by wmcs. One or two are mostly just copied there, but even then, I see at least one formatting edit one case. They aren't specifically from the OpenStack project, so it seems like we should be responsible for keeping them good and sensibly formatted (and may very well need to update them ourselves in the future when we upgrade things). I don't think I'd want to exclude even the ones that are copied in because of that (presuming all are still in use, right @Andrew?)

I think we could do some test of the real impact of migrating to a py3 flake8. Thoughts?

+1 on pushing for py3 and abandoning py2 as supported

hashar removed a subscriber: hashar.

T144169 is/was to detect extension less files that might be python, though that never has been worked on, potentially it could lead to a way to dispatch files to python2 or python3 based linters.

We can not mix python 2 and python 3 in the same virtual environment (to the best of my knowledge).

A possible migration path would be:

  • consider .py files to be python2 for now.
  • have python 3 files with the extension .py3
  • introduce a new virtualenv that is python3 based and only check .py3 files
  • migrate files from .py to .py3

Once the grand migration has been completed, the python 2 based linter can be removed and all files renamed from .py3 to .py2.

The transient python3 based linter would look like:

tox.ini
[testenv:lint-py3]
basepython = python3
deps = flake8=3.3.0
commands = flake8 {posargs}

Or someone tackles T144169 to have a system to determinate whether a file has python and which version, then route it to the proper linter.

I'm not convinced this is a good solution for few reasons:

  • quite some overhead to rename all existing py3 files to .py3
  • quite some overhead to rename back those to .py once the migration is completed
  • this method doesn't work if we have any place in which we deploy a set of files in a directory with the puppet sync directory feature (and I'm afraid we do, although I didn't check as of now)

Ideally in Puppet only single files should reside and those have a shebang and we can auto-detect which flake8 to run based on that.
Anything that is more than one file and has inclusions is a good candidate to get moved to its own repo and deploy procedures.
For the few exceptions that don't fit in either of those two cases, we could simply add the shebang or a special comment in the first line to declare it, as a temporary measure, and then remove them when the migration is completed.
This overhead should be much smaller IMHO and should not have any side effect with the integration with puppet.

jbond added a subscriber: jbond.May 15 2019, 4:58 PM