- Notifications
You must be signed in to change notification settings - Fork928
AGPL Entitlements API#3523
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
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
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.
FE ✅
Uh oh!
There was an error while loading.Please reload this page.
| EntitlementEntitled Entitlement = "entitled" | ||
| EntitlementGracePeriod Entitlement = "grace_period" | ||
| EntitlementNotEntitled Entitlement = "not_entitled" |
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.
Instead ofEntitlementEntitled wouldEntitled suffice? Seems like these are implicitEntitlement based on the name.
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 like the consistency of either always or never including the prefix. It'd be nice if Go would support enums natively one day.
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 copied
type UserStatus stringconst (UserStatusActive UserStatus = "active"UserStatusSuspended UserStatus = "suspended")for the sake of consistency. But, if I were writing the package myself for the first time I'd have dropped the prefix. I don't really mind either way.
Uh oh!
There was an error while loading.Please reload this page.
coderd/features_internal_test.go Outdated
| func TestEntitlements(t *testing.T) { | ||
| t.Parallel() | ||
| t.Run("GET", func(t *testing.T) { | ||
| t.Parallel() | ||
| r := httptest.NewRequest("GET", "https://example.com/api/v2/entitlements", nil) | ||
| rw := httptest.NewRecorder() | ||
| entitlements(rw, r) | ||
| resp := rw.Result() | ||
| assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
| dec := json.NewDecoder(resp.Body) | ||
| var result codersdk.Entitlements | ||
| err := dec.Decode(&result) | ||
| require.NoError(t, err) | ||
| assert.False(t, result.HasLicense) | ||
| assert.Empty(t, result.Warnings) | ||
| for _, f := range codersdk.AllFeatures { | ||
| require.Contains(t, result.Features, f) | ||
| fe := result.Features[f] | ||
| assert.False(t, fe.Enabled) | ||
| assert.Equal(t, codersdk.EntitlementNotEntitled, fe.Entitlement) | ||
| } | ||
| }) | ||
| } |
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.
Doing this works around any middlewares we have that could make this not function.
Could we do this like a normalcoderd test instead? Then we'd buildcodersdk functions to fetch entitlements, which seems valuable for future API consumers.
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 could. That makes the test more complex to write and slower to execute. Thisunit test is just testing the handler itself returns what we expect.
There is another test case incoderd_test.go that hits the endpoint through all the API routing and middlewares, so that's covered too.
I agree a codersdk function is important, but that's in my next PR that will include a CLI command to display these entitlements.
coderd/features_internal_test.go Outdated
| func TestEntitlements(t *testing.T) { | ||
| t.Parallel() | ||
| t.Run("GET", func(t *testing.T) { | ||
| t.Parallel() | ||
| r := httptest.NewRequest("GET", "https://example.com/api/v2/entitlements", nil) | ||
| rw := httptest.NewRecorder() | ||
| entitlements(rw, r) | ||
| resp := rw.Result() | ||
| assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
| dec := json.NewDecoder(resp.Body) | ||
| var result codersdk.Entitlements | ||
| err := dec.Decode(&result) | ||
| require.NoError(t, err) | ||
| assert.False(t, result.HasLicense) | ||
| assert.Empty(t, result.Warnings) | ||
| for _, f := range codersdk.AllFeatures { | ||
| require.Contains(t, result.Features, f) | ||
| fe := result.Features[f] | ||
| assert.False(t, fe.Enabled) | ||
| assert.Equal(t, codersdk.EntitlementNotEntitled, fe.Entitlement) | ||
| } | ||
| }) | ||
| } |
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.
Doing this works around any middlewares we have that could make this not function.
Could we do this like a normalcoderd test instead? Then we'd buildcodersdk functions to fetch entitlements, which seems valuable for future API consumers.
Uh oh!
There was an error while loading.Please reload this page.
Signed-off-by: Spike Curtis <spike@coder.com>
Uh oh!
There was an error while loading.Please reload this page.
Relates to#3278
I intend to follow up this PR with a CLI command to view the information in the Entitlements API.
RFC description of this API