Better describe AccessCapabilities response when role filtering is applied #49062
Conversation
kopiczko
added
the
no-changelog
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| disableFilter bool | ||
| allowedResourceIDs []types.ResourceID | ||
| expectedRoles []string | ||
| name string |
There was a problem hiding this comment.
Does this change is intended PR desc says:
Better describe AccessCapabilities response when role filtering is applied
However, while the changes look good overall, they reorder tests (like filter by resources <-->"filtering disabled, rename some test cases, and add additional expectedApplicableRolesForResources and additional test case. From a review perspective, this makes it a bit messy.
There was a problem hiding this comment.
Test changes:
- rename
can view resource but not assume role→can request resource but not assume role - add
confirm can request resource when filtering disabled - don’t know how I managed to reorder
filtering disabledwithfilter by resources- sorry for the unnecessary nosie
Adding expectedApplicableRolesForResources made it easier for me to comprehend what's going on here and how those two thing relate, but it may be subjective. I won't disagree with that.
Naming PR Better ... may be controversial. It was easier to understand it for me. I hoped it may be the same for others. If the general consensus is it makes less understandable then I'm happy to close it.
|
I'm closing it. After learning more, I'm not certain about the value of this PR. The whole access capabilities thing is quite complicated unfortunately. |
Some clarifications I added while trying to understand access capabilities rpc.