-
Notifications
You must be signed in to change notification settings - Fork 415
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
feat: Add use_identity_overrides_in_local_eval setting #4612
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Docker builds report
|
Uffizzi Preview |
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.
Other than that looks good, but I'd like to have some explanation on why users would want to have this enabled or not. There are also docs like https://docs.flagsmith.com/clients/#local-evaluation-1 that need to be updated to take into account this new setting, but we can add this in a different PR
Good question as to why they'd want this enabled or not, I get the feeling that we'd start enabling this by default @gagantrivedi ? The only reason I see this being useful as an option is if a project uses flags on the backend and frontend and want to reserve identity overrides for just development and never want it hitting their backend. |
The reasoning is captured here https://flagsmith.slack.com/archives/CTF0THS2D/p1725270568904189?thread_ts=1724865246.072169&cid=CTF0THS2D |
Ok yep, bringing here for visibility
So essentially this is to avoid any unexpected changes |
@rolodato based on this, should we just have a small note somewhere on this rather than having it be too prominent in docs ? |
Let's merge this as-is and improve the docs later. Getting more people onboarded to this is more important than having perfect docs. We'd need to share more detailed context in the docs by explaining that this is only for SaaS, was/will be enabled by default for all new environments since a specific Flagsmith version - too much to fit in a small note IMO. |
Flagsmith feature linked:
|
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Flagsmith Feature
|
Thanks for submitting a PR! Please check the boxes below:
docs/
if required so people know about the feature!Changes
How did you test this code?
Toggled setting