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

task: Add banner encouraging edge upgrade #6018

Merged
merged 4 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
15 changes: 15 additions & 0 deletions frontend/src/component/banners/internalBanners/InternalBanners.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
import { Banner } from 'component/banners/Banner/Banner';
import { useBanners } from 'hooks/api/getters/useBanners/useBanners';
import { useUiFlag } from '../../../hooks/useUiFlag';
import { ConditionallyRender } from '../../common/ConditionallyRender/ConditionallyRender';
import { IBanner } from '../../../interfaces/banner';

export const InternalBanners = () => {
const { banners } = useBanners();
const displayUpgradeEdgeBanner = useUiFlag('displayUpgradeEdgeBanner');

const upgradeEdgeBanner: IBanner = {
message: `We noticed that you're using an outdated Unleash Edge. To ensure you continue to receive metrics, we recommend upgrading to v17.0.0 or later.`,
link: 'https://github.com/Unleash/unleash-edge',
linkText: 'Get latest',
variant: 'warning',
};

return (
<>
Expand All @@ -11,6 +22,10 @@ export const InternalBanners = () => {
.map((banner) => (
<Banner key={banner.id} banner={banner} />
))}
<ConditionallyRender
condition={displayUpgradeEdgeBanner}
show={<Banner key={'upgradeEdge'} banner={upgradeEdgeBanner} />}
/>
Copy link
Member

Choose a reason for hiding this comment

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

Similarly to #5239 (comment) - I would recommend we create a new EdgeUpgradeBanner (or similar) component and add it directly to App.tsx. I don't think it makes much sense to add it here, especially as it is not an internal banner.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Will fix

</>
);
};
1 change: 1 addition & 0 deletions frontend/src/interfaces/uiConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ export type UiFlags = {
executiveDashboard?: boolean;
changeRequestConflictHandling?: boolean;
feedbackComments?: Variant;
displayUpgradeEdgeBanner?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth considering making this a Variant flag instead. That way we can control not only whether the banner is shown, but also its contents through the flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is something we'd like to have in OSS deploys as well, and hopefully not for very long.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, I think having it as a normal flag is fine.

};

export interface IVersionInfo {
Expand Down
9 changes: 9 additions & 0 deletions src/lib/db/client-instance-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,15 @@ export default class ClientInstanceStore implements IClientInstanceStore {
return rows.map(mapRow);
}

async getBySdkName(sdkName: string): Promise<IClientInstance[]> {
const rows = await this.db
.select()
.from(TABLE)
.whereRaw(`sdk_version LIKE '??%'`, [sdkName])
.orderBy('last_seen', 'desc');
return rows.map(mapRow);
}

async getDistinctApplications(): Promise<string[]> {
const rows = await this.db
.distinct('app_name')
Expand Down
17 changes: 17 additions & 0 deletions src/lib/features/metrics/instance/instance-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { IPrivateProjectChecker } from '../../private-project/privateProjectChec
import { IFlagResolver, SYSTEM_USER } from '../../../types';
import { ALL_PROJECTS } from '../../../util';
import { Logger } from '../../../logger';
import { SemVer } from 'semver';

export default class ClientInstanceService {
apps = {};
Expand Down Expand Up @@ -224,4 +225,20 @@ export default class ClientInstanceService {
async removeInstancesOlderThanTwoDays(): Promise<void> {
return this.clientInstanceStore.removeInstancesOlderThanTwoDays();
}

async usesSdkOlderThan(
sdkName: string,
sdkVersion: string,
): Promise<boolean> {
const semver = new SemVer(sdkVersion);
const instancesOfSdk =
await this.clientInstanceStore.getBySdkName(sdkName);
return instancesOfSdk.some((instance) => {
if (instance.sdkVersion) {
const [_sdkName, sdkVersion] = instance.sdkVersion.split(':');
const instanceUsedSemver = new SemVer(sdkVersion);
return instanceUsedSemver < semver;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I was just checking that SemVer can throw some exceptions: https://github.com/npm/node-semver/blob/main/classes/semver.js and we have a helper class that does the try catch: https://github.com/Unleash/unleash/blob/chore/update-features-created-by-user-id/src/lib/util/semver.ts maybe we should just try catch here

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Good point. I'll have a look

}
}
46 changes: 46 additions & 0 deletions src/lib/routes/admin-api/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
DEFAULT_STRATEGY_SEGMENTS_LIMIT,
} from '../../util/segments';
import TestAgent from 'supertest/lib/agent';
import { IUnleashStores } from '../../types';

const uiConfig = {
headerBackground: 'red',
Expand All @@ -28,17 +29,20 @@ async function getSetup() {

return {
base,
stores,
request: supertest(app),
};
}

let request: TestAgent<Test>;
let base: string;
let stores: IUnleashStores;

beforeEach(async () => {
const setup = await getSetup();
request = setup.request;
base = setup.base;
stores = setup.stores;
});

test('should get ui config', async () => {
Expand All @@ -52,3 +56,45 @@ test('should get ui config', async () => {
expect(body.segmentValuesLimit).toEqual(DEFAULT_SEGMENT_VALUES_LIMIT);
expect(body.strategySegmentsLimit).toEqual(DEFAULT_STRATEGY_SEGMENTS_LIMIT);
});

describe('displayUpgradeEdgeBanner', () => {
test('ui config should have displayUpgradeEdgeBanner to be set if an instance using edge has been seen', async () => {
await stores.clientInstanceStore.insert({
appName: 'my-app',
instanceId: 'some-instance',
sdkVersion: 'unleash-edge:16.0.0',
});
const { body } = await request
.get(`${base}/api/admin/ui-config`)
.expect('Content-Type', /json/)
.expect(200);
expect(body.flags).toBeTruthy();
expect(body.flags.displayUpgradeEdgeBanner).toBeTruthy();
});
test('ui config should not get displayUpgradeEdgeBanner flag if edge >= 17.0.0 has been seen', async () => {
await stores.clientInstanceStore.insert({
appName: 'my-app',
instanceId: 'some-instance',
sdkVersion: 'unleash-edge:17.1.0',
});
const { body } = await request
.get(`${base}/api/admin/ui-config`)
.expect('Content-Type', /json/)
.expect(200);
expect(body.flags).toBeTruthy();
expect(body.flags.displayUpgradeEdgeBanner).toEqual(false);
});
test('ui config should not get displayUpgradeEdgeBanner flag if java-client has been seen', async () => {
await stores.clientInstanceStore.insert({
appName: 'my-app',
instanceId: 'some-instance',
sdkVersion: 'unleash-client-java:9.1.0',
});
const { body } = await request
.get(`${base}/api/admin/ui-config`)
.expect('Content-Type', /json/)
.expect(200);
expect(body.flags).toBeTruthy();
expect(body.flags.displayUpgradeEdgeBanner).toEqual(false);
});
});
46 changes: 37 additions & 9 deletions src/lib/routes/admin-api/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ import { SetUiConfigSchema } from '../../openapi/spec/set-ui-config-schema';
import { createRequestSchema } from '../../openapi/util/create-request-schema';
import { ProxyService } from '../../services';
import MaintenanceService from '../../features/maintenance/maintenance-service';
import memoizee from 'memoizee';
import { minutesToMilliseconds } from 'date-fns';
import ClientInstanceService from '../../features/metrics/instance/instance-service';

class ConfigController extends Controller {
private versionService: VersionService;
Expand All @@ -36,8 +39,12 @@ class ConfigController extends Controller {

private emailService: EmailService;

private clientInstanceService: ClientInstanceService;

private maintenanceService: MaintenanceService;

private usesOldEdgeFunction: () => Promise<boolean>;

private readonly openApiService: OpenApiService;

constructor(
Expand All @@ -49,6 +56,7 @@ class ConfigController extends Controller {
openApiService,
proxyService,
maintenanceService,
clientInstanceService,
}: Pick<
IUnleashServices,
| 'versionService'
Expand All @@ -57,6 +65,7 @@ class ConfigController extends Controller {
| 'openApiService'
| 'proxyService'
| 'maintenanceService'
| 'clientInstanceService'
>,
) {
super(config);
Expand All @@ -66,6 +75,18 @@ class ConfigController extends Controller {
this.openApiService = openApiService;
this.proxyService = proxyService;
this.maintenanceService = maintenanceService;
this.clientInstanceService = clientInstanceService;
this.usesOldEdgeFunction = memoizee(
async () =>
this.clientInstanceService.usesSdkOlderThan(
'unleash-edge',
'17.0.0',
),
{
promise: true,
maxAge: minutesToMilliseconds(10),
},
);

this.route({
method: 'get',
Expand Down Expand Up @@ -109,14 +130,17 @@ class ConfigController extends Controller {
req: AuthedRequest,
res: Response<UiConfigSchema>,
): Promise<void> {
const [frontendSettings, simpleAuthSettings, maintenanceMode] =
await Promise.all([
this.proxyService.getFrontendSettings(false),
this.settingService.get<SimpleAuthSettings>(
simpleAuthSettingsKey,
),
this.maintenanceService.isMaintenanceMode(),
]);
const [
frontendSettings,
simpleAuthSettings,
maintenanceMode,
usesOldEdge,
] = await Promise.all([
this.proxyService.getFrontendSettings(false),
this.settingService.get<SimpleAuthSettings>(simpleAuthSettingsKey),
this.maintenanceService.isMaintenanceMode(),
this.usesOldEdgeFunction(),
]);

const disablePasswordAuth =
simpleAuthSettings?.disabled ||
Expand All @@ -126,7 +150,11 @@ class ConfigController extends Controller {
email: req.user.email,
});

const flags = { ...this.config.ui.flags, ...expFlags };
const flags = {
...this.config.ui.flags,
...expFlags,
displayUpgradeEdgeBanner: usesOldEdge,
};

const response: UiConfigSchema = {
...this.config.ui,
Expand Down
1 change: 1 addition & 0 deletions src/lib/types/stores/client-instance-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export interface IClientInstanceStore
setLastSeen(INewClientInstance): Promise<void>;
insert(details: INewClientInstance): Promise<void>;
getByAppName(appName: string): Promise<IClientInstance[]>;
getBySdkName(sdkName: string): Promise<IClientInstance[]>;
getDistinctApplications(): Promise<string[]>;
getDistinctApplicationsCount(daysBefore?: number): Promise<number>;
deleteForApplication(appName: string): Promise<void>;
Expand Down
8 changes: 8 additions & 0 deletions src/test/fixtures/fake-client-instance-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ export default class FakeClientInstanceStore implements IClientInstanceStore {
return;
}

async getBySdkName(sdkName: string): Promise<IClientInstance[]> {
return Promise.resolve(
this.instances.filter((instance) =>
instance.sdkVersion?.startsWith(sdkName),
),
);
}

async deleteAll(): Promise<void> {
this.instances = [];
}
Expand Down
Loading