Skip to content

Add Python flake8 linting for W291 trailing whitespace with Super-Linter#4687

Merged
nvazquez merged 3 commits intoapache:mainfrom
jbampton:remove-trailing-whitespace
Mar 28, 2022
Merged

Add Python flake8 linting for W291 trailing whitespace with Super-Linter#4687
nvazquez merged 3 commits intoapache:mainfrom
jbampton:remove-trailing-whitespace

Conversation

@jbampton
Copy link
Copy Markdown
Member

@jbampton jbampton commented Feb 12, 2021

Description

Need to have standardized Python code.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

@DaanHoogland
Copy link
Copy Markdown
Contributor

thanks @jbampton , I will try to merge this asap, Have you just removed trailing spaces or also looked at tab (especially component tests have that problem)?

@DaanHoogland DaanHoogland added this to the 4.16.0.0 milestone Feb 12, 2021
@DaanHoogland DaanHoogland added complexity:trivial 10 minnutes to a few days at most Python Warning... Python code Ahead! Severity:Trivial type:cleanup labels Feb 12, 2021
@DaanHoogland
Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2685

@jbampton jbampton changed the title refactor: remove trailing whitespace from Python files [WIP] - refactor: remove trailing whitespace from Python files Feb 12, 2021
@jbampton
Copy link
Copy Markdown
Member Author

Hey @DaanHoogland I only looked at trailing whitespace so far.

I was going to add a GitHub Action to run flake8 to test for W291 for trailing whitespace.

By adding this check it will mean it will stop regressions.

Yes I am interested in cleaning up tab problems in future and / or other Python linting problems.

https://flake8.pycqa.org/en/latest/
https://www.flake8rules.com/rules/W291.html

@DaanHoogland
Copy link
Copy Markdown
Contributor

@blueorangutan test

@DaanHoogland DaanHoogland requested a review from yadvr February 13, 2021 10:14
@yadvr
Copy link
Copy Markdown
Member

yadvr commented Sep 14, 2021

Ping @jbampton are you working on this, can you resolve conflicts? Thanks

@nvazquez
Copy link
Copy Markdown
Contributor

Moving it to the next milestone

@nvazquez nvazquez modified the milestones: 4.16.0.0, 4.17.0.0 Sep 21, 2021
@nvazquez
Copy link
Copy Markdown
Contributor

Hi @jbampton can you please fix the conflicts?

@jbampton jbampton force-pushed the remove-trailing-whitespace branch from a9176b2 to 898ee37 Compare February 19, 2022 06:24
@jbampton
Copy link
Copy Markdown
Member Author

@rohityadavcloud and @nvazquez I have removed the conflicts now

@jbampton
Copy link
Copy Markdown
Member Author

@DaanHoogland I have included both spaces and tab in this update.

@jbampton jbampton changed the title [WIP] - refactor: remove trailing whitespace from Python files refactor: remove trailing whitespace from Python files Feb 19, 2022
Add Python flake8 linting for W291 trailing whitespace
@jbampton jbampton changed the title refactor: remove trailing whitespace from Python files Add Python flake8 linting for W291 trailing whitespace with Super-Linter Feb 19, 2022
@nvazquez
Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@nvazquez a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2913

@nvazquez
Copy link
Copy Markdown
Contributor

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link
Copy Markdown

Trillian test result (tid-3665)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31260 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4687-t3665-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test Result Time (s) Test File

@nvazquez
Copy link
Copy Markdown
Contributor

Hi @DaanHoogland @weizhouapache @Pearl1594 could you please review? Smoke tests look good, no major changes besides removing spaces, I think it should be good to merge after approvals

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm

@jbampton can you tell how to configure the check on github ?

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks generally good but licenses need to be attended

Comment thread .github/linters/.flake8
Comment thread .github/workflows/linter.yml
@jbampton
Copy link
Copy Markdown
Member Author

Hey @weizhouapache GitHub Actions don't run the first time they are added to a repo with a pull request.

So this PR will need to be merged and then new PRs will run this GitHub Action.

@weizhouapache
Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@weizhouapache a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link
Copy Markdown

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2967

@nvazquez
Copy link
Copy Markdown
Contributor

@blueorangutan test matrix

@blueorangutan
Copy link
Copy Markdown

@nvazquez a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

@blueorangutan
Copy link
Copy Markdown

Trillian test result (tid-3730)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28925 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4687-t3730-vmware-65u2.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py

@blueorangutan
Copy link
Copy Markdown

Trillian test result (tid-3728)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28840 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4687-t3728-xenserver-71.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py

@blueorangutan
Copy link
Copy Markdown

Trillian test result (tid-3729)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28721 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4687-t3729-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py

@nvazquez nvazquez merged commit 182899d into apache:main Mar 28, 2022
@nvazquez
Copy link
Copy Markdown
Contributor

Many thanks @jbampton PR merged, I can see the Github Action working for new PRs

@jbampton jbampton deleted the remove-trailing-whitespace branch March 29, 2022 04:50
@jbampton
Copy link
Copy Markdown
Member Author

Thanks everyone 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

complexity:trivial 10 minnutes to a few days at most Python Warning... Python code Ahead! Severity:Trivial type:cleanup

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants