Page MenuHomePhabricator

Flake8 for python files without extension in puppet repo
Open, NormalPublic

Description

I've found that we have a lot of Python files in our Puppet repository that are not checked by flake8 because they are not ending with the .py extension:

$ find . \( -path ./.git -o -path ./.tox \) -prune -o -type f ! -name "*.*" -exec head -n1 '{}' \; | sort | uniq -c | sort -n | grep python
   1 #! /usr/bin/env python
   1 #!/usr/bin/env /srv/sentry/bin/python
   1 #!/usr/bin/env python2
   1 #!/usr/bin/env python3
   4 #! /usr/bin/python3
   5 #! /usr/bin/python
  11 #!/usr/bin/python3
  32 #!/usr/bin/python
  41 #!/usr/bin/env python

TOTAL:
python:  80 files
python3: 16 files
python2:  1 file

Of those, 66 are currently not passing Flake8.

Here 2 different proposal on how we could fix this:

  1. Improve the Jenkins job to search for those (all the files without extension with a python shebang) and dynamically add them to the Flake8 run. Or, if there is a way, have tox call a script that does this.
    • PRO: generic fix, can be applied to any repo; doesn't require people to remember to add them manually
    • CON: need a cleanup of all the errors before enabling it
  2. Given the specific nature of the Puppet repo, add the .py extension to the file and change the reference in the Puppet classes so that the target file is generated without the extension as it is now but the source is taken from the .py one
    • PRO: allow to opt-in on a per-file basis
    • CON: applicable only to the Puppet repo, subject to people not adding other files without extension

When a single project has it's own repo or it's own sub-tox file it could add all the executable in a bin/ directory and explicitely add it to the flake8 parameters.
Alternatively in some coding styles it is suggested to have all Python code inside .py files and if an executable file is needed without the extension, a symlink or a bash wrapper should be added.

{V10}

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 29 2016, 11:15 AM

A bit of context for the CI part:

The Jenkins job operations-puppet-tox is pretty simple, it basically:

  • clone the repo
  • fetch and checkout the patch
  • run tox

