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

Handle Client(..., security=False) #8980

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

jacobtomlinson
Copy link
Member

Currently the security kwarg of Client takes a few options:

  • A Security object with certs loaded
  • A dict of certs that get converted into a Security object
  • True to auto generate certs
  • None to skip using TLS (the default)

Given that it accepts a bool type this can cause confusion when passing False, which currently raises an exception #8973.

This PR makes security=False behave the same as security=None.

  • Tests added / passed
  • Passes pre-commit run --all-files

Closes #8973

Copy link
Contributor

github-actions bot commented Jan 9, 2025

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

    27 files  ± 0      27 suites  ±0   11h 34m 25s ⏱️ - 2m 20s
 4 126 tests + 1   3 997 ✅ + 1    125 💤 ±0  3 ❌  - 1  1 🔥 +1 
51 704 runs  +14  49 269 ✅ +15  2 429 💤 ±0  5 ❌  - 2  1 🔥 +1 

For more details on these failures and errors, see this check.

Results for commit 58b1dfa. ± Comparison against base commit 8d8c878.

♻️ This comment has been updated with latest results.

Copy link
Member

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

LGTM. Only minor suggestion might be an assertion about c.security to make sure we get the expected thing. Something like assert c.security.require_encrpytion is False? But I suspect we already test that with security=None so not a big deal.

@jacobtomlinson
Copy link
Member Author

Thanks for the review @TomAugspurger. That's a good suggestion, I've added that assertion.

@jacobtomlinson
Copy link
Member Author

CI failures are related to #8936. I'm going to merge this based on @TomAugspurger's review.

@jacobtomlinson jacobtomlinson merged commit c2ba834 into dask:main Jan 10, 2025
26 of 31 checks passed
@jacobtomlinson jacobtomlinson deleted the security-false branch January 10, 2025 11:43
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.

"TypeError: security must be a Security object" when initiating Client(security=False)
2 participants