[Connect] Add SuggestedReviewers and RequestableRoles to LoggedInUser #19466
[Connect] Add SuggestedReviewers and RequestableRoles to LoggedInUser #19466
Conversation
|
I recall the discussion about the Maybe we could just return |
|
I think that the decision made in that discussion is still relevant. We just document and move on. The reasoning is that the caller knows what information is available, and if documentation is written well, and future caller with know how to get that information as well. To add on to this with a look to the future, once we keep open the clients, this will be available at anytime anyway so it might not be worth agonizing over much more.
I think this is a good place for this too. |
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
The overall approach looks good, I left some minor questions and suggestions.
To add on to this with a look to the future, once we keep open the clients, this will be available at anytime anyway so it might not be worth agonizing over much more.
What do you mean by "available at anytime"? Wouldn't it still require a network request (and being logged in, depending on the answer to my last question)?
| // GetClusterFeatures returns a list of features enabled/disabled by the auth server | ||
| func (c *Cluster) GetClusterFeatures(ctx context.Context) (*proto.Features, error) { | ||
| var authPingResponse proto.PingResponse | ||
| func (c *Cluster) GetClusterDetails(ctx context.Context) (*GetClusterDetailsResponse, error) { |
There was a problem hiding this comment.
Could you add a comment to this method? It could note how this method grabs details that cannot be read from disk (as they are not stored on disk).
There was a problem hiding this comment.
Also, could you document how this method behaves with regards to authentication? I don't remember if it's needed here or not and how the new changes affect that.
There was a problem hiding this comment.
Ah, when I said "document how this method behaves with regards to authentication" I meant documenting whether this requires the user to have a valid cert or if it's going to return some default data if the user is not logged in.
| cluster.LoggedInUser = clusterDetails.LoggedInUser | ||
| cluster.Features = clusterDetails.Features |
There was a problem hiding this comment.
How do you feel about setting those in cluster.GetClusterDetails?
There was a problem hiding this comment.
I wouldn't mind but my reasoning is, because the name cluster.GetClusterDetails, it seems like it should return details. This can be reused elsewhere. If we chose to set them in cluster.GetClusterDetails then it would have to return a Cluster instead and should probably change the name to like... AddClusterDetails or something and that seems a bit weirder to me.
I won't die on any hill, I opt for the way it is, but I'm open to any debate on the matter.
There was a problem hiding this comment.
If we chose to set them in
cluster.GetClusterDetailsthen it would have to return aClusterinstead and should probably change the name to like...AddClusterDetailsor something and that seems a bit weirder to me.
Yeah, I'm not sure what the convention for this is; the only thing which comes to my mind is something like uhhhh Enrich…?
My argument for this would consist of two points. First, since we set those fields on Cluster anyway, we could let the struct itself set them rather than doing it from outside.
Second, I generally try to avoid doing things that might be useful later unless I have reasons to believe that there will be a place to use them. So far I haven't heard about reusing those cluster details elsewhere and if we ever ended up needing them elsewhere, we could always refactor the existing code.
Ultimately this particular usage is also not something super important for me. What I care about more than that is what patterns we establish and avoiding adding stuff for which we don't have use at the moment. We can have a nuanced discussion under this PR but the next person who's going to need a similar object which requires "enriching" is going to copy what's already in the project.
| // GetClusterFeatures returns a list of features enabled/disabled by the auth server | ||
| func (c *Cluster) GetClusterFeatures(ctx context.Context) (*proto.Features, error) { | ||
| var authPingResponse proto.PingResponse | ||
| func (c *Cluster) GetClusterDetails(ctx context.Context) (*GetClusterDetailsResponse, error) { |
There was a problem hiding this comment.
Ah, when I said "document how this method behaves with regards to authentication" I meant documenting whether this requires the user to have a valid cert or if it's going to return some default data if the user is not logged in.
| cluster.LoggedInUser = clusterDetails.LoggedInUser | ||
| cluster.Features = clusterDetails.Features |
There was a problem hiding this comment.
If we chose to set them in
cluster.GetClusterDetailsthen it would have to return aClusterinstead and should probably change the name to like...AddClusterDetailsor something and that seems a bit weirder to me.
Yeah, I'm not sure what the convention for this is; the only thing which comes to my mind is something like uhhhh Enrich…?
My argument for this would consist of two points. First, since we set those fields on Cluster anyway, we could let the struct itself set them rather than doing it from outside.
Second, I generally try to avoid doing things that might be useful later unless I have reasons to believe that there will be a place to use them. So far I haven't heard about reusing those cluster details elsewhere and if we ever ended up needing them elsewhere, we could always refactor the existing code.
Ultimately this particular usage is also not something super important for me. What I care about more than that is what patterns we establish and avoiding adding stuff for which we don't have use at the moment. We can have a nuanced discussion under this PR but the next person who's going to need a similar object which requires "enriching" is going to copy what's already in the project.
| // EnrichCluster will make a network request to the auth server and add details to the | ||
| // current Cluster that cannot be found on the disk only, including details about the LoggedInUser | ||
| // and enabled enterprise features. This method requires a valid cert. | ||
| func (c *Cluster) EnrichCluster(ctx context.Context) (*Cluster, error) { |
There was a problem hiding this comment.
Nit: since we're already in the clusters package and this is a method on the Cluster struct, to avoid repetition it might be good to drop Cluster from the name of this method and rename it to something like EnrichWithDetails.
See the third paragraph here https://go.dev/doc/effective_go#package-names
4 participants
Closes https://github.com/gravitational/webapps.e/issues/410
I want to draft this first to make sure I'm going down the right path. A couple notes on the implementation.
My original idea was to pass around the auth client to a bunch of discrete methods but that was a bit weird as the
deferto close the auth client was in an unintuitive spot (the daemon service). So it made a bit more sense to just open up the connection in one method and get everything we needed, which is just two methodsauthClient.PingandauthClient.GetAccessCapabilities.I will be adding more comments to the new methods and fields before the pull request is finalized.
The frontend changes won't be too substantial but I wanted this side in a good spot before I PR that. The main thing is when creating a role based Access Request now, we no longer need to make a network request, as the roles are already present in the LoggedInUser. "But Michael, doesn't that mean we can remove the
GetRequestableRoleshandler in the backend?" Actually, we still need it as it's also used to get the minimum required roles in the new roles selection dropdown for Resource Access Requests. Although, I should probably rename it fromGetRequestableRolestoGetMinimumRequireRolesor something that makes more sense for its only purpose now.