Skip to content
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

Merged
merged 48 commits into from
Aug 27, 2024
Merged

feat: Add cohort sync #49

merged 48 commits into from
Aug 27, 2024

Conversation

zhukaihan
Copy link
Collaborator

@zhukaihan zhukaihan commented Jun 24, 2024

Summary

Add cohort targeting and download to SDK.
GH Action changes:

  • Using LTS node versions: 16, 18, 20, 22, 24. All are in either maintenance or active LTS.
  • Added env secrets.

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

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: no

@zhukaihan zhukaihan changed the title Add cohort sync feat: Add cohort sync Jun 24, 2024
@zhukaihan zhukaihan marked this pull request as ready for review July 15, 2024 17:12
packages/node/src/local/cohort/poller.ts Outdated Show resolved Hide resolved
packages/node/src/local/cohort/poller.ts Outdated Show resolved Hide resolved
packages/node/src/local/cohort/poller.ts Outdated Show resolved Hide resolved
packages/node/src/local/client.ts Outdated Show resolved Hide resolved
const cohortIds = CohortUtils.extractCohortIds(flagConfigs);
if (cohortIds && cohortIds.size > 0 && !this.cohortFetcher) {
throw Error(
'cohort found in flag configs but no cohort download configured',
Copy link
Collaborator

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)

Copy link
Collaborator

@bgiori bgiori left a 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

export const CohortConfigDefaults: Omit<CohortConfig, 'apiKey' | 'secretKey'> =
{
cohortServerUrl: 'https://cohort-v2.lab.amplitude.com',
maxCohortSize: 10_000_000,
Copy link
Collaborator

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

@zhukaihan zhukaihan merged commit 74221bf into main Aug 27, 2024
10 checks passed
@zhukaihan zhukaihan deleted the add-cohort-sync branch August 27, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants