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.

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

Shellcheck: bazel/ examples/ and other remaining linting issues #13106

Merged
merged 14 commits into from
Sep 30, 2020

Conversation

phlax
Copy link
Member

@phlax phlax commented Sep 15, 2020

Commit Message: Shellcheck: bazel/ examples/
Additional Description:
Remaining shellcheck cleanups
Risk Level: low
Testing:
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue] fixes #7793
[Optional Deprecated:]

@phlax phlax marked this pull request as draft September 15, 2020 12:21
bazel/sh_test_wrapper.sh Outdated Show resolved Hide resolved
@phlax phlax force-pushed the ci-run-shellchecks--rest branch 3 times, most recently from 3748b0e to 20b3a28 Compare September 18, 2020 14:08
@phlax phlax changed the title [WIP] Shellcheck: bazel/ examples/ Shellcheck: bazel/ examples/ Sep 21, 2020
@phlax phlax marked this pull request as ready for review September 21, 2020 10:09
@phlax phlax force-pushed the ci-run-shellchecks--rest branch from 261a3fc to d8e54e3 Compare September 21, 2020 11:42
# TODO(asraa): Remove manual `|| true`, but this shouldn't be necessary.
${TEST_BINARY} $(find fuzz_corpus -type f) -rss_limit_mb=8192 || true
# TODO(asraa): Remove manual `|| :`, but this shouldn't be necessary.
${TEST_BINARY} "$(find fuzz_corpus -type f)" -rss_limit_mb=8192 || :
Copy link
Member Author

Choose a reason for hiding this comment

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

im wondering if this will handle multiline output from find correctly

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it does:

$ printf "1: %s, 2: %s\n" $(printf "a\nb\n")
1: a, 2: b
$ printf "1: %s, 2: %s\n" "$(printf "a\nb\n")"
1: a
b, 2: 

Copy link
Member Author

@phlax phlax Sep 23, 2020

Choose a reason for hiding this comment

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

which without readarray means this i think

$ _args="$(printf "a\nb\n")"
$ while read -r line; do args+=("$line"); done \
    <<< "$_args"
$ printf "1: %s, 2: %s\n" "${args[@]}"
1: a, 2: b

@phlax phlax force-pushed the ci-run-shellchecks--rest branch from 2a22d6f to de07a44 Compare September 23, 2020 05:54
@phlax phlax changed the title Shellcheck: bazel/ examples/ Shellcheck: bazel/ examples/ and other remaining linting issues Sep 23, 2020
@phlax phlax mentioned this pull request Sep 23, 2020
@phlax
Copy link
Member Author

phlax commented Sep 23, 2020

im thinking that #12889 should probably land before this one as i think one or other will need to rebase

@zuercher
Copy link
Member

This looks good to me. Do you want to hold it until #12889 lands?

@phlax
Copy link
Member Author

phlax commented Sep 23, 2020

This looks good to me. Do you want to hold it until #12889 lands?

yep waiting...

8)

@phlax phlax force-pushed the ci-run-shellchecks--rest branch from 705d19b to 409f62e Compare September 24, 2020 16:17
@phlax
Copy link
Member Author

phlax commented Sep 25, 2020

/azp run

@azure-pipelines
Copy link

Command 'rerun' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 13106 in repo envoyproxy/envoy

Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
this fails locally but not in ci - perhaps shellcheck version

Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the ci-run-shellchecks--rest branch from ec49ff3 to 004ac11 Compare September 25, 2020 10:18
Signed-off-by: Ryan Northey <ryan@synca.io>
@zuercher
Copy link
Member

Is this ready to go now?

@phlax
Copy link
Member Author

phlax commented Sep 29, 2020

yep

@mattklein123 mattklein123 merged commit 29e60a1 into envoyproxy:master Sep 30, 2020
@phlax phlax mentioned this pull request Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Shellcheck on CI for static analysis of Bash scripts
3 participants