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

[CI] Fix shellcheck warning in pre-commit #50288

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

[CI] Fix shellcheck warning in pre-commit #50288

wants to merge 6 commits into from
+10 −11

Conversation

owenowenisme
Copy link

Why are these changes needed?

Related issue number

#47991

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@owenowenisme owenowenisme requested a review from a team as a code owner February 6, 2025 14:26
@owenowenisme
Copy link
Author

@MortalHappiness PTAL.

@MortalHappiness MortalHappiness mentioned this pull request Feb 6, 2025
Open
37 tasks
@MortalHappiness
Copy link
Member

Please also add shellcheck here.

ray/ci/lint/lint.sh

Lines 18 to 34 in 8a8a03a

HOOKS=(
ruff
check-added-large-files
check-ast
check-toml
black
prettier
mypy
rst-directive-colons
rst-inline-touching-normal
python-check-mock-methods
clang-format
docstyle
check-import-order
check-cpp-files-inclusion
end-of-file-fixer
)

@owenowenisme
Copy link
Author

Added.

aslonnie
aslonnie reviewed Feb 7, 2025
View reviewed changes
ci/ray_ci/rllib_contrib/rllib_contrib_ci.sh Outdated Show resolved Hide resolved
ci/ray_ci/macos/macos_ci_build.sh
@@ -54,7 +56,9 @@ build_x86_64() {
build_aarch64() {
# Cleanup environments
rm -rf /tmp/bazel_event_logs
cleanup() { if [ "${BUILDKITE_PULL_REQUEST}" = "false" ]; then ./ci/build/upload_build_info.sh; fi }; trap cleanup EXIT
# shellcheck disable=SC2317
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does here need SC2317?

Copy link
Author

Choose a reason for hiding this comment

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

Cause here we are using trap to invoke cleanup function, and it will make the shellcheck think it is unreachable, but this is a false positive.

@owenowenisme owenowenisme requested a review from aslonnie February 7, 2025 03:11
@owenowenisme owenowenisme force-pushed the fix-shellcheck-warning branch from b25dc3e to ab4960f Compare February 7, 2025 04:31
@aslonnie aslonnie added go add ONLY when ready to merge, run all tests and removed go add ONLY when ready to merge, run all tests labels Feb 7, 2025
aslonnie
aslonnie reviewed Feb 7, 2025
View reviewed changes
Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

lint check seems failing?

@owenowenisme owenowenisme marked this pull request as draft February 8, 2025 01:19
@owenowenisme
test using shellcheck-py
c27bfee 
Signed-off-by: owenowenisme <[email protected]>
@owenowenisme owenowenisme marked this pull request as ready for review February 8, 2025 01:59
@owenowenisme owenowenisme marked this pull request as draft February 8, 2025 02:00
@owenowenisme
Copy link
Author

@aslonnie
Looks like there's a problem in shellcheck in container, which I run normal in local.

[2025-02-07T04:50:46Z] java/build-jar-multiplatform.sh: java/build-jar-multiplatform.sh: openBinaryFile: does not exist (No such file or directory)
[2025-02-07T04:50:46Z] release/k8s_tests/prepare.sh: release/k8s_tests/prepare.sh: openBinaryFile: does not exist (No such file or directory)

I refer to the issue koalaman/shellcheck#2437
and use shellcheck-py and it pass.
But I don't know if I should deal with the problem this way or not.

@owenowenisme owenowenisme marked this pull request as ready for review February 8, 2025 02:04
@aslonnie
Copy link
Collaborator

aslonnie commented Feb 8, 2025

we should not use pre-commit that uses containers.. we should not assume that docker exists on the system for pre-commit to run.

@owenowenisme
Copy link
Author

So is it okay for ray to use shellcheck-py instead of shellcheck?
FYI kuberay just change to shellcheck-py for such reason.
ray-project/kuberay#2932 (comment)

@aslonnie
Copy link
Collaborator

aslonnie commented Feb 8, 2025 •
edited
Loading

So is it okay for ray to use shellcheck-py instead of shellcheck? FYI kuberay just change to shellcheck-py for such reason. ray-project/kuberay#2932 (comment)

seems fine to me.

or just remove it from pre-commit..

or write our own. pre-commit supports custom/local scripts too.

@owenowenisme
Copy link
Author

@aslonnie Already changed to shellcheck-py and passed the ci-linter check.

@aslonnie aslonnie added the go add ONLY when ready to merge, run all tests label Feb 8, 2025
@aslonnie aslonnie self-requested a review February 8, 2025 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aslonnie aslonnie Awaiting requested review from aslonnie

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

Assignees

@aslonnie aslonnie

Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@owenowenisme @MortalHappiness @aslonnie