chore: Add appsmith user existence check for auth tests
#38069
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Learnings (1)deploy/docker/tests/test-pg-auth.sh (1)🔇 Additional comments (3)deploy/docker/tests/test-pg-auth.sh (3)
The wait_for_postgres call ensures PostgreSQL is ready before proceeding with instance checks.
The error handling implementation is consistent with the first test case.
The implementation includes proper error handling and clear error messages. Let's verify the check_user_exists implementation: ✅ Verification successfulImplementation of check_user_exists function is robust and secure The function implementation includes:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Verify check_user_exists implementation in pg-test-utils.sh
rg -l "check_user_exists" ./deploy/docker/tests/pg-test-utils.sh
Length of output: 103 Script: #!/bin/bash
# Get the implementation of check_user_exists function
rg -A 10 "^check_user_exists" ./deploy/docker/tests/pg-test-utils.sh
Length of output: 489 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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
deploy/docker/tests/test-pg-auth.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
deploy/docker/tests/test-pg-auth.sh
[warning] 115-115: user_exists appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (2)
deploy/docker/tests/test-pg-auth.sh (2)
207-215: LGTM - Good integration of user existence check
The check is properly placed before testing access permissions, which should help reduce test flakiness.
282-290: LGTM - Consistent implementation across test functions
The integration follows the same pattern as the first test function, maintaining consistency.
Let's verify the effectiveness of this change in reducing test flakiness:
✅ Verification successful
The previous GitHub API query didn't yield results, so let's examine the test implementation directly to verify the consistency and potential flakiness improvements.
I need to see the complete test file to verify the consistency of error handling patterns and test implementation across functions, as the previous output only showed a partial implementation.
Implementation verified - Consistent error handling and test patterns
The test functions maintain consistent implementation patterns:
- Both test functions use identical error checking with
check_user_datasource_access_with_auth - Error handling follows the same pattern with proper exit codes
- Both functions verify user existence before access checks
- The implementation includes proper wait conditions and retries to reduce flakiness
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check recent test runs to compare failure rates
gh api graphql -f query='
{
repository(owner: "appsmithorg", name: "appsmith") {
pullRequests(last: 50, states: [OPEN, MERGED]) {
nodes {
title
commits(last: 1) {
nodes {
commit {
checkSuites(first: 1) {
nodes {
checkRuns(first: 100) {
nodes {
name
conclusion
}
}
}
}
}
}
}
}
}
}
}' | jq '.data.repository.pullRequests.nodes[] | select(.title | contains("pg-auth-test"))'
Length of output: 645
Script:
#!/bin/bash
# Let's check the test file content and its recent changes
git log -p --max-count=1 deploy/docker/tests/test-pg-auth.sh
# Also check for any related test files
fd -e sh . deploy/docker/tests/ -x grep -l "check_user_datasource_access_with_auth"
# And examine the function implementation
grep -A 10 "check_user_datasource_access_with_auth()" deploy/docker/tests/test-pg-auth.sh
Length of output: 636
Script:
#!/bin/bash
# Let's examine the full test file to understand the error handling and test patterns
cat deploy/docker/tests/test-pg-auth.sh
# Also check for any recent issues or PRs related to this test
fd . deploy/docker/tests/ -x grep -l "test-pg-auth"
Length of output: 12278
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
deploy/docker/tests/test-pg-auth.sh (1)
206-214: Improve error handling in test functionsBoth test functions should handle the user existence check failure explicitly:
- check_user_exists appsmith + if ! check_user_exists appsmith; then + echo "Failed to verify appsmith user existence" + echo "Test ${FUNCNAME[0]} Failed ❌" + exit 1 + fiAlso applies to: 281-289
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
deploy/docker/tests/test-pg-auth.sh(3 hunks)
🔇 Additional comments (2)
deploy/docker/tests/test-pg-auth.sh (2)
206-208: Verify potential race condition between user check and access check
There's a potential race condition between checking user existence and verifying access. Consider combining these checks or implementing a transaction-like approach.
Also applies to: 281-283
111-128: 🛠️ Refactor suggestion
Add input validation and error handling to improve robustness
The function implementation needs several improvements for better reliability:
check_user_exists() {
local user
user=$1
+ # Validate input parameter
+ if [ -z "$user" ]; then
+ echo "Error: User parameter is required"
+ return 1
+ }
+ # Use consistent retry configuration with other functions
+ local max_retries=${PG_MAX_RETRIES:-200}
+ local retry_interval=${PG_RETRY_INTERVAL:-2}
local retry_count=0
- local max_retries=200
while [ $retry_count -lt $max_retries ]; do
- if docker exec "${container_name}" bash -c "psql -p 5432 -U postgres -c \"\du\" | grep -q -w \"$user\""; then
+ if docker exec "${container_name}" bash -c "psql -p 5432 -U postgres -c \"\du\" 2>/dev/null | grep -q -w \"$user\""; then
echo "$user user exists."
return 0
fi
echo "Waiting for $user user to be created... (Attempt: $((retry_count + 1)))"
retry_count=$((retry_count + 1))
- sleep 1
+ sleep "$retry_interval"
done
echo "$user user does not exist."
return 1
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
deploy/docker/tests/test-pg-auth.sh (1)
140-147: Enhance error handling for user existence checkConsider adding descriptive output messages for the user existence check to improve test debugging.
- check_user_exists appsmith + echo "Checking if appsmith user exists..." + if ! check_user_exists appsmith; then + echo "Test failed: appsmith user does not exist ❌" + exit 1 + fi + echo "appsmith user exists ✅"Also applies to: 214-221
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
deploy/docker/tests/pg-test-utils.sh (1)
2-18: Consider making retry parameters configurableThe retry count and sleep duration are hardcoded. Consider making these configurable via environment variables.
+# Default retry settings +: "${MAX_HEALTH_RETRIES:=200}" +: "${HEALTH_RETRY_INTERVAL:=2}" + is_appsmith_instance_ready() { - local max_retries=200 + local max_retries="$MAX_HEALTH_RETRIES" local retry_count=0 local response_code
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deploy/docker/tests/pg-test-utils.sh(1 hunks)deploy/docker/tests/test-pg-auth.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
deploy/docker/tests/pg-test-utils.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 26-26: container_name is referenced but not assigned.
(SC2154)
🔇 Additional comments (2)
deploy/docker/tests/test-pg-auth.sh (2)
123-124: LGTM: Proper sequencing of operations
The wait_for_postgres call before checking user existence ensures proper test setup.
201-208:
Add error handling for check_user_exists
Similar to the previous instance, add error handling here as well.
- check_user_exists appsmith
+ if ! check_user_exists appsmith; then
+ echo "Appsmith user does not exist"
+ echo "Test ${FUNCNAME[0]} Failed ❌"
+ exit 1
+ fi
# Check if the Appsmith user has read access to databases
Description
PR to add the check for
appsmithuser existence before any assertions in pg-auth-test to remove the flakiness.🔍 Cypress test results
Warning
Tests have not run on the HEAD 0ac8736 yet
Fri, 13 Dec 2024 05:18:03 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Tests