-
Notifications
You must be signed in to change notification settings - Fork 51
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
Update chart to Qdrant 1.12.3 #263
base: main
Are you sure you want to change the base?
Conversation
@test "no startup warnings in logs" { | ||
skip "Skip for 1.12.2 which has a known warning" | ||
run kubectl logs -n qdrant-helm-integration qdrant-0 | ||
[ $status -eq 0 ] | ||
[[ "${output}" =~ .*INFO.* ]] | ||
if [[ ! "${output}" =~ .*WARN.* ]]; then | ||
echo "Found warning output:" | ||
echo "${output}" | ||
return 1 | ||
fi | ||
[[ ! "${output}" =~ .*WARN.* ]] | ||
} |
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.
Here I reverted the test change made in #262 because it's fixed in Qdrant 1.12.3.
Not entirely sure if reverting this is correct.
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.
@timvisee I think we can remove the skip
as it is fixed now, but we can keep the additional echo
's to see directly what is wrong.
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.
The logic seems wrong here, it is written as if output does not contain WARN
, then return 1 and echo output. While it should be if it contains WARN
I would say, so:
if [[ "${output}" =~ .*WARN.* ]]; then
This should fix the test as it currently errors without output that does not contain WARN
😛
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.
Makes sense, I just reverted it. It seems it was wrong before as well.
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.
Patched in 8d9c4ce
f5cb782
to
49c9734
Compare
Update for Qdrant 1.12.3.
Follows the pattern of #262.