Update pr/issue field in fragment/changelog to store the URL #95

lucian-ioan · 2022-09-12T11:20:25Z

Closes #89

remove current fragments/changelog
add fragment
remove repository field
fix script for Windows

elasticmachine · 2022-09-12T11:23:09Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-09-20T14:22:29.996+0000
Duration: 3 min 47 sec

Test stats 🧪

Test	Results
Failed	0
Passed	71
Skipped	0
Total	71

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

endorama · 2022-09-12T14:17:52Z

Please also remove the repository field from Fragment/Entry (and the tools script)

endorama · 2022-09-19T10:57:24Z

internal/changelog/builder.go

@@ -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 := ExtactEventNumber("issue", entry.LinkedIssue[0])


nit: typo in function name. I tried to change all occurrences, but please double check!

Suggested change

_, err := ExtactEventNumber("issue", entry.LinkedIssue[0])

_, err := ExtractEventNumber("issue", entry.LinkedIssue[0])

endorama · 2022-09-19T10:57:31Z

internal/changelog/builder.go

@@ -161,6 +168,39 @@ func collectFragment(fs afero.Fs, path string, info os.FileInfo, err error, file
 	return nil
 }

+func ExtactEventNumber(linkType, eventURL string) (string, error) {


Suggested change

func ExtactEventNumber(linkType, eventURL string) (string, error) {

func ExtractEventNumber(linkType, eventURL string) (string, error) {

endorama · 2022-09-19T10:58:41Z

internal/changelog/builder.go

-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 := ExtactEventNumber("pr", prURL)


Suggested change

prID, err := ExtactEventNumber("pr", prURL)

prID, err := ExtractEventNumber("pr", prURL)

endorama · 2022-09-19T11:02:38Z

internal/changelog/builder.go

@@ -161,6 +168,39 @@ func collectFragment(fs afero.Fs, path string, info os.FileInfo, err error, file
 	return nil
 }

+func ExtactEventNumber(linkType, eventURL string) (string, error) {
+	urlParts := strings.Split(eventURL, "/")


Would be better to use URL parsing here? https://gobyexample.com/url-parsing https://pkg.go.dev/net/url#URL

URL parsing unfortunately doesn't help much as I still have to split the link. I've added it as an additional validation for the link.

I don't think it's necessary to add it as an additional step, I was wondering if it could be of help for instead of string matching, but as I see it is not.

Alright, will also remove it.

endorama · 2022-09-19T11:08:02Z

internal/changelog/builder.go

+	case "pr":
+		return fmt.Sprintf("https://github.com/%s/%s/pull/%s", owner, repo, eventID)
+	default:
+		return ""


Instead of returning an empty string here, let's panic. This is a developer error (passing the wrong linkType value), returning an empty string would shadow the error, making it hard to back track. By panicking we signals developers the error.

Suggested change

return ""

panic("wrong linkType")

endorama · 2022-09-19T11:08:54Z

internal/changelog/builder.go

-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 := ExtactEventNumber("pr", prURL)


Suggested change

linkedPR, err := ExtactEventNumber("pr", prURL)

linkedPR, err := ExtractEventNumber("pr", prURL)

endorama · 2022-09-19T11:09:46Z

internal/changelog/entry.go

@@ -42,12 +44,12 @@ func EntryFromFragment(f fragment.File) Entry {
 		},
 	}

-	if f.Fragment.Pr > 0 {
-		e.LinkedPR = []int{f.Fragment.Pr}
+	if len(f.Fragment.Pr) > 0 {


Suggested change

if len(f.Fragment.Pr) > 0 {

if f.Fragment.Pr != "" {

endorama · 2022-09-19T11:10:00Z

internal/changelog/entry.go

 	}

-	if f.Fragment.Issue > 0 {
-		e.LinkedIssue = []int{f.Fragment.Issue}
+	if len(f.Fragment.Issue) > 0 {


Suggested change

if len(f.Fragment.Issue) > 0 {

if f.Fragment.Issue != "" {

Will do, any particular reason for not using len? The logic appears identical in this case.

No change in behaviour but checking string length is not idiomatic in Go.

endorama · 2022-09-20T13:04:58Z

tools/asciidoc_to_fragments.py

@@ -64,6 +64,19 @@ def get_event_timestamp(repository, event, number):
        date = datetime.fromisoformat(data["closed_at"].replace('Z', '+00:00'))
        return str(int(datetime.timestamp(date)))

+
+def sanitize_title(title):


I'm doing the same in a separate PR #104 so I would not include this here

Please use it there if useful, will remove it now.

endorama · 2022-09-20T13:09:48Z

@LucianPy also don't remove fragments here, I forgot to build the changelog for 0.2.1 and 0.2.2, I'll do it afterwards

Update pr/issue fields to full url

f5a8e2f

endorama added this to the v0.3.0 milestone Sep 12, 2022

endorama requested changes Sep 19, 2022

View reviewed changes

endorama removed this from the v0.3.0 milestone Sep 19, 2022

Add pr-has-fragment failure guidance (elastic#99)

3371f57

endorama mentioned this pull request Sep 20, 2022

Update changelog tool conversion script to support string pr and issue fields #100

Closed

LucianPy added 3 commits September 20, 2022 13:56

Merge remote-tracking branch 'upstream/main' into pr-issue-field-to-url

7ce529a

Remove repository field and fix typos

8146a3c

Add fragment and remove current fragments/changelogs

5e63d12

lucian-ioan requested a review from endorama September 20, 2022 12:42

Replace bad chars in fragment title from script

b65ad1f

lucian-ioan force-pushed the pr-issue-field-to-url branch from acde685 to b65ad1f Compare September 20, 2022 12:46

endorama reviewed Sep 20, 2022

View reviewed changes

endorama force-pushed the main branch from 1805762 to 44ede0b Compare September 20, 2022 13:18

LucianPy added 2 commits September 20, 2022 16:52

Revert changes and remove url parsing

d052565

Update python script

0ac7494

endorama approved these changes Sep 20, 2022

View reviewed changes

lucian-ioan merged commit df9f0f9 into elastic:main Sep 20, 2022

lucian-ioan deleted the pr-issue-field-to-url branch October 26, 2022 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update pr/issue field in fragment/changelog to store the URL #95

Update pr/issue field in fragment/changelog to store the URL #95

lucian-ioan commented Sep 12, 2022 •

edited

Loading

elasticmachine commented Sep 12, 2022 •

edited

Loading

Build stats

Test stats 🧪

endorama commented Sep 12, 2022 •

edited

Loading

endorama Sep 19, 2022

endorama Sep 19, 2022

endorama Sep 19, 2022

endorama Sep 19, 2022

lucian-ioan Sep 20, 2022

endorama Sep 20, 2022

lucian-ioan Sep 20, 2022

endorama Sep 19, 2022

endorama Sep 19, 2022

endorama Sep 19, 2022

endorama Sep 19, 2022

lucian-ioan Sep 20, 2022

endorama Sep 20, 2022

endorama Sep 20, 2022

lucian-ioan Sep 20, 2022

endorama commented Sep 20, 2022

	_, err := ExtactEventNumber("issue", entry.LinkedIssue[0])
	_, err := ExtractEventNumber("issue", entry.LinkedIssue[0])

	func ExtactEventNumber(linkType, eventURL string) (string, error) {
	func ExtractEventNumber(linkType, eventURL string) (string, error) {

	prID, err := ExtactEventNumber("pr", prURL)
	prID, err := ExtractEventNumber("pr", prURL)

	linkedPR, err := ExtactEventNumber("pr", prURL)
	linkedPR, err := ExtractEventNumber("pr", prURL)

Update pr/issue field in fragment/changelog to store the URL #95

Update pr/issue field in fragment/changelog to store the URL #95

Conversation

lucian-ioan commented Sep 12, 2022 • edited Loading

elasticmachine commented Sep 12, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

endorama commented Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endorama commented Sep 20, 2022

lucian-ioan commented Sep 12, 2022 •

edited

Loading

elasticmachine commented Sep 12, 2022 •

edited

Loading

endorama commented Sep 12, 2022 •

edited

Loading