- Notifications
You must be signed in to change notification settings - Fork1k
feat: implement premium vs enterprise licenses#13907
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
0036916 to26f7b66Comparecodersdk/deployment.go Outdated
| // CompareFeatures compares two features and returns an integer representing | ||
| // if the first feature is greater than, equal to, or less than the second feature. | ||
| // "Greater than" means the first feature has more functionality than the | ||
| // second feature. It is assumed the features are for the same FeatureName. | ||
| // | ||
| // A feature is considered greater than another feature if: | ||
| // - Graceful & capable > Entitled & not capable | ||
| // - The entitlement is greater | ||
| // - The limit is greater | ||
| // - Enabled is greater than disabled | ||
| // - The actual is greater | ||
| funcCompareFeatures(a,bFeature)int { | ||
| if!a.Capable()||!b.Capable() { | ||
| // If either is incapable, then it is possible a grace period | ||
| // feature can be "greater" than an entitled. | ||
| // If either is "NotEntitled" then we can defer to a strict entitlement | ||
| // check. | ||
| ifentitlementWeight(a.Entitlement)>=0&&entitlementWeight(b.Entitlement)>=0 { | ||
| ifa.Capable()&&!b.Capable() { | ||
| return1 | ||
| } | ||
| ifb.Capable()&&!a.Capable() { | ||
| return-1 | ||
| } | ||
| } | ||
| } | ||
| entitlement:=CompareEntitlements(a.Entitlement,b.Entitlement) | ||
| ifentitlement>0 { | ||
| return1 | ||
| } | ||
| ifentitlement<0 { | ||
| return-1 | ||
| } | ||
| // If the entitlement is the same, then we can compare the limits. | ||
| ifa.Limit==nil&&b.Limit!=nil { | ||
| return-1 | ||
| } | ||
| ifa.Limit!=nil&&b.Limit==nil { | ||
| return1 | ||
| } | ||
| ifa.Limit!=nil&&b.Limit!=nil { | ||
| difference:=*a.Limit-*b.Limit | ||
| if*a.Limit-*b.Limit!=0 { | ||
| returnint(difference) | ||
| } | ||
| } | ||
| // Enabled is better than disabled. | ||
| ifa.Enabled&&!b.Enabled { | ||
| return1 | ||
| } | ||
| if!a.Enabled&&b.Enabled { | ||
| return-1 | ||
| } | ||
| // Higher actual is better | ||
| ifa.Actual==nil&&b.Actual!=nil { | ||
| return-1 | ||
| } | ||
| ifa.Actual!=nil&&b.Actual==nil { | ||
| return1 | ||
| } | ||
| ifa.Actual!=nil&&b.Actual!=nil { | ||
| difference:=*a.Actual-*b.Actual | ||
| if*a.Actual-*b.Actual!=0 { | ||
| returnint(difference) | ||
| } | ||
| } | ||
| return0 | ||
| } |
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.
This is the real meat and potatoes.
This function compares 2 features from 2 different licenses, and decides which one is to be kept. To change the behavior on how multiple licenses are handled, this is where to do it.
| require.Equal(t,inputYAML,output,"re-marshaled is the same as input") | ||
| } | ||
| funcTestFeatureComparison(t*testing.T) { |
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.
The refactor mainly leads to this unit test. Much easier to write than the previous tests that required starting a coderd to test license logic.
codersdk/deployment.go Outdated
| // entitlementWeight converts the enum types to a numerical value for easier | ||
| // comparisons. Easier than sets of if statements. | ||
| funcentitlementWeight(eEntitlement)int { |
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.
Might be cleaner to add this as a func onEntitlement type itself.e.Weight()
| func (setFeatureSet)Features() []FeatureName { | ||
| switchFeatureSet(strings.ToLower(string(set))) { | ||
| caseFeatureSetEnterprise: |
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.
A while ago Ammar did work to make it so we don't have to list new features in a bunch of places.
Instead of this, could we do the inverse where Premium simply detracts from the list instead? Seems easier to mentally model.
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.
If I understand correctly, you are saying we have 1 list,All() (it's FeatureNames() or something atm).
And we defineenterprise = All() - [multi-org],premium = All(). Rather than this explicit listing?
If we define it that way, it feels more likely to accidentally include a feature in "enterprise", and then we'd have to revoke it later. This current method might be a bit of a nuisance to deal with, but it errors on the side of restrictive.
Now our unit tests do not use feature sets, they manually define features. So this might reveal something lacking in our unit tests that only running a real server will pickup 🤔.
I'm still a bit split on the ideal way.
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.
Implement different sets of licensed features.Refactor the license logic to fix up some edge cases
Refactor license logic to keep license features atomic (no merging).Refactor logic to be more unit testable.
8e11ff9 to1c4a305CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This deprecates the
AllFeaturesboolean for aFeatureSetenum.Refactored
Entitlementsto be more unit testable. Added more unit tests.Closes#13934