From 45343115e206e2278729e5f837eb3d678b7cb1f8 Mon Sep 17 00:00:00 2001 From: Alex Pavel Date: Mon, 13 Jun 2022 16:19:56 -0700 Subject: [PATCH] jira: general client improvements This PR fixes the fakejira `CloneIssue` and `CreateIssue` functions along with adding unit tests for them, adds more extensive issue function errors for the fakeclient, and modifies the `DeleteRemoteLinkViaURL` function to return a `bool` indicating whether a link with the provided URL was found and removed (no remote link with the provided URL is not an error). These improvements will be used in a new openshift external prow plugin called the `jira-lifecycle` plugin, which will replicate the `bugzilla` plugins functionality, but with the Jira service instead of bugzilla. --- go.mod | 2 +- prow/jira/fakejira/fake.go | 170 ++++++++++++++------------ prow/jira/fakejira/fake_test.go | 208 +++++++++++++++++++++++++++++++- prow/jira/jira.go | 24 ++-- prow/plugins/jira/jira_test.go | 4 +- 5 files changed, 318 insertions(+), 90 deletions(-) diff --git a/go.mod b/go.mod index d5bc1ac655ce..283c23e6aeaf 100644 --- a/go.mod +++ b/go.mod @@ -83,6 +83,7 @@ require ( github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.7.0 github.com/tektoncd/pipeline v0.14.1-0.20200710073957-5eeb17f81999 + github.com/trivago/tgo v1.0.7 go.uber.org/zap v1.19.0 go4.org v0.0.0-20201209231011-d4a079459e60 gocloud.dev v0.19.0 @@ -190,7 +191,6 @@ require ( github.com/prometheus/procfs v0.6.0 // indirect github.com/sergi/go-diff v1.1.0 // indirect github.com/shurcooL/graphql v0.0.0-20181231061246-d48a9a75455f // indirect - github.com/trivago/tgo v1.0.7 // indirect github.com/xanzy/ssh-agent v0.3.0 // indirect go.opencensus.io v0.23.0 // indirect go.uber.org/atomic v1.7.0 // indirect diff --git a/prow/jira/fakejira/fake.go b/prow/jira/fakejira/fake.go index 2ed503ff2c24..d74ad26c6780 100644 --- a/prow/jira/fakejira/fake.go +++ b/prow/jira/fakejira/fake.go @@ -19,7 +19,6 @@ package fakejira import ( "context" "encoding/json" - "errors" "fmt" "strconv" "strings" @@ -31,14 +30,17 @@ import ( ) type FakeClient struct { - Issues []*jira.Issue - ExistingLinks map[string][]jira.RemoteLink - NewLinks []jira.RemoteLink - IssueLinks []*jira.IssueLink - GetIssueError error - Transitions []jira.Transition - Users []*jira.User - SearchResponses map[SearchRequest]SearchResponse + Issues []*jira.Issue + ExistingLinks map[string][]jira.RemoteLink + NewLinks []jira.RemoteLink + RemovedLinks []jira.RemoteLink + IssueLinks []*jira.IssueLink + GetIssueError map[string]error + CreateIssueError map[string]error + UpdateIssueError map[string]error + Transitions []jira.Transition + Users []*jira.User + SearchResponses map[SearchRequest]SearchResponse } func (f *FakeClient) ListProjects() (*jira.ProjectList, error) { @@ -47,7 +49,9 @@ func (f *FakeClient) ListProjects() (*jira.ProjectList, error) { func (f *FakeClient) GetIssue(id string) (*jira.Issue, error) { if f.GetIssueError != nil { - return nil, f.GetIssueError + if err, ok := f.GetIssueError[id]; ok { + return nil, err + } } for _, existingIssue := range f.Issues { if existingIssue.ID == id || existingIssue.Key == id { @@ -58,7 +62,11 @@ func (f *FakeClient) GetIssue(id string) (*jira.Issue, error) { } func (f *FakeClient) GetRemoteLinks(id string) ([]jira.RemoteLink, error) { - return f.ExistingLinks[id], nil + issue, err := f.GetIssue(id) + if err != nil { + return nil, fmt.Errorf("Failed to get issue when chekcing from remote links: %+v", err) + } + return append(f.ExistingLinks[issue.ID], f.ExistingLinks[issue.Key]...), nil } func (f *FakeClient) AddRemoteLink(id string, link *jira.RemoteLink) (*jira.RemoteLink, error) { @@ -107,6 +115,13 @@ func (f *FakeClient) AddComment(issueID string, comment *jira.Comment) (*jira.Co if err != nil { return nil, fmt.Errorf("Issue %s not found: %v", issueID, err) } + // make sure the fields exist + if issue.Fields == nil { + issue.Fields = &jira.IssueFields{} + } + if issue.Fields.Comments == nil { + issue.Fields.Comments = &jira.Comments{} + } issue.Fields.Comments.Comments = append(issue.Fields.Comments.Comments, comment) return comment, nil } @@ -116,44 +131,73 @@ func (f *FakeClient) CreateIssueLink(link *jira.IssueLink) error { if err != nil { return fmt.Errorf("failed to get outward link issue: %v", err) } - outward.Fields.IssueLinks = append(outward.Fields.IssueLinks, link) + // links in an issue struct do not include the short definition of the the issue they are part of. + // This behavior is used by the external jira-lifecycle hook plugin to identify which direction a clone happened, + // so it needs to be replicated in the fake client + linkForOutward := *link + linkForOutward.OutwardIssue = nil + outward.Fields.IssueLinks = append(outward.Fields.IssueLinks, &linkForOutward) inward, err := f.GetIssue(link.InwardIssue.ID) if err != nil { return fmt.Errorf("failed to get inward link issue: %v", err) } - inward.Fields.IssueLinks = append(inward.Fields.IssueLinks, link) + linkForInward := *link + linkForInward.InwardIssue = nil + inward.Fields.IssueLinks = append(inward.Fields.IssueLinks, &linkForInward) f.IssueLinks = append(f.IssueLinks, link) return nil } func (f *FakeClient) CloneIssue(issue *jira.Issue) (*jira.Issue, error) { - // create deep copy of parent so we can modify key and id for child - data, err := json.Marshal(issue) - if err != nil { - return nil, err - } - issueCopy := &jira.Issue{} - err = json.Unmarshal(data, issueCopy) - if err != nil { - return nil, err - } - // set ID and Key to unused id and key - f.updateIssueIDAndKey(issueCopy) - // run generic cloning function - return jiraclient.CloneIssue(f, issueCopy) + return jiraclient.CloneIssue(f, issue) } func (f *FakeClient) CreateIssue(issue *jira.Issue) (*jira.Issue, error) { - // check that there are no ID collisions - for _, currIssue := range f.Issues { - if currIssue.ID == issue.ID { - return nil, fmt.Errorf("Issue ID %s already exists", issue.ID) + if f.CreateIssueError != nil { + if err, ok := f.CreateIssueError[issue.Key]; ok { + return nil, err } - if currIssue.Key == issue.Key { - return nil, fmt.Errorf("Issue key %s already exists", issue.Key) + } + if issue.Fields == nil || issue.Fields.Project.Name == "" { + return nil, fmt.Errorf("issue.fields.project must be set to create new issue") + } + issueCreationErrors := jiraclient.CreateIssueError{} + issueCreationErrors.Errors = make(map[string]string) + // simulate unsettable fields + if issue.Fields.Comments != nil { + issueCreationErrors.Errors["comment"] = "this field cannot be set" + } + if issue.Fields.Status != nil { + issueCreationErrors.Errors["status"] = "this field cannot be set" + } + if len(issueCreationErrors.Errors) != 0 { + data, err := json.Marshal(issueCreationErrors) + if err != nil { + return nil, err + } + return nil, &jiraclient.JiraError{StatusCode: 400, Body: string(data)} + } + // find highest issueID and make new issue one higher + highestID := 0 + // find highest ID for issues in the same project to make new key one higher + highestKeyID := 0 + keyPrefix := issue.Fields.Project.Name + "-" + for _, issue := range f.Issues { + // all IDs are ints, but represented as strings... + intID, _ := strconv.Atoi(issue.ID) + if intID > highestID { + highestID = intID + } + if strings.HasPrefix(issue.Key, keyPrefix) { + stringID := strings.TrimPrefix(issue.Key, keyPrefix) + intID, _ := strconv.Atoi(stringID) + if intID > highestKeyID { + highestKeyID = intID + } } } - f.updateIssueIDAndKey(issue) + issue.ID = strconv.Itoa(highestID + 1) + issue.Key = fmt.Sprintf("%s%d", keyPrefix, highestKeyID+1) f.Issues = append(f.Issues, issue) return issue, nil } @@ -211,53 +255,22 @@ func (f *FakeClient) DeleteLink(id string) error { func (f *FakeClient) DeleteRemoteLink(issueID string, linkID int) error { for index, remoteLink := range f.ExistingLinks[issueID] { if remoteLink.ID == linkID { - f.ExistingLinks[issueID] = append(f.ExistingLinks[issueID][:index], f.ExistingLinks[issueID][index+1:]...) + f.RemovedLinks = append(f.RemovedLinks, remoteLink) + if len(f.ExistingLinks[issueID]) == index+1 { + f.ExistingLinks[issueID] = f.ExistingLinks[issueID][:index] + } else { + f.ExistingLinks[issueID] = append(f.ExistingLinks[issueID][:index], f.ExistingLinks[issueID][index+1:]...) + } + return nil } } return fmt.Errorf("failed to find link id %d in issue %s", linkID, issueID) } -func (f *FakeClient) DeleteRemoteLinkViaURL(issueID, url string) error { +func (f *FakeClient) DeleteRemoteLinkViaURL(issueID, url string) (bool, error) { return jiraclient.DeleteRemoteLinkViaURL(f, issueID, url) } -func (f *FakeClient) updateIssueIDAndKey(newIssue *jira.Issue) error { - // ensure that a key is set - if newIssue.Key == "" { - return errors.New("Issue key must be set") - } - // ensure key format is correct - splitKey := strings.Split(newIssue.Key, "-") - if len(splitKey) != 2 { - return fmt.Errorf("Invalid issue key: %s", newIssue.Key) - } - - // find highest issueID and make new issue one higher - highestID := -1 - for _, issue := range f.Issues { - // all IDs are ints, but represented as strings... - intID, _ := strconv.Atoi(issue.ID) - if intID > highestID { - highestID = intID - } - } - newIssue.ID = strconv.Itoa(highestID + 1) - // if there are issues in the same project, make new issue one above those - highestKeyID := 0 - keyPrefix := fmt.Sprintf("%s-", splitKey[0]) - for _, issue := range f.Issues { - if strings.HasPrefix(issue.Key, keyPrefix) { - stringID := strings.TrimPrefix(issue.Key, keyPrefix) - intID, _ := strconv.Atoi(stringID) - if intID > highestKeyID { - highestKeyID = intID - } - } - } - newIssue.Key = fmt.Sprintf("%s%d", keyPrefix, highestKeyID) - return nil -} - func (f *FakeClient) GetTransitions(issueID string) ([]jira.Transition, error) { return f.Transitions, nil } @@ -311,6 +324,11 @@ func (f *FakeClient) GetIssueTargetVersion(issue *jira.Issue) (*[]*jira.Version, } func (f *FakeClient) UpdateIssue(issue *jira.Issue) (*jira.Issue, error) { + if f.UpdateIssueError != nil { + if err, ok := f.UpdateIssueError[issue.ID]; ok { + return nil, err + } + } retrievedIssue, err := f.GetIssue(issue.ID) if err != nil { return nil, fmt.Errorf("unable to find issue to update: %v", err) @@ -339,11 +357,11 @@ func (f *FakeClient) UpdateIssue(issue *jira.Issue) (*jira.Issue, error) { if err != nil { return nil, fmt.Errorf("error converting updated issue to json: %v", err) } - var newFields *jira.IssueFields - if err := json.Unmarshal(updatedIssueBytes, newFields); err != nil { + var newFields jira.IssueFields + if err := json.Unmarshal(updatedIssueBytes, &newFields); err != nil { return nil, fmt.Errorf("failed converting updated issue to struct: %v", err) } - retrievedIssue.Fields = newFields + retrievedIssue.Fields = &newFields return retrievedIssue, nil } diff --git a/prow/jira/fakejira/fake_test.go b/prow/jira/fakejira/fake_test.go index c2addd0552e5..3640659ad32f 100644 --- a/prow/jira/fakejira/fake_test.go +++ b/prow/jira/fakejira/fake_test.go @@ -18,12 +18,18 @@ package fakejira import ( "context" + "reflect" + "testing" + "github.com/andygrunwald/go-jira" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - "testing" + "github.com/trivago/tgo/tcontainer" + jiraclient "k8s.io/test-infra/prow/jira" ) +var allowJiraDate = cmp.AllowUnexported(jira.Date{}) + func TestFakeClient_SearchWithContext(t *testing.T) { s := make(map[SearchRequest]SearchResponse) issueList := []jira.Issue{ @@ -72,3 +78,203 @@ func TestFakeClient_SearchWithContext(t *testing.T) { t.Fatal("expected invalid query to fail, but got no error") } } + +func TestCreateIssue(t *testing.T) { + testCases := []struct { + name string + issue jira.Issue + existingIssues []jira.Issue + expectedIssue *jira.Issue + createIssueError *jiraclient.CreateIssueError + }{{ + name: "create single issue", + issue: jira.Issue{ + Fields: &jira.IssueFields{ + Project: jira.Project{ + Name: "ABC", + }, + }, + }, + expectedIssue: &jira.Issue{ + ID: "1", + Key: "ABC-1", + Fields: &jira.IssueFields{ + Project: jira.Project{ + Name: "ABC", + }, + }, + }, + }, { + name: "create issue with other issues in other projects", + issue: jira.Issue{ + Fields: &jira.IssueFields{ + Project: jira.Project{ + Name: "ABC", + }, + }, + }, + existingIssues: []jira.Issue{ + { + ID: "22", + Key: "ABC-41", + Fields: &jira.IssueFields{ + Project: jira.Project{ + Name: "ABC", + }, + }, + }, + { + ID: "52", + Key: "DEF-16", + Fields: &jira.IssueFields{ + Project: jira.Project{ + Name: "DEF", + }, + }, + }, + }, + expectedIssue: &jira.Issue{ + ID: "53", + Key: "ABC-42", + Fields: &jira.IssueFields{ + Project: jira.Project{ + Name: "ABC", + }, + }, + }, + }, { + name: "create issue with comments and Status set", + issue: jira.Issue{ + Fields: &jira.IssueFields{ + Project: jira.Project{ + Name: "ABC", + }, + Status: &jira.Status{ + Name: "NEW", + }, + Comments: &jira.Comments{ + Comments: []*jira.Comment{{Body: "Hello"}}, + }, + }, + }, + createIssueError: &jiraclient.CreateIssueError{ + Errors: map[string]string{ + "comment": "this field cannot be set", + "status": "this field cannot be set", + }, + }, + }} + for _, tc := range testCases { + ptrIssues := []*jira.Issue{} + for index := range tc.existingIssues { + ptrIssues = append(ptrIssues, &tc.existingIssues[index]) + } + fakeClient := &FakeClient{Issues: ptrIssues} + newIssue, err := fakeClient.CreateIssue(&tc.issue) + if err != nil && tc.createIssueError == nil { + t.Fatalf("%s: received error where none was expected: %v", tc.name, err) + } + if err == nil && tc.createIssueError != nil { + t.Fatalf("%s: received no error where one was expected", tc.name) + } + if tc.expectedIssue != nil { + if !reflect.DeepEqual(newIssue, tc.expectedIssue) { + t.Errorf("%s: got incorrect issue after clone: %s", tc.name, cmp.Diff(newIssue, tc.expectedIssue, allowJiraDate)) + } + } + } +} + +func TestCloneIssue(t *testing.T) { + testCases := []struct { + name string + issue jira.Issue + existingIssues []jira.Issue + expectedIssue *jira.Issue + expectedIssueLink *jira.IssueLink + }{{ + name: "clone a basic issue with only project, description, and status set", + issue: jira.Issue{ + ID: "22", + Key: "ABC-41", + Fields: &jira.IssueFields{ + Description: "This is a test issue", + Status: &jira.Status{ + Name: "POST", + }, + Project: jira.Project{ + Name: "ABC", + }, + }, + }, + existingIssues: []jira.Issue{ + { + ID: "22", + Key: "ABC-41", + Fields: &jira.IssueFields{ + Description: "This is a test issue", + Status: &jira.Status{ + Name: "POST", + }, + Project: jira.Project{ + Name: "ABC", + }, + }, + }, + }, + expectedIssue: &jira.Issue{ + ID: "23", + Key: "ABC-42", + Fields: &jira.IssueFields{ + Description: "This is a clone of issue ABC-41. The following is the description of the original issue: \n---\nThis is a test issue", + Project: jira.Project{ + Name: "ABC", + }, + IssueLinks: []*jira.IssueLink{{ + Type: jira.IssueLinkType{ + Name: "Cloners", + Inward: "is cloned by", + Outward: "clones", + }, + OutwardIssue: &jira.Issue{ID: "22"}, + }}, + Unknowns: tcontainer.MarshalMap{}, + }, + }, + expectedIssueLink: &jira.IssueLink{ + Type: jira.IssueLinkType{ + Name: "Cloners", + Inward: "is cloned by", + Outward: "clones", + }, + OutwardIssue: &jira.Issue{ID: "22"}, + InwardIssue: &jira.Issue{ID: "23"}, + }, + }} + for _, tc := range testCases { + ptrIssues := []*jira.Issue{} + for index := range tc.existingIssues { + ptrIssues = append(ptrIssues, &tc.existingIssues[index]) + } + fakeClient := &FakeClient{Issues: ptrIssues} + newIssue, err := fakeClient.CloneIssue(&tc.issue) + if err != nil { + t.Fatalf("%s: received error where none was expected: %v", tc.name, err) + } + if tc.expectedIssue != nil { + if !reflect.DeepEqual(newIssue, tc.expectedIssue) { + t.Errorf("%s: got incorrect issue after clone: %s", tc.name, cmp.Diff(newIssue, tc.expectedIssue, allowJiraDate)) + } + } + if tc.expectedIssueLink != nil { + if len(fakeClient.IssueLinks) != 1 { + t.Fatalf("%s: expected 1 issue link, got %d", tc.name, len(fakeClient.IssueLinks)) + } + if !reflect.DeepEqual(newIssue, tc.expectedIssue) { + t.Errorf("%s: got incorrect issue link: %s", tc.name, cmp.Diff(fakeClient.IssueLinks[0], tc.expectedIssueLink, allowJiraDate)) + } + } else if len(fakeClient.IssueLinks) != 0 { + t.Fatalf("%s: got %d issue links when none were expected", tc.name, len(fakeClient.IssueLinks)) + } + } +} diff --git a/prow/jira/jira.go b/prow/jira/jira.go index aec36246d222..fd92aa70d1b1 100644 --- a/prow/jira/jira.go +++ b/prow/jira/jira.go @@ -83,8 +83,10 @@ type Client interface { DeleteLink(id string) error DeleteRemoteLink(issueID string, linkID int) error // DeleteRemoteLinkViaURL identifies and removes a remote link from an issue - // the has the provided URL. - DeleteRemoteLinkViaURL(issueID, url string) error + // the has the provided URL. The returned bool indicates whether a change + // was made during the operation as a remote link with the URL not existing + // is not consider an error for this function. + DeleteRemoteLinkViaURL(issueID, url string) (bool, error) ForPlugin(plugin string) Client AddComment(issueID string, comment *jira.Comment) (*jira.Comment, error) ListProjects() (*jira.ProjectList, error) @@ -328,21 +330,23 @@ func (jc *client) DeleteRemoteLink(issueID string, linkID int) error { } // DeleteRemoteLinkViaURL identifies and removes a remote link from an issue -// the has the provided URL. -func DeleteRemoteLinkViaURL(jc Client, issueID, url string) error { +// the has the provided URL. The returned bool indicates whether a change +// was made during the operation as a remote link with the URL not existing +// is not consider an error for this function. +func DeleteRemoteLinkViaURL(jc Client, issueID, url string) (bool, error) { links, err := jc.GetRemoteLinks(issueID) if err != nil { - return err + return false, err } for _, link := range links { if link.Object.URL == url { - return jc.DeleteRemoteLink(issueID, link.ID) + return true, jc.DeleteRemoteLink(issueID, link.ID) } } - return fmt.Errorf("could not find remote link on issue with URL `%s`", url) + return false, fmt.Errorf("could not find remote link on issue with URL `%s`", url) } -func (jc *client) DeleteRemoteLinkViaURL(issueID, url string) error { +func (jc *client) DeleteRemoteLinkViaURL(issueID, url string) (bool, error) { return DeleteRemoteLinkViaURL(jc, issueID, url) } @@ -514,7 +518,7 @@ func (jc *client) CloneIssue(parent *jira.Issue) (*jira.Issue, error) { func unsetProblematicFields(issue *jira.Issue, responseBody string) (*jira.Issue, error) { // handle unsettable "unknown" fields - processedResponse := createIssueError{} + processedResponse := CreateIssueError{} if newErr := json.Unmarshal([]byte(responseBody), &processedResponse); newErr != nil { return nil, fmt.Errorf("Error processing jira error: %w", newErr) } @@ -544,7 +548,7 @@ func unsetProblematicFields(issue *jira.Issue, responseBody string) (*jira.Issue return &newIssue, nil } -type createIssueError struct { +type CreateIssueError struct { ErrorMessages []string `json:"errorMessages"` Errors map[string]string `json:"errors"` } diff --git a/prow/plugins/jira/jira_test.go b/prow/plugins/jira/jira_test.go index e97e94068fb8..d00a4a58c243 100644 --- a/prow/plugins/jira/jira_test.go +++ b/prow/plugins/jira/jira_test.go @@ -128,7 +128,7 @@ func TestHandle(t *testing.T) { event github.GenericCommentEvent cfg *plugins.Jira projectCache *threadsafeSet - getIssueClientError error + getIssueClientError map[string]error existingIssues []jira.Issue existingLinks map[string][]jira.RemoteLink expectedNewLinks []jira.RemoteLink @@ -328,7 +328,7 @@ func TestHandle(t *testing.T) { Number: 3, }, projectCache: &threadsafeSet{}, - getIssueClientError: errors.New("error: didn't serve 404 from cache"), + getIssueClientError: map[string]error{"ABC-123": errors.New("error: didn't serve 404 from cache")}, }, }