feat(flags): Fetch flags and team from database #23120
Conversation
| pub payloads: Option<serde_json::Value>, | ||
| pub super_groups: Option<Vec<FlagGroupType>>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Deserialize)] | ||
| pub struct FeatureFlag { | ||
| pub id: i64, | ||
| pub team_id: i64, | ||
| pub id: i32, |
There was a problem hiding this comment.
switched types here to reflect db type, otherwise it'd complain about the conversion
| pub team_id: i32, | ||
| pub name: Option<String>, | ||
| pub key: String, | ||
| pub filters: serde_json::Value, |
There was a problem hiding this comment.
I've opted to first parse filters out as just a json value, and then deserialise that into FlagFilters for better handling, and because it was getting pretty hard to try and coerce directly into the struct form I want.
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
There was a problem hiding this comment.
I'm going to conditionally approve this to see if CI behaves in the way I expect – we're getting this odd "status could not be reported" error from the Rust services test, but each of the test jobs themselves have run correctly. I want to see if that job reporting a yellow status blocks our ability to merge (my guess is that it does not), but just checking here.
There was a problem hiding this comment.
I'm going to conditionally approve this to see if CI behaves in the way I expect – we're getting this odd "status could not be reported" error from the Rust services test, but each of the test jobs themselves have run correctly. I want to see if that job reporting a yellow status blocks our ability to merge (my guess is that it does), but just checking here.
dmarticus
dismissed
their stale review
turns out this hanging job is actually blocking our ability to merge, how dumb.
|
(this is happening because I updated CI to use a matrix which changes the names of the test 😮💨 ) - you can remove this test requirement for now, merge, and then re-add it to branch protection rules with the (flags / others) suffix ✅ |
Hi Neil, I hope this didn't bother you too much during your break! I chatted with Jams about this and we came to a similar conclusion, but thank you for weighing in :) |
| @@ -22,3 +22,39 @@ where | |||
| .route("/flags", post(v0_endpoint::flags).get(v0_endpoint::flags)) | |||
| .with_state(state) | |||
| } | |||
|
|
|||
| // TODO, eventually we can differentiate read and write postgres clients, if needed | |||
| // I _think_ everything is read-only, but I'm not 100% sure yet | |||
There was a problem hiding this comment.
there shall cometh a write client too 🫠
see the part below this line: https://github.com/PostHog/posthog/blob/master/posthog/models/feature_flag/flag_matching.py#L797
Co-authored-by: Dylan Martin <[email protected]> Co-authored-by: James Greenhill <[email protected]>
Problem
Pipeline-folks, would love some eyes on the CI changes, if its fine with you, and if you have any comments on the db handling here :)
TODO:
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?