search

kubernetes-client / python

Official Python client library for kubernetes
http://kubernetes.io/
Apache License 2.0
6.73k stars 3.27k forks source link

add pylint to tox #52

Closed mbohlool closed 5 years ago

mbohlool commented 7 years ago

following up on @marun's comment on #50:

Consider adding a linter check (e.g. flake8/pylint) to the tox configuration to catch these issues automatically.

SEJeff commented 7 years ago

@mbohlool I'm happy taking this on and have started doing it, however I was curious about all of the autogenerated code. Since much of this is autogenerated, how do we want to fix this? I'd rather not waste time on a bunch of fixup commits that are then blown away the next time things are stepped after an autogeneration is complete.

One approach is for me to fix swagger's codegen for python which is doable, but I really was never a java fan and don't relish the thought of installing and dealing with a JDK.

SEJeff commented 7 years ago

I'd also like to fix the requirements and test-requirements to look like every other file out there and not include spaces (ie: the sorted output from pip freeze more or less, but with the >=, etc version specifiers).

mbohlool commented 7 years ago

Manual changes to generated code is out of question as you argued. If there is a way to automatically fix some of those problems (look at scripts/update-pep8.sh), we should do it (and we can repeat it after each code generation) otherwise we should just ignore kubernetes.client package in the pylint checks.

marun commented 7 years ago

I'd recommend flake8 over pylint. And it's entirely possible to exclude files and paths from linting. I'm assuming generated code is cleanly separated from non-generated code? Here's an example of configuring flake8 in tox.ini:

https://github.com/openstack/neutron/blob/master/tox.ini#L132

SEJeff commented 7 years ago

@marun Yeah you just need a [flake8] section in the tox config, it isn't hard. I just wanted to flesh out the approach here with approval before spending time doing it. I'm going to implement it in this branch.

@mbohlool funnily enough, I've got some mad sed skills (a career mostly as a sysadmin will do that for you) and was 1/2 tempted to write a script to remove some of the obvious silliness in that autogenerated code. I'll have a go at that first and we can see if it is sensible. Also, do you have thoughts on adding coverage? I'd like to see coverage stats added if possible to this when we run tests. Seeing all of the noop tests that don't do anything does actually make me a bit sad.

I'd planned on doing it just like we do in home assistant, another OSS project I contribute to.

marun commented 7 years ago

I see value in automating linting of non-generated code, but I don't see the point of doing the same for generated code. If linting reveals functional problems with the generated code, the place to fix it is in the generator. Scripted cleanup can't help but be brittle in the face of generator changes.

mbohlool commented 7 years ago

Agreed with @marun, no value in linting generated code. I also sent a PR for coverage of only non-generated code #54

SEJeff commented 7 years ago

@mbohlool Cool, I can update my branch with those changes (excluding tests and kubernetes/client) and send it up tonight or tomorrow. I also added the py35 environment since my workstation distro, Fedora 25, lacks python 3.4, but default installs 3.5

mbohlool commented 7 years ago

@SEJeff use pyenv to create as much environment as you like and activate 3.4 and 2.7 for this repo:

pip install pyenv
pyenv install 2.7.12
pyenv install 3.4.3
cd client-python
pyenv local 2.7.12 3.4.3

You may need to restart your shell for it to take effect but tox should pick up these two newly installed version. I've never tested the whole client with 3.5 yet.

SEJeff commented 7 years ago

Feel free to give this a go via:

git pull git@github.com:SEJeff/kubernetes-client-python.git update-tox-config

And to run the flake8 task:

TOXENV=lint tox

I'm not sure of the best way to silence the errors in kubernetes.init, kubernetes.watch.init, and kubernetes.config.init. The most obvious thing is noqa: F401 on each line with the imports. Normally you could do from foo import bar and control the public bits with __all__, but that doesn't seemingly do the right thing with absolute imports + flake8.

So the choices I see are:

  1. Disable F401 (unused import) on those lines and clutter up a few __init__.py files
  2. Exclude the offending __init__.py files from flake8 entirely :neutral_face:
  3. Disabling F401 globally :-1:

I'm certainly not in love with 1, but it seems like the least evil. By design, there is no way to exclude specific errors globally from a specific file in flake8.

What do you prefer?

Once this is done, I'll enable it by default and I can open a PR for it.

mbohlool commented 7 years ago

init files suppose to be all imports (at least for us), I think (2) is better, just ignore init files.

mbohlool commented 7 years ago

Look like we have a 4th option:

  1. Adding all in init files, for example for kubernetes/config/init.py:
__all__ = ["ConfigException", "load_incluster_config",
           "list_kube_config_contexts", "load_kube_config"]

I think this 4th option is sightly better than 2nd one.

dims commented 5 years ago

May be we should add the pep8/flake8 to the python-base

iamneha commented 5 years ago

/assign @iamneha

k8s-ci-robot commented 5 years ago

@iamneha: GitHub didn't allow me to assign the following users: iamneha.

Note that only kubernetes-client members and repo collaborators can be assigned. For more information please see the contributor guide

In response to [this](https://github.com/kubernetes-client/python/issues/52#issuecomment-443797082): >/assign @iamneha Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
fejta-bot commented 5 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

fejta-bot commented 5 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle rotten

fejta-bot commented 5 years ago

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /close

k8s-ci-robot commented 5 years ago

@fejta-bot: Closing this issue.

In response to [this](https://github.com/kubernetes-client/python/issues/52#issuecomment-505794835): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen`. >Mark the issue as fresh with `/remove-lifecycle rotten`. > >Send feedback to sig-testing, kubernetes/test-infra and/or [fejta](https://github.com/fejta). >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.