search

kubernetes / test-infra

Test infrastructure for the Kubernetes project.
Apache License 2.0
3.83k stars 2.64k forks source link

Add a new command to find Python syntax errors and undefined names #8676

Closed cclauss closed 5 years ago

cclauss commented 6 years ago

It would be super cool if we could teach @k8s-ci-robot a new command, /flake8-core which would execute flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics which is a really nice sanity check of Python code.

And maybe another new command /flake8-strict which would execute flake8 . --count --show-source --statistics

E901,E999,F821,F822,F823 are the "showstopper" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc. The other flake8 issues are merely "style violations" -- useful for readability but they do not effect runtime safety.

BenTheElder commented 6 years ago

Normally how we'd do this is run flake8 (or any other linter) the same as one of the test jobs / Presubmit checks on PRs. Set up a job then you can /test pull-foo-flake8

I'm not sure this warrants it's own command. All of the tooling necessary for running any other test should work well for linters.

On Fri, Jul 13, 2018, 00:13 cclauss notifications@github.com wrote:

It would be super cool if we could teach @k8s-ci-robot https://github.com/k8s-ci-robot a new command, /flake8-core which would execute flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics which is a really nice sanity check of Python code.

And maybe another new command /flake8-strict which would execute flake8 . --count --show-source --statistics

E901,E999,F821,F822,F823 are the "showstopper" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc. The other flake8 issues are merely "style violations" -- useful for readability but they do not effect runtime safety.

  • F821: undefined name name
  • F822: undefined name name in all
  • F823: local variable name referenced before assignment
  • E901: SyntaxError or IndentationError
  • E999: SyntaxError -- failed to compile a file into an Abstract Syntax Tree

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/kubernetes/test-infra/issues/8676, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4Bq4Xv4ovqh9foV2rjz5Wlj8BpDpKoks5uGEh8gaJpZM4VOaPf .

cclauss commented 6 years ago

Do we really need to have users manually set up a job?

Doesn't @k8s-ci-robot already know everything that it needs to know to automatically set up a lint job given that the commands in question do not change from one repo to the next?

I discovered this need by going to https://github.com/tensorflow/minigo and wanting to run a test that the maintainers had not previously considered. I am not a maintainer so I had perform a number of manual steps on my local machine and then encourage the maintainers to repeat those same steps in their repo so they could see what I was seeing.

In a ployglot repo the maintainers might be focused on language A but they might have a showstopper bug in a single script written in language B. Instead of asking them to change their Travis/Circle/Jenkins file to rediscover the bug that I am seeing, I could merely encourage them to /lint-language-B

In the case of /flake8-core, @k8s-ci-robot would merely need to:

  1. Make a container with the repo inside
  2. Run pip install flake8
  3. Run flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics
  4. Pipe the output into a PR comment

This does not change from on repo to the next and I believe that this approach would work consistently across almost all language linters.

cclauss commented 6 years ago

The current /lint command runs golint on all modified files in a PR. That would be useful to have for other languages.

BenTheElder commented 6 years ago

Do we really need to have users manually set up a job?

Yes, we should focus on making that easier..

In the case of /flake8-core, @k8s-ci-robot would merely need to:

Everything except step 4) is a what a normal job might do. 4 can still be accomplished by a tool that runs in a job.

We have excellent capacity for scaling execution within test jobs, and a lint test being run is not different from any other test. There are also security concerns to running linter scripts within Prow instead of in the build & test clusters.

I think the answer here is for someone to write a reusable flake8 script, optionally with commenting support. If it's only posting a comment with the output though, I don't think that's more helpful than the output of any other job, which we also have better support for and can handle longer output better.

BenTheElder commented 6 years ago

The current /lint command runs golint on all modified files in a PR. That would be useful to have for other languages.

This isn't widely used. The reason we have this built into Prow is:

BenTheElder commented 6 years ago

The /lint command in fact only runs on the modified code. It doesn't lint the whole tree, which is something else golint as a go librarary gives us, and is why we don't have say, gometalinter instead.

BenTheElder commented 6 years ago

The other problem we've avoided so far just by not upgrading the library and by not having much usage of /lint is that if we have a global linter baked in upgrading the underlying linter is a breaking change across all the repos under that CI. Upgrades to tools like this really should be rolled out per-repo.

The lint-as-presubmit model has worked well there, and as a bonus it allows making the lint script / lint failures a blocking failure for PR merges, which is typically how we see them used.

cclauss commented 6 years ago

Thanks... I will study up over the weekend.

fejta-bot commented 6 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/test-infra/issues/8676#issuecomment-445784991): >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.