feat: manage non-k8s members assigning issues#698
Conversation
✅ Deploy Preview for k8s-prow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: TineoC The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @TineoC! |
|
Hi @TineoC. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| if len(toAdd) > 0 { | ||
| h.log.Printf("Adding %s to %s/%s#%d: %v", h.userType, org, repo, e.Number, toAdd) | ||
| if h.userType == "assignee(s)" { | ||
| isMember, err := h.gc.IsMember(org, e.User.Login) |
There was a problem hiding this comment.
Do we know if this call is typically cached? We're sensitive to API usage, especially because prow is still running on PAT rather than a github app (quota scales with org size) IIRC ...
Something we should really fix at some point ...
There was a problem hiding this comment.
@petr-muller probably has the most context currently ...
Amulyam24
left a comment
There was a problem hiding this comment.
@TineoC, this is an interesting enhancement!
A question on the error scenarios- if GetIssueLabels or IsMember fails, we are currently logging it as a warning. In such cases, it would leave the user with no understanding on what went wrong when they issue the command, how about we add a user friendly message to retry?
| if err != nil { | ||
| h.log.WithError(err).Warn("Failed to get issue labels") | ||
| } | ||
| hasGoodFirstIssue := false |
There was a problem hiding this comment.
| hasGoodFirstIssue := false | |
| isGoodFirstIssue := false |
Could be as above for better readability, wdyt?
| return nil | ||
| } | ||
| err := handle(newAssignHandler(e, pc.GitHubClient, pc.Logger)) | ||
| err := handle(newAssignHandler(e, pc.GitHubClient, pc.Logger, pc.PluginConfig)) |
There was a problem hiding this comment.
Any reason for passing the entire instance of PluginConfig here?
| guideLink = fmt.Sprintf(" and our [contributor guide](%s)", h.config.Help.HelpGuidelinesURL) | ||
| } | ||
|
|
||
| msg := fmt.Sprintf("It looks like you're new! This issue hasn't been vetted for beginners yet. Please check out the [Good First Issues List](https://github.com/%s/%s/labels/good-first-issue)%s to get started.", org, repo, guideLink) |
There was a problem hiding this comment.
| msg := fmt.Sprintf("It looks like you're new! This issue hasn't been vetted for beginners yet. Please check out the [Good First Issues List](https://github.com/%s/%s/labels/good-first-issue)%s to get started.", org, repo, guideLink) | |
| msg := fmt.Sprintf("Thank you for your interest in contributing! It looks like you're new, this issue hasn't been vetted for beginners yet. Please check out the [Good First Issues List](https://github.com/%s/%s/labels/good-first-issue)%s to get started.", org, repo, guideLink) |
How about this? Feel free to enhance it accordingly for a more welcoming message :)
39b863b to
33c9216
Compare
Block non-member self-assignment on issues that are not marked good-first-issue, provide clearer retry and guidance comments, and refactor the assign flow into smaller helpers to keep the happy path flat.
33c9216 to
cb384e0
Compare
Rename the self-assignment enforcement helper to better reflect that it applies the evaluated policy rather than conditionally doing work.
|
Hi @TineoC, revisiting the change regarding the hard restriction being imposed for non-members assigning non-good first issues, I'm not quite sure if this should be the intended behaviour. For example, there may be issues with the help-wanted label that a non-member would like to pick up and contribute to. Similarly, there could be low hanging issues, such as small bugs or minor feature enhancements, that may not necessarily be categorised as a good first issue but could still be approachable and interesting for new contributors. Do you mind sharing a bit more context on the reason behind this enhancement? |
For context, It comes from this conversation with @BenTheElder |
Thank you @TineoC! Hi @BenTheElder @petr-muller, curious to know your thoughts on the above, PTAL! |
I think this is a good example of the situation. kubernetes/kubernetes#131724 (comment) Over +10 non-members self-assigned the issue in a short period of time and seems to be they found an exception where you're able to self-assign the issue if you've commented in that issue. cc. @lmktfy @BenTheElder |
I agree that it is a valid concern. Not sure how feasible this would be - considering |
|
Here's the algorithm I think will help. • if there is already an assignee, non-members can't reassign the issues |
|
For k/website I would like Prow to be able to point out SIG Docs' no cookie licking policy. But that's a separate thing. |
+1, sounds reasonable to me! |
How this restriction will work if an org member tries to assign a non-org member to any issue? From my past experience there are cases where I would want to be able to do that, and it shouldn't conflict with the problem you're trying to solve here. |
AFAIK, currently it does not assign the issue by returning with the following comment.
|
Refined the assign logic to restrict self-assignment for non-organization members.
Non-members can now only self-assign issues labeled good-first-issue.
If a non-member attempts to assign a standard issue, the plugin will trigger an educational response, redirecting them with issues with
good-first-issue.