Improve cluster|project list-members subcommands #399
Conversation
| func getMemberNameFromPrincipal(principals principalGetter, principalID string) string { | ||
| principal, err := principals.ByID(url.PathEscape(principalID)) | ||
| if err != nil { | ||
| principal = parsePrincipalID(principalID) |
There was a problem hiding this comment.
Should this maybe only happen if it's not found? Feels a little odd to show the user details if we got a network error communicating with the cluster?
There was a problem hiding this comment.
Nope. This is deliberate. Up until rancher/rancher#47702 this endpoint always returned 500. And the fix is not backported yet. But more importantly there is no reason to break the whole command (that lists {C|P}RTB's) just because we can't look up a principal. We can do the next best thing - parse the principal name and produce the output. More over I think that's what UI should do too.
|
|
||
| err := listProjectMembers(cctx, &out, userConfig, prtbs, principals) | ||
| require.NoError(t, err) | ||
| require.NotEmpty(t, out) |
There was a problem hiding this comment.
nit: you don't need require.NotEmpty(t, out) as you have a deeper assertion below.
There was a problem hiding this comment.
This was intentional. There is no need to do any other work if the output is empty. This assertion is cheap.
| FocusedProject() string | ||
| } | ||
|
|
||
| func listClusterMembers(ctx *cli.Context, out io.Writer, config userConfig, crtbs crtbLister, principals principalGetter) error { |
There was a problem hiding this comment.
Great change for testability
|
Do not merge: this is slated for v2.11. |
Ref: rancher/rancher#47692