Skip to content
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

Merged
merged 10 commits into from
Mar 6, 2024
Merged

Conversation

akurdyukov
Copy link
Contributor

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • [-] Add or update relevant docs, in the docs folder and in code.
  • Ensure PR doesn't contain untouched code reformatting: spaces, etc.
  • Run flake8 and fix issues.
  • Run pytest no tests failed. See https://clickhouse-driver.readthedocs.io/en/latest/development.html.

Copy link
Member

@xzkostyan xzkostyan left a 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
Copy link
Member

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.

Copy link
Contributor Author

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

@@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's:

  1. add secureport=9440 into setup.cfg;
  2. add attribute secure_port to class tests.testcase.BaseTestCase;
  3. use self.secure_port here instead of hardcoding.

Copy link
Contributor Author

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"
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@xzkostyan xzkostyan merged commit f72db86 into mymarilyn:master Mar 6, 2024
405 checks passed
@coveralls
Copy link

Coverage Status

coverage: 96.472% (+0.4%) from 96.061%
when pulling a04d3ab on akurdyukov:verify-cert
into a317102 on mymarilyn:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail to ignore certificate problems with verify=False
3 participants