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

Chaos local setup #895

Merged
merged 5 commits into from
May 22, 2024
Merged

Chaos local setup #895

merged 5 commits into from
May 22, 2024

Conversation

renancloudwalk
Copy link
Contributor

No description provided.

@renancloudwalk renancloudwalk requested a review from a team as a code owner May 22, 2024 12:14
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

4, due to the complexity and breadth of the changes across multiple files and technologies including shell scripts, Docker, Kubernetes, and Rust configuration. The PR introduces significant functionality for setting up a local Kubernetes environment and requires careful validation of scripts and configurations.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The script install-dependencies.sh does not handle the case where the OS is neither Darwin nor Linux, which might lead to unhandled scenarios.

Possible Bug: The install-dependencies.sh script uses hardcoded paths and versions which might not be compatible with all system configurations or future versions.

🔒 Security concerns

No

Code feedback:
relevant filechaos/install-dependencies.sh
suggestion      

Consider adding a check for unsupported operating systems in the install-dependencies.sh script. This will prevent the script from running on unsupported systems and provide a clear message to the user. For example:

else
    echo "Unsupported OS. Exiting..."
    exit 1

[important]

relevant linefi

relevant filechaos/install-dependencies.sh
suggestion      

To avoid potential issues with hardcoded versions, consider fetching the latest stable versions dynamically or allowing the user to specify versions as arguments to the script. This makes the script more flexible and maintainable. For example, replace the hardcoded version of kind with a variable that can be set via command line or defaults to a stable version.
[important]

relevant linecurl -Lo ./kind https://kind.sigs.k8s.io/dl/v0.11.1/kind-linux-amd64

relevant filechaos/install-dependencies.sh
suggestion      

Implement error handling after critical commands to ensure that the script stops on errors and provides a clear message. This can be done using set -e at the beginning of the script or checking the exit status of each command. For example:

curl -Lo ./kind https://kind.sigs.k8s.io/dl/v0.11.1/kind-linux-amd64 || { echo "Failed to download kind"; exit 1; }

[important]

relevant linecurl -Lo ./kind https://kind.sigs.k8s.io/dl/v0.11.1/kind-linux-amd64

relevant filejustfile
suggestion      

Consider adding a check to ensure that Docker is running before attempting to build and load Docker images. This can prevent the script from failing at a later stage and provide a clearer error message to the user. For example:

docker info >/dev/null 2>&1 || { echo "Docker is not running. Please start Docker and retry."; exit 1; }

[medium]

relevant linedocker build -t local/run_with_importer -f ./docker/Dockerfile.run_with_importer .

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Best practice
Define resource requests and limits for containers to improve resource management

Add resource limits and requests to the container specification to ensure predictable
resource usage and better cluster resource management.

chaos/local-deployment.yaml [18-20]

 containers:
   - name: stratus-api
     image: local/run_with_importer:latest
+    resources:
+      requests:
+        memory: "64Mi"
+        cpu: "250m"
+      limits:
+        memory: "128Mi"
+        cpu: "500m"
 
Suggestion importance[1-10]: 10

Why: Adding resource limits and requests is a best practice in Kubernetes deployments to ensure predictable resource usage and improve cluster resource management, which is crucial for maintaining stability and performance.

10
Possible issue
Ensure necessary commands are available before attempting to use them

Replace the direct usage of brew and curl commands with checks to ensure these commands
are available on the system. This will prevent the script from failing in environments
where these commands are not installed.

chaos/install-dependencies.sh [8-18]

-brew install kind
-curl -Lo ./kind https://kind.sigs.k8s.io/dl/v0.11.1/kind-linux-amd64
+if command -v brew >/dev/null 2>&1; then
+    brew install kind
+else
+    echo "Error: brew is not installed."
+    exit 1
+fi
+if command -v curl >/dev/null 2>&1; then
+    curl -Lo ./kind https://kind.sigs.k8s.io/dl/v0.11.1/kind-linux-amd64
+    chmod +x ./kind
+    sudo mv ./kind /usr/local/bin/kind
+else
+    echo "Error: curl is not installed."
+    exit 1
+fi
 
Suggestion importance[1-10]: 9

Why: This suggestion adds robustness to the script by ensuring that necessary commands are available before attempting to use them, which can prevent failures in environments where these commands are not installed.

9
Implement error handling for critical operations in the script

Add error handling for commands that might fail during the setup and cleanup processes to
ensure the script handles failures gracefully.

justfile [473-475]

-kind delete cluster --name local-testing || true
-kind create cluster --name local-testing
+if ! kind delete cluster --name local-testing; then
+    echo "Failed to delete existing Kind cluster."
+    exit 1
+fi
+if ! kind create cluster --name local-testing; then
+    echo "Failed to create Kind cluster."
+    exit 1
+fi
 
Suggestion importance[1-10]: 9

Why: Implementing error handling for critical operations improves the robustness of the script by ensuring that failures are handled gracefully, which can prevent partial or inconsistent states.

9
Maintainability
Standardize the revm dependency version and features across configurations

Ensure consistent versioning for the revm dependency by using the same version and feature
set across different target configurations to avoid potential compatibility issues.

Cargo.toml [65-67]

 revm = { version = "=8.0.0", features = ["asm-keccak"] }
-revm = { version = "8.0.0", default-features = false }
+[target.'cfg(all(target_arch = "aarch64", target_os = "linux"))'.dependencies]
+revm = { version = "=8.0.0", default-features = false, features = ["asm-keccak"] }
 
Suggestion importance[1-10]: 8

Why: Standardizing the revm dependency version and features across configurations can prevent compatibility issues and ensure consistent behavior across different environments.

8

@renancloudwalk renancloudwalk enabled auto-merge (squash) May 22, 2024 14:59
@renancloudwalk renancloudwalk merged commit fee3df5 into main May 22, 2024
25 checks passed
@renancloudwalk renancloudwalk deleted the chaos-setup2 branch May 22, 2024 15:00
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.

1 participant