CSS-9830 patch group entitlements #1318
Conversation
kian99
changed the title
kian99
force-pushed
the
CSS-9830-patch-group-entitlements
branch
from
cd1e25d
to
f145d67
Compare
There was a problem hiding this comment.
lgtm bar the slice of errs
I think a custom error type (which is capable of having a slice of errs inside) to be used in all rebac_admin methods where multiple errors can be returned.
Let's do that plz, and then handler can cast err into that type and extract them into an error DTO.
| }, Op: resources.GroupEntitlementsPatchItemOpAdd}, | ||
| } | ||
| _, err = groupSvc.PatchGroupEntitlements(ctx, newUUID.String(), operationsWithInvalidTag) | ||
| c.Assert(err, qt.ErrorMatches, `\"invalidType-foo\" is not a valid tag\n\"invalidType2-foo1\" is not a valid tag`) |
There was a problem hiding this comment.
I don't like us appending errors and sending them back in a line with \n, it feels weird... What if we wanted a clear response and formatted in json array? I'm guessing this service will be run in a handler and then perhaps it can format the error based on \n but idk... Perhaps a custom error type which can be cast by the caller to get a nice slice of errs out
There was a problem hiding this comment.
I've made a struct that we can adapt in the future to do different things. Right now I don't know that we have a format for returning a list of errors. When that is defined we can modify the struct. Let me know what you think.
| @@ -117,8 +117,10 @@ func (c *comboToken) MarshalToken() (string, error) { | |||
| func (c *comboToken) UnmarshalToken(token string) error { | |||
| out, err := base64.StdEncoding.DecodeString(token) | |||
| if err != nil { | |||
| return fmt.Errorf("marshal entitlement token: %w", err) | |||
| return fmt.Errorf("unmarshal entitlement token: %w", err) | |||
There was a problem hiding this comment.
nitpick: failed to decode the entitlement token
There was a problem hiding this comment.
I'll go with "failed to decode token"
|
|
||
| return json.Unmarshal(out, c) | ||
| if err := json.Unmarshal(out, c); err != nil { | ||
| return fmt.Errorf("failed to unmarshal combo token: %w", err) |
There was a problem hiding this comment.
nitpick: failed to unmarshal the entitlement token
There was a problem hiding this comment.
I don't want to put "entitlement" in the error string because someone coming to this function later might be confused why we're saying entitlement token when the struct is called comboToken. So I'll just tweak the message to "failed to unmarshal token"
kian99
dismissed stale reviews from alesstimec, ale8k, and pkulik0
via
747374b
|
|
||
| import "errors" | ||
|
|
||
| // MultiErr handles cases where multiple errors need to be collected. |
| @@ -0,0 +1,26 @@ | |||
| package utils | |||
There was a problem hiding this comment.
also.. should this perhaps live in our errors package?
There was a problem hiding this comment.
I'll add the copyright in a follow-up.
I think it should live here for now because this is specifically for how the rebac-handlers deals with multiple errors.
Description
This PR builds on #1315, either view the last commit here or wait for the previous PR to land before reviewing.
This PR implements
PatchGroupEntitlementsto update the relations (i.e. entitlements) for a group. The most notable part of this PR is the integration test. To actually list relationship tuples, a resource needs to exist in JIMM's database. In the test I've opted to use a model as the resource to relate a group to.In order to create a model in an integration test we would call jimm's
AddModel()method but because the Rebac-admin test suite does not spin up a Juju controller this is not possible. So instead I've opted to use the testing functionality used ininternal/jimmwhere a YAML defined string is parsed and used to create database resources. This is very convenient because creating a model requires that all the following database entries also exist controller/cloud/cloud-credentials.The final hurdle to cater for the above is figuring out how to pass a GoCheck.C object into the existing methods for parsing and applying the yaml environment which currently accepts a QuickTest object. I've switched out the qt struct in favor of our
Testerinterface which both Gocheck and Quicktest implement.Completes CSS-9830
Engineering checklist
Check only items that apply