-
Notifications
You must be signed in to change notification settings - Fork 20
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
[Feature Flag] Fix a bug in versions comparison that was disabling all PC 3.2.0+ features on PC 3.10.0+. #341
[Feature Flag] Fix a bug in versions comparison that was disabling all PC 3.2.0+ features on PC 3.10.0+. #341
Conversation
d7f8568
to
34c71fc
Compare
function isVersionGraterOrEqualThan(version: string, other: string): boolean { | ||
return version != null && version.localeCompare(other, undefined, { numeric: true, sensitivity: 'base' }) >= 0 | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use an existing solution for this instead of writing it ourselves? https://www.npmjs.com/package/compare-versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per note in the PR description: I think it's overkilling to introduce a new dependency for a simple logic.
We should add env dependencies only if strictly required, otherwise we are introducing vector of vulns and consequently ops load.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally think its worth it to avoid adding extra logic that can create errors. Especially with something as deceptively complicated as versioning comparisons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An interesting reading about this: https://semaphoreci.com/blog/project-dependencies
In our case the simplicity of our project makes the article sound a bit overkilling, but it is still an interesting best practice reading.
…l PC 3.2.0+ features on PC 3.10.0+.
34c71fc
to
44fc6ae
Compare
Description
Fix a bug in feature flagging that was disabling of all PC 3.2.0+ features on PC 3.10.0+.
In particular, we needed to fix the logic that compares semantic versions.
Before this change 3.10.0 and 3.10.1 were considered lower than 3.2.0 and greater than 3.1.0, so every feature between 3.2.0 and 3.9.0 was disabled.
With this fix, feature flags are correct.
Example: the feature flexible instance type was disablòed for 3.10.0 because of the bug.
With the fix it is now enabled (see screenshot)
Note
There is a package dedicated to semver comparison (semver), but it's not worth it to add an extra dependency for such an easy logic.
How Has This Been Tested?
References
PR Quality Checklist
react-i18next
library (useTranslation hook and/or Trans component), see an example herenpm run build
builds without any errorIn order to increase the likelihood of your contribution being accepted, please make sure you have read both the Contributing Guidelines and the Project Guidelines
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.