-
Notifications
You must be signed in to change notification settings - Fork 160
plugins: add new plugin for linking and unlinking issues to a PR #556
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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: Amulyam24 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2dc443d to
30cf25e
Compare
| @@ -0,0 +1,95 @@ | |||
| /* | |||
| Copyright 2025 The Kubernetes Authors. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI, current guidance is to not include the year for these copyright headers. I don't think it matters, just pointing it out to help spread awareness.
| unlinkIssueRegex = regexp.MustCompile(`(?mi)^/unlink-issue((?: +(?:\d+|[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+#\d+))+)\b`) | ||
| ) | ||
|
|
||
| type githubClient interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe there is a reason for defining this interface unless you are going to mock it for unit testing. Is that planned as a follow up? Might be good to have that included in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was planning that as a follow up. Sure, I'll add it to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @stmcginnis, I explored a couple of ways on adding the UT.
At the end I feel having this interface and using the existing fake client is convenient than mocking the githubClient or using the plugins.PluginGitHubClient directly instead of githubClient as that would need mocking as well.
PTAL and let me know your thoughts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a small local interface is somewhat established pattern in GH-facing plugins (retitle, blunderbuss), the GH client interface surface is massive and it is often useful to have an idea of what GH operations the plugin is limited to (besides the mentioned test mock use case). I'd prefer to keep it.
|
@Amulyam24: The label(s) `/label do-not-merge/work-in-progress
In response to this:
Instructions 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. |
|
/hold WIP: Adding UT |
30cf25e to
427609d
Compare
2419527 to
afde1e0
Compare
This PR adds a new plugin issue-management which has commands for linking and unlinking issues to a PR. - The commands can be used to link an issue to a PR in the current repository or in a different repository as well as handle multiple issues - This is done by adding the supported keyword Fixes to the body of the PR if it doesn't already exist or by appending the issue to the existing Fixes line - Supported formats are issue-number and org/repo-name#issue-number Signed-off-by: Amulyam24 <amulmek1@in.ibm.com>
afde1e0 to
c65a580
Compare
|
/unhold |
| unlinkIssueRegex = regexp.MustCompile(`(?mi)^/unlink-issue((?: +(?:\d+|[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+#\d+))+)\b`) | ||
| ) | ||
|
|
||
| type githubClient interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a small local interface is somewhat established pattern in GH-facing plugins (retitle, blunderbuss), the GH client interface surface is massive and it is often useful to have an idea of what GH operations the plugin is limited to (besides the mentioned test mock use case). I'd prefer to keep it.
| log.WithFields(logrus.Fields{ | ||
| "org": e.Repo.Owner.Login, | ||
| "repo": e.Repo.Name, | ||
| "number": e.Number, | ||
| "user": e.User.Login, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make this logrus.Fieds instance in handleGenericComment and pass it to here with pc.Logger.WithFields, then this method already gets a populated logger and can just log.Info the necessary message
| regex := linkIssueRegex | ||
| if !linkIssue { | ||
| regex = unlinkIssueRegex | ||
| } | ||
|
|
||
| matches := regex.FindStringSubmatch(e.Body) | ||
| if len(matches) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| issues := strings.Fields(matches[1]) | ||
| if len(issues) == 0 { | ||
| log.Info("No issue references provided in the comment.") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we first do a MatchString at the callsite (in handleIssues), set a boolean based on that, then we use that boolean to select the regex again and match it again with FindStringSubmatch
feels like we can do all this at the callsite (in handleIssues) and just pass issues to this method (in addition to the link/unlink boolean)
| } | ||
|
|
||
| // Handling issue references in format org/repo#issue-number | ||
| if strings.Contains(issue, "/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would slightly prefer making this a standard guard clause
| if strings.Contains(issue, "/") { | |
| if !strings.Contains(issue, "/") { | |
| return nil, fmt.Errorf("unrecognized issue reference: %s", issue) | |
| } | |
| parts := strings.Split(issue, "#") | |
| ... | |
| return &IssueRef{Org: orgRepo[0], Repo: orgRepo[1], Num: num}, nil |
|
|
||
| newBody := updateFixesLine(pr.Body, issueRefs, linkIssue) | ||
| if newBody == pr.Body { | ||
| log.Info("PR body is already up-to-date. No changes needed.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls downgrade to debug
| case unlinkIssueRegex.MatchString(e.Body): | ||
| log.WithFields(logrus.Fields{ | ||
| "org": e.Repo.Owner.Login, | ||
| "repo": e.Repo.Name, | ||
| "number": e.Number, | ||
| "user": e.User.Login, | ||
| }).Info("Handling unlink issue command") | ||
| return handleLinkIssue(gc, log, e, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm this means the user can either say /link or say /unlink but not both; if they do both, then only the /link will count. But these operations are sometimes symmetrical, let's say you make a typo and link something wrong, then you'd probably want to fix it like this:
/unlink wrong
/link right
So I think we should have something like a method that is given a comment body and it produces issues to add links to, and issues to remove links to, and then we'd pass both to handleLinkIssue instead of the add boolean`:
toLink, to unlink := parseCommentForLinkCommands(...)
if len(toLink) == 0 && len(toUnlink) == 0 {
return nothing to do
}
return handleLinkIssue(..., toLink, toUnlink)
| // If repo in issue reference is different from the PR, check if it exists | ||
| if repo != issueRef.Repo { | ||
| if _, err := gc.GetRepo(issueRef.Org, issueRef.Repo); err != nil { | ||
| return fmt.Errorf("failed to get repo: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collect errors and respond with summary comment
| // Verify if the issue exists | ||
| fetchedIssue, err := gc.GetIssue(issueRef.Org, issueRef.Repo, issueRef.Num) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get issue: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collect errors and respond with summary comment
| } | ||
| if fetchedIssue.IsPullRequest() { | ||
| response := fmt.Sprintf("Skipping #%d of repo **%s** and org **%s** as it is a *pull request*.", fetchedIssue.Number, issueRef.Repo, issueRef.Org) | ||
| if err := gc.CreateComment(org, repo, number, plugins.FormatResponseRaw(e.Body, e.HTMLURL, user, response)); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collect errors and respond with summary comment
| return fmt.Sprintf("%s/%s#%d", ref.Org, ref.Repo, ref.Num) | ||
| } | ||
|
|
||
| func updateFixesLine(body string, issueRefs []string, add bool) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now if this is changed to the toLink / toUnlink concept I propsed above, then I think we can use sets to compute the resulting issues that the pr should have links to: existing.Difference(toUnlink).Union(toLink)
| } | ||
|
|
||
| // If repo in issue reference is different from the PR, check if it exists | ||
| if repo != issueRef.Repo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if repo != issueRef.Repo { | |
| if org != issueRef.Org || repo != issueRef.Repo { |
I think we need to do the same if the repo name is the same but org differs (like in a fork)
|
I think this is pretty close, I appreciate the test coverage. Left some comment about the code structure. |
This PR adds a new plugin issue-management which has commands for linking and unlinking issues to a PR.
Ref - https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue
Fixesto the body of the PR if it doesn't already exist or by appending the issue to the existing Fixes lineissue-numberandorg/repo-name#issue-numberA new plugin has been added to accommodate any existing issue commands or provide flexibility to support more issue commands in the future.
Fixes #359