feat: Toggleable use_https param for flags - #198
Conversation
Confidence Score: 5/5The PR appears safe to merge with no outstanding code findings. The concrete configurations preserve their existing positional layouts, default to HTTPS, and consistently apply the selected scheme to both synchronous and asynchronous clients. Both previous threads were manually resolved.
|
| Filename | Overview |
|---|---|
| mixpanel/flags/types.py | Places use_https after existing concrete configuration fields to preserve positional argument compatibility. |
| mixpanel/flags/local_feature_flags.py | Selects the local feature-flag client URL scheme from use_https. |
| mixpanel/flags/remote_feature_flags.py | Selects the remote feature-flag client URL scheme from use_https. |
| mixpanel/flags/test_local_feature_flags.py | Covers transport selection, positional compatibility, URL construction, and proxy headers. |
| mixpanel/flags/test_remote_feature_flags.py | Covers transport selection, URL construction, and proxy headers. |
Reviews (4): Last reviewed commit: "fix positional bug" | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #198 +/- ##
==========================================
+ Coverage 96.39% 96.48% +0.08%
==========================================
Files 15 15
Lines 2888 2959 +71
Branches 176 180 +4
==========================================
+ Hits 2784 2855 +71
Misses 67 67
Partials 37 37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
efahk
left a comment
There was a problem hiding this comment.
recommend using 'verifyCert' similar to what tracking uses in init.py
use_https paramverify_cert param
This reverts commit 2255793.
verify_cert paramuse_https param for flags
| class LocalFlagsConfig(FlagsConfig): | ||
| enable_polling: bool = True | ||
| polling_interval_in_seconds: int = 60 | ||
| use_https: bool = True |
There was a problem hiding this comment.
Just needs to be on the base FlagsConfig
|
Is this change coming from as customer request? I'd prefer not to lower the security posture, even if https is still the default. |
@tylerjroach This is coming from internal needs to develop against. Some of our internal devbox servers are sending traffic through http, and if we don't have an option to support it here we have to standup a certain amount of infrastructure just to allow a request to go through in devbox |
|
Got it. Maybe we can sync tomorrow and discuss options here a bit more. |
Defaults to true. Allows disabling
use_httpsto allow passing through http connections without cert checking or failing the TLS handshake. To be used for dev / local environments.