-
Notifications
You must be signed in to change notification settings - Fork 5
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 cohort sync #49
Conversation
f19d00b
to
f23680c
Compare
packages/node/src/local/updater.ts
Outdated
const cohortIds = CohortUtils.extractCohortIds(flagConfigs); | ||
if (cohortIds && cohortIds.size > 0 && !this.cohortFetcher) { | ||
throw Error( | ||
'cohort found in flag configs but no cohort download configured', |
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.
Good that this is checked - but what if they want to use only a subset of flags in a deployment, and that subset doesn't require cohorts 🤔 (cc: @bgiori)
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.
Hold off on merging yet
packages/node/src/types/config.ts
Outdated
export const CohortConfigDefaults: Omit<CohortConfig, 'apiKey' | 'secretKey'> = | ||
{ | ||
cohortServerUrl: 'https://cohort-v2.lab.amplitude.com', | ||
maxCohortSize: 10_000_000, |
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.
max cohort size should be 2147483647
Summary
Add cohort targeting and download to SDK.
GH Action changes:
Tests
Unit tests created for all added functions.
Integration test created for user id and group id and group name cohort targeting for US endpoint.
Integration test for EU endpoint contains simple cohort targeting.
Manually tested cohort syncs at flag update interval when cohort was updated.
Checklist