Add Access List owners to suggested reviewers.#34091
Conversation
When creating an access request, any owners of potential access list promotions will be added as suggested reviewers. These users will, in turn be notified through regular notification mechanisms.
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
1 similar comment
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
| // update the request with additional reviewers if possible. | ||
| updateAccessRequestWithAdditionalReviewers(ctx, req, a.AccessListClient(), promotions) |
There was a problem hiding this comment.
Why not move this inside if promotions != nil { below? I think it's cleaner to process a promotion when you know it exists instead of checking inside the function and a few lines below.
There was a problem hiding this comment.
I did this specifically to ensure that the suggested reviewers is modified during the creation of the access request -- without doing this here, we can't update the suggested reviewers in the access request itself without doing a subsequent update after the fact.
mdwn
force-pushed
the
mike.wilson/add-access-list-owners-to-suggested-reviewers
branch
from
ddff9f0 to
4dc4541
Compare
public-teleport-github-review-bot
Bot
removed the request for review
from hugoShaka
r0mant
left a comment
r0mant
left a comment
There was a problem hiding this comment.
I'd like to understand the UX of this better. Specifically, the questions I have are:
- What's the UX in the access request creation dialog? Are suggested reviewers get displayed there automatically so users can update them?
- Have we verified suggested reviewers actually do get notified with our existing access request plugins?
r0mant
left a comment
r0mant
left a comment
There was a problem hiding this comment.
From what I understand what this does is just setting suggested reviewers in the request but what we want is to populate the "suggested reviewers" in the access list creation dialog in the web UI so I think we need some kind of API to return them and then display them in the UI?
You're right, this doesn't show up in the UI. We'd need an API for that as you suggest. Do we want these reviewers to be suggested only in the UI, or do we want them to be populated for requests made by the CLI as well? |
|
@jakule @AntonAM Hey guys, I would appreciate a second once over. I changed this so that it only populates suggested reviewers for dry run requests so that it can be used by the UI. The corresponding |
@mdwn I don't think CLI is as important tbh. |
jakule
left a comment
jakule
left a comment
There was a problem hiding this comment.
A few nits. I think that the implementation is fine. I'm a bit worried about the performance as the computation of promoted lists can take a while and a user will need to wait for it.
| } | ||
|
|
||
| // getAccessRequestPromotions will return potential access list promotions for an access request. | ||
| func (a *Server) getAccessRequestPromotions(ctx context.Context, req types.AccessRequest) (types.AccessRequest, *types.AccessRequestAllowedPromotions) { |
There was a problem hiding this comment.
Do we really need to return request copy from this function? I don't think we actually modify the request, so calling function can also just do req.Copy() itself. It will require one more Copy() call but unless it's really expensive, I think code will look better.
There was a problem hiding this comment.
AFAICT no, but I didn't want to change things in this regard without knowing for certain that this request was guaranteed not to be changed. Given that this copy is being made, I'm guessing it's possible this could modify the original request?
mdwn
deleted the
mike.wilson/add-access-list-owners-to-suggested-reviewers
branch
4 participants
When creating an access request, any owners of potential access list promotions will be added as suggested reviewers. These users will, in turn be notified through regular notification mechanisms.
changelog: Access List owners will be added as reviewers to promotable Access Requests if the Access Request targets resources that belong to an Access List. Reviewers will be notified via existing notification mechanisms.
Note: This has been adjusted so that it will only populated suggested reviewers when dry run is set. Dry run is used by the UI in order to populate access request defaults, so we'll latch into this mechanism for pre-populating suggested reviewers.