docs: Pass in Jira tickets in the body of product change issues #97789
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nickvigilante)
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 688 at r1 (raw file):
} err := queryGraphQL(query, queryVariables, token, &search) if err != nil {
Does it mean we swallow the error here? What happens in this case?
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 677 at r2 (raw file):
// getJiraIssueFromGitHubIssue is specified as a function closure to allow for testing // of getJiraIssueFromGitHubIssue* methods. var getJiraIssueFromGitHubIssue = func(org, repo string, issue int, token string) string {
Is there any particular reason to change from the function declaration style to the var one? For a top level function it looks unusual to me.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 703 at r2 (raw file):
jiraIssue = jiraIssuePartRE.FindString(exalateRef) } jiraGHIssueCache[key] = jiraIssue
I see the cache is populated, but it's not used anywhere. Maybe I'm missing something?
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 707 at r2 (raw file):
} func Split(r rune) bool {
maybe rename the function to something like splitBySlashOrHash so it's not exported (lowercase).
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 688 at r1 (raw file):
Previously, rail (Rail Aliiev) wrote…
Does it mean we swallow the error here? What happens in this case?
I made it here so that we don't swallow the error in the event of a network outage. I didn't want the lack of issue or epic reference to block the creation of a docs issue ticket.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 677 at r2 (raw file):
Previously, rail (Rail Aliiev) wrote…
Is there any particular reason to change from the function declaration style to the var one? For a top level function it looks unusual to me.
Certain test cases that once did not rely on network resources now rely on network resources. I had a conversation with Celia and David H about this, and the best way that I could make my tests not rely on network resources was to use https://github.com/cockroachdb/cockroach/tree/master/pkg/testutils, and David suggested I change that function to be a variable declaration that I could then defer as part of this test: https://github.com/nickvigilante/cockroach/blob/doc-7019/pkg/cmd/docs-issue-generation/docs_issue_generation_test.go#L227
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 703 at r2 (raw file):
Previously, rail (Rail Aliiev) wrote…
I see the cache is populated, but it's not used anywhere. Maybe I'm missing something?
Deleted the artifact.
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 707 at r2 (raw file):
Previously, rail (Rail Aliiev) wrote…
maybe rename the function to something like
splitBySlashOrHashso it's not exported (lowercase).
Done!
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 677 at r2 (raw file):
Previously, nickvigilante (Nick Vigilante) wrote…
Certain test cases that once did not rely on network resources now rely on network resources. I had a conversation with Celia and David H about this, and the best way that I could make my tests not rely on network resources was to use https://github.com/cockroachdb/cockroach/tree/master/pkg/testutils, and David suggested I change that function to be a variable declaration that I could then defer as part of this test: https://github.com/nickvigilante/cockroach/blob/doc-7019/pkg/cmd/docs-issue-generation/docs_issue_generation_test.go#L227
You also need to update the test to reflect the function signature change. Otherwise LGTM.
panic: reflect.Set: value of type func(string, string, int, string) string is not assignable to type func(string, string, int, string) (string, error)
Code quote:
getJiraIssueFromGitHubIssue
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/docs-issue-generation/docs_issue_generation.go line 677 at r2 (raw file):
Previously, rail (Rail Aliiev) wrote…
You also need to update the test to reflect the function signature change. Otherwise LGTM.
panic: reflect.Set: value of type func(string, string, int, string) string is not assignable to type func(string, string, int, string) (string, error)
Done, I think! One more review?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:
|
bors r+ |
|
Build failed: |
|
bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
bors r+ |
|
Already running a review |
|
Build succeeded: |
Epic and issue references are now pointing to the associated Jira
tickets.
Fixes DOC-7019
Release note: None