feat: implement premium vs enterprise licenses #13907
Conversation
0036916 to
26f7b66
Compare
codersdk/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 | ||
| func CompareFeatures(a, b Feature) 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. | ||
| if entitlementWeight(a.Entitlement) >= 0 && entitlementWeight(b.Entitlement) >= 0 { | ||
| if a.Capable() && !b.Capable() { | ||
| return 1 | ||
| } | ||
| if b.Capable() && !a.Capable() { | ||
| return -1 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| entitlement := CompareEntitlements(a.Entitlement, b.Entitlement) | ||
| if entitlement > 0 { | ||
| return 1 | ||
| } | ||
| if entitlement < 0 { | ||
| return -1 | ||
| } | ||
|
|
||
| // If the entitlement is the same, then we can compare the limits. | ||
| if a.Limit == nil && b.Limit != nil { | ||
| return -1 | ||
| } | ||
| if a.Limit != nil && b.Limit == nil { | ||
| return 1 | ||
| } | ||
| if a.Limit != nil && b.Limit != nil { | ||
| difference := *a.Limit - *b.Limit | ||
| if *a.Limit-*b.Limit != 0 { | ||
| return int(difference) | ||
| } | ||
| } | ||
|
|
||
| // Enabled is better than disabled. | ||
| if a.Enabled && !b.Enabled { | ||
| return 1 | ||
| } | ||
| if !a.Enabled && b.Enabled { | ||
| return -1 | ||
| } | ||
|
|
||
| // Higher actual is better | ||
| if a.Actual == nil && b.Actual != nil { | ||
| return -1 | ||
| } | ||
| if a.Actual != nil && b.Actual == nil { | ||
| return 1 | ||
| } | ||
| if a.Actual != nil && b.Actual != nil { | ||
| difference := *a.Actual - *b.Actual | ||
| if *a.Actual-*b.Actual != 0 { | ||
| return int(difference) | ||
| } | ||
| } | ||
|
|
||
| return 0 | ||
| } |
There was a problem hiding this comment.
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.
| @@ -379,3 +382,164 @@ func TestExternalAuthYAMLConfig(t *testing.T) { | |||
| output := strings.Replace(out.String(), "value:", "externalAuthProviders:", 1) | |||
| require.Equal(t, inputYAML, output, "re-marshaled is the same as input") | |||
| } | |||
|
|
|||
| func TestFeatureComparison(t *testing.T) { | |||
There was a problem hiding this comment.
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
| @@ -33,6 +33,21 @@ const ( | |||
| EntitlementNotEntitled Entitlement = "not_entitled" | |||
| ) | |||
|
|
|||
| // entitlementWeight converts the enum types to a numerical value for easier | |||
| // comparisons. Easier than sets of if statements. | |||
| func entitlementWeight(e Entitlement) int { | |||
There was a problem hiding this comment.
Might be cleaner to add this as a func on Entitlement type itself. e.Weight()
|
|
||
| func (set FeatureSet) Features() []FeatureName { | ||
| switch FeatureSet(strings.ToLower(string(set))) { | ||
| case FeatureSetEnterprise: |
There was a problem hiding this comment.
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.
If I understand correctly, you are saying we have 1 list, All() (it's FeatureNames() or something atm).
And we define enterprise = 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.
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 to
1c4a305
Compare
This deprecates the
AllFeaturesboolean for aFeatureSetenum.Refactored
Entitlementsto be more unit testable. Added more unit tests.Closes #13934