You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/08/01 00:20:24 UTC
[GitHub] [beam] KevinGG opened a new pull request #12444: Added a whitespace lint as part of python lint precommit
KevinGG opened a new pull request #12444:
URL: https://github.com/apache/beam/pull/12444
Py37lint now does whitespace linting for all markdown files under sdks/python/apache_beam.
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)**](https://beam.apache.org/contribute/#make-your-change) 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](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-675098960
Run PythonDocker PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on a change in pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
aaltay commented on a change in pull request #12444:
URL: https://github.com/apache/beam/pull/12444#discussion_r464672903
##########
File path: .test-infra/jenkins/job_PreCommit_Whitespace.groovy
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import PrecommitJobBuilder
+
+PrecommitJobBuilder builder = new PrecommitJobBuilder(
+ scope: this,
+ nameBase: 'Whitespace',
+ gradleTask: ':whitespacePreCommit',
+ triggerPathPatterns: ['^sdks/.*$'])
Review comment:
Is it possible to limit the trigger pattern to `*.md` and `build.gradle` files only? (The task is checking only for those files anyway.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-667450301
Did a rebase to take in the trailing whitespace fix of a README from a previous PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-674312654
Run Python2_PVR_Flink PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on a change in pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on a change in pull request #12444:
URL: https://github.com/apache/beam/pull/12444#discussion_r474211452
##########
File path: sdks/python/test-suites/tox/py38/build.gradle
##########
@@ -33,3 +33,27 @@ apply from: "../common.gradle"
// TODO(BEAM-8954): Remove this once tox uses isolated builds.
testPy38Cython.mustRunAfter testPython38, testPy38CloudCoverage
+
+toxTask "whitespacelint", "whitespacelint"
+
+task archiveFilesToLint(type: Zip) {
+ archiveFileName = "files-to-whitespacelint.zip"
+ destinationDirectory = file("$buildDir/dist")
+
+ from ("$rootProject.projectDir") {
+ include "**/*.md"
+ include "**/build.gradle"
+ }
+}
+
+task unpackFilesToLint(type: Copy) {
+ from zipTree("$buildDir/dist/files-to-whitespacelint.zip")
+ into "$buildDir/files-to-whitespacelint"
+}
+
+whitespacelint.dependsOn archiveFilesToLint, unpackFilesToLint
+unpackFilesToLint.dependsOn archiveFilesToLint
+archiveFilesToLint.dependsOn cleanPython
+archiveFilesToLint.mustRunAfter cleanPython
+unpackFilesToLint.mustRunAfter cleanPython, archiveFilesToLint
Review comment:
Removed the mustRunAfter statements.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damgad commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
damgad commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-669140081
Run Whitespace PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on a change in pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
pabloem commented on a change in pull request #12444:
URL: https://github.com/apache/beam/pull/12444#discussion_r471836163
##########
File path: .test-infra/jenkins/job_PreCommit_Whitespace.groovy
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import PrecommitJobBuilder
+
+PrecommitJobBuilder builder = new PrecommitJobBuilder(
Review comment:
Can you add mention of this job in `.test-infra/jenkins/README.md`? probably also in the PR template tables, please
##########
File path: sdks/python/scripts/run_whitespacelint.sh
##########
@@ -0,0 +1,56 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+set -e
Review comment:
Can you document in this file which groups of files are meant to be linted by this test?
##########
File path: sdks/python/test-suites/tox/py38/build.gradle
##########
@@ -33,3 +33,27 @@ apply from: "../common.gradle"
// TODO(BEAM-8954): Remove this once tox uses isolated builds.
testPy38Cython.mustRunAfter testPython38, testPy38CloudCoverage
+
+toxTask "whitespacelint", "whitespacelint"
+
+task archiveFilesToLint(type: Zip) {
+ archiveFileName = "files-to-whitespacelint.zip"
+ destinationDirectory = file("$buildDir/dist")
+
+ from ("$rootProject.projectDir") {
+ include "**/*.md"
+ include "**/build.gradle"
+ }
+}
+
+task unpackFilesToLint(type: Copy) {
+ from zipTree("$buildDir/dist/files-to-whitespacelint.zip")
+ into "$buildDir/files-to-whitespacelint"
+}
+
+whitespacelint.dependsOn archiveFilesToLint, unpackFilesToLint
+unpackFilesToLint.dependsOn archiveFilesToLint
+archiveFilesToLint.dependsOn cleanPython
+archiveFilesToLint.mustRunAfter cleanPython
+unpackFilesToLint.mustRunAfter cleanPython, archiveFilesToLint
Review comment:
did you find that you needed the `mustRunAfter` rules even though you also had the `dependsOn` rules?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-677931618
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-678014728
Run Seed Job
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-674299941
Run Python2_PVR_Flink PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-674296809
Run Python2_PVR_Flink PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-674289812
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on a change in pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
aaltay commented on a change in pull request #12444:
URL: https://github.com/apache/beam/pull/12444#discussion_r463896381
##########
File path: sdks/python/scripts/run_pylint.sh
##########
@@ -37,6 +37,11 @@ fi
set -o errexit
set -o pipefail
+# Check for trailing spaces in non-Python text files.
+echo "Running whitespacelint..."
+MARKDOWN=$(find apache_beam -name '*.md' -print0)
Review comment:
a bit strange to apply this only when there is a python change. md files could be anywhere in the repo.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-668737553
Run Python2_PVR_Flink PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-677977251
Run Seed Job
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-674296618
This PR now contains 2 commits and the [first commit](https://github.com/apache/beam/pull/12444/commits/135890c38725ff88d300bca7e9d6db4f2a67ad4e) contains test infra changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-675150290
R: @pabloem
PTAL, thx!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damgad commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
damgad commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-669137527
run seed job
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-672309994
> Few things regarding this change
>
> 1. 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.
> 2. 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.
> 1. 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
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-678041424
Run Seed Job
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on a change in pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on a change in pull request #12444:
URL: https://github.com/apache/beam/pull/12444#discussion_r468881675
##########
File path: sdks/python/scripts/run_whitespacelint.sh
##########
@@ -0,0 +1,32 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+set -o errexit
+
+pushd ../../
+
+# Check for trailing spaces in non-code text files.
+# Currently include markdown files and gradle build files.
+echo "Running whitespacelint..."
+FILES=$(find . \( -name '*.md' -o -name 'build.gradle' \))
+for FILE in $FILES
+do
+ whitespacelint $FILE
Review comment:
Thanks, changing it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-668183158
Run Python2_PVR_Flink PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-668183674
Run Python2_PVR_Flink PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-678415797
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on a change in pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on a change in pull request #12444:
URL: https://github.com/apache/beam/pull/12444#discussion_r463905294
##########
File path: sdks/python/scripts/run_pylint.sh
##########
@@ -37,6 +37,11 @@ fi
set -o errexit
set -o pipefail
+# Check for trailing spaces in non-Python text files.
+echo "Running whitespacelint..."
+MARKDOWN=$(find apache_beam -name '*.md' -print0)
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-677977519
LGTM. I'll just run the seed job to verify the jenkins config.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-677977582
Run Seed Job
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-675098786
Run Portable_Python PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-668175310
LGTM. Would it make sense to announce this on the dev@ list? Or do you think this is will sufficiently have minimal impact.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem merged pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
pabloem merged pull request #12444:
URL: https://github.com/apache/beam/pull/12444
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-675098895
Run Python2_PVR_Flink PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-674312720
Run Java PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damgad commented on a change in pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
damgad commented on a change in pull request #12444:
URL: https://github.com/apache/beam/pull/12444#discussion_r465644067
##########
File path: sdks/python/scripts/run_whitespacelint.sh
##########
@@ -0,0 +1,32 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+set -o errexit
+
+pushd ../../
+
+# Check for trailing spaces in non-code text files.
+# Currently include markdown files and gradle build files.
+echo "Running whitespacelint..."
+FILES=$(find . \( -name '*.md' -o -name 'build.gradle' \))
+for FILE in $FILES
+do
+ whitespacelint $FILE
Review comment:
I suggest `whitespacelint "$FILE"` to correctly handle whitespaces and prevent globbing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-674311048
The tasks failed at `buildLinuxAmd64` are caused by https://github.com/apache/beam/pull/12554.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on a change in pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on a change in pull request #12444:
URL: https://github.com/apache/beam/pull/12444#discussion_r465225286
##########
File path: .test-infra/jenkins/job_PreCommit_Whitespace.groovy
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import PrecommitJobBuilder
+
+PrecommitJobBuilder builder = new PrecommitJobBuilder(
+ scope: this,
+ nameBase: 'Whitespace',
+ gradleTask: ':whitespacePreCommit',
+ triggerPathPatterns: [
+ '.*\\.md$',
+ '.*build\\.gradle$',
+ ]
Review comment:
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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-677848959
> Can you create a Jira issue for this?
Created BEAM-10771 for this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG edited a comment on pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG edited a comment on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-677848959
> Can you create a Jira issue for this?
Created BEAM-10771 for this.
The change is done in the third commit: 344599e77038ed79757307903389b847ac76881e
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-674291758
> @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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on a change in pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on a change in pull request #12444:
URL: https://github.com/apache/beam/pull/12444#discussion_r468885295
##########
File path: .test-infra/jenkins/job_PreCommit_Whitespace.groovy
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import PrecommitJobBuilder
+
+PrecommitJobBuilder builder = new PrecommitJobBuilder(
+ scope: this,
+ nameBase: 'Whitespace',
+ gradleTask: ':whitespacePreCommit',
+ triggerPathPatterns: [
+ '.*\\.md$',
+ '.*build\\.gradle$',
+ ]
Review comment:
Thanks, this is really helpful information!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damgad commented on a change in pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
damgad commented on a change in pull request #12444:
URL: https://github.com/apache/beam/pull/12444#discussion_r465674025
##########
File path: .test-infra/jenkins/job_PreCommit_Whitespace.groovy
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import PrecommitJobBuilder
+
+PrecommitJobBuilder builder = new PrecommitJobBuilder(
+ scope: this,
+ nameBase: 'Whitespace',
+ gradleTask: ':whitespacePreCommit',
+ triggerPathPatterns: [
+ '.*\\.md$',
+ '.*build\\.gradle$',
+ ]
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-675071234
Run Java PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on a change in pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on a change in pull request #12444:
URL: https://github.com/apache/beam/pull/12444#discussion_r474210889
##########
File path: .test-infra/jenkins/job_PreCommit_Whitespace.groovy
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import PrecommitJobBuilder
+
+PrecommitJobBuilder builder = new PrecommitJobBuilder(
Review comment:
Added the item
##########
File path: sdks/python/scripts/run_whitespacelint.sh
##########
@@ -0,0 +1,56 @@
+#!/bin/bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+set -e
Review comment:
Documented.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-668183021
> 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](https://github.com/apache/beam/tree/master/.test-infra/jenkins#precommit-jobs) 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-675098497
Run Portable_Python PreCommit
Run Python2_PVR_Flink PreCommit
Run PythonDocker PreCommit
Run PythonDocs PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-675099038
Run PythonDocs PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] KevinGG commented on pull request #12444: [BEAM-10771] Added a whitespace lint as part of python lint precommit
Posted by GitBox <gi...@apache.org>.
KevinGG commented on pull request #12444:
URL: https://github.com/apache/beam/pull/12444#issuecomment-677849912
Rebased to catch up with the HEAD. Edited two more new files with whitespaces.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org