Skip to content

fix: parse PUSHER_USE_SSL and PUSHER_DISABLED env booleans correctly - #6583

Open
SueJianjian wants to merge 3 commits into
keephq:mainfrom
SueJianjian:codex/fix-pusher-use-ssl-bool
Open

fix: parse PUSHER_USE_SSL and PUSHER_DISABLED env booleans correctly#6583
SueJianjian wants to merge 3 commits into
keephq:mainfrom
SueJianjian:codex/fix-pusher-use-ssl-bool

Conversation

@SueJianjian

@SueJianjian SueJianjian commented Jun 16, 2026

Copy link
Copy Markdown

Summary

  • parse PUSHER_USE_SSL and PUSHER_DISABLED using explicit boolean coercion instead of Python truthiness
  • preserve existing behavior for already-boolean values
  • add focused regression coverage for string env inputs

Closes #6582

Testing

  • .venv313\Scripts\python.exe -m pytest tests/test_dependencies.py -q

@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jun 16, 2026
@CLAassistant

CLAassistant commented Jun 16, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@SueJianjian SueJianjian changed the title Fix PUSHER_USE_SSL boolean parsing fix: parse PUSHER_USE_SSL and PUSHER_DISABLED env booleans correctly Jun 16, 2026

@shahargl shahargl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is a bit over engineering, you can use Config class for that

@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jun 23, 2026
@SueJianjian

Copy link
Copy Markdown
Author

Updated this to use the existing Config bool parsing instead of a custom helper, and switched the tests to cover get_pusher_client() behavior directly. Could you take another look when you have a moment?

@SueJianjian

Copy link
Copy Markdown
Author

Hi @shahargl, I updated this to use the existing Config bool parsing path and adjusted the tests to cover get_pusher_client() behavior directly. Could you please take another look when you have a moment?

@SueJianjian

Copy link
Copy Markdown
Author

Quick follow-up for review: the requested Config-based boolean parsing is in place on the current head, with focused get_pusher_client() tests and all checks passing. @shahargl, could you re-review when convenient?

@shahargl
shahargl self-requested a review July 23, 2026 08:14

@shahargl shahargl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: PUSHER_USE_SSL is incorrectly parsed

3 participants