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.

Sign up for GitHub

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

Jump to bottom

Add a linter runnable locally and in the CI to ease and enforce trailing blanks trimming #15115

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add a linter runnable locally and in the CI to ease and enforce trailing blanks trimming #15115

wants to merge 3 commits into from

Conversation

guillaumelambert
Copy link
Contributor

@guillaumelambert guillaumelambert commented May 17, 2023 •
edited
Loading

CI changes to prepare and enforce trailing blanks complete clean-up

  • add a related pre-commit linter callable via tox
  • add it to the Azure CI
  • update documentation accordingly

Why I did it

ease code review and maintainability

#15114

How I did it

Via tox and pre-commit.
The ~17400 lines w/ trailing blanks could hardly be cleaned up manually.

How to verify it

  • run tox and/or pre-commit locally
  • look at new Azure CI Linters stage

Which release branch to backport (provide reason below if selected)

potentially all

Tested branch (Please provide the tested image version)

master

Description for the changelog

Clean trailing blanks and add a related linter runnable locally and in the CI

A picture of a cute animal (not mandatory but encouraged)

//
('>
/rr
*))_

@guillaumelambert
Copy link
Contributor Author

@collivier @bbailleux @odd22 FYI

@msosyak
Copy link
Contributor

msosyak commented May 17, 2023 •
edited
Loading

It is even hard to review what new is added to the CI due to big amount of changed files.

IMHO: PR1 - extend CI with new check; PR2 - code cleanup
IMHO2: No need to check all files. It is better to check only on changed files like in sonic-mgmt repo. Then there will be no need for big code cleanup PR as code will be cleaned up iteratively by each future PRs

@guillaumelambert
Copy link
Contributor Author

It is even hard to review what new is added to the CI due to big amount of changed files.

IMHO: PR1 - extend CI with new check; PR2 - code cleanup IMHO2: No need to check all files. It is better to check only on changed files like in sonic-mgmt repo. Then there will be no need for big code cleanup PR as code will be cleaned up iteratively by each future PRs

Thanks for the feedback.
This PR is composed of 4 commits. 2 of them are dedicated to the linters and the CI.
Github interface allows to check only the content of one commit, for instance
6fb67d0
209d3ef

As explained in the commit message of the second one, the profile run by the CI only checks files modified since this commit.

@guillaumelambert
Copy link
Contributor Author

It is even hard to review what new is added to the CI due to big amount of changed files.
IMHO: PR1 - extend CI with new check; PR2 - code cleanup IMHO2: No need to check all files. It is better to check only on changed files like in sonic-mgmt repo. Then there will be no need for big code cleanup PR as code will be cleaned up iteratively by each future PRs

Thanks for the feedback. This PR is composed of 4 commits. 2 of them are dedicated to the linters and the CI. Github interface allows to check only the content of one commit, for instance 6fb67d0 209d3ef

As explained in the commit message of the second one, the profile run by the CI only checks files modified since this commit.

There are again a few files in merge conflicts soon after the second update.
So you are probably right about the strategy of splitting this contributions into several PR.

Though, from my experience in similar cases, if we rely only on the CI for files changed since this commit, some files are likely to remain untouched. And we will probably never converge to a complete clean-up of the repo.
Also rebasing one big commit such as 9113c35 that corrects all files is a bit tricky.
It will always generate merge conflicts soon after the rebase if maintainers do not sync to merge it.
So I propose we also split the PR2 you propose into several pieces so they can be merged more easily.

@guillaumelambert guillaumelambert changed the title Clean trailing blanks and add a related linter runnable locally and in the CI Add a linter runnable locally and in the CI to ease and enforce trailing blanks trimming May 19, 2023
This was referenced May 19, 2023
Open
Open
Open
Open
Open
Merged
Merged
@guillaumelambert guillaumelambert force-pushed the trailing_blanks branch 6 times, most recently from ec4999b to df6a80f Compare May 24, 2023 19:12
@guillaumelambert
[CI] Add a pre-commit trailing blanks linter via tox
1aebc16 
- add a pre-commit yaml configuration file with a basic profile to
  remove trailing blanks
- add a rule to exclude from its scope
  - patch files (generated from git diff)
    Trailing blanks are part of their syntax.
  - .conf files in the src folder
    Their trailing blanks removal make some tests fail during build.
  - jinja templates in docker folder
    Though trailing blank should not be part of the jinja2 syntax,
    these files use a known hack that requires trailing blanks.
    Their removal makes build fail.
- create a tox configuration to run/install/uninstall/autoupdate
  pre-commit in a python venv
- create a tox profile for the CI
  to run pre-commit only on files modified since this commit
- add a shell script to ease running pre-commit inside various CI

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <[email protected]>
@guillaumelambert
[CI] Trigger tox/pre-commit linter(s) in Azure CI
1b1341d 
In azure pipelines configuration
- add a linter stage
- add to the linter stage a tox job w/ 2 steps to run tox

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <[email protected]>

fix
@guillaumelambert
[CI][doc] Add some documentation for tox and pre-commit
53568f4 
Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <[email protected]>
@guillaumelambert guillaumelambert mentioned this pull request May 25, 2023
Open
liushilongbuaa
liushilongbuaa reviewed May 25, 2023
View reviewed changes
tox.ini
deps = pre-commit
passenv = HOME
commands =
pre-commit run --all-files --show-diff-on-failure --from-ref a73d443c1d02698d9f3e030947c469677798bd32 --to-ref HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

What is it "a73d443c1d02698d9f3e030947c469677798bd32"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is it "a73d443c1d02698d9f3e030947c469677798bd32"?

It is the hash that references the commit just below this PR.
Only file modified since this commit will be linted by pre-commit.

$ git log -4 --oneline
53568f4 (HEAD -> trailing_blanks, orangeopensource/trailing_blanks) [CI][doc] Add some documentation for tox and pre-commit
1b1341d [CI] Trigger tox/pre-commit linter(s) in Azure CI
1aebc16 [CI] Add a pre-commit trailing blanks linter via tox
a73d443 [CI][doc][build] Trim src folder files trailing blanks (#15162)

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

@liushilongbuaa liushilongbuaa liushilongbuaa left review comments

@StormLiangMS StormLiangMS Awaiting requested review from StormLiangMS

@qiluo-msft qiluo-msft Awaiting requested review from qiluo-msft qiluo-msft is a code owner

@xumia xumia Awaiting requested review from xumia xumia is a code owner

@lguohan lguohan Awaiting requested review from lguohan lguohan is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@guillaumelambert @msosyak @liushilongbuaa