Skip to content

WIP: removing the strategy for new installations of Unleash #9800

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.

Sign up for GitHub

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

WIP: removing the strategy for new installations of Unleash #9800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a65cfa4
WIP: removing the strategy for new installations of Unleash
gastonfournier Apr 17, 2025
ae56c35
Small fixes
gastonfournier Apr 17, 2025
5e5f87f
Fix 304 test
gastonfournier Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions src/lib/addons/__snapshots__/feature-event-formatter-md.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,11 +239,3 @@ exports[`Should format specialised text for events when strategy removed 1`] = `
"url": "unleashUrl/projects/my-other-project/features/new-feature",
}
`;

exports[`Should format specialised text for events when userIds changed 1`] = `
{
"label": "Flag strategy updated",
"text": "*user@company.com* updated *[new-feature](unleashUrl/projects/my-other-project/features/new-feature)* in project *[my-other-project](unleashUrl/projects/my-other-project)* by updating strategy *userWithId* in *production* userIds from empty set of userIds to [a,b]; constraints from empty set of constraints to [appName is one of (x,y)]",
"url": "unleashUrl/projects/my-other-project/features/new-feature",
}
`;
40 changes: 0 additions & 40 deletions src/lib/addons/feature-event-formatter-md.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,46 +352,6 @@ const testCases: [string, IEvent][] = [
},
],
),
[
'when userIds changed',
{
id: 920,
type: FEATURE_STRATEGY_UPDATE,
createdBy: 'user@company.com',
createdByUserId: SYSTEM_USER_ID,
createdAt: new Date('2022-06-01T10:03:11.549Z'),
data: {
name: 'userWithId',
constraints: [
{
values: ['x', 'y'],
inverted: false,
operator: IN,
contextName: 'appName',
caseInsensitive: false,
},
],
parameters: {
userIds: 'a,b',
},
sortOrder: 9999,
id: '9a995d94-5944-4897-a82f-0f7e65c2fb3f',
},
preData: {
name: 'userWithId',
constraints: [],
parameters: {
userIds: '',
},
sortOrder: 9999,
id: '9a995d94-5944-4897-a82f-0f7e65c2fb3f',
},
tags: [],
featureName: 'new-feature',
project: 'my-other-project',
environment: 'production',
},
],
[
'when IPs changed',
{
Expand Down
6 changes: 0 additions & 6 deletions src/lib/addons/feature-event-formatter-md.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,6 @@ export class FeatureEventFormatterMd implements FeatureEventFormatter {
return this.flexibleRolloutStrategyChangeText(event);
case 'default':
return this.defaultStrategyChangeText(event);
case 'userWithId':
return this.userWithIdStrategyChangeText(event);
case 'remoteAddress':
return this.remoteAddressStrategyChangeText(event);
case 'applicationHostname':
Expand All @@ -162,10 +160,6 @@ export class FeatureEventFormatterMd implements FeatureEventFormatter {
return this.listOfValuesStrategyChangeText(event, 'IPs');
}

private userWithIdStrategyChangeText(event: IEvent) {
return this.listOfValuesStrategyChangeText(event, 'userIds');
}

private listOfValuesStrategyChangeText(
event: IEvent,
propertyName: string,
Expand Down
4 changes: 2 additions & 2 deletions ...atures/client-feature-toggles/tests/__snapshots__/client-feature-toggles.e2e.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ exports[`should match snapshot from /api/client/features 1`] = `
},
],
"meta": {
"etag": ""61824cd0:11"",
"etag": ""61824cd0:10"",
"queryHash": "61824cd0",
"revisionId": 11,
"revisionId": 10,
},
"query": {
"environment": "default",
Expand Down
2 changes: 0 additions & 2 deletions src/lib/features/playground/feature-evaluator/strategy/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import DefaultStrategy from './default-strategy';
import GradualRolloutRandomStrategy from './gradual-rollout-random';
import GradualRolloutUserIdStrategy from './gradual-rollout-user-id';
import GradualRolloutSessionIdStrategy from './gradual-rollout-session-id';
import UserWithIdStrategy from './user-with-id-strategy';
import RemoteAddressStrategy from './remote-address-strategy';
import FlexibleRolloutStrategy from './flexible-rollout-strategy';
import type { Strategy } from './strategy';
Expand All @@ -18,7 +17,6 @@ export const defaultStrategies: Array<Strategy> = [
new GradualRolloutRandomStrategy(),
new GradualRolloutUserIdStrategy(),
new GradualRolloutSessionIdStrategy(),
new UserWithIdStrategy(),
new RemoteAddressStrategy(),
new FlexibleRolloutStrategy(),
new UnknownStrategy(),
Expand Down
17 changes: 0 additions & 17 deletions src/lib/features/playground/feature-evaluator/strategy/user-with-id-strategy.ts
View file
Open in desktop

This file was deleted.

7 changes: 0 additions & 7 deletions src/lib/features/playground/offline-unleash-client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,13 +432,6 @@ describe('offline client', () => {
stickiness: 'userId',
},
},
{
name: 'userWithId',
constraints: [],
parameters: {
userIds: 'uoea,ueoa',
},
},
{
name: 'remoteAddress',
constraints: [],
Expand Down
8 changes: 0 additions & 8 deletions src/lib/openapi/spec/playground-feature-schema.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,6 @@ const playgroundStrategies = (): Arbitrary<PlaygroundStrategySchema[]> =>
}),
),

playgroundStrategy(
'userWithId',
fc.record({
userIds: fc
.uniqueArray(fc.emailAddress())
.map((ids) => ids.join(',')),
}),
),
playgroundStrategy(
'remoteAddress',
fc.record({
Expand Down
1 change: 0 additions & 1 deletion src/migrations/20210421133845-add-sort-order-to-strategies.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member

@nunogois nunogois Apr 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Instead, we should add a new migration that removes the strategy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

UPDATE strategies SET sort_order = 3 WHERE name = 'remoteAddress';
UPDATE strategies SET sort_order = 4 WHERE name = 'applicationHostname';
`,
Expand Down
1 change: 0 additions & 1 deletion src/migrations/20210421135405-add-display-name-and-update-description-for-strategies.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member

Choose a reason for hiding this comment

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

Same as https://github.com/Unleash/unleash/pull/9800/files#r2052191420

UPDATE strategies SET display_name = 'IPs', description = 'Enable the feature for a specific set of IP addresses.' WHERE name = 'remoteAddress';
UPDATE strategies SET display_name = 'Hosts', description = 'Enable the feature for a specific set of hostnames.' WHERE name = 'applicationHostname';
`,
Expand Down
4 changes: 0 additions & 4 deletions src/migrations/20230420125500-v5-strategy-changes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

Same as https://github.com/Unleash/unleash/pull/9800/files#r2052191420


-- 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';
Copy link
Member

Choose a reason for hiding this comment

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

Same as https://github.com/Unleash/unleash/pull/9800/files#r2052191420

`,
callback,
);
Expand Down
12 changes: 0 additions & 12 deletions src/migrations/default-strategies.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,6 @@
"description": "Default on/off strategy.",
"parameters": []
},
{
"name": "userWithId",
"description": "Active for users with a userId defined in the userIds-list",
"parameters": [
{
"name": "userIds",
"type": "list",
"description": "",
"required": false
}
]
},
Copy link
Member

Choose a reason for hiding this comment

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

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

{
"name": "applicationHostname",
"description": "Active for client instances with a hostName in the hostNames-list.",
Expand Down
8 changes: 0 additions & 8 deletions src/test/arbitraries.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,6 @@ export const strategies = (): Arbitrary<FeatureStrategySchema[]> =>
}),
),

strategy(
'userWithId',
fc.record({
userIds: fc
.uniqueArray(fc.emailAddress())
.map((ids) => ids.join(',')),
}),
),
strategy(
'remoteAddress',
fc.record({
Expand Down
22 changes: 11 additions & 11 deletions src/test/e2e/api/client/feature.optimal304.e2e.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,26 +152,26 @@ describe.each([
.expect(200);

if (etagVariant.feature_enabled) {
expect(res.headers.etag).toBe(`"61824cd0:17:${etagVariant.name}"`);
expect(res.headers.etag).toBe(`"61824cd0:16:${etagVariant.name}"`);
expect(res.body.meta.etag).toBe(
`"61824cd0:17:${etagVariant.name}"`,
`"61824cd0:16:${etagVariant.name}"`,
);
} else {
expect(res.headers.etag).toBe('"61824cd0:16"');
expect(res.body.meta.etag).toBe('"61824cd0:16"');
expect(res.headers.etag).toBe('"61824cd0:15"');
expect(res.body.meta.etag).toBe('"61824cd0:15"');
}
});

test(`returns ${etagVariant.feature_enabled ? 200 : 304} for pre-calculated hash${etagVariant.feature_enabled ? ' because hash changed' : ''}`, async () => {
const res = await app.request
.get('/api/client/features')
.set('if-none-match', '"61824cd0:16"')
.set('if-none-match', '"61824cd0:15"')
.expect(etagVariant.feature_enabled ? 200 : 304);

if (etagVariant.feature_enabled) {
expect(res.headers.etag).toBe(`"61824cd0:17:${etagVariant.name}"`);
expect(res.headers.etag).toBe(`"61824cd0:16:${etagVariant.name}"`);
expect(res.body.meta.etag).toBe(
`"61824cd0:17:${etagVariant.name}"`,
`"61824cd0:16:${etagVariant.name}"`,
);
}
});
Expand All @@ -193,13 +193,13 @@ describe.each([
.expect(200);

if (etagVariant.feature_enabled) {
expect(res.headers.etag).toBe(`"61824cd0:17:${etagVariant.name}"`);
expect(res.headers.etag).toBe(`"61824cd0:16:${etagVariant.name}"`);
expect(res.body.meta.etag).toBe(
`"61824cd0:17:${etagVariant.name}"`,
`"61824cd0:16:${etagVariant.name}"`,
);
} else {
expect(res.headers.etag).toBe('"61824cd0:17"');
expect(res.body.meta.etag).toBe('"61824cd0:17"');
expect(res.headers.etag).toBe('"61824cd0:16"');
expect(res.body.meta.etag).toBe('"61824cd0:16"');
}
});
});
Expand Down
Loading