Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
Co-authored-by: John Schaeffer <[email protected]>
Signed-off-by: Bailin He <[email protected]>
  • Loading branch information
bailinhe and jnschaeffer committed Apr 22, 2024
1 parent c357346 commit 6f0d6c2
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 8 deletions.
4 changes: 2 additions & 2 deletions internal/query/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ var (
// ErrResourceNotFound represents an error when no matching resource was found
ErrResourceNotFound = errors.New("resource not found")

// ErrRoleBindingNotFound represents an error when no matching role-binding was found
ErrRoleBindingNotFound = errors.New("role-binding not found")
// ErrRoleBindingNotFound represents an error when no matching role binding was found
ErrRoleBindingNotFound = errors.New("role binding not found")

// ErrRoleHasTooManyResources represents an error which a role has too many resources
ErrRoleHasTooManyResources = errors.New("role has too many resources")
Expand Down
12 changes: 6 additions & 6 deletions internal/query/roles_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ func (e *engine) GetRoleV2(ctx context.Context, role types.Resource) (types.Role
ctx, span := e.tracer.Start(
ctx,
"engine.GetRoleV2",
trace.WithAttributes(attribute.Stringer("role", role.ID)),
trace.WithAttributes(attribute.Stringer("permissions.role_id", role.ID)),
)
defer span.End()

Expand Down Expand Up @@ -284,7 +284,7 @@ func (e *engine) UpdateRoleV2(ctx context.Context, actor, roleResource types.Res
return role, nil
}

// 1. update role in permission-api DB
// 1. update role in permissions-api DB
dbRole, err := e.store.UpdateRole(dbCtx, actor.ID, role.ID, newName)
if err != nil {
span.RecordError(err)
Expand All @@ -295,7 +295,7 @@ func (e *engine) UpdateRoleV2(ctx context.Context, actor, roleResource types.Res
return types.Role{}, err
}

// 2. update permissions relationships in spice db
// 2. update permissions relationships in SpiceDB
updates := []*pb.RelationshipUpdate{}
roleRef := resourceToSpiceDBRef(e.namespace, roleResource)

Expand All @@ -321,7 +321,7 @@ func (e *engine) UpdateRoleV2(ctx context.Context, actor, roleResource types.Res
)
}

// 2.c write updates to spicedb
// 2.c write updates to SpiceDB
request := &pb.WriteRelationshipsRequest{Updates: updates}

if _, err := e.client.WriteRelationships(ctx, request); err != nil {
Expand All @@ -339,10 +339,10 @@ func (e *engine) UpdateRoleV2(ctx context.Context, actor, roleResource types.Res

logRollbackErr(e.logger, e.store.RollbackContext(dbCtx))

// At this point, spicedb changes have already been applied.
// At this point, SpiceDB changes have already been applied.
// Attempting to rollback could result in failures that could result in the same situation.
//
// TODO: add spicedb rollback logic along with rollback failure scenarios.
// TODO: add SpiceDB rollback logic along with rollback failure scenarios.

return types.Role{}, err
}
Expand Down

0 comments on commit 6f0d6c2

Please sign in to comment.