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 feature flag tables #10184

Merged
merged 17 commits into from
Oct 7, 2024
Merged

feat: add feature flag tables #10184

merged 17 commits into from
Oct 7, 2024

Conversation

nickoferrall
Copy link
Contributor

@nickoferrall nickoferrall commented Sep 3, 2024

Related issue: #10099

This PR was the first in a series of PRs. I've merged all child PRs into this one.

It includes:

  • Adding featureFlag and featureFlagOwner tables
  • refactoring the array of featureFlag enums on the Org & User objects to the new feature flag tables
  • featureFlag can now exist in the Org, User, or Team
  • migrates noAISummary. This is now only scoped to the Org rather than the Org and User

@github-actions github-actions bot added the size/m label Sep 3, 2024
@nickoferrall nickoferrall marked this pull request as ready for review September 4, 2024 12:41
@github-actions github-actions bot requested a review from tianrunhe September 4, 2024 12:41
@nickoferrall nickoferrall removed the request for review from tianrunhe September 4, 2024 12:42
@nickoferrall
Copy link
Contributor Author

Hey @Dschoordsch, would you mind reviewing just the migration in this PR? And not the rest of the code.

That's the building block for the rest of this refactor.

My plan:

  • In this PR, I create the migration and addFeatureFlag, which is where we can add new feature flags like Insights.
  • In a follow-up PR, I'm working on adding applyFeatureFlag, which applies the feature flag to the feature flag owner, so we can apply the Insights feature flag to a specific team, org, or user, for example. I'll also and updateFeatureFlag, and deleteFeatureFlag
  • And then in another PR, I'll refactor the application to use the new feature flag

Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few smaller things, looks good otherwise.

Comment on lines 44 to 61
await pg.schema
.createIndex('idx_feature_flag_owner_user')
.on('FeatureFlagOwner')
.columns(['userId', 'featureFlagId'])
.execute()

await pg.schema
.createIndex('idx_feature_flag_owner_team')
.on('FeatureFlagOwner')
.columns(['teamId', 'featureFlagId'])
.execute()

await pg.schema
.createIndex('idx_feature_flag_owner_org')
.on('FeatureFlagOwner')
.columns(['orgId', 'featureFlagId'])
.execute()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 I don't see how we would use these indexes.
I think what we want though is the same set of columns ['userId', 'featureFlagId'], ... as unique constraints. Thanks to Matt I just learned that Postgres is treating null values as distinct by default1

Footnotes

  1. https://www.postgresql.org/about/featurematrix/detail/392/

@mattkrick mattkrick mentioned this pull request Sep 17, 2024
3 tasks
@@ -0,0 +1,16 @@
type FeatureFlagNew {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from my learnings with NewMeeting, adding New to an entity can suck later on. I did a search & it looks like FeatureFlag is available?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added FeatureFlagNew as a temporary name to minimise conflicts with the old FeatureFlag. Sorry, I should have added a comment to clarify that. It's FeatureFlag in the subsequent PR.

await pg.schema
.createTable('FeatureFlag')
.ifNotExists()
.addColumn('id', 'uuid', (col) => col.primaryKey().defaultTo(sql`gen_random_uuid()`))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 why a UUID instead of a serial?

Copy link
Contributor

github-actions bot commented Oct 4, 2024

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

@github-actions github-actions bot added size/xl and removed size/m labels Oct 4, 2024
Copy link
Contributor

github-actions bot commented Oct 4, 2024

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

@nickoferrall nickoferrall merged commit ff6c25e into master Oct 7, 2024
8 checks passed
@nickoferrall nickoferrall deleted the feat/refactor-feature-flag branch October 7, 2024 14:08
@github-actions github-actions bot mentioned this pull request Oct 7, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants