You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/03/07 09:13:45 UTC
[GitHub] [spark] stczwd opened a new pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
stczwd opened a new pull request #35751:
URL: https://github.com/apache/spark/pull/35751
### What changes were proposed in this pull request?
There is no shell code check in the current spark github actions. Some shell codes are written incorrectly and run abnormally in special cases. Besides, they cannot also pass the check of the shellcheck plugin, especially in IDEA or shellcheck actions. This PR try to add [shellstyle](https://github.com/marketplace/actions/shellcheck) action, which use [shellcheck](https://github.com/koalaman/shellcheck) to check code.
### Why are the changes needed?
This action can ensure the normal execution of the spark shell script
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Origin UT with new action.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
stczwd commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1061910585
> Quick question: don't we have a style feature together too? I know many scripts have inconsistent styles. e.g., 2 spaces vs 4 spaces.
@HyukjinKwon I found [shfmt](https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd) can check shell code style with spaces, maybe it is better to use both `shfmt` and `shellcheck`.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on a change in pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
stczwd commented on a change in pull request #35751:
URL: https://github.com/apache/spark/pull/35751#discussion_r821406723
##########
File path: sbin/spark-config.sh
##########
@@ -1,3 +1,5 @@
+#!/usr/bin/env bash
Review comment:
Hm, actually, all the other scripts under sbin directory use `#!/usr/bin/env bash`.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1061366639
I think it's fine - we don't package together with the distribution in any event.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
stczwd commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1061509774
https://github.com/apache/spark/pull/35763
@dongjoon-hyun The PR for script code style change has been created, we can talk about it in this 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35751:
URL: https://github.com/apache/spark/pull/35751#discussion_r820535552
##########
File path: .github/workflows/build_and_test.yml
##########
@@ -821,3 +821,27 @@ jobs:
with:
name: unit-tests-log-docker-integration--8-${{ needs.configure-jobs.outputs.hadoop }}-hive2.3
path: "**/target/unit-tests.log"
+
+ shellcheck:
+ needs: [configure-jobs, precondition]
+ name: Run shell code style check
+ if: needs.configure-jobs.outputs.type == 'regular' && fromJson(needs.precondition.outputs.required).build == 'true'
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout Spark repository
+ uses: actions/checkout@v2
+ with:
+ fetch-depth: 0
+ repository: apache/spark
+ ref: master
+ - name: Sync the current branch with the latest in Apache Spark
+ if: github.repository != 'apache/spark'
+ run: |
+ git fetch https://github.com/$GITHUB_REPOSITORY.git ${GITHUB_REF#refs/heads/}
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' merge --no-commit --progress --squash FETCH_HEAD
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' commit -m "Merged commit"
+ - name: Run ShellCheck
+ uses: ludeeus/action-shellcheck@master
Review comment:
quick question, can we add a dev script? like dev/linter-python, dev/linter-scala
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1060902443
Thank you for pinging me, @LuciferYang .
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1061295414
Quick question: don't we have a style feature together too? I know many scripts have inconsistent styles. e.g., 2 spaces vs 4 spaces.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1061496602
BTW, IMO, I don't think this is an urgent item. I'd like to recommend this until we cut `branch-3.3`.
cc @MaxGekk since he is the release manager for Apache Spark 3.3.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on a change in pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
stczwd commented on a change in pull request #35751:
URL: https://github.com/apache/spark/pull/35751#discussion_r820544527
##########
File path: .github/workflows/build_and_test.yml
##########
@@ -821,3 +821,27 @@ jobs:
with:
name: unit-tests-log-docker-integration--8-${{ needs.configure-jobs.outputs.hadoop }}-hive2.3
path: "**/target/unit-tests.log"
+
+ shellcheck:
+ needs: [configure-jobs, precondition]
+ name: Run shell code style check
+ if: needs.configure-jobs.outputs.type == 'regular' && fromJson(needs.precondition.outputs.required).build == 'true'
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout Spark repository
+ uses: actions/checkout@v2
+ with:
+ fetch-depth: 0
+ repository: apache/spark
+ ref: master
+ - name: Sync the current branch with the latest in Apache Spark
+ if: github.repository != 'apache/spark'
+ run: |
+ git fetch https://github.com/$GITHUB_REPOSITORY.git ${GITHUB_REF#refs/heads/}
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' merge --no-commit --progress --squash FETCH_HEAD
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' commit -m "Merged commit"
+ - name: Run ShellCheck
+ uses: ludeeus/action-shellcheck@master
Review comment:
Sure, I will add a `dev/linter-shell` to do 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35751:
URL: https://github.com/apache/spark/pull/35751#discussion_r821402951
##########
File path: sbin/spark-config.sh
##########
@@ -1,3 +1,5 @@
+#!/usr/bin/env bash
Review comment:
> This chage is because [SC2148](https://github.com/koalaman/shellcheck/wiki/SC2148).
@stczwd . I'm asking about the behavior change of `bash` binary in Spark environment. Your change could switch the `bash` location from `/bin/bash` to `/usr/bin/bash` silently. Frequently, those two bash versions are not the same.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35751:
URL: https://github.com/apache/spark/pull/35751#discussion_r820535785
##########
File path: .github/workflows/build_and_test.yml
##########
@@ -821,3 +821,27 @@ jobs:
with:
name: unit-tests-log-docker-integration--8-${{ needs.configure-jobs.outputs.hadoop }}-hive2.3
path: "**/target/unit-tests.log"
+
+ shellcheck:
+ needs: [configure-jobs, precondition]
+ name: Run shell code style check
+ if: needs.configure-jobs.outputs.type == 'regular' && fromJson(needs.precondition.outputs.required).build == 'true'
+ runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout Spark repository
+ uses: actions/checkout@v2
+ with:
+ fetch-depth: 0
+ repository: apache/spark
+ ref: master
+ - name: Sync the current branch with the latest in Apache Spark
+ if: github.repository != 'apache/spark'
+ run: |
+ git fetch https://github.com/$GITHUB_REPOSITORY.git ${GITHUB_REF#refs/heads/}
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' merge --no-commit --progress --squash FETCH_HEAD
+ git -c user.name='Apache Spark Test Account' -c user.email='sparktestacc@gmail.com' commit -m "Merged commit"
+ - name: Run ShellCheck
+ uses: ludeeus/action-shellcheck@master
Review comment:
and we could do that together at `linter` 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
stczwd commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1061341858
@HyukjinKwon Another question: shellcheck is based on [GPL Lisence](https://github.com/koalaman/shellcheck/blob/master/LICENSE), can we use it in our code?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
stczwd commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1063564505
@dongjoon-hyun @HyukjinKwon @LuciferYang @MaxGekk
[Discussion about supporting indentition check in shellcheck](https://github.com/koalaman/shellcheck/issues/2463#issuecomment-1063439416)
Since the owner of shellcheck recommends using `shfmt` for indentition check, I will open another PR to adjust the indentition after [35763](https://github.com/apache/spark/pull/35763) merged.
Therefore, there will be a total of 3 PRs, one to fix the code style error, one to fix the indentition, and the last one add `shellcheck` and `shfmt`.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1060384762
cc @srowen @HyukjinKwon @dongjoon-hyun
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1061295003
Actually Josh suggested this a while ago. FYI @JoshRosen
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35751:
URL: https://github.com/apache/spark/pull/35751#discussion_r821394294
##########
File path: sbin/spark-config.sh
##########
@@ -1,3 +1,5 @@
+#!/usr/bin/env bash
Review comment:
Is this a safe change, @stczwd ? This looks like that we need to handle in a separate PR, not in this SPARK-38433, to give a proper visibility.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd edited a comment on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
stczwd edited a comment on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1061336368
>
> Quick question: don't we have a style feature together too? I know many scripts have inconsistent styles. e.g., 2 spaces vs 4 spaces.
Yes, based on [Google Shell Style Guide](https://google.github.io/styleguide/shellguide.html#s5.1-indentation), 2 spaces and no tabs for indentition. However, the `shellcheck` doesn't support this now, which means no indentition check for now.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
stczwd commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1061336368
>
> Quick question: don't we have a style feature together too? I know many scripts have inconsistent styles. e.g., 2 spaces vs 4 spaces.
Yes, based on Google Shell Style Guide, 2 spaces and no tabs for indentition. However, the `shellcheck` doesn't support this now, which means no indentition check for now.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
stczwd commented on pull request #35751:
URL: https://github.com/apache/spark/pull/35751#issuecomment-1061500909
> This PR is touching core scripts in a style check PR. We had better pay more attention to the core scripts change. Please split this PR into two PRs: One for core script change PR and the other for enabling `shellcheck`.
Okey, I will submit a new PR for script code style change. And this PR will be used to add `shellcheck` only.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on a change in pull request #35751: [SPARK-38433][BUILD] Add shell code style check Actions
Posted by GitBox <gi...@apache.org>.
stczwd commented on a change in pull request #35751:
URL: https://github.com/apache/spark/pull/35751#discussion_r821400709
##########
File path: sbin/spark-config.sh
##########
@@ -1,3 +1,5 @@
+#!/usr/bin/env bash
Review comment:
This chage is because [SC2148](https://github.com/koalaman/shellcheck/wiki/SC2148).
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org