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

Use flake8 for sanity testing #22820

Closed
wants to merge 1 commit into from

Conversation

willthames
Copy link
Contributor

SUMMARY

flake8 catches more errors than pep8, so will result in fewer bugs making the codebase

Would have caught the error introduced in #20214 addressed by #22818

ERROR: Found 1 pep8 issue(s) which need to be resolved:
ERROR: lib/ansible/modules/cloud/amazon/ec2_vpc_nat_gateway_facts.py:118:13: F821 undefined name 'camel_dict_to_snake_dict'
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

test/sanity

ANSIBLE VERSION
ansible 2.4.0 (flake8_tests e6e557cd76) last updated 2017/03/21 15:06:56 (GMT +1000)
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.13 (default, Jan 12 2017, 17:59:37) [GCC 6.3.1 20161221 (Red Hat 6.3.1-1)]
ADDITIONAL INFORMATION

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 bugfix_pull_request needs_triage Needs a first human triage before being processed. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 21, 2017
@mattclay mattclay added ci_verified Changes made in this PR are causing tests to fail. and removed needs_triage Needs a first human triage before being processed. labels Mar 21, 2017
@mattclay
Copy link
Member

Since we're still working through cleaning up existing issues caught by pep8, this is a little premature. We'll also need to discuss which of the new tests need to be excluded.

@mattclay
Copy link
Member

I've added this to the TWG agenda for further discussion.

@willthames
Copy link
Contributor Author

@mattclay fair enough, it does introduce more checks than I was thinking of

http://flake8.pycqa.org/en/latest/user/error-codes.html

There are some tests in there that simplify existing checks (e.g. module importing '*'), but mostly I wanted to avoid catchable bugs, not make things more complicated.

@sivel
Copy link
Member

sivel commented Mar 21, 2017

I feel like it's a bit too early for this. We still have the 5 legacy pep8 issues to resolve, and 30 current.

In some cases we have some overlapping checks. import * is allowed in "legacy" modules but not new, and validate-modules is handling that.

My feeling is that this would be a change targeted to 2.5 at earliest.

I'm all for it, I just don't want it distract us from our other already planned efforts.

@mattclay
Copy link
Member

This was discussed at the March 23rd TWG meeting. The general consensus is that flake8 will be an improvement over pep8. As long as the change doesn't interfere with the ongoing work to clean up the remaining issues currently reported by pep8, this should be fine.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 11, 2017
flake8 catches more errors than pep8, so will result in fewer
bugs making the codebase

Restrict use of flake8 to catching just undefined names for
now, and expand later
@willthames
Copy link
Contributor Author

I suspect that work on pylint will have similar effect, so I'll close this one.

@willthames willthames closed this Apr 18, 2017
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 bug This issue/PR relates to a bug. ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants