CSS-9389 Implement the EntitlementsService interface
#1277
Conversation
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
SimoneDutto
force-pushed
the
add-entitlements-api
branch
from
7cac0d2 to
02c5b4c
Compare
SimoneDutto
force-pushed
the
add-entitlements-api
branch
from
02c5b4c to
c297be2
Compare
SimoneDutto
changed the title
EntitlementsService interface
SimoneDutto
force-pushed
the
add-entitlements-api
branch
from
59d1790 to
a85d537
Compare
service_test.go
Outdated
| @@ -293,6 +293,32 @@ func TestRebacAdminApi(t *testing.T) { | |||
| c.Assert(response.StatusCode, qt.Equals, 200) | |||
| } | |||
|
|
|||
| func TestRebacEntitlementsApi(t *testing.T) { | |||
There was a problem hiding this comment.
I don't think we should be adding this test in here, otherwise we might start to test all our HTTP endpoints in here which is probably not what we want.
@ale8k @babakks @alesstimec What do you guys think about this?
In other packages like internal/jimmjwx/ we create a JIMM service by calling jimm.NewService. Should service layer tests exist here (in service_test.go) or in the package internal/rebac_admin?
There was a problem hiding this comment.
@SimoneDutto Yeah, please remove this test.
@kian99 I agree. We already have a test for ReBAC Admin API in service_test.go but that's just for the sake of composition and to make sure we're correctly enabling the API.
Other endpoints should be tested inside the rebac_admin package. Also, I think we shouldn't test against the HTTP endpoints (i.e., calling /entitlements directly) because that's the responsibility of the rebac-admin-ui-handlers library (and we should know as little as possible about the HTTP API endpoints in JIMM). So, we can safely assume the library works as expected.
Generally, in the rebac_admin package tests we should just call the methods (like EntitlementsService.RawEntitlements) directly and assert the returned value against an expected value. However, in our static implementation of EntitlementsService interface, I don't see a point of having tests only to assert against a the same static values.
| {EntitlementType: "writer", EntityName: "user", EntityType: "model"}, | ||
| {EntitlementType: "writer", EntityName: "user:*", EntityType: "model"}, | ||
| {EntitlementType: "writer", EntityName: "group#member", EntityType: "model"}, | ||
| {EntitlementType: "controller", EntityName: "controller", EntityType: "model"}, |
There was a problem hiding this comment.
This should be removed. Generally, let's remove all entitlements that has a receiver other than user, user:*, or group#member.
| {EntitlementType: "can_addmodel", EntityName: "user", EntityType: "cloud"}, | ||
| {EntitlementType: "can_addmodel", EntityName: "user:*", EntityType: "cloud"}, | ||
| {EntitlementType: "can_addmodel", EntityName: "group#member", EntityType: "cloud"}, | ||
| {EntitlementType: "controller", EntityName: "controller", EntityType: "cloud"}, |
| {EntitlementType: "reader", EntityName: "user", EntityType: "applicationoffer"}, | ||
| {EntitlementType: "reader", EntityName: "user:*", EntityType: "applicationoffer"}, | ||
| {EntitlementType: "reader", EntityName: "group#member", EntityType: "applicationoffer"}, | ||
| {EntitlementType: "model", EntityName: "model", EntityType: "applicationoffer"}, |
service_test.go
Outdated
| @@ -293,6 +293,32 @@ func TestRebacAdminApi(t *testing.T) { | |||
| c.Assert(response.StatusCode, qt.Equals, 200) | |||
| } | |||
|
|
|||
| func TestRebacEntitlementsApi(t *testing.T) { | |||
There was a problem hiding this comment.
@SimoneDutto Yeah, please remove this test.
@kian99 I agree. We already have a test for ReBAC Admin API in service_test.go but that's just for the sake of composition and to make sure we're correctly enabling the API.
Other endpoints should be tested inside the rebac_admin package. Also, I think we shouldn't test against the HTTP endpoints (i.e., calling /entitlements directly) because that's the responsibility of the rebac-admin-ui-handlers library (and we should know as little as possible about the HTTP API endpoints in JIMM). So, we can safely assume the library works as expected.
Generally, in the rebac_admin package tests we should just call the methods (like EntitlementsService.RawEntitlements) directly and assert the returned value against an expected value. However, in our static implementation of EntitlementsService interface, I don't see a point of having tests only to assert against a the same static values.
|
|
||
| import "github.com/canonical/rebac-admin-ui-handlers/v1/resources" | ||
|
|
||
| // For rebac v1 this list is kept manually. |
There was a problem hiding this comment.
To make things simpler I'd move this static slice to the entitlements.go file.
Description
Add entitlements Api.
Atm it is done via a static variables declaration.
Fixes JIRA/GitHub issue number
Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers