-
-
Notifications
You must be signed in to change notification settings - Fork 658
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
#4205: mark potentially stale features #4217
#4205: mark potentially stale features #4217
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
The migration adds a new `potentially_stale` column to the features table and sets this to true for any toggles that have exceeded their expected lifetime and that have not already been marked as `stale`.
1973498
to
85704e5
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.
While the update is fine, it will be out of date already tomorrow. We need a scheduler to do it anyways, so question is if there is value of putting this out of date data with migration.
currentTime?: string, | ||
): Promise<string[]> { | ||
const query = this.db(TABLE) | ||
.update('potentially_stale', true) |
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.
Should we also unmark them if configuration changes?
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.
That should be handled in the update method, I think. Do you disagree?
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
This PR lays most of the groundwork required for emitting events when features are marked as potentially stale by Unleash. It does not emit any events just yet. The summary is:
It is currently about 220 lines of tests and about 100 lines of application code (primarily db migration and two new methods on the IFeatureToggleStore interface).
The reason I wanted to put this into a single PR (instead of just the db migration, then just the potentially stale marking, then the update logic) is:
If users get the db migration first, but not the rest of the update logic until the events are fired, then they could get a bunch of new events for features that should have been marked as potentially stale several days/weeks/months ago. That seemed undesirable to me, so I decided to bunch those changes together. Of course, I'd be happy to break it into smaller parts.
Rules
A toggle will be marked as potentially stale iff:
Migration
The migration adds a new
potentially_stale
column to the featurestable and sets this to true for any toggles that have exceeded their
expected lifetime and that have not already been marked as
stale
.Discussion
The
currentTime
parameter ofmarkPotentiallyStaleFeatures
The
markPotentiallyStaleFetaures
method takes an optionalcurrentTime
parameter. This was added to make it easier to test (so you can test "into the future"), but it's not used in the application. We can rewrite the tests to instead update feature toggles manually, but that wouldn't test the actual marking method. Happy to discuss.