[BEAM-10771] Added a whitespace lint as part of python lint precommit by nika-qubit · Pull Request #12444 · apache/beam

nika-qubit · 2020-08-01T00:20:08Z

Added a Jenkins whitespacePreCommit that now lints whitespace for markdown files and gradle build files in the repo.
Triggered whenever files under sdks directory are changed.
Whitespace lint for other non code files can be added in this way too.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	Dataflow	Samza	Twister2
Go	---	---	---
Java
Python		---	---
XLang	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

aaltay · 2020-08-01T00:30:38Z

a bit strange to apply this only when there is a python change. md files could be anywhere in the repo.

Yes, it makes sense to put it at the root.
I've made a new task to run the tool in a python virtual env, but for all files.
It'll be triggered if there are markdown files or build.gradle files changed.

nika-qubit · 2020-08-01T01:53:46Z

Did a rebase to take in the trailing whitespace fix of a README from a previous PR.

aaltay · 2020-08-03T18:29:29Z

LGTM. Would it make sense to announce this on the dev@ list? Or do you think this is will sufficiently have minimal impact.

nika-qubit · 2020-08-03T18:46:49Z

LGTM. Would it make sense to announce this on the dev@ list? Or do you think this is will sufficiently have minimal impact.

Once we merge it, I'll check if new PRs would trigger it as expected and update the README of Jenkins trigger phrases.
Then I'll announce it in the dev list since this is a new precommit job and potentially has actionable items.

nika-qubit · 2020-08-03T18:47:09Z

Run Python2_PVR_Flink PreCommit

nika-qubit · 2020-08-03T18:48:12Z

Run Python2_PVR_Flink PreCommit

aaltay · 2020-08-03T21:30:57Z

Is it possible to limit the trigger pattern to *.md and build.gradle files only? (The task is checking only for those files anyway.)

nika-qubit · 2020-08-04T17:49:54Z

R: @damgad
Hi Damian, sorry to bother you on this PR. Tyson told me you might be able to give some advice on this topic: does this regex look fine to you and could we test it before merging it? Thanks!

Hey, no worries. The regex looks fine, and I've also checked that it works. FYI: by running the seed job from the PR you can see your changes in action on Jenkins. However (practically) only committers could trigger that.

Thanks, this is really helpful information!

nika-qubit · 2020-08-04T17:50:44Z

Run Python2_PVR_Flink PreCommit

damgad

run seed job

damgad · 2020-08-05T11:00:45Z

I suggest whitespacelint "$FILE" to correctly handle whitespaces and prevent globbing.

Thanks, changing it.

damgad · 2020-08-05T11:22:46Z

run seed job

damgad · 2020-08-05T11:29:11Z

Run Whitespace PreCommit

damgad

Few things regarding this change

Isn't the spotless pre-commit right place to extend with such checks? Many developers probably already added that to their pre-commit hooks so the introduction of this check will be almost unnoticeable.
Seems like it's using python2.7 virtualenv [1]:

00:00:40.125 Running virtualenv with interpreter /usr/bin/python2.7
00:00:40.425 DEPRECATION: Python 2.7 reached the end of its life on January 1st, 2020.

If possible, I would suggest avoiding that due to the end of life.

The check is really "quiet" in terms of logging, in case of failure there are no details regarding the file/line and the exact issue. Check out my sample PR #12470 and the build output [2]. Would be helpful if it prints the issue similarly to the spottles e.g. [3].

[1] https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Commit/1/console
[2] https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Commit/3/console
[3] https://ci-beam.apache.org/job/beam_PreCommit_Spotless_Commit/10575/console

damgad · 2020-08-05T11:58:04Z

Hey, no worries. The regex looks fine, and I've also checked that it works. FYI: by running the seed job from the PR you can see your changes in action on Jenkins. However (practically) only committers could trigger that.

nika-qubit · 2020-08-11T22:11:50Z

Few things regarding this change
Isn't the spotless pre-commit right place to extend with such checks? Many developers probably already added that to their pre-commit hooks so the introduction of this check will be almost unnoticeable.