̀tox` then look at tox.ini, set up all the virtualenv listed in envlist each having more specific commands in their [testenv:*] sub-sections. So for flake8 that installs flake8==2.5.5 and invokes flake8 with options from the [flake8] section.

flake8 traverse the file hierarchy and check files matching --filename=<patterns> which default solely to *.py.


So for 1 Fix the Jenkins job to search for those files and include them in the Flake8 run one would add each of the suffix less files to tox.ini flake8.filename so flake8 can find them. Would still need to opt-in each of the suffix less files :(

2 add .py extension to files kind of has my preference, that means having to change a lot of Puppet file {} stanza. Would still require people to remember to use a .py.


For standalone Python packages, setuptools has a feature known has console_scripts which define the name of a suffixless script to add and the corresponding package.method to invoke. When running setup.py install it will take care of installing the script which would just import whatever && whatever.method().

https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html#the-console-scripts-entry-point

Volans updated the task description. (Show Details)Aug 29 2016, 12:55 PM

So for 1 Fix the Jenkins job to search for those files and include them in the Flake8 run one would add each of the suffix less files to tox.ini flake8.filename so flake8 can find them. Would still need to opt-in each of the suffix less files :(

I didn't explain myself very well, I've updated the description of the task. I was suggesting to have Jenkins or tox include those files in the check dynamically.

2 add .py extension to files kind of has my preference, that means having to change a lot of Puppet file {} stanza. Would still require people to remember to use a .py.

Unfortunately yes, added as a CON in the description.

For standalone Python packages, setuptools has a feature known has console_scripts which define the name of a suffixless script to add and the corresponding package.method to invoke. When running setup.py install it will take care of installing the script which would just import whatever && whatever.method().
https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html#the-console-scripts-entry-point

Yes, my suggestion of symlinks or bash wrappers was for non dedicated repo/projects that have just single scripts or small stuff that doesn't have a setup.py.

Volans updated the task description. (Show Details)Aug 30 2016, 10:12 AM
Volans updated the task description. (Show Details)Aug 30 2016, 11:16 AM
hashar triaged this task as Normal priority.Sep 3 2016, 12:22 PM

@Volans maybe drive this again? I have voted for my preference, but really one way or the other is all fine to me :]

gfind . -type f ! -path '*/.git/*' -a ! -name '*.*' -exec file --mime {} +|grep x-python|cut -d\ -f1 shows there are 91 python files with out an extension :(

./files/jsbench/jsbench:
./files/misc/scripts/pcntl:
./modules/admin/files/home/ori/.binned/branchdir:
./modules/admin/files/home/ori/.binned/dist:
./modules/admin/files/home/ori/.binned/lcz:
./modules/admin/files/home/ori/.binned/py:
./modules/admin/files/home/ori/.binned/tg:
./modules/base/files/phaste:
./modules/cassandra/files/cassandra-ca-manager:
./modules/cdh/files/hadoop/check_hdfs_active_namenode:
./modules/coal/files/coal:
./modules/coal/files/coal-web:
./modules/deployment/files/git-deploy/utils/deploy-info:
./modules/deployment/files/git-deploy/utils/service-restart:
./modules/dnsrecursor/files/pdns_gmetric:
./modules/elasticsearch/files/es-tool:
./modules/etcd/files/etcd-manage:
./modules/grafana/files/grafana-dashboard:
./modules/grafana/files/grafana_create_anon_user:
./modules/graphite/files/archive-instances:
./modules/graphite/files/graphite-auth:
./modules/graphite/files/graphite-index:
./modules/keyholder/files/ssh-agent-proxy:
./modules/labstore/files/archive-project-volumes:
./modules/labstore/files/create-dbusers:
./modules/labstore/files/delete-dbuser:
./modules/labstore/files/monitor/check_drbd_cluster_ip:
./modules/labstore/files/monitor/check_drbd_role:
./modules/labstore/files/monitor/check_drbd_status:
./modules/labstore/files/monitor/check_nfs_status:
./modules/labstore/files/nfs-exportd:
./modules/labstore/files/nfs-manage-binds:
./modules/ldap/files/reset-ldap-password:
./modules/ldap/files/rewrite-group-for-memberof:
./modules/ldap/files/scripts/add-ldap-group:
./modules/ldap/files/scripts/ldaplist:
./modules/ldap/files/scripts/ssh-key-ldap-lookup:
./modules/letsencrypt/files/acme-setup:
./modules/mediawiki/files/hhvm/cleanup_cache:
./modules/mediawiki/files/mediawiki-firejail-convert:
./modules/mediawiki/files/mediawiki-firejail-ffmpeg:
./modules/mediawiki/files/mediawiki-firejail-ffmpeg2theora:
./modules/mediawiki/files/mediawiki-firejail-ghostscript:
./modules/mediawiki/files/mediawiki-firejail-rsvg-convert:
./modules/nagios_common/files/check_commands/check_graphite:
./modules/nagios_common/files/check_commands/check_graphite_freshness:
./modules/nrpe/files/plugins/check_systemd_unit_state:
./modules/ocg/files/nagios/check_ocg_health:
./modules/openstack/files/kilo/virtscripts/cold-migrate:
./modules/openstack/files/kilo/virtscripts/cold-nova-migrate:
./modules/openstack/files/kilo/virtscripts/live-migrate:
./modules/openstack/files/liberty/virtscripts/cold-migrate:
./modules/openstack/files/liberty/virtscripts/cold-nova-migrate:
./modules/openstack/files/liberty/virtscripts/live-migrate:
./modules/openstack/files/liberty/virtscripts/makedomain:
./modules/openstack/files/mitaka/virtscripts/cold-migrate:
./modules/openstack/files/mitaka/virtscripts/cold-nova-migrate:
./modules/openstack/files/mitaka/virtscripts/live-migrate:
./modules/openstack/files/mitaka/virtscripts/makedomain:
./modules/openstack/files/novastats/imagestats:
./modules/openstack/files/novastats/saltstats:
./modules/openstack/files/rabbitmq/rabbitmqadmin:
./modules/prometheus/files/usr/local/bin/prometheus-labs-targets:
./modules/puppetmaster/files/naggen2:
./modules/puppetmaster/files/prometheus-ganglia-gen:
./modules/puppetmaster/files/sshknowngen:
./modules/role/files/toollabs/clush/tools-clush-generator:
./modules/role/files/toollabs/clush/tools-clush-interpreter:
./modules/scap/files/mwgrep:
./modules/shinken/files/shinkengen:
./modules/sslcert/files/update-ocsp:
./modules/sslcert/files/x509-bundle:
./modules/swift/files/swift-account-stats:
./modules/swift/files/swift-container-stats:
./modules/swift/files/swift-dispersion-stats:
./modules/swift/files/swift-drive-audit:
./modules/toollabs/files/cdnjs-packages-gen:
./modules/toollabs/files/log-command-invocation:
./modules/toollabs/files/maintain-kubeusers:
./modules/toollabs/files/portgrabber:
./modules/toollabs/files/portreleaser:
./modules/toollabs/files/updatetools:
./modules/varnish/files/varnishmedia:
./modules/varnish/files/varnishreqstats:
./modules/varnish/files/varnishrls:
./modules/varnish/files/varnishstatsd:
./modules/varnish/files/varnishxcache:
./modules/varnish/files/varnishxcps:
./modules/xenon/files/xenon-grep:
./modules/xenon/files/xenon-log:
./utils/pcc:

@hashar thanks for the update, this is still on my radar, but I didn't had time to work on/drive it yet

After some discussion in https://gerrit.wikimedia.org/r/#/c/323559/ I've changed my vote to "automatically discover python files".

There is a mixture of conventions in python scripts inside puppet.git: underscore vs dash, extension vs no extension but having pep/flake tests is important even without explicit opt-in, the tests can be non-voting until the violations are fixed.
Re: naming, I think an obvious convention would be to keep the name in puppet the same as what's installed on the filesystem, especially for things expected to be in PATH.

Volans moved this task from Backlog to In Progress on the SRE-tools board.Jun 5 2017, 11:21 AM

Change 357197 had a related patch set uploaded (by Volans; owner: Volans):
[operations/puppet@production] Tox: find and check Python files without extension

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

Volans moved this task from In Progress to In Code Review on the SRE-tools board.Jun 5 2017, 11:41 AM
Joe added a subscriber: Joe.Jun 6 2017, 8:22 AM

After some discussion in https://gerrit.wikimedia.org/r/#/c/323559/ I've changed my vote to "automatically discover python files".
There is a mixture of conventions in python scripts inside puppet.git: underscore vs dash, extension vs no extension but having pep/flake tests is important even without explicit opt-in, the tests can be non-voting until the violations are fixed.
Re: naming, I think an obvious convention would be to keep the name in puppet the same as what's installed on the filesystem, especially for things expected to be in PATH.

Actually what most people do out of here is exactly the opposite: have the file on the puppet repo have an explicit extension to help with CI checkers (not just flake8, but rubocop, shellcheck, etc), and I second that choice.

It would make all of this so much simpler and faster, too.

Re: naming, I think an obvious convention would be to keep the name in puppet the same as what's installed on the filesystem, especially for things expected to be in PATH.

Actually what most people do out of here is exactly the opposite: have the file on the puppet repo have an explicit extension to help with CI checkers (not just flake8, but rubocop, shellcheck, etc), and I second that choice.
It would make all of this so much simpler and faster, too.

I'm not convinced the effort of converting all files with missing extensions is worth the gain of simplifying this, we'd also find a way to enforce files without an extension being committed or they'd lose linting/checking.
Also the first time we have to make an exception then either we miss checking/linting on those files or we're back to autodetection

Change 439563 had a related patch set uploaded (by Ema; owner: Ema):
[operations/puppet@production] reload-vcl: fix shell injection, add .py suffix

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

Change 439563 had a related patch set uploaded (by Ema; owner: Ema):
[operations/puppet@production] reload-vcl: fix shell injection, add .py suffix

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

Change 439563 merged by Ema:
[operations/puppet@production] reload-vcl: fix shell injection, add .py suffix

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

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

Change 498379 had a related patch set uploaded (by GTirloni; owner: GTirloni):
[operations/puppet@production] wmcs: Add .py extension to various scripts

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

Change 498379 abandoned by GTirloni:
wmcs: Add .py extension to various scripts

Reason:
Will submit separate changes.

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

Change 509426 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8: Add python extension so theses scripts can be picked up via CI

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

Change 509444 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8: update python file extensions so they are detected by CI

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

Change 509459 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8: Add python extension so theses scripts can be picked up via CI

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

Change 509467 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8: Add python extension so theses scripts can be picked up via CI

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

Change 509476 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8: Add python extension so theses scripts can be picked up via CI

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

Change 509484 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8: puppetmaster - Add python extension so theses scripts can be picked up via CI

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

Change 509459 merged by Jbond:
[operations/puppet@production] flake8: Add python extension so theses scripts can be picked up via CI

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

Change 509426 merged by Jbond:
[operations/puppet@production] flake8: Add python extension so theses scripts can be picked up via CI

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

Change 509484 merged by Jbond:
[operations/puppet@production] flake8: puppetmaster - Add python extension so scripts can be picked up via CI

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

Change 509875 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8: update python file extensions so they are detected by CI

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

Change 509885 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8: update python file extensions so they are detected by CI

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

Change 509909 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - mediawiki: update file extensions so they are detected by CI

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

Change 509917 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - rabbitmq: update python file extensions so they are detected by CI

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

Change 509921 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - varnish: update python file extensions so they are detected by CI

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

Change 509923 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - arclamp: update python file extensions so they are detected by CI

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

Change 509925 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - sslcert: update python file extensions so they are detected by CI

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

Change 509927 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - grafana: update python file extensions so they are detected by CI

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

Change 509936 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - mwgrep: update python file extensions so they are detected by CI

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

Change 509937 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - cassandra: update python file extensions so they are detected by CI

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

Change 509945 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - acme-setup: Add python extension so theses scripts can be picked up via CI

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

Change 509946 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - diffscan: Add python extension so CI is run

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

Change 509467 merged by Jbond:
[operations/puppet@production] flake8: Add python extension so theses scripts can be picked up via CI

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

Change 509885 merged by Jbond:
[operations/puppet@production] flake8: update python file extensions so they are detected by CI

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

Change 509909 merged by Jbond:
[operations/puppet@production] flake8 - mediawiki: update file extensions so they are detected by CI

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

Change 509925 merged by Jbond:
[operations/puppet@production] flake8 - sslcert: update python file extensions so they are detected by CI

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

Change 509875 merged by Jbond:
[operations/puppet@production] flake8: update python file extensions so they are detected by CI

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

Change 509936 merged by Jbond:
[operations/puppet@production] flake8 - mwgrep: update python file extensions so they are detected by CI

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

Change 509927 merged by Jbond:
[operations/puppet@production] flake8 - grafana: update python file extensions so they are detected by CI

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

Change 509937 merged by Jbond:
[operations/puppet@production] flake8 - cassandra: update python file extensions so they are detected by CI

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

Change 509923 merged by Jbond:
[operations/puppet@production] flake8 - arclamp: update python file extensions so they are detected by CI

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

Change 509917 merged by Jbond:
[operations/puppet@production] flake8 - rabbitmq: update python file extensions so they are detected by CI

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

Change 509921 merged by Jbond:
[operations/puppet@production] flake8 - varnish: update python file extensions so they are detected by CI

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

Change 509945 merged by Jbond:
[operations/puppet@production] flake8 - letsencrypt: Add python extension so CI is run

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

Change 509946 merged by Jbond:
[operations/puppet@production] flake8 - diffscan: Add python extension so CI is run

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

Change 510216 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] CI Tests: add a check to ensure all python files have a py extension

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

As i was on clinic duty last week i decided to have a crack at doing the manual work to rename everything in the repo so that they can be checked by CI. Thanks to all the reviewers i have mostly got that complete with only two changes still wanting review. I have also created a change to add a CI check to ensure python files have the correct extension. It would be good to get the latter committed asap to prevent more files from being added.

Once this is done i would be keen to explore the suggestion from hashar on T184435 to add additional checkers for the py3 files. if all are happy with that way forward and @hashar is happy to add the tox stuff, i will volunteer to do a similar rename for the py3 files

That is a nice sprint @jbond thank you. For the tox work I am not too worried and @Volans should be able to help as well (he knows more about tox than me).

@jbond thanks a lot for this sprint, really appreciated. I've commented on the CI patch.
Regarding @hashar proposal I'll comment on the other task, as I guess this one could be resolved as soon as this last 3 CRs are merged.

@jbond apart the mentioned in the CR .erb.py files and modules/mediawiki/files/mediawiki-firejail-ffmpeg I think only these few remains if I'm not mistaken:

utils/pcc
modules/openstack/files/mitaka/neutron/l3/original/router_info.original
modules/labstore/files/archive-project-volumes
modules/cdh/files/hadoop/check_hdfs_active_namenode

@Volans

utils/pcc

left this till last as im not sure how/where its called

modules/mediawiki/files/mediawiki-firejail-ffmpeg

This appears unused i propose we remove it (but as its unused should not be a problem to rename)

modules/openstack/files/mitaka/neutron/l3/original/router_info.original
modules/labstore/files/archive-project-volumes

not sure how i missed this will fix up now

modules/cdh/files/hadoop/check_hdfs_active_namenode

this one is in a submodule so shouldn't trigger CI

Change 510216 merged by Jbond:
[operations/puppet@production] CI Tests: add a check to ensure all python files have a py extension

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

Change 510465 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - misc: add py extension so CI can run

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

@Volans

utils/pcc

left this till last as im not sure how/where its called

This is just called manually by us on our local computers to run the puppet compiler. We can either rename and notify people of the change (people can set a local alias if needed) or rename and add a 2 line bash script without extension that calls the python :)

modules/mediawiki/files/mediawiki-firejail-ffmpeg

This appears unused i propose we remove it (but as its unused should not be a problem to rename)

Agree, I don't have much additional context on it. It was added by @MoritzMuehlenhoff on https://gerrit.wikimedia.org/r/c/operations/puppet/+/292563, so I guess ask him/serviceops for more info :)

modules/openstack/files/mitaka/neutron/l3/original/router_info.original
modules/labstore/files/archive-project-volumes

not sure how i missed this will fix up now

No prob, thanks!

modules/cdh/files/hadoop/check_hdfs_active_namenode

this one is in a submodule so shouldn't trigger CI

Right, it should in the submodule but I don't think that module has CI for python files at all from a quick look.

Change 510489 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] flake8 - pcc: add extension so file can be checked

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

have created changes for pcc and archive-project-volumes

@aborrero rejected the change to modules/openstack/files/mitaka/neutron/l3/original/router_info.original* (and i thought i had got away with no exception) Currently this file will not get detected by the CI check as it has no python shebang (however i consider that a bug) so changes wont get blocked. That said it would be good to get a permanent solution for this once one exception is excepted more will definitely creep in, possible options

  • store it somewhere else
  • rename and fix the style issues, after all the same style issues will need fixing in our copy
  • rename and add # flake8: noqa
  • leave as is, may break with future ci improvements
  • other options.

*modules/openstack/files/mitaka/neutron/l3/original/config.original should be treated the same as router_info.original

Change 510575 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] CI - python: update python type checking to use mime type

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

Change 510465 merged by Jbond:
[operations/puppet@production] flake8 - misc: add py extension so CI can run

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

@jbond those OpenStack .original files, I am assuming they are copied verbatim from the upstream project. So one would assume that we would never modify them and that they already have been validated. So I guess it is fine to miss them or eventually explicitly exclude them. We can surely afford a few holes here and there!

Change 510575 merged by Jbond:
[operations/puppet@production] CI - python: update python type checking to use mime type

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

Change 509444 merged by Jbond:
[operations/puppet@production] flake8: update python file extensions so they are detected by CI

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

Change 509476 merged by Jbond:
[operations/puppet@production] flake8: Add python extension so theses scripts can be picked up via CI

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

Change 510489 merged by Jbond:
[operations/puppet@production] flake8 - pcc: add extension so file can be checked

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