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

e2e(generic): deal with proc not found #1539

Merged
merged 3 commits into from
Jul 24, 2024
Merged

Conversation

mayconamaroCW
Copy link
Contributor

@mayconamaroCW mayconamaroCW commented Jul 24, 2024

PR Type

Enhancement, Bug fix


Description

  • Enhanced the find command in the cleanup function to include the -xdev option, preventing traversal of directories from other filesystems.
  • Updated the killport command in the run_test function to include || true, ensuring the script does not block if the command fails.

Changes walkthrough 📝

Relevant files
Enhancement
main.sh
Enhance cleanup and killport handling in main.sh                 

chaos/experiments/main.sh

  • Added -xdev option to find commands to prevent traversing directories
    from other filesystems.
  • Modified killport command to not block execution if it fails.
  • +3/-3     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Smell
    The killport command in line 267 uses || true to ensure the script does not block if the command fails. This can potentially hide errors that should be addressed. Consider logging the error instead of ignoring it.

    Copy link

    github-actions bot commented Jul 24, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add double quotes around variables to prevent word splitting or globbing

    Use double quotes around variable $leader_port to prevent potential word splitting
    or globbing issues.

    chaos/experiments/main.sh [267]

    -killport --quiet $leader_port || true
    +killport --quiet "$leader_port" || true
     
    Suggestion importance[1-10]: 10

    Why: This suggestion follows best practices for shell scripting by preventing potential word splitting or globbing issues, which can lead to unexpected behavior. It is a crucial improvement for script reliability.

    10
    Performance
    Combine redundant find commands into a single command for better performance

    The find command in lines 64 and 67 can be optimized by combining them into a single
    command to reduce redundancy and improve performance.

    chaos/experiments/main.sh [64-67]

    -find . -xdev -type d -name "instance_*" -print0 | xargs -0 rm -rf 2>/dev/null || true
    -find . -xdev -type d -name "tmp_rocks_*" -print0 | xargs -0 rm -rf 2>/dev/null || true
    +find . -xdev \( -type d -name "instance_*" -o -type d -name "tmp_rocks_*" \) -print0 | xargs -0 rm -rf 2>/dev/null || true
     
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies and addresses redundancy in the find commands, which can improve performance and readability. The combined command is logically sound and maintains the intended functionality.

    9

    @mayconamaroCW mayconamaroCW enabled auto-merge (squash) July 24, 2024 19:26
    @mayconamaroCW mayconamaroCW merged commit 87bf2d3 into main Jul 24, 2024
    31 checks passed
    @mayconamaroCW mayconamaroCW deleted the e2e/generic-response branch July 24, 2024 19:30
    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.

    2 participants