It will be the best if the check is unnoticeable. This check can also get triggered when a commit only changes some markdown or build.gradle files in the repo. It's not needed if the commit just changes code since different linters in those programming languages already covers the whitespace issues in code files.

Seems like it's using python2.7 virtualenv [1]:
00:00:40.125 Running virtualenv with interpreter /usr/bin/python2.7
00:00:40.425 DEPRECATION: Python 2.7 reached the end of its life on January 1st, 2020.
If possible, I would suggest avoiding that due to the end of life.

Changing it to use python3.8.

The check is really "quiet" in terms of logging, in case of failure there are no details regarding the file/line and the exact issue. Check out my sample PR DISREGARD - test PR #12470 and the build output [2]. Would be helpful if it prints the issue similarly to the spottles e.g. [3].
[1] https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Commit/1/console
[2] https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Commit/3/console
[3] https://ci-beam.apache.org/job/beam_PreCommit_Spotless_Commit/10575/console

Yes, it doesn't print much except Running whitespacelint... if it succeeds.
But if it fails, it does tell you which line and what problem it is.
The [2] actually fails because it couldn't find the precommit task: Task 'whitespacePreCommit' not found in root project 'beam'.

I'll modify the script to output more information.

aaltay · 2020-08-14T22:01:50Z

@kevingg - one suggestion, separate the update md files to a single commit and people can review infra changes without reviewing all the md file changes.

nika-qubit · 2020-08-14T22:08:43Z

@kevingg - one suggestion, separate the update md files to a single commit and people can review infra changes without reviewing all the md file changes.

Got it, will rebase it into 2 separate commits.

nika-qubit · 2020-08-14T22:26:09Z

This PR now contains 2 commits and the first commit contains test infra changes.

nika-qubit · 2020-08-14T22:26:53Z

Run Python2_PVR_Flink PreCommit

nika-qubit · 2020-08-14T22:40:53Z

Run Python2_PVR_Flink PreCommit

nika-qubit · 2020-08-14T23:34:14Z

The tasks failed at buildLinuxAmd64 are caused by #12554.

aaltay · 2020-08-14T23:43:01Z

Run Python2_PVR_Flink PreCommit

nika-qubit · 2020-08-17T20:32:41Z

Run Python2_PVR_Flink PreCommit

nika-qubit · 2020-08-17T20:32:50Z

Run PythonDocker PreCommit

nika-qubit · 2020-08-17T20:32:59Z

Run PythonDocs PreCommit

nika-qubit · 2020-08-17T22:40:57Z

R: @pabloem
PTAL, thx!

pabloem

Can you create a Jira issue for this?

pabloem · 2020-08-17T23:51:38Z

did you find that you needed the mustRunAfter rules even though you also had the dependsOn rules?

Removed the mustRunAfter statements.

pabloem · 2020-08-17T23:53:18Z

Can you add mention of this job in .test-infra/jenkins/README.md? probably also in the PR template tables, please

Added the item

pabloem · 2020-08-17T23:54:00Z

Can you document in this file which groups of files are meant to be linted by this test?

Documented.

It is triggered on changes of any markdown or gradle build file in the Beam repo.

… be linted.

nika-qubit · 2020-08-20T19:13:41Z

Can you create a Jira issue for this?

Created BEAM-10771 for this.
The change is done in the third commit: 344599e

nika-qubit · 2020-08-20T19:15:23Z

Rebased to catch up with the HEAD. Edited two more new files with whitespaces.

nika-qubit · 2020-08-20T22:08:06Z

Run Java PreCommit

nika-qubit · 2020-08-20T22:08:17Z

Run Python PreCommit

pabloem · 2020-08-21T00:49:06Z

Run Seed Job

pabloem · 2020-08-21T00:50:18Z

LGTM. I'll just run the seed job to verify the jenkins config.

pabloem · 2020-08-21T00:50:35Z

Run Seed Job

pabloem · 2020-08-21T03:16:53Z

Run Seed Job

pabloem · 2020-08-21T05:09:45Z

Run Seed Job

pabloem · 2020-08-21T17:57:45Z

Thanks @kevingg . Can you please make a follow up change to include it here: https://github.com/apache/beam/blob/master/.github/PULL_REQUEST_TEMPLATE.md so it will show up in the PR template?

1. The job was created in apache#12444. 2. Its cron job is not enabled on Jenkins, thus no status is available in the template at this time.

probot-autolabelerBot added the python label Aug 1, 2020

aaltay reviewed Aug 1, 2020
View reviewed changes

probot-autolabelerBot added build infra labels Aug 1, 2020

nika-qubit force-pushed the whitespacelint branch from 62571bb to 5b08fe9Compare August 1, 2020 01:52

aaltay approved these changes Aug 3, 2020
View reviewed changes

aaltay reviewed Aug 3, 2020
View reviewed changes

nika-qubit commented Aug 4, 2020
View reviewed changes

damgad reviewed Aug 5, 2020
View reviewed changes

kkucharc mentioned this pull request Aug 5, 2020
[BEAM-9897] Add cross-language support to SnowflakeIO.Read #12149
Merged
4 tasks

damgad reviewed Aug 5, 2020
View reviewed changes

nika-qubit mentioned this pull request Aug 12, 2020
[BEAM-10545] HtmlView module #12460
Merged
4 tasks

probot-autolabelerBot added the learning label Aug 14, 2020

nika-qubit force-pushed the whitespacelint branch from 9bd0f5d to c494469Compare August 14, 2020 22:22

nika-qubit requested a review from aaltay August 17, 2020 21:07

pabloem reviewed Aug 17, 2020
View reviewed changes

nika-qubit changed the title ~~Added a whitespace lint as part of python lint precommit~~[BEAM-10771] Added a whitespace lint as part of python lint precommitAug 20, 2020

Ning Kang added 4 commits August 20, 2020 12:07

Added a whitespace lint precommit job.
cdc6778
It is triggered on changes of any markdown or gradle build file in the Beam repo.

Removed trailing whitespaces from all markdown and gradle build files.
e81c6f4

Updated jenkins precommit job README, added comments of what needs to…
344599e
… be linted.

Removed trailing whitespaces from HEAD changes
4e573fd

nika-qubit force-pushed the whitespacelint branch from f2d3040 to 4e573fdCompare August 20, 2020 19:14

nika-qubit requested a review from pabloem August 20, 2020 19:15

pabloem merged commit dfb5cd5 into apache:masterAug 21, 2020

nika-qubit mentioned this pull request Aug 21, 2020
[BEAM-10771] Added Whitespacelint job to pull request template #12665
Merged
4 tasks

lukecwik mentioned this pull request Aug 26, 2020
[BEAM-10771] Fix whitespace error causing whitespace lint to fail #12689
Merged
4 tasks

Uh oh!

Conversation

nika-qubit commented Aug 1, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nika-qubit commented Aug 1, 2020

Uh oh!

aaltay commented Aug 3, 2020

Uh oh!

nika-qubit commented Aug 3, 2020

Uh oh!

nika-qubit commented Aug 3, 2020

Uh oh!

nika-qubit commented Aug 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nika-qubit commented Aug 4, 2020

Uh oh!

damgad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damgad commented Aug 5, 2020

Uh oh!

damgad commented Aug 5, 2020

Uh oh!

damgad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nika-qubit commented Aug 11, 2020

Uh oh!

aaltay commented Aug 14, 2020

Uh oh!

nika-qubit commented Aug 14, 2020

Uh oh!

nika-qubit commented Aug 14, 2020

Uh oh!

nika-qubit commented Aug 14, 2020

Uh oh!

nika-qubit commented Aug 14, 2020

Uh oh!

nika-qubit commented Aug 14, 2020

Uh oh!

aaltay commented Aug 14, 2020

Uh oh!

nika-qubit commented Aug 17, 2020

Uh oh!

nika-qubit commented Aug 17, 2020

Uh oh!

nika-qubit commented Aug 17, 2020

Uh oh!

nika-qubit commented Aug 17, 2020

Uh oh!

pabloem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nika-qubit commented Aug 1, 2020•
edited
Loading

nika-qubit commented Aug 20, 2020•
edited
Loading