-
-
Notifications
You must be signed in to change notification settings - Fork 214
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
Fix for verify=False ignoring #421
Conversation
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.
Testing with certificates is not so elegant. But I've no idea how to make it better.
Good job!
@@ -70,6 +70,7 @@ jobs: | |||
run: | | |||
echo "VERSION=${{ matrix.clickhouse-version }}" > tests/.env | |||
if [[ "${{ matrix.clickhouse-version }}" > "21.7" ]]; then echo "ORG=clickhouse"; else echo "ORG=yandex" ; fi >> tests/.env | |||
if [[ "21.10" = "`echo -e "21.10\n${{ matrix.clickhouse-version }}" | sort -V | head -n1`" ]]; then echo "TOP_LEVEL=clickhouse"; else echo "TOP_LEVEL=yandex" ; fi >> tests/.env |
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.
Can we replace "21.10" = "``echo -e "21.10\n${{ matrix.clickhouse-version }}" | sort -V | head -n1``"
with "${{ matrix.clickhouse-version }}" >= "21.10"
?
And the same in another place.
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.
No, we cannot. Because bash makes alphanumeric comparison, not version-wise. Please check if [[ "21.9" < "21.12" ]]; then echo "fine"; else echo "fail"; fi
tests/test_connect.py
Outdated
@@ -306,6 +306,11 @@ def test_client_revision(self): | |||
with self.created_client(client_revision=54032) as client: | |||
client.execute('SELECT 1') | |||
|
|||
def test_client_with_no_cert_validation(self): | |||
with self.created_client(port=9440, |
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
secureport=9440
intosetup.cfg
; - add attribute
secure_port
to classtests.testcase.BaseTestCase
; - use
self.secure_port
here instead of hardcoding.
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.
Fixed
@@ -8,6 +8,10 @@ services: | |||
- TZ=Europe/Moscow | |||
ports: | |||
- "127.0.0.1:9000:9000" | |||
- "127.0.0.1:9440:9440" |
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.
We should add port 9440 in development docs: docs/development.rst:48
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.
Fixed
Checklist:
flake8
and fix issues.pytest
no tests failed. See https://clickhouse-driver.readthedocs.io/en/latest/development.html.