-
Notifications
You must be signed in to change notification settings - Fork 375
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
[O11y][Couchbase] Fix system tests #9896
[O11y][Couchbase] Fix system tests #9896
Conversation
🚀 Benchmarks reportTo see the full report comment with |
/test |
@@ -5,13 +5,16 @@ do | |||
done | |||
|
|||
# add "beer-sample" bucket from sampleBuckets | |||
curl -v -u Administrator:password -X POST http://127.0.0.1:8091/sampleBuckets/install -d '["beer-sample"]' | |||
until [ "$(curl -v -u Administrator:password -X POST http://localhost:8091/sampleBuckets/install -d '["beer-sample"]' -o /dev/null -w '%{http_code}')" -eq 202 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of variables are repeating. Let's rewrite this script to be more readable, robust and maintainable.
ADMIN_USER="Administrator"
ADMIN_PASSWORD="password"
CB_HOST="localhost"
CB_PORT="8091"
BUCKET_NAME="beer-sample"
XDCR_CLUSTER_NAME="cluster"
XDCR_HOSTNAME="127.0.0.1"
REPLICATION_FROM_BUCKET="beer-sample"
REPLICATION_TO_CLUSTER="cluster"
REPLICATION_TO_BUCKET="travel-sample"
We can also use a single var for the host i.e., localhost or 127.0.0.1. Currently, both are there.
Also, if you wrap it in functions, this scripts looks cleaner.
is_couchbase_ready() {
curl --silent --fail "http://${ADMIN_USER}:${ADMIN_PASSWORD}@${CB_HOST}:${CB_PORT}/pools/default" >/dev/null 2>&1
}
# To call the function
is_couchbase_ready
As no output from curl is necessary, redirect the standard streams to /dev/null. Health checks are typically written in this manner to avoid unnecessary actions.
Another sample function to make it more readable:
# Function to add sample bucket
add_cb_sample_bucket() {
local bucket_name="$1"
local http_code
http_code=$(curl --silent --output /dev/null --write-out "%{http_code}" \
--user "${ADMIN_USER}:${ADMIN_PASSWORD}" \
--header "Content-Type: application/json" \
--request POST \
--data "[\"${bucket_name}\"]" \
"http://${CB_HOST}:${CB_PORT}/sampleBuckets/install")
if [[ "${http_code}" -eq 202 ]]; then
return 0
else
return 1
fi
}
And call it like:
while ! add_cb_sample_bucket "${BUCKET_NAME}"; do
sleep 5
done
With this approach, the script looks much cleaner and also maintainable. Let me know what you think?
until [ "$(curl -v -u Administrator:password -X POST http://localhost:8091/sampleBuckets/install -d '["beer-sample"]' -o /dev/null -w '%{http_code}')" -eq 202 ] | ||
do | ||
sleep 5s | ||
done | ||
|
||
# using couchbase-cli run xdcr-setup for the cluster | ||
couchbase-cli xdcr-setup -c 127.0.0.1 -u Administrator -p password --create --xdcr-cluster-name cluster --xdcr-hostname 127.0.0.1 --xdcr-username Administrator --xdcr-password password | ||
|
||
# wait till the xdcr-setup creates cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it is better to write that you are waiting for bucket to be ready. Comment is not clear here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a command to check if bucket is ready.
@@ -7,7 +7,7 @@ services: | |||
- ./files/setup.sh:/setup.sh | |||
- ./files/entrypoint.sh:/entrypoint.sh | |||
healthcheck: | |||
test: ["CMD", "curl", "-f", "http://admin:password@localhost:8091/pools/default/"] | |||
test: ["CMD", "curl", "-f", "http://Administrator:password@localhost:8091/pools/default/buckets/beer-sample/stats"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add -s as well here. silent and fail to all curl
… into fix_couchbase_system_tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Script could still be better but let's do it some other time. For now, this is good. Thanks for addressing the comments.
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
/test |
1 similar comment
/test |
Co-authored-by: subham sarkar <sarkar.subhams2@gmail.com>
/test |
… into fix_couchbase_system_tests
💚 Build Succeeded
History
|
Quality Gate passedIssues Measures |
Proposed commit message
Fixes the flaky tests which result in error message
one or more errors found in documents stored in metrics-couchbase.xdcr-ep data stream: [0] found error.message in event: [invalid character 'R' looking for beginning of value]
Checklist
changelog
as this change doesn't relate to the changes that should be visible at user's endHow to test this PR locally
Related issues
Screenshots