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

chore: remove deprecated strategies and allow deprecate default #3575

Merged
merged 2 commits into from Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -216,7 +216,6 @@ export const StrategiesList = () => {
<StrategySwitch
deprecated={original.deprecated}
onToggle={onToggle(original)}
disabled={original.name === 'default'}
/>
<ActionCell.Divider />
<StrategyEditButton
Expand Down
8 changes: 0 additions & 8 deletions src/lib/routes/admin-api/strategy.test.ts
Expand Up @@ -207,11 +207,3 @@ test('reactivating a non-existent strategy yields 404', async () => {
.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);
});
5 changes: 0 additions & 5 deletions src/lib/routes/admin-api/strategy.ts
Expand Up @@ -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();
}
Expand Down
72 changes: 72 additions & 0 deletions 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This migration only deletes the strategies if they match the happy path. As we discussed there are two blockers here for deleting them:

  1. the strategy is in use
  2. the strategy is in use and it has been modified.

For 1. we can probably migrate the use into using gradualRollout with the same parameters as before.
For 2. we can change the name and make it clear that its handled by the user and not unleash anymore just like other custom strategies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very good comment. Ideally, we'd like to get rid of them, but there might be unexpected side effects that make me worry.

For 1. we can probably migrate the use into using gradualRollout with the same parameters as before.

Yes, but we need to merge 3 strategies into one, which might be doable in some situations, but still won't cover all cases. I'm also not that sure how stickiness will behave if we change the strategy, because even if the strategy implementation is the same, because of the name change we might reassign users to another treatment. So I rather keep it like this

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this deletion should be safe, since we're explciitly checking if the strategy is in use.


-- 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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to reverse the deprecation of userWithId because the user can change that manually and there might be cases where the customer could have manually deprecated this strategy and we don't want to enable it back

-- revert sort order
update strategies set sort_order = 0 WHERE name = 'default';
update strategies set sort_order = 1 WHERE name = 'flexibleRollout';
`,
callback,
);
};
48 changes: 0 additions & 48 deletions src/migrations/default-strategies.json
Expand Up @@ -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.",
Expand Down
9 changes: 0 additions & 9 deletions src/test/e2e/api/admin/strategy.e2e.test.ts
Expand Up @@ -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')
Expand Down
12 changes: 6 additions & 6 deletions src/test/e2e/api/client/feature.optimal304.e2e.test.ts
Expand Up @@ -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"');
Comment on lines +125 to +126
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Min version changed because of the way we're creating strategies. We removed 3 of them from a fresh installation: https://github.com/Unleash/unleash/pull/3575/files#diff-46265b0d51725a5acbf251589b98a8970e45d3a1c6f3d7ea77bdcc3714e672d8L31-L78, therefore, we expect to have minus 3 events in the eventlog.

});

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);
});

Expand All @@ -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"');
});