Update pr/issue field in fragment/changelog to store URLs (#95)

Author: lucian-ioan

changelog/fragments/1663677931-Add-URL-support-for-pr-and-issue-fields-in-fragments-and-changelog.yaml

@@ -0,0 +1,31 @@
+# Kind can be one of:
+# - breaking-change: a change to previously-documented behavior
+# - deprecation: functionality that is being removed in a later release
+# - bug-fix: fixes a problem in a previous version
+# - enhancement: extends functionality but does not break or fix existing behavior
+# - feature: new functionality
+# - known-issue: problems that we are aware of in a given version
+# - security: impacts on the security of a product or a user’s deployment.
+# - upgrade: important information for someone upgrading from a prior version
+# - other: does not fit into any of the other categories
+kind: feature
+
+# Change summary; a 80ish characters long description of the change.
+summary: Add URL support for pr and issue fields in fragments and changelog
+
+# Long description; in case the summary is not enough to describe the change
+# this field accommodate a description without length limits.
+#description:
+
+# Affected component; a word indicating the component this changeset affects.
+component:
+
+# PR number; optional; the PR number that added the changeset.
+# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
+# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
+# Please provide it if you are adding a fragment for a different PR.
+#pr: 1234
+
+# Issue number; optional; the GitHub issue related to this changeset (either closes or is part of).
+# If not present is automatically filled by the tooling with the issue linked to the PR number.
+#issue: 1234

internal/changelog/builder.go

@@ -13,6 +13,7 @@ import (
 	"os/exec"
 	"path"
 	"path/filepath"
+	"strconv"
 	"strings"
 
 	"github.com/elastic/elastic-agent-changelog-tool/internal/changelog/fragment"
@@ -110,20 +111,20 @@ func (b Builder) Build(owner, repo string) error {
 			// Applying heuristics to PR fields
 			originalPR, err := FindOriginalPR(entry.LinkedPR[0], owner, repo, c)
 			if err != nil {
-				log.Printf("%s: check if the PR field is correct in changelog", entry.File.Name)
+				log.Printf("%s: check if the PR field is correct in changelog: %s", entry.File.Name, err.Error())
 				continue
 			}
 
-			b.changelog.Entries[i].LinkedPR = []int{originalPR}
+			b.changelog.Entries[i].LinkedPR = []string{originalPR}
 		}
 
 		if len(entry.LinkedIssue) == 0 && len(b.changelog.Entries[i].LinkedPR) > 0 {
-			linkedIssues := []int{}
+			linkedIssues := []string{}
 
-			for _, pr := range b.changelog.Entries[i].LinkedPR {
-				tempIssues, err := FindIssues(graphqlClient, context.Background(), owner, repo, pr, 50)
+			for _, prURL := range b.changelog.Entries[i].LinkedPR {
+				tempIssues, err := FindIssues(graphqlClient, context.Background(), owner, repo, prURL, 50)
 				if err != nil {
-					log.Printf("%s: could not find linked issues for pr: %s/pull/%d", entry.File.Name, entry.Repository, entry.LinkedPR)
+					log.Printf("%s: could not find linked issues for pr: %s: %s", entry.File.Name, entry.LinkedPR, err.Error())
 					continue
 				}
 
@@ -134,7 +135,13 @@ func (b Builder) Build(owner, repo string) error {
 			}
 
 			b.changelog.Entries[i].LinkedIssue = linkedIssues
+		} else if len(entry.LinkedIssue) == 1 {
+			_, err := ExtractEventNumber("issue", entry.LinkedIssue[0])
+			if err != nil {
+				log.Printf("%s: check if the issue field is correct in changelog: %s", entry.File.Name, err.Error())
+			}
 		}
+
 	}
 
 	data, err := yaml.Marshal(&b.changelog)
@@ -161,6 +168,39 @@ func collectFragment(fs afero.Fs, path string, info os.FileInfo, err error, file
 	return nil
 }
 
+func ExtractEventNumber(linkType, eventURL string) (string, error) {
+	urlParts := strings.Split(eventURL, "/")
+
+	if len(urlParts) < 1 {
+		return "", fmt.Errorf("cant get event number")
+	}
+
+	switch linkType {
+	// maybe use regex to validate instead of a simple string check
+	case "pr":
+		if !strings.Contains(eventURL, "pull") {
+			return "", fmt.Errorf("link is invalid for pr")
+		}
+	case "issue":
+		if !strings.Contains(eventURL, "issues") {
+			return "", fmt.Errorf("link is invalid for issue")
+		}
+	}
+
+	return urlParts[len(urlParts)-1], nil
+}
+
+func CreateEventLink(linkType, owner, repo, eventID string) string {
+	switch linkType {
+	case "issue":
+		return fmt.Sprintf("https://github.com/%s/%s/issues/%s", owner, repo, eventID)
+	case "pr":
+		return fmt.Sprintf("https://github.com/%s/%s/pull/%s", owner, repo, eventID)
+	default:
+		panic("wrong linkType")
+	}
+}
+
 func GetLatestCommitHash(fileName string) (string, error) {
 	response, err := exec.Command("git", "log", "--diff-filter=A", "--format=%H", "changelog/fragments/"+fileName).Output()
 	if err != nil {
@@ -170,40 +210,61 @@ func GetLatestCommitHash(fileName string) (string, error) {
 	return strings.ReplaceAll(string(response), "\n", ""), nil
 }
 
-func FindIssues(graphqlClient *github.ClientGraphQL, ctx context.Context, owner, name string, prID, issuesLen int) ([]int, error) {
-	issues, err := graphqlClient.PR.FindIssues(ctx, owner, name, prID, issuesLen)
+func FindIssues(graphqlClient *github.ClientGraphQL, ctx context.Context, owner, name string, prURL string, issuesLen int) ([]string, error) {
+	prID, err := ExtractEventNumber("pr", prURL)
 	if err != nil {
 		return nil, err
 	}
 
-	return issues, nil
+	prIDInt, _ := strconv.Atoi(prID)
+
+	issues, err := graphqlClient.PR.FindIssues(ctx, owner, name, prIDInt, issuesLen)
+	if err != nil {
+		return nil, err
+	}
+
+	issueLinks := make([]string, len(issues))
+
+	for i, issue := range issues {
+		issueLinks[i] = CreateEventLink("issue", owner, name, issue)
+	}
+
+	return issueLinks, nil
 }
 
-func FillEmptyPRField(commitHash, owner, repo string, c *github.Client) ([]int, error) {
+func FillEmptyPRField(commitHash, owner, repo string, c *github.Client) ([]string, error) {
 	pr, err := github.FindPR(context.Background(), c, owner, repo, commitHash)
 	if err != nil {
-		return []int{}, err
+		return []string{}, err
 	}
 
-	var prIDs []int
+	prLinks := []string{}
 
 	for _, item := range pr.Items {
-		prIDs = append(prIDs, item.PullRequestID)
+		prLinks = append(prLinks, CreateEventLink("pr", owner, repo, strconv.Itoa(item.PullRequestID)))
 	}
 
-	return prIDs, nil
+	return prLinks, nil
 }
 
-func FindOriginalPR(linkedPR int, owner, repo string, c *github.Client) (int, error) {
-	pr, _, err := c.PullRequests.Get(context.Background(), owner, repo, linkedPR)
+func FindOriginalPR(prURL string, owner, repo string, c *github.Client) (string, error) {
+	linkedPR, err := ExtractEventNumber("pr", prURL)
 	if err != nil {
-		return 0, err
+		return "", err
+	}
+
+	linkedPRString, _ := strconv.Atoi(linkedPR)
+
+	pr, _, err := c.PullRequests.Get(context.Background(), owner, repo, linkedPRString)
+	if err != nil {
+		return "", err
 	}
 
 	prID, err := github.TestStrategies(pr, &github.BackportPRNumber{}, &github.PRNumber{})
 	if err != nil {
-		return 0, err
+		return "", err
 	}
 
-	return prID, nil
+	prLink := CreateEventLink("pr", owner, repo, strconv.Itoa(prID))
+	return prLink, nil
 }

internal/changelog/builder_test.go

@@ -41,7 +41,8 @@ func TestFillEmptyPRField(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, prIDs, 2)
 	require.NotEmpty(t, prIDs)
-	require.ElementsMatch(t, []int{30979, 31279}, prIDs)
+	require.ElementsMatch(t, []string{
+		"https://github.com/elastic/beats/pull/30979", "https://github.com/elastic/beats/pull/31279"}, prIDs)
 }
 
 func TestFillEmptyPRFieldBadHash(t *testing.T) {
@@ -61,9 +62,15 @@ func TestFindIssues(t *testing.T) {
 
 	graphqlClient := github.NewGraphQLClient(hc)
 
-	issues, err := changelog.FindIssues(graphqlClient, context.Background(), "elastic", "beats", 32501, 50)
+	issues, err := changelog.FindIssues(graphqlClient, context.Background(), "elastic", "beats", "https://github.com/elastic/beats/pull/32501", 50)
 	require.NoError(t, err)
 	require.NotEmpty(t, issues)
 	require.Len(t, issues, 1)
-	require.ElementsMatch(t, issues, []int{32483})
+	require.ElementsMatch(t, issues, []string{"https://github.com/elastic/beats/issues/32483"})
+}
+
+func TestExtractEventNumber(t *testing.T) {
+	id, err := changelog.ExtractEventNumber("pr", "https://github.com/elastic/elastic-agent-changelog-tool/pull/99")
+	require.NoError(t, err)
+	require.Equal(t, id, "99")
 }

internal/changelog/entry.go

@@ -4,21 +4,22 @@
 
 package changelog
 
-import "github.com/elastic/elastic-agent-changelog-tool/internal/changelog/fragment"
+import (
+	"github.com/elastic/elastic-agent-changelog-tool/internal/changelog/fragment"
+)
 
 type FragmentFileInfo struct {
 	Name     string `yaml:"name"`
 	Checksum string `yaml:"checksum"`
 }
 
 type Entry struct {
-	Kind        Kind   `yaml:"kind"`
-	Summary     string `yaml:"summary"`
-	Description string `yaml:"description"`
-	Component   string `yaml:"component" `
-	LinkedPR    []int  `yaml:"pr"`
-	LinkedIssue []int  `yaml:"issue"`
-	Repository  string `yaml:"repository"`
+	Kind        Kind     `yaml:"kind"`
+	Summary     string   `yaml:"summary"`
+	Description string   `yaml:"description"`
+	Component   string   `yaml:"component"`
+	LinkedPR    []string `yaml:"pr"`
+	LinkedIssue []string `yaml:"issue"`
 
 	Timestamp int64            `yaml:"timestamp"`
 	File      FragmentFileInfo `yaml:"file"`
@@ -32,22 +33,21 @@ func EntryFromFragment(f fragment.File) Entry {
 		Summary:     f.Fragment.Summary,
 		Description: f.Fragment.Description,
 		Component:   f.Fragment.Component,
-		LinkedPR:    []int{},
-		LinkedIssue: []int{},
-		Repository:  f.Fragment.Repository,
+		LinkedPR:    []string{},
+		LinkedIssue: []string{},
 		Timestamp:   f.Timestamp,
 		File: FragmentFileInfo{
 			Name:     f.Name,
 			Checksum: f.Checksum(),
 		},
 	}
 
-	if f.Fragment.Pr > 0 {
-		e.LinkedPR = []int{f.Fragment.Pr}
+	if f.Fragment.Pr != "" {
+		e.LinkedPR = []string{f.Fragment.Pr}
 	}
 
-	if f.Fragment.Issue > 0 {
-		e.LinkedIssue = []int{f.Fragment.Issue}
+	if f.Fragment.Issue != "" {
+		e.LinkedIssue = []string{f.Fragment.Issue}
 	}
 
 	return e

internal/changelog/fragment/creator_internal_test.go

@@ -103,10 +103,6 @@ component:
 # Issue number; optional; the GitHub issue related to this changeset (either closes or is part of).
 # If not present is automatically filled by the tooling with the issue linked to the PR number.
 #issue: 1234
-
-# Repository URL; optional; the repository URL related to this changeset and pr and issue numbers.
-# If not present is automatically filled by the tooling based on the repository this file has been committed in.
-#repository: https://github.com/elastic/elastic-agent-changelog-tool
 `
 	got := string(content)
 	assert.Equal(t, expected, got)

internal/changelog/fragment/fragment.go

@@ -9,7 +9,6 @@ type Fragment struct {
 	Summary     string `yaml:"summary"`
 	Description string `yaml:"description"`
 	Component   string `yaml:"component"`
-	Pr          int    `yaml:"pr"`
-	Issue       int    `yaml:"issue"`
-	Repository  string `yaml:"repository"`
+	Pr          string `yaml:"pr"`
+	Issue       string `yaml:"issue"`
 }

internal/changelog/fragment/template.yaml

@@ -29,7 +29,3 @@ component:
 # Issue number; optional; the GitHub issue related to this changeset (either closes or is part of).
 # If not present is automatically filled by the tooling with the issue linked to the PR number.
 #issue: 1234
-
-# Repository URL; optional; the repository URL related to this changeset and pr and issue numbers.
-# If not present is automatically filled by the tooling based on the repository this file has been committed in.
-#repository: https://github.com/elastic/elastic-agent-changelog-tool

internal/changelog/renderer.go

@@ -74,7 +74,7 @@ func (r Renderer) Render() error {
 	tmpl, err := template.New("asciidoc-release-notes").
 		Funcs(template.FuncMap{
 			// nolint:staticcheck // ignoring for now, supports for multiple component is not implemented
-			"linkPRSource": func(component string, ids []int) string {
+			"linkPRSource": func(component string, ids []string) string {
 				component = "agent" // TODO: remove this when implementing support for multiple components
 
 				res := make([]string, len(ids))
@@ -86,7 +86,7 @@ func (r Renderer) Render() error {
 				return strings.Join(res, " ")
 			},
 			// nolint:staticcheck // ignoring for now, supports for multiple component is not implemented
-			"linkIssueSource": func(component string, ids []int) string {
+			"linkIssueSource": func(component string, ids []string) string {
 				component = "agent" // TODO: remove this when implementing support for multiple components
 				res := make([]string, len(ids))
 

internal/github/client.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"strconv"
 
 	gh "github.com/google/go-github/v32/github"
 	"github.com/shurcooL/githubv4"
@@ -45,7 +46,7 @@ type graphqlService struct {
 	client *githubv4.Client
 }
 
-func (s *graphqlService) FindIssues(ctx context.Context, owner, repo string, prID, issuesLen int) ([]int, error) {
+func (s *graphqlService) FindIssues(ctx context.Context, owner, repo string, prID, issuesLen int) ([]string, error) {
 	variables := map[string]interface{}{
 		"issuesLen": githubv4.Int(issuesLen),
 		"prID":      githubv4.Int(prID),
@@ -72,10 +73,10 @@ func (s *graphqlService) FindIssues(ctx context.Context, owner, repo string, prI
 		return nil, fmt.Errorf("graphql mutate: %w", err)
 	}
 
-	issues := make([]int, len(q.Repository.PullRequest.ClosingIssuesReferences.Edges))
+	issues := make([]string, len(q.Repository.PullRequest.ClosingIssuesReferences.Edges))
 
 	for i, e := range q.Repository.PullRequest.ClosingIssuesReferences.Edges {
-		issues[i] = int(e.Node.Number)
+		issues[i] = strconv.FormatInt(int64(e.Node.Number), 10)
 	}
 
 	return issues, nil

internal/github/interfaces.go

@@ -31,5 +31,5 @@ type githubUsersService interface {
 }
 
 type githubGraphQLPRService interface {
-	FindIssues(ctx context.Context, owner, repo string, prID, issuesLen int) ([]int, error)
+	FindIssues(ctx context.Context, owner, repo string, prID, issuesLen int) ([]string, error)
 }