-
-
Notifications
You must be signed in to change notification settings - Fork 731
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: adds created_by_user_id to all events #5619
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
7241ef9
to
4c36da7
Compare
4e17b4d
to
4e8290c
Compare
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.
LG! Big one 😅
@@ -45,6 +45,7 @@ test('Should call datadog webhook', async () => { | |||
createdAt: new Date(), | |||
type: FEATURE_CREATED, | |||
createdBy: '[email protected]', | |||
createdByUserId: -1337, |
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.
nitpicking but we could use the constant here as well no?
@@ -555,13 +562,15 @@ class FeatureToggleService { | |||
strategyConfig, | |||
context, | |||
createdBy, | |||
user?.id || SYSTEM_USER_ID, |
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.
is this being nullable related to how change requests work?
@@ -1746,6 +1795,7 @@ class FeatureToggleService { | |||
featureName, | |||
}, | |||
createdBy, | |||
user?.id || SYSTEM_USER_ID, |
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.
Same question here, and goes for the other instances as well
What
Adds
createdByUserId
to all events exposed by unleash. In addition this PR updates all tests and usages of the methods in this codebase to include the required number.Dependencies
Points of discussion
This will break enterprise hard when merged, since all events constructed from enterprise codebase will not have the required
createdByUserId
field. So, we'll need a planned merge, tomorrow (Wednesday) I'll prepare a PR on enterprise using this as a linked dependency to make sure I update all calls in enterprise as well.That should allow us to merge this, and then merge the PR on enterprise to reduce the broken build time to a minimum.