task: Add banner encouraging edge upgrade #6018
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
chriswk
force-pushed
the
task/addUpgradeEdgeBanner
branch
from
e24be66 to
847d570
Compare
Only triggers if there is any rows in client instances that have sdk_version: unleash-edge with version < 17.0.0 The function that checks this memoizes the check for 10 minutes at the time to avoid scanning the client instances table too often.
|
Don't approve yet, I'm fixing the possible SQL Injection first. |
| <ConditionallyRender | ||
| condition={displayUpgradeEdgeBanner} | ||
| show={<Banner key={'upgradeEdge'} banner={upgradeEdgeBanner} />} | ||
| /> |
There was a problem hiding this comment.
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.
| 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; | ||
| } | ||
| }); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks. Good point. I'll have a look
| @@ -79,6 +79,7 @@ export type UiFlags = { | |||
| executiveDashboard?: boolean; | |||
| changeRequestConflictHandling?: boolean; | |||
| feedbackComments?: Variant; | |||
| displayUpgradeEdgeBanner?: boolean; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is something we'd like to have in OSS deploys as well, and hopefully not for very long.
There was a problem hiding this comment.
So, I think having it as a normal flag is fine.
There was a problem hiding this comment.
LG, but like mentioned in https://github.com/Unleash/unleash/pull/6018/files#r1464683982, I don't think it belongs inside InternalBanners.
It's no longer in InternalBanners. |
| <> | ||
| <ConditionallyRender | ||
| condition={displayUpgradeEdgeBanner} | ||
| show={<Banner key={'upgradeEdge'} banner={upgradeEdgeBanner} />} |
There was a problem hiding this comment.
No need for key here, since it's only 1 element.
Only triggers if there is any rows in client instances that have
The function that checks this memoizes the check for 10 minutes to avoid scanning the client instances table too often.