WIP: removing the strategy for new installations of Unleash #9800
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
@gastonfournier, core features have been modified in this pull request. Please review carefully! |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
| @@ -6,7 +6,6 @@ exports.up = function (db, cb) { | |||
| ALTER TABLE strategies ADD COLUMN sort_order integer DEFAULT 9999; | |||
| UPDATE strategies SET sort_order = 0 WHERE name = 'default'; | |||
| UPDATE strategies SET sort_order = 1 WHERE name = 'flexibleRollout'; | |||
| UPDATE strategies SET sort_order = 2 WHERE name = 'userWithId'; | |||
There was a problem hiding this comment.
I'm pretty sure we shouldn't update existing migrations and that this would silently fail if it can't find any row with name = 'userWithId', which is fine.
There was a problem hiding this comment.
Instead, we should add a new migration that removes the strategy.
There was a problem hiding this comment.
Yes, it's intentional, that way we only affect new installations and don't change existing installations. We don't want to remove the strategy on existing installations, only if it's new.
It's true that the other migrations would not apply if you don't find the strategy due to the where filter, but it feels cleaner to remove those.
| @@ -6,7 +6,6 @@ exports.up = function (db, cb) { | |||
| ALTER TABLE strategies ADD COLUMN display_name text; | |||
| UPDATE strategies SET display_name = 'Standard', description = 'The standard strategy is strictly on / off for your entire userbase.' WHERE name = 'default'; | |||
| UPDATE strategies SET display_name = 'Gradual rollout', description = 'Roll out to a percentage of your userbase, and ensure that the experience is the same for the user on each visit.' WHERE name = 'flexibleRollout'; | |||
| UPDATE strategies SET display_name = 'UserIDs', description = 'Enable the feature for a specific set of userIds.' WHERE name = 'userWithId'; | |||
There was a problem hiding this comment.
| -- update strategy descriptions and sort order | ||
| update strategies set sort_order = 1, description = 'This strategy turns on / off for your entire userbase. Prefer using "Gradual rollout" strategy (100%=on, 0%=off).' WHERE name = 'default'; | ||
| update strategies set sort_order = 0 WHERE name = 'flexibleRollout'; | ||
| update strategies set description = 'Enable the feature for a specific set of userIds. Prefer using "Gradual rollout" strategy with user id constraints.' WHERE name = 'userWithId'; |
There was a problem hiding this comment.
| @@ -9,13 +9,9 @@ exports.up = function (db, callback) { | |||
| and deprecated | |||
| and not exists (select * from feature_strategies where strategy_name = name limit 1); | |||
|
|
|||
| -- deprecate strategies on v5 | |||
| update strategies set deprecated = true where name in ('userWithId'); | |||
There was a problem hiding this comment.
| "required": false | ||
| } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
This affects an existing migration, 20170211090541-add-default-strategies.js, so same as /Users/nunogois/dev/unleash/unleash/src/migrations/20170211090541-add-default-strategies.js
No description provided.