From 4dd4d43ccee3150e5195c5f1a8cd0ca56fb0b382 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Fri, 21 Apr 2023 11:42:36 +0200 Subject: [PATCH] chore: remove deprecated strategies and allow deprecate default (#3575) ## About the changes Improvements around strategies to steer users toward using [strategy constraints](https://docs.getunleash.io/reference/strategy-constraints) instead of [custom strategies](https://docs.getunleash.io/reference/custom-activation-strategies) Related to #1265 Important changes: 1.[ Ability to deprecate default strategy](https://github.com/Unleash/unleash/pull/3575/files#diff-3b49e566565b01580ff7a15f662d2cb824547fdfb3935d0a0baa7fae326d28d6L232-L235) 2. [Remove strategies that were deprecated in v4 only if they're not used](https://github.com/Unleash/unleash/pull/3575/files#diff-a04de7adf327c48d54b030bed70d9b1d62ff14df3efc668eb6897a210124ebe0R7-R10) 3. [Fresh Unleash installations will not include deprecated strategies from v4](https://github.com/Unleash/unleash/pull/3575/files#diff-46265b0d51725a5acbf251589b98a8970e45d3a1c6f3d7ea77bdcc3714e672d8L31-L78) 4. [Deprecate userWithId strategy](https://github.com/Unleash/unleash/pull/3575/files#diff-a04de7adf327c48d54b030bed70d9b1d62ff14df3efc668eb6897a210124ebe0R13) (note: if in use it will continue to work, it'll be not listed when selecting strategies for a feature toggle). 5. [Switch the order of strategies to make gradual rollout first and change the comment to suggest using gradual rollout instead of other existing strategies](https://github.com/Unleash/unleash/pull/3575/files#diff-a04de7adf327c48d54b030bed70d9b1d62ff14df3efc668eb6897a210124ebe0R16-R18) --- .../StrategiesList/StrategiesList.tsx | 1 - src/lib/routes/admin-api/strategy.test.ts | 8 --- src/lib/routes/admin-api/strategy.ts | 5 -- .../20230420125500-v5-strategy-changes.js | 72 +++++++++++++++++++ src/migrations/default-strategies.json | 48 ------------- src/test/e2e/api/admin/strategy.e2e.test.ts | 9 --- .../api/client/feature.optimal304.e2e.test.ts | 12 ++-- 7 files changed, 78 insertions(+), 77 deletions(-) create mode 100644 src/migrations/20230420125500-v5-strategy-changes.js diff --git a/frontend/src/component/strategies/StrategiesList/StrategiesList.tsx b/frontend/src/component/strategies/StrategiesList/StrategiesList.tsx index 7d4605fb14..7a726349e7 100644 --- a/frontend/src/component/strategies/StrategiesList/StrategiesList.tsx +++ b/frontend/src/component/strategies/StrategiesList/StrategiesList.tsx @@ -216,7 +216,6 @@ export const StrategiesList = () => { { .set('Content-Type', 'application/json') .expect(404); }); -test("deprecating 'default' strategy will yield 403", async () => { - jest.spyOn(global.console, 'error').mockImplementation(() => jest.fn()); - const { request, base } = await getSetup(); - return request - .post(`${base}/api/admin/strategies/default/deprecate`) - .set('Content-Type', 'application/json') - .expect(403); -}); diff --git a/src/lib/routes/admin-api/strategy.ts b/src/lib/routes/admin-api/strategy.ts index 07e08c2629..940080ad46 100644 --- a/src/lib/routes/admin-api/strategy.ts +++ b/src/lib/routes/admin-api/strategy.ts @@ -229,11 +229,6 @@ class StrategyController extends Controller { const userName = extractUsername(req); const { strategyName } = req.params; - if (strategyName === 'default') { - res.status(403).end(); - return; - } - await this.strategyService.deprecateStrategy(strategyName, userName); res.status(200).end(); } diff --git a/src/migrations/20230420125500-v5-strategy-changes.js b/src/migrations/20230420125500-v5-strategy-changes.js new file mode 100644 index 0000000000..33f49037c8 --- /dev/null +++ b/src/migrations/20230420125500-v5-strategy-changes.js @@ -0,0 +1,72 @@ +'use strict'; + +exports.up = function (db, callback) { + db.runSql( + ` + -- delete deprecated strategies still present in v4 + delete from strategies + where name in ('gradualRolloutUserId', 'gradualRolloutRandom', 'gradualRolloutSessionId') + 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'); + + -- 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'; + `, + callback, + ); +}; + +exports.down = function (db, callback) { + db.runSql( + ` + -- restore deleted strategies + insert into strategies (name, description, parameters, deprecated, sort_order) values + ('gradualRolloutRandom', 'Randomly activate the feature toggle. No stickiness.', [ + { + "name": "percentage", + "type": "percentage", + "description": "", + "required": false + } + ], true, 3), + ('gradualRolloutSessionId', 'Gradually activate feature toggle. Stickiness based on session id.', [ + { + "name": "percentage", + "type": "percentage", + "description": "", + "required": false + }, + { + "name": "groupId", + "type": "string", + "description": "Used to define a activation groups, which allows you to correlate across feature toggles.", + "required": true + } + ], true, 4), + ('gradualRolloutUserId', 'Gradually activate feature toggle for logged in users. Stickiness based on user id.', [ + { + "name": "percentage", + "type": "percentage", + "description": "", + "required": false + }, + { + "name": "groupId", + "type": "string", + "description": "Used to define a activation groups, which allows you to correlate across feature toggles.", + "required": true + } + ], true, 5); + + -- revert sort order + update strategies set sort_order = 0 WHERE name = 'default'; + update strategies set sort_order = 1 WHERE name = 'flexibleRollout'; + `, + callback, + ); +}; diff --git a/src/migrations/default-strategies.json b/src/migrations/default-strategies.json index 2afefb52d5..e430f150db 100644 --- a/src/migrations/default-strategies.json +++ b/src/migrations/default-strategies.json @@ -28,54 +28,6 @@ } ] }, - { - "name": "gradualRolloutRandom", - "description": "Randomly activate the feature toggle. No stickiness.", - "parameters": [ - { - "name": "percentage", - "type": "percentage", - "description": "", - "required": false - } - ] - }, - { - "name": "gradualRolloutSessionId", - "description": "Gradually activate feature toggle. Stickiness based on session id.", - "parameters": [ - { - "name": "percentage", - "type": "percentage", - "description": "", - "required": false - }, - { - "name": "groupId", - "type": "string", - "description": "Used to define a activation groups, which allows you to correlate across feature toggles.", - "required": true - } - ] - }, - { - "name": "gradualRolloutUserId", - "description": "Gradually activate feature toggle for logged in users. Stickiness based on user id.", - "parameters": [ - { - "name": "percentage", - "type": "percentage", - "description": "", - "required": false - }, - { - "name": "groupId", - "type": "string", - "description": "Used to define a activation groups, which allows you to correlate across feature toggles.", - "required": true - } - ] - }, { "name": "remoteAddress", "description": "Active for remote addresses defined in the IPs list.", diff --git a/src/test/e2e/api/admin/strategy.e2e.test.ts b/src/test/e2e/api/admin/strategy.e2e.test.ts index 1d99cde56b..646a6b7fca 100644 --- a/src/test/e2e/api/admin/strategy.e2e.test.ts +++ b/src/test/e2e/api/admin/strategy.e2e.test.ts @@ -171,15 +171,6 @@ test('can reactivate a deprecated strategy', async () => { .expect((res) => expect(res.body.deprecated).toBe(false)); }); -test('cannot deprecate default strategy', async () => { - expect.assertions(0); - - await app.request - .post('/api/admin/strategies/default/deprecate') - .set('Content-Type', 'application/json') - .expect(403); -}); - test('can update a exiting strategy with deprecated', async () => { await app.request .post('/api/admin/strategies') diff --git a/src/test/e2e/api/client/feature.optimal304.e2e.test.ts b/src/test/e2e/api/client/feature.optimal304.e2e.test.ts index 56bbb91461..3ce6f411fd 100644 --- a/src/test/e2e/api/client/feature.optimal304.e2e.test.ts +++ b/src/test/e2e/api/client/feature.optimal304.e2e.test.ts @@ -122,14 +122,14 @@ test('returns calculated hash', async () => { .get('/api/client/features') .expect('Content-Type', /json/) .expect(200); - expect(res.headers.etag).toBe('"ae443048:19"'); - expect(res.body.meta.etag).toBe('"ae443048:19"'); + expect(res.headers.etag).toBe('"ae443048:16"'); + expect(res.body.meta.etag).toBe('"ae443048:16"'); }); test('returns 304 for pre-calculated hash', async () => { return app.request .get('/api/client/features') - .set('if-none-match', '"ae443048:19"') + .set('if-none-match', '"ae443048:16"') .expect(304); }); @@ -146,9 +146,9 @@ test('returns 200 when content updates and hash does not match anymore', async ( const res = await app.request .get('/api/client/features') - .set('if-none-match', 'ae443048:19') + .set('if-none-match', 'ae443048:16') .expect(200); - expect(res.headers.etag).toBe('"ae443048:20"'); - expect(res.body.meta.etag).toBe('"ae443048:20"'); + expect(res.headers.etag).toBe('"ae443048:17"'); + expect(res.body.meta.etag).toBe('"ae443048:17"'); });