tests: fixed running with run_code_analysis.sh script#11487
tests: fixed running with run_code_analysis.sh script#11487mabrarov wants to merge 1 commit intofluent:masterfrom
Conversation
📝 WalkthroughWalkthroughAdded a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/runtime_shell/go_plugins/build_test_plugins.sh (1)
110-118: Consider movingsudo_if_not_rootbefore its callers for clearer dependency order.The function is defined after
install_go_if_needed(which calls it at lines 49/51). This works because bash resolves function names at call time, but it's unconventional — a reader skimming top-to-bottom may assume a missing-function error. Moving it to beforeinstall_go_if_neededmakes the dependency obvious.♻️ Proposed move
+# Support of environment without sudo, but running as root user. +# Like container used in run_code_analysis.sh script. +sudo_if_not_root() { + if [ "$(id -u)" -eq 0 ]; then + "$@" + else + sudo "$@" + fi +} + install_go_if_needed() { ... } ... -# Support of environment without sudo, but running as root user. -# Like container used in run_code_analysis.sh script. -sudo_if_not_root() { - if [ "$(id -u)" -eq 0 ]; then - "$@" - else - sudo "$@" - fi -} - echo "Setting up Go build environment..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime_shell/go_plugins/build_test_plugins.sh` around lines 110 - 118, Move the sudo_if_not_root function definition so it appears before any callers such as install_go_if_needed; specifically, relocate the sudo_if_not_root function above the install_go_if_needed function (which calls sudo_if_not_root at lines referencing its calls) to make the dependency order explicit and avoid top-to-bottom confusion for readers while leaving the function body and its behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/runtime_shell/go_plugins/build_test_plugins.sh`:
- Around line 110-118: Move the sudo_if_not_root function definition so it
appears before any callers such as install_go_if_needed; specifically, relocate
the sudo_if_not_root function above the install_go_if_needed function (which
calls sudo_if_not_root at lines referencing its calls) to make the dependency
order explicit and avoid top-to-bottom confusion for readers while leaving the
function body and its behavior unchanged.
Signed-off-by: Marat Abrarov <abrarov@gmail.com>
mabrarov
force-pushed
the
feature/runtime_shell_run_code_analysis
branch
from
aaac15c to
8ff0e7b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/runtime_shell/go_plugins/build_test_plugins.sh (1)
57-69:⚠️ Potential issue | 🟡 MinorMinor edge-case regression:
sudois called when the user already holds write access.Both call sites are inside
if [ -w "/usr/local" ], which already guarantees that the current user can write to/usr/local. For a non-root user in that branch,sudo_if_not_rootwill invokesudounnecessarily — and will fail if that user has direct write access but nosudoconfigured, a regression from the previous direct-command behaviour.The primary use case (root in container without sudo) is handled correctly. If the broader non-root case also matters, consider removing the redundant
sudocall for users who already have direct write access:💡 Suggested alternative — only elevate when write access requires it
if [ -w "/usr/local" ]; then if [ -d /usr/local/go ]; then - sudo_if_not_root rm -rf /usr/local/go + rm -rf /usr/local/go fi - sudo_if_not_root mv go /usr/local/go + mv go /usr/local/go export PATH="/usr/local/go/bin:$PATH" elseWith this approach,
sudo_if_not_rootis only needed when the install target requires elevated access (i.e., the user does not have direct write access). For that scenario, a separate writable-path-with-fallback-to-sudo pattern would be needed if privileged install to/usr/localis desired. If the only environments in practice are root-in-container and non-root-with-home-install, the diff above is sufficient and removes the inconsistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime_shell/go_plugins/build_test_plugins.sh` around lines 57 - 69, The branch that checks if /usr/local is writable should not call sudo_if_not_root because [ -w "/usr/local" ] already guarantees the current user can write; update the /usr/local branch to perform rm -rf /usr/local/go and mv go /usr/local/go directly (use the existing sudo_if_not_root only in the fallback case where /usr/local is not writable), modifying the logic around the sudo_if_not_root calls so sudo is only used when write permission is absent (refer to the sudo_if_not_root helper and the if [ -w "/usr/local" ] branch to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/runtime_shell/go_plugins/build_test_plugins.sh`:
- Around line 57-69: The branch that checks if /usr/local is writable should not
call sudo_if_not_root because [ -w "/usr/local" ] already guarantees the current
user can write; update the /usr/local branch to perform rm -rf /usr/local/go and
mv go /usr/local/go directly (use the existing sudo_if_not_root only in the
fallback case where /usr/local is not writable), modifying the logic around the
sudo_if_not_root calls so sudo is only used when write permission is absent
(refer to the sudo_if_not_root helper and the if [ -w "/usr/local" ] branch to
locate the code).
2 participants
Fixed execution of tests within run_code_analysis.sh script which is recommended in Guide to Contributing to Fluent Bit.
Fixes #11011 (comment) introduced in #11011.
Testing
Before we can approve your change; please submit the following in a comment:
TEST_PRESET=valgrind SKIP_TESTS='flb-rt-out_td flb-it-network' ./run_code_analysis.shok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit