-
Notifications
You must be signed in to change notification settings - Fork 164
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| /* | ||
| Copyright 2025 The Kubernetes Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| // Package issuemanagement implements issue management commands. | ||
| package issuemanagement | ||
|
|
||
| import ( | ||
| "regexp" | ||
|
|
||
| "github.com/sirupsen/logrus" | ||
| "sigs.k8s.io/prow/pkg/config" | ||
| "sigs.k8s.io/prow/pkg/github" | ||
| "sigs.k8s.io/prow/pkg/pluginhelp" | ||
| "sigs.k8s.io/prow/pkg/plugins" | ||
| ) | ||
|
|
||
| const pluginName = "issue-management" | ||
|
|
||
| var ( | ||
| linkIssueRegex = regexp.MustCompile(`(?mi)^/link-issue((?: +(?:\d+|[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+#\d+))+)\b`) | ||
| unlinkIssueRegex = regexp.MustCompile(`(?mi)^/unlink-issue((?: +(?:\d+|[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+#\d+))+)\b`) | ||
| ) | ||
|
|
||
| type githubClient interface { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 PTAL and let me know your thoughts!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| CreateComment(org, repo string, number int, comment string) error | ||
| GetIssue(org, repo string, number int) (*github.Issue, error) | ||
| GetPullRequest(org, repo string, number int) (*github.PullRequest, error) | ||
| GetRepo(org, name string) (github.FullRepo, error) | ||
| IsMember(org, user string) (bool, error) | ||
| UpdatePullRequest(org, repo string, number int, title, body *string, open *bool, branch *string, canModify *bool) error | ||
| } | ||
|
|
||
| func helpProvider(_ *plugins.Configuration, _ []config.OrgRepo) (*pluginhelp.PluginHelp, error) { | ||
| pluginHelp := &pluginhelp.PluginHelp{ | ||
| Description: "The issue management plugin provides commands for linking and unlinking issues to a PR.", | ||
| } | ||
| pluginHelp.AddCommand(pluginhelp.Command{ | ||
| Usage: "/link-issue <issue(s)>", | ||
| Description: "Links issue(s) to a PR in the same or different repo.", | ||
| WhoCanUse: "Org members", | ||
| Examples: []string{"/link-issue 1234", "/link-issue org/repo#789"}, | ||
| }) | ||
| pluginHelp.AddCommand(pluginhelp.Command{ | ||
| Usage: "/unlink-issue <issue(s)>", | ||
| Description: "Unlinks issue(s) from a PR in the same or different repo.", | ||
| WhoCanUse: "Org members", | ||
| Examples: []string{"/unlink-issue 1234", "/unlink-issue org/repo#789"}, | ||
| }) | ||
| return pluginHelp, nil | ||
| } | ||
|
|
||
| func init() { | ||
| plugins.RegisterGenericCommentHandler(pluginName, handleGenericComment, helpProvider) | ||
| } | ||
|
|
||
| func handleGenericComment(pc plugins.Agent, e github.GenericCommentEvent) error { | ||
| return handleIssues(pc.GitHubClient, pc.Logger.WithFields(logrus.Fields{ | ||
| "org": e.Repo.Owner.Login, | ||
| "repo": e.Repo.Name, | ||
| "number": e.Number, | ||
| "user": e.User.Login, | ||
| }), e) | ||
| } | ||
|
|
||
| func handleIssues(gc githubClient, log *logrus.Entry, e github.GenericCommentEvent) error { | ||
|
|
||
| switch { | ||
| case linkIssueRegex.MatchString(e.Body): | ||
| log.Info("Handling link issue command") | ||
| return handleLinkIssue(gc, log, e, true) | ||
| case unlinkIssueRegex.MatchString(e.Body): | ||
| log.Info("Handling unlink issue command") | ||
| return handleLinkIssue(gc, log, e, false) | ||
| default: | ||
| return nil | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,218 @@ | ||
| /* | ||
| Copyright 2025 The Kubernetes Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| // The `/link-issue` and `/unlink-issue` command allows | ||
| // members of the org to link and unlink issues to PRs. | ||
| package issuemanagement | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "regexp" | ||
| "sort" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/sirupsen/logrus" | ||
| "sigs.k8s.io/prow/pkg/github" | ||
| "sigs.k8s.io/prow/pkg/plugins" | ||
| ) | ||
|
|
||
| var ( | ||
| fixesRegex = regexp.MustCompile(`(?i)^fixes\s+(.*)$`) | ||
| ) | ||
|
|
||
| type IssueRef struct { | ||
| Org string | ||
| Repo string | ||
| Num int | ||
| } | ||
|
|
||
| func handleLinkIssue(gc githubClient, log *logrus.Entry, e github.GenericCommentEvent, linkIssue bool) error { | ||
| org := e.Repo.Owner.Login | ||
| repo := e.Repo.Name | ||
| number := e.Number | ||
| user := e.User.Login | ||
|
|
||
| if !e.IsPR || e.Action != github.GenericCommentActionCreated { | ||
| return gc.CreateComment(org, repo, number, plugins.FormatResponseRaw( | ||
| e.Body, e.HTMLURL, user, "This command can only be used on pull requests.")) | ||
| } | ||
|
|
||
| isMember, err := gc.IsMember(org, user) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to fetch if %s is an org member of %s: %w", user, org, err) | ||
| } | ||
| if !isMember { | ||
| return gc.CreateComment(org, repo, number, plugins.FormatResponseRaw( | ||
| e.Body, e.HTMLURL, user, "You must be an org member to use this command.")) | ||
| } | ||
|
|
||
| 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 | ||
| } | ||
|
Comment on lines
+63
to
+77
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so we first do a feels like we can do all this at the callsite (in |
||
|
|
||
| var issueRefs []string | ||
| for _, issue := range issues { | ||
| issueRef, err := parseIssueRef(issue, org, repo) | ||
| if err != nil { | ||
| log.Debugf("Skipping invalid issue: %s", issue) | ||
| continue | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is an invalid user input, it does not make sense to log it as warning - whoever is looking at hook logs is unlikely to care or do something about it. if anything log it as In general, ideally any problem with what the user provided should be surfaced to them through a response comment (like you do with "linked issue is actually a PR" case). There can be more issues and they can fail differently, so I think we need to accumulate all errors of this type and if non-empty, surface it to the user through a comment with bullet point list or something. |
||
|
|
||
| // If repo or org of the issue reference is different from the one in which the PR is created, check if it exists | ||
| if org != issueRef.Org || repo != issueRef.Repo { | ||
| if _, err := gc.GetRepo(org, repo); err != nil { | ||
| return fmt.Errorf("failed to get repo: %w", err) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. collect errors and respond with summary comment |
||
| log.WithError(err).Error("Failed to leave comment") | ||
| } | ||
| continue | ||
| } | ||
| issueRefs = append(issueRefs, formatIssueRef(issueRef, org, repo)) | ||
| } | ||
|
|
||
| if len(issueRefs) == 0 { | ||
| log.Info("No valid issues to process.") | ||
| return nil | ||
| } | ||
|
|
||
| pr, err := gc.GetPullRequest(org, repo, number) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get PR: %w", err) | ||
| } | ||
|
|
||
| newBody := updateFixesLine(pr.Body, issueRefs, linkIssue) | ||
| if newBody == pr.Body { | ||
| log.Debug("PR body is already up-to-date. No changes needed.") | ||
| return nil | ||
| } | ||
|
|
||
| if err := gc.UpdatePullRequest(org, repo, number, nil, &newBody, nil, nil, nil); err != nil { | ||
| return fmt.Errorf("failed to update PR body: %w", err) | ||
| } | ||
|
|
||
| log.Infof("Successfully updated the PR body") | ||
| return nil | ||
| } | ||
|
|
||
| func parseIssueRef(issue, defaultOrg, defaultRepo string) (*IssueRef, error) { | ||
| // Handling single issue references | ||
| if num, err := strconv.Atoi(issue); err == nil { | ||
| return &IssueRef{Org: defaultOrg, Repo: defaultRepo, Num: num}, nil | ||
| } | ||
|
|
||
| // Handling issue references in format org/repo#issue-number | ||
| if !strings.Contains(issue, "/") { | ||
| return nil, fmt.Errorf("unrecognized issue reference: %s", issue) | ||
| } | ||
|
|
||
| parts := strings.Split(issue, "#") | ||
| if len(parts) != 2 { | ||
| return nil, fmt.Errorf("invalid issue ref: %s", issue) | ||
| } | ||
| orgRepo := strings.Split(parts[0], "/") | ||
| if len(orgRepo) != 2 { | ||
| return nil, fmt.Errorf("invalid org/repo format: %s", issue) | ||
| } | ||
| num, err := strconv.Atoi(parts[1]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid issue number: %s", issue) | ||
| } | ||
| return &IssueRef{Org: orgRepo[0], Repo: orgRepo[1], Num: num}, nil | ||
|
|
||
| } | ||
|
|
||
| func formatIssueRef(ref *IssueRef, defaultOrg, defaultRepo string) string { | ||
| if ref.Org == defaultOrg && ref.Repo == defaultRepo { | ||
| return fmt.Sprintf("#%d", ref.Num) | ||
| } | ||
| return fmt.Sprintf("%s/%s#%d", ref.Org, ref.Repo, ref.Num) | ||
| } | ||
|
|
||
| func updateFixesLine(body string, issueRefs []string, add bool) string { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
||
| lines := strings.Split(body, "\n") | ||
| var fixesLine string | ||
| fixesIndex := -1 | ||
| issueList := make(map[string]bool) | ||
|
|
||
| // Find and parse existing Fixes line | ||
| for i, line := range lines { | ||
| if m := fixesRegex.FindStringSubmatch(line); m != nil { | ||
| fixesIndex = i | ||
| for _, i := range strings.Fields(m[1]) { | ||
| issueList[i] = true | ||
| } | ||
| break | ||
| } | ||
| } | ||
|
|
||
| for _, ref := range issueRefs { | ||
| if add { | ||
| issueList[ref] = true | ||
| } else { | ||
| delete(issueList, ref) | ||
| } | ||
| } | ||
|
|
||
| if len(issueList) == 0 { | ||
| // All linked issues have been removed, the fixes line can be deleted from the PR body. | ||
| if fixesIndex != -1 { | ||
| lines = append(lines[:fixesIndex], lines[fixesIndex+1:]...) | ||
| } | ||
| return strings.Join(lines, "\n") | ||
| } | ||
|
|
||
| var newIssueRefs []string | ||
| for ref := range issueList { | ||
| newIssueRefs = append(newIssueRefs, ref) | ||
| } | ||
|
|
||
| sort.Strings(newIssueRefs) | ||
| fixesLine = "Fixes " + strings.Join(newIssueRefs, " ") | ||
|
|
||
| if fixesIndex >= 0 { | ||
| lines[fixesIndex] = fixesLine | ||
| } else { | ||
| if len(lines) > 0 && lines[len(lines)-1] != "" { | ||
| lines = append(lines, "") | ||
| } | ||
| lines = append(lines, fixesLine) | ||
| } | ||
|
|
||
| return strings.Join(lines, "\n") | ||
| } | ||
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.