feat:add macos test #2943
feat:add macos test #2943
Conversation
WalkthroughThe changes in this pull request introduce a new job named Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
✏️ Feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/pika.yml (1)
Line range hint
241-245: Consider using version tag for docker/metadata-action.Instead of using a commit hash, consider using a version tag for better maintainability.
- uses: docker/metadata-action@9ec57ed1fcdbf14dcef7dfbe97b2010124a938b7 + uses: docker/metadata-action@v5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(1 hunks)
🔇 Additional comments (3)
.github/workflows/pika.yml (3)
221-221: LGTM: Integration test enabled for macOS.
The integration test execution is now properly enabled for macOS, maintaining consistency with other platforms (Ubuntu and Rocky Linux).
Line range hint 246-249: Verify artifact path in Dockerfile.
Ensure that the artifact download path (artifact/) matches the path expected in the Dockerfile's COPY/ADD instructions.
#!/bin/bash
# Check if Dockerfile references the correct artifact path
if [ -f "./ci/Dockerfile" ]; then
echo "Checking Dockerfile for artifact path references:"
grep -A 2 "COPY\|ADD" "./ci/Dockerfile"
fiLine range hint 223-260: Verify Docker build configuration and dependencies.
The Docker image build job looks well-structured but has a few points to consider:
- The job is configured to not push the image (
push: false). Is this intentional for testing purposes? - The job references
./ci/Dockerfilewhich needs verification.
Let's verify the Dockerfile existence and artifact path:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/integration/start_codis.sh (2)
32-34: Simplify working directory debug output.The current implementation uses two redundant commands to print the working directory. Consider keeping just one command with a descriptive comment explaining its purpose.
-pwd -echo "$(pwd)" +# Print current working directory for debugging Codis setup +pwd
Script needs cross-platform compatibility improvements
The script is used in multiple environments (Ubuntu, Rocky Linux, and macOS) as shown in the workflow file. The current macOS-specific sed syntax (
-i '') will cause failures in Linux environments. Here's what needs to be fixed:
- The sed commands need to be made cross-platform compatible
- The script lacks proper error handling and service readiness checks
- Missing cleanup and dependency verification
#!/bin/bash + set -e # Exit on error + # Cross-platform sed in-place replacement + sed_in_place() { + local file=$1 + local expr=$2 + if [[ "$OSTYPE" == "darwin"* ]]; then + sed -i '' "$expr" "$file" + else + sed -i "$expr" "$file" + fi + } + # Cleanup function + cleanup() { + pkill -9 pika || true + pkill -9 codis || true + rm -rf /tmp/codis codis_data_1 codis_data_2 + } + trap cleanup EXIT + # Check dependencies + for cmd in pika make nc; do + command -v "$cmd" >/dev/null 2>&1 || { echo >&2 "$cmd is required but not installed"; exit 1; } + done # ... rest of the script ... - sed -i '' -e 's|databases : 1|databases : 2|' ... ./pika_8000.conf + sed_in_place ./pika_8000.conf 's|databases : 1|databases : 2|' # ... rest of the script ... - sleep 10 # Replace arbitrary sleeps with service checks + timeout=30 + until nc -z 127.0.0.1 8000 >/dev/null 2>&1; do + ((timeout--)) || { echo "Timeout waiting for pika server"; exit 1; } + sleep 1 + done🔗 Analysis chain
Line range hint
1-99: Several critical improvements needed for script reliability.The script has several areas that could benefit from improvements:
- The commented-out cleanup code should be implemented as a proper cleanup function
- Sleep commands are not reliable for ensuring service readiness
- The sed commands use macOS-specific syntax (-i ''), which will fail on Linux
- Critical operations lack error checking
- No verification of required dependencies
Here's a suggested implementation structure:
#!/bin/bash +# Exit on error +set -e + +# Cleanup function +cleanup() { + echo "Cleaning up..." + pkill -9 pika || true + pkill -9 codis || true + rm -rf /tmp/codis codis_data_1 codis_data_2 +} + +# Check dependencies +check_dependencies() { + command -v pika >/dev/null 2>&1 || { echo "pika is required but not installed"; exit 1; } + command -v make >/dev/null 2>&1 || { echo "make is required but not installed"; exit 1; } +} + +# Function to wait for service +wait_for_service() { + local host=$1 + local port=$2 + local timeout=$3 + local start_time=$(date +%s) + + while ! nc -z "$host" "$port" >/dev/null 2>&1; do + if [ $(($(date +%s) - start_time)) -gt "$timeout" ]; then + echo "Timeout waiting for $host:$port" + exit 1 + fi + sleep 1 + done +} + +# Handle cleanup on script exit +trap cleanup EXIT + +# Check dependencies +check_dependencies + # Rest of your existing code... -sleep 10 +# Replace sleep with proper service checks +wait_for_service 127.0.0.1 8000 30 +wait_for_service 127.0.0.1 8001 30 # Modify sed commands to work on both Linux and macOS -sed -i '' -e 's|databases : 1|databases : 2|' ... +sed -i.bak -e 's|databases : 1|databases : 2|' ... && rm -f pika_8000.conf.bakLet's verify if this script is used in different OS environments:
Let's verify the OS environment where this script is being used to confirm if the macOS-specific sed syntax is indeed a problem.
Let's verify the runner OS to confirm our concerns about the OS-specific syntax.
Let's examine the complete workflow file to understand the execution environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this script is used in different OS environments rg -l "start_codis.sh" .github/workflows/Length of output: 68
Script:
#!/bin/bash # Check the OS environment used in the workflow rg -A 10 "start_codis.sh" .github/workflows/pika.ymlLength of output: 1111
Script:
#!/bin/bash # Check the runner OS specified in the workflow rg -B 5 "start_codis.sh" .github/workflows/pika.yml | rg "runs-on:"Length of output: 65
Script:
#!/bin/bash # Let's try a different approach to check the workflow file content cat .github/workflows/pika.ymlLength of output: 8125
🧰 Tools
🪛 Shellcheck
[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/integration/start_codis.sh (2)
32-34: Remove redundant pwd command.The commands
pwdandecho "$(pwd)"are redundant as they both print the current working directory. Use just one of them for clarity.-pwd -echo "$(pwd)" +pwd
Line range hint
1-1: Consider improving script reliability and maintainability.The script handles critical infrastructure setup but could benefit from several improvements:
- Add proper documentation of prerequisites and expected environment
- Implement proper cleanup handling (currently commented out)
- Add validation for required tools (pika, codis, etc.)
- Add status checks after starting services
- Implement consistent error handling throughout the script
- Add logging for better debugging
Would you like me to help implement these improvements? I can provide a more robust implementation that includes these features.
🧰 Tools
🪛 Shellcheck
[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/integration/start_codis.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
tests/integration/start_codis.sh
[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
tests/integration/start_codis.sh
Outdated
| pwd | ||
| echo "$(pwd)" | ||
|
|
||
| cd ../../codis |
There was a problem hiding this comment.
💡 Codebase verification
The review comment's concerns are valid but need adjustment
The script is part of an integration test setup where the codis directory exists at the correct relative path from the test directory. However, error handling should still be added for robustness.
Apply this modified diff that preserves the test context:
-cd ../../codis
-make
+if ! cd ../../codis; then
+ echo "Error: Failed to change to codis directory from $(pwd)" >&2
+ exit 1
+fi
+
+if ! make; then
+ echo "Error: Codis build failed" >&2
+ exit 1
+fi🔗 Analysis chain
Add error handling for directory navigation and build operations.
The current implementation has several reliability issues:
- No handling of cd command failure
- No verification of make command success
- Using relative path without validation
Apply this diff to improve reliability:
-cd ../../codis
-make
+if ! cd ../../codis; then
+ echo "Error: Failed to change directory to codis" >&2
+ exit 1
+fi
+
+if ! make; then
+ echo "Error: Failed to build codis" >&2
+ exit 1
+fiLet's verify the codis directory structure:
Let me verify the repository structure to better understand the context.
Let me check the content of the start_codis.sh script to understand its context better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the expected directory structure exists
if [ -d "../../codis" ]; then
echo "Codis directory found"
ls -la ../../codis
else
echo "Warning: Codis directory not found at expected location"
fi
Length of output: 145
Script:
#!/bin/bash
# Check current directory and list contents
pwd
echo "Current directory contents:"
ls -la
# Find any directory named codis
echo -e "\nSearching for codis directory:"
fd -t d codis
# Check the test file location
echo -e "\nLocating start_codis.sh:"
fd start_codis.sh
Length of output: 2762
Script:
#!/bin/bash
# Check the content and location of start_codis.sh
cat tests/integration/start_codis.sh
# Check if there's a Makefile in the codis directory
ls -la codis/Makefile
Length of output: 2713
🧰 Tools
🪛 Shellcheck
[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/pika.yml (3)
213-213: Remove trailing spaces.There are trailing spaces on line 213 that should be removed.
🧰 Tools
🪛 yamllint
[error] 213-213: trailing spaces
(trailing-spaces)
Line range hint
233-238: Use semantic versioning for docker/metadata-action.Instead of using a commit hash, prefer semantic versioning for better maintainability and security tracking.
Apply this change:
- uses: docker/metadata-action@9ec57ed1fcdbf14dcef7dfbe97b2010124a938b7 + uses: docker/metadata-action@v5🧰 Tools
🪛 yamllint
[error] 213-213: trailing spaces
(trailing-spaces)
Line range hint
247-254: Consider enabling Docker layer caching.Enable Docker layer caching to speed up builds and reduce resource usage.
Add cache configuration:
uses: docker/build-push-action@v5 with: context: . file: ./ci/Dockerfile push: false tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} + cache-from: type=gha + cache-to: type=gha,mode=max🧰 Tools
🪛 yamllint
[error] 213-213: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 213-213: trailing spaces
(trailing-spaces)
| working-directory: ${{ github.workspace }}/build | ||
| run: | | ||
| cd tests/integration/ | ||
| cd ../../tests/integration/ | ||
| chmod +x integrate_test.sh | ||
| # sh integrate_test.sh | ||
| sh integrate_test.sh |
There was a problem hiding this comment.
Fix incorrect directory navigation in Go E2E tests.
The cd ../../tests/integration/ command from the build directory would navigate outside the workspace. This should be adjusted to use the correct relative path.
Apply this fix:
working-directory: ${{ github.workspace }}/build
run: |
- cd ../../tests/integration/
+ cd ../tests/integration/
chmod +x integrate_test.sh
sh integrate_test.shCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/pika.yml (3)
175-186: Consider pinning the zlib version for better reproducibility.While the zlib management steps are good, consider pinning the version to ensure consistent builds across different environments.
- brew install zlib + brew install zlib@1.2.13 # or your desired stable version
Line range hint
234-270: Consider adding a test step for the Docker image.The Docker image building job looks well-structured, but it would be beneficial to add a test step to verify the built image works correctly before potential future pushes.
Example test step to add:
- name: Test Docker image run: | docker run --rm pikadb/pika:${{ github.sha }} --version # Add more functional tests as needed🧰 Tools
🪛 yamllint
[error] 225-225: trailing spaces
(trailing-spaces)
225-225: Remove trailing spaces.There are trailing spaces at the end of this line.
🧰 Tools
🪛 yamllint
[error] 225-225: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pika.yml(2 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pika.yml
[error] 225-225: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/pika.yml (1)
230-232:
Fix incorrect directory navigation in Go E2E tests.
The cd ../../tests/integration/ command from the build directory would navigate outside the workspace.
- cd ../../tests/integration/
+ cd ../tests/integration/
Summary by CodeRabbit
New Features
start_codis.shscript for improved functionality in starting the Codis environment, including expanded configurations and debugging aids.Bug Fixes