From c114992a8ae57a3b031ec2dd30c4f6ed9c819f68 Mon Sep 17 00:00:00 2001 From: Bibin Kurian George Date: Thu, 11 Jun 2020 19:54:12 -0400 Subject: [PATCH 1/4] Add GetAllClones and GetRoot functions These functions have been added as part of the bugzilla backporter tool. The GetAllClones function would return all the clones of a particular bug(including its parent, grandparent etc.), unlike the GetClones function which would return only the clones which were cloned from that particular bug. The GetRoot function would return the original bug which produced the clone even if that bug is a clone of a clone. --- prow/bugzilla/client.go | 130 ++++++++++++++++++++++++++ prow/bugzilla/client_test.go | 176 +++++++++++++++++++++++++++++++++-- prow/bugzilla/fake.go | 16 ++++ prow/bugzilla/types.go | 2 + 4 files changed, 316 insertions(+), 8 deletions(-) diff --git a/prow/bugzilla/client.go b/prow/bugzilla/client.go index b71eec85a73f..e88d4124488b 100644 --- a/prow/bugzilla/client.go +++ b/prow/bugzilla/client.go @@ -37,16 +37,47 @@ const ( type Client interface { Endpoint() string + // GetBug retrieves a Bug from the server GetBug(id int) (*Bug, error) + // GetComments gets a list of comments for a specific bug ID. + // https://bugzilla.readthedocs.io/en/latest/api/core/v1/comment.html#get-comments GetComments(id int) ([]Comment, error) + // GetExternalBugPRsOnBug retrieves external bugs on a Bug from the server + // and returns any that reference a Pull Request in GitHub + // https://bugzilla.readthedocs.io/en/latest/api/core/v1/bug.html#get-bug GetExternalBugPRsOnBug(id int) ([]ExternalBug, error) + // GetSubComponentsOnBug retrieves a the list of SubComponents of the bug. + // SubComponents are a Red Hat bugzilla specific extra field. GetSubComponentsOnBug(id int) (map[string][]string, error) + // GetClones gets the list of bugs that the provided bug blocks that also have a matching summary. GetClones(bug *Bug) ([]*Bug, error) + // CreateBug creates a new bug on the server. + // https://bugzilla.readthedocs.io/en/latest/api/core/v1/bug.html#create-bug CreateBug(bug *BugCreate) (int, error) + // CloneBug clones a bug by creating a new bug with the same fields, copying the description, and updating the bug to depend on the original bug CloneBug(bug *Bug) (int, error) + // UpdateBug updates the fields of a bug on the server + // https://bugzilla.readthedocs.io/en/latest/api/core/v1/bug.html#update-bug UpdateBug(id int, update BugUpdate) error + // AddPullRequestAsExternalBug attempts to add a PR to the external tracker list. + // External bugs are assumed to fall under the type identified by their hostname, + // so we will provide https://github.com/ here for the URL identifier. We return + // any error as well as whether a change was actually made. + // This will be done via JSONRPC: + // https://bugzilla.redhat.com/docs/en/html/integrating/api/Bugzilla/Extension/ExternalBugs/WebService.html#add-external-bug AddPullRequestAsExternalBug(id int, org, repo string, num int) (bool, error) + // RemovePullRequestAsExternalBug attempts to remove a PR from the external tracker list. + // External bugs are assumed to fall under the type identified by their hostname, + // so we will provide https://github.com/ here for the URL identifier. We return + // any error as well as whether a change was actually made. + // This will be done via JSONRPC: + // https://bugzilla.redhat.com/docs/en/html/integrating/api/Bugzilla/Extension/ExternalBugs/WebService.html#remove-external-bug RemovePullRequestAsExternalBug(id int, org, repo string, num int) (bool, error) + // GetAllClones returns all the clones of the bug including itself + // Differs from GetClones as GetClones only gets the child clones which are one level lower + GetAllClones(bug *Bug) ([]*Bug, error) + // GetRootForClone returns the original bug. + GetRootForClone(bug *Bug) (*Bug, error) } func NewClient(getAPIKey func() []byte, endpoint string) Client { @@ -117,6 +148,105 @@ func (c *client) GetClones(bug *Bug) ([]*Bug, error) { return getClones(c, bug) } +// Gets children clones recursively using a mechanism similar to bfs +func getRecursiveClones(c Client, root *Bug) ([]*Bug, error) { + var errs []error + var bug *Bug + clones := []*Bug{} + childrenQ := []*Bug{} + childrenQ = append(childrenQ, root) + // FYI Cannot think of any situation for circular clones + // But might need to revisit in case there are infinite loops at any point + for len(childrenQ) > 0 { + bug, childrenQ = childrenQ[0], childrenQ[1:] + clones = append(clones, bug) + children, err := getClones(c, bug) + if err != nil { + errs = append(errs, fmt.Errorf("Error finding clones Bug#%d: %v", bug.ID, err)) + } + if len(children) > 0 { + childrenQ = append(childrenQ, children...) + } + } + return clones, utilerrors.NewAggregate(errs) +} + +// getImmediateParents gets the Immediate parents of bugs with a matching summary +func getImmediateParents(c Client, bug *Bug) ([]*Bug, error) { + var errs []error + parents := []*Bug{} + // One option would be to return as soon as the first parent is found + // ideally that should be enough, although there is a check in the getRootForClone function to verify this + // Logs would need to be monitored to verify this behavior + for _, parentID := range bug.DependsOn { + parent, err := c.GetBug(parentID) + if err != nil { + errs = append(errs, fmt.Errorf("Failed to get parent bug #%d: %v", parentID, err)) + continue + } + if parent.Summary == bug.Summary { + parents = append(parents, parent) + } + } + return parents, utilerrors.NewAggregate(errs) +} + +func getRootForClone(c Client, bug *Bug) (*Bug, error) { + curr := bug + var errs []error + for len(curr.DependsOn) > 0 { + parent, err := getImmediateParents(c, curr) + if err != nil { + errs = append(errs, err) + } + switch l := len(parent); { + case l <= 0: + break + case l == 1: + curr = parent[0] + case l > 1: + curr = parent[0] + errs = append(errs, fmt.Errorf("More than one parent found for bug #%d", curr.ID)) + } + } + return curr, utilerrors.NewAggregate(errs) +} + +// GetRootForClone returns the original bug. +func (c *client) GetRootForClone(bug *Bug) (*Bug, error) { + return getRootForClone(c, bug) +} + +// GetAllClones returns all the clones of the bug including itself +// Differs from GetClones as GetClones only gets the child clones which are one level lower +func (c *client) GetAllClones(bug *Bug) ([]*Bug, error) { + return getAllClones(c, bug) +} +func getAllClones(c Client, bug *Bug) ([]*Bug, error) { + root, err := getRootForClone(c, bug) + if err != nil { + return nil, err + } + clones, err := getRecursiveClones(c, root) + if err != nil { + return nil, err + } + // Getting rid of the bug whose clones we are searching for + // we could optimize this (maybe?) if the list turns out to be too long + indexOfOriginal := -1 + for index, clone := range clones { + if clone.ID == bug.ID { + // Check if there is a more elegant way to do this + indexOfOriginal = index + } + } + if indexOfOriginal == -1 { + return nil, fmt.Errorf("Original bug not found in list of clones. Error in logic for getRecursiveClones/getRootForClone") + } + clones = append(clones[:indexOfOriginal], clones[indexOfOriginal+1:]...) + return clones, nil +} + // GetSubComponentsOnBug retrieves a the list of SubComponents of the bug. // SubComponents are a Red Hat bugzilla specific extra field. func (c *client) GetSubComponentsOnBug(id int) (map[string][]string, error) { diff --git a/prow/bugzilla/client_test.go b/prow/bugzilla/client_test.go index a6334b0877f8..eaebc0d13ae8 100644 --- a/prow/bugzilla/client_test.go +++ b/prow/bugzilla/client_test.go @@ -897,20 +897,21 @@ func TestGetExternalBugPRsOnBug(t *testing.T) { http.Error(w, "400 Bad Request", http.StatusBadRequest) return } - if id, err := strconv.Atoi(strings.TrimPrefix(r.URL.Path, "/rest/bug/")); err != nil { + id, err := strconv.Atoi(strings.TrimPrefix(r.URL.Path, "/rest/bug/")) + if err != nil { t.Errorf("malformed bug id: %s", r.URL.Path) http.Error(w, "400 Bad Request", http.StatusBadRequest) return - } else { - for _, testCase := range testCases { - if id == testCase.id { - if _, err := w.Write([]byte(testCase.response)); err != nil { - t.Fatalf("%s: failed to send response: %v", testCase.name, err) - } - return + } + for _, testCase := range testCases { + if id == testCase.id { + if _, err := w.Write([]byte(testCase.response)); err != nil { + t.Fatalf("%s: failed to send response: %v", testCase.name, err) } + return } } + })) defer testServer.Close() client := clientForUrl(testServer.URL) @@ -930,3 +931,162 @@ func TestGetExternalBugPRsOnBug(t *testing.T) { }) } } + +func TestGetAllClones(t *testing.T) { + exists := struct{}{} + testcases := map[int]struct { + bugDetails string + prDetails string + expectedClones map[int]struct{} + }{ + 1843407: { + `{"bugs" : [{ "alias" : [], "blocks" : [1844101 ], "cf_clone_of" : null, "cf_doc_type" : "If docs needed, set a value", "cf_environment" : "", "cf_fixed_in" : "", "cf_last_closed" : null, "cf_release_notes" : "", "cf_target_upstream_version" : "", "classification" : "Red Hat", "component" : ["Unknown" ], "creation_time" : "2020-06-03T09:03:41Z","deadline" : null, "depends_on" : [], "docs_contact" : "", "dupe_of" : null, "groups" : [], "id" : 1843407, "is_cc_accessible" : true, "is_confirmed" : true, "is_creator_accessible" : true, "is_open" : true, "keywords" : [], "last_change_time" : "2020-06-07T11:08:27Z", "op_sys" : "Unspecified", "platform" : "Unspecified", "priority" : "unspecified", "product" : "OpenShift Container Platform", "resolution" : "", "see_also" : [], "severity" : "medium", "status" : "VERIFIED", "summary" : "[oVirt] add oVirt as a provide to openshift tests", "target_milestone" : "---", "target_release" : ["4.6.0" ], "url" : "", "version" : ["4.5" ], "whiteboard" : ""}],"faults" : []}`, + `{"bugs" : [{ "external_bugs" : [{"bug_id" : 1843407,"ext_bz_bug_id" : "openshift/origin/pull/25057","ext_bz_id" : 131,"ext_description" : "Bug 1843407: oVirt, add oVirt as a provide to openshift tests","ext_priority" : "None","ext_status" : "closed","id" : 1185327,"type" : {"can_get" : true,"can_send" : false,"description" : "Github","full_url" : "https://github.com/%id%","id" : 131,"must_send" : false,"send_once" : false,"type" : "GitHub","url" : "https://github.com/"}} ]}],"faults" : []}`, + map[int]struct{}{1844101: exists, 1844102: exists}, + }, + 1844101: { + `{"bugs" : [{ "alias" : [], "blocks" : [1844102 ], "cf_clone_of" : null, "cf_doc_type" : "If docs needed, set a value", "cf_environment" : "", "cf_fixed_in" : "", "cf_last_closed" : null, "cf_release_notes" : "", "cf_target_upstream_version" : "", "classification" : "Red Hat", "component" : ["Unknown" ], "creation_time" : "2020-06-04T15:43:10Z", "deadline" : null, "depends_on" : [1843407 ], "docs_contact" : "", "dupe_of" : null, "groups" : [], "id" : 1844101, "is_cc_accessible" : true, "is_confirmed" : true, "is_creator_accessible" : true, "is_open" : true, "keywords" : ["CodeChange" ], "last_change_time" : "2020-06-11T05:39:35Z", "op_sys" : "Unspecified", "platform" : "Unspecified", "priority" : "unspecified", "product" : "OpenShift Container Platform", "resolution" : "", "see_also" : [], "severity" : "medium", "status" : "VERIFIED", "summary" : "[oVirt] add oVirt as a provide to openshift tests", "target_milestone" : "---", "target_release" : ["4.5.0" ], "url" : "", "version" : ["4.5" ], "whiteboard" : ""}],"faults" : []}`, + `{"bugs" : [{ "external_bugs" : [{"bug_id" : 1843407,"ext_bz_bug_id" : "openshift/origin/pull/25057","ext_bz_id" : 131,"ext_description" : "Bug 1843407: oVirt, add oVirt as a provide to openshift tests","ext_priority" : "None","ext_status" : "closed","id" : 1185327,"type" : {"can_get" : true,"can_send" : false,"description" : "Github","full_url" : "https://github.com/%id%","id" : 131,"must_send" : false,"send_once" : false,"type{"bugs" : [{ "external_bugs" : [{"bug_id" : 1844101,"ext_bz_bug_id" : "openshift/origin/pull/25065","ext_bz_id" : 131,"ext_description" : "[release-4.5] Bug 1844101: oVirt, add oVirt as a provide to openshift tests","ext_priority" : "None","ext_status" : "open","id" : 1186600,"type" : {"can_get" : true,"can_send" : false,"description" : "Github","full_url" : "https://github.com/%id%","id" : 131,"must_send" : false,"send_once" : false,"type" : "GitHub","url" : "https://github.com/"}} ]}],"faults" : []}`, + map[int]struct{}{1843407: exists, 1844102: exists}, + }, + 1844102: { + `{"bugs" : [ {"alias" : [],"blocks" : [],"cf_clone_of" : null,"cf_doc_type" : "If docs needed, set a value","cf_environment" : "","cf_fixed_in" : "","cf_last_closed" : null,"cf_release_notes" : "","cf_target_upstream_version" : "","classification" : "Red Hat","component" : ["Cloud Compute"],"creation_time" : "2020-06-04T15:43:52Z","deadline" : null,"depends_on" : [1844101],"docs_contact" : "","dupe_of" : null,"groups" : [],"id" : 1844102,"is_cc_accessible" : true,"is_confirmed" : true,"is_creator_accessible" : true,"is_open" : true,"keywords" : [],"last_change_time" : "2020-06-11T05:39:58Z","op_sys" : "Unspecified","platform" : "Unspecified","priority" : "unspecified","product" : "OpenShift Container Platform","resolution" : "","see_also" : [],"severity" : "medium","status" : "POST","summary" : "[oVirt] add oVirt as a provide to openshift tests","target_milestone" : "---","target_release" : ["4.4.z"],"url" : "","version" : ["4.5"],"whiteboard" : "" }],"faults" : []}`, + `{"bugs" : [{ "external_bugs" : [{"bug_id" : 1844102,"ext_bz_bug_id" : "openshift/origin/pull/25066","ext_bz_id" : 131,"ext_description" : "[release-4.4] Bug 1844102: oVirt, add oVirt as a provide to openshift tests","ext_priority" : "None","ext_status" : "open","id" : 1190587,"type" : {"can_get" : true,"can_send" : false,"description" : "Github","full_url" : "https://github.com/%id%","id" : 131,"must_send" : false,"send_once" : false,"type" : "GitHub","url" : "https://github.com/"}} ]}],"faults" : []}`, + map[int]struct{}{1843407: exists, 1844101: exists}, + }, + } + + testServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("X-BUGZILLA-API-KEY") != "api-key" { + t.Error("did not get api-key passed in X-BUGZILLA-API-KEY header") + http.Error(w, "403 Forbidden", http.StatusForbidden) + return + } + if r.URL.Query().Get("api_key") != "api-key" { + t.Error("did not get api-key passed in api_key query parameter") + http.Error(w, "403 Forbidden", http.StatusForbidden) + return + } + if r.Method != http.MethodGet { + t.Errorf("incorrect method to get a bug: %s", r.Method) + http.Error(w, "400 Bad Request", http.StatusBadRequest) + return + } + if !strings.HasPrefix(r.URL.Path, "/rest/bug/") { + t.Errorf("incorrect path to get a bug: %s", r.URL.Path) + http.Error(w, "400 Bad Request", http.StatusBadRequest) + return + } + if strings.HasPrefix(r.URL.Path, "/rest/bug/") { + id, err := strconv.Atoi(strings.TrimPrefix(r.URL.Path, "/rest/bug/")) + if err != nil { + t.Errorf("malformed bug id: %s", r.URL.Path) + http.Error(w, "400 Bad Request", http.StatusBadRequest) + return + } + if r.URL.Query().Get("include_fields") == "external_bugs" { + if val, ok := testcases[id]; ok { + w.Write([]byte(val.prDetails)) + } else { + http.Error(w, "404 Not Found", http.StatusNotFound) + } + } else { + if val, ok := testcases[id]; ok { + w.Write([]byte(val.bugDetails)) + } else { + http.Error(w, "404 Not Found", http.StatusNotFound) + } + } + + } + + })) + defer testServer.Close() + client := clientForUrl(testServer.URL) + for tc, tp := range testcases { + t.Run(strconv.Itoa(tc), func(t *testing.T) { + var parsedResponse struct { + Bugs []*Bug `json:"bugs,omitempty"` + } + if err := json.Unmarshal([]byte(tp.bugDetails), &parsedResponse); err != nil { + t.Fatal(err) + } + clones, err := client.GetAllClones(parsedResponse.Bugs[0]) + if err != nil { + t.Errorf("Error occurred when none was expected: %v", err) + } + if len(tp.expectedClones) != len(clones) { + t.Errorf("Mismatch in number of clones - expected: %d, got %d", len(tp.expectedClones), len(clones)) + } + for _, clone := range clones { + if _, ok := tp.expectedClones[clone.ID]; !ok { + t.Errorf("Unexpected clone found in list - expecting: %v, got: %d", tp.expectedClones, clone.ID) + } + } + }) + + } + +} + +func TestGetRootForClone(t *testing.T) { + bugMappings := map[int]string{ + 1843407: `{"bugs" : [{ "alias" : [], "blocks" : [1844101 ], "cc" : [], "cc_detail" : [ ], "cf_clone_of" : null, "cf_doc_type" : "If docs needed, set a value", "cf_environment" : "", "cf_fixed_in" : "", "cf_last_closed" : null, "cf_release_notes" : "", "cf_target_upstream_version" : "", "classification" : "Red Hat", "component" : ["Unknown" ], "creation_time" : "2020-06-03T09:03:41Z", "deadline" : null, "depends_on" : [], "docs_contact" : "", "dupe_of" : null, "groups" : [], "id" : 1843407, "is_cc_accessible" : true, "is_confirmed" : true, "is_creator_accessible" : true, "is_open" : true, "keywords" : [], "last_change_time" : "2020-06-07T11:08:27Z", "op_sys" : "Unspecified", "platform" : "Unspecified", "priority" : "unspecified", "product" : "OpenShift Container Platform", "resolution" : "", "see_also" : [], "severity" : "medium", "status" : "VERIFIED", "summary" : "[oVirt] add oVirt as a provide to openshift tests", "target_milestone" : "---", "target_release" : ["4.6.0" ], "url" : "", "version" : ["4.5" ], "whiteboard" : ""}],"faults" : []}`, + 1844101: `{"bugs" : [{ "alias" : [], "blocks" : [1844102 ], "cc" : [ ], "cc_detail" : [], "cf_clone_of" : null, "cf_doc_type" : "If docs needed, set a value", "cf_environment" : "", "cf_fixed_in" : "", "cf_last_closed" : null, "cf_release_notes" : "", "cf_target_upstream_version" : "", "classification" : "Red Hat", "component" : ["Unknown" ], "creation_time" : "2020-06-04T15:43:10Z", "deadline" : null, "depends_on" : [1843407 ], "docs_contact" : "", "dupe_of" : null, "groups" : [], "id" : 1844101, "is_cc_accessible" : true, "is_confirmed" : true, "is_creator_accessible" : true, "is_open" : true, "keywords" : ["CodeChange" ], "last_change_time" : "2020-06-11T05:39:35Z", "op_sys" : "Unspecified", "platform" : "Unspecified", "priority" : "unspecified", "product" : "OpenShift Container Platform", "resolution" : "", "see_also" : [], "severity" : "medium", "status" : "VERIFIED", "summary" : "[oVirt] add oVirt as a provide to openshift tests", "target_milestone" : "---", "target_release" : ["4.5.0" ], "url" : "", "version" : ["4.5" ], "whiteboard" : ""}],"faults" : []}`, + 1844102: `{"bugs" : [ {"alias" : [],"blocks" : [],"cc" : [],"cc_detail" : [],"cf_clone_of" : null,"cf_doc_type" : "If docs needed, set a value","cf_environment" : "","cf_fixed_in" : "","cf_last_closed" : null,"cf_release_notes" : "","cf_target_upstream_version" : "","classification" : "Red Hat","component" : ["Cloud Compute"],"creation_time" : "2020-06-04T15:43:52Z","deadline" : null,"depends_on" : [1844101],"docs_contact" : "","dupe_of" : null,"groups" : [],"id" : 1844102,"is_cc_accessible" : true,"is_confirmed" : true,"is_creator_accessible" : true,"is_open" : true,"keywords" : [],"last_change_time" : "2020-06-11T05:39:58Z","op_sys" : "Unspecified","platform" : "Unspecified","priority" : "unspecified","product" : "OpenShift Container Platform","resolution" : "","see_also" : [],"severity" : "medium","status" : "POST","summary" : "[oVirt] add oVirt as a provide to openshift tests","target_milestone" : "---","target_release" : ["4.4.z"],"url" : "","version" : ["4.5"],"whiteboard" : "" }],"faults" : []}`, + } + testServer := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("X-BUGZILLA-API-KEY") != "api-key" { + t.Error("did not get api-key passed in X-BUGZILLA-API-KEY header") + http.Error(w, "403 Forbidden", http.StatusForbidden) + return + } + if r.URL.Query().Get("api_key") != "api-key" { + t.Error("did not get api-key passed in api_key query parameter") + http.Error(w, "403 Forbidden", http.StatusForbidden) + return + } + if r.Method != http.MethodGet { + t.Errorf("incorrect method to get a bug: %s", r.Method) + http.Error(w, "400 Bad Request", http.StatusBadRequest) + return + } + if !strings.HasPrefix(r.URL.Path, "/rest/bug/") { + t.Errorf("incorrect path to get a bug: %s", r.URL.Path) + http.Error(w, "400 Bad Request", http.StatusBadRequest) + return + } + id, err := strconv.Atoi(strings.TrimPrefix(r.URL.Path, "/rest/bug/")) + if err != nil { + t.Errorf("malformed bug id: %s", r.URL.Path) + http.Error(w, "400 Bad Request", http.StatusBadRequest) + return + } + if val, ok := bugMappings[id]; ok { + w.Write([]byte(val)) + } else { + http.Error(w, "404 Not Found", http.StatusNotFound) + } + + })) + defer testServer.Close() + client := clientForUrl(testServer.URL) + for tc, tp := range bugMappings { + t.Run(strconv.Itoa(tc), func(t *testing.T) { + var parsedResponse struct { + Bugs []*Bug `json:"bugs,omitempty"` + } + if err := json.Unmarshal([]byte(tp), &parsedResponse); err != nil { + t.Fatal(err) + } + // this should run get the root + root, err := client.GetRootForClone(parsedResponse.Bugs[0]) + if err != nil { + t.Errorf("Error occurred when error not expected: %v", err) + } + if root.ID != 1843407 { + t.Errorf("ID of root incorrect.") + } + }) + } +} diff --git a/prow/bugzilla/fake.go b/prow/bugzilla/fake.go index b9989286e005..5acf17386823 100644 --- a/prow/bugzilla/fake.go +++ b/prow/bugzilla/fake.go @@ -236,5 +236,21 @@ func (c *Fake) GetClones(bug *Bug) ([]*Bug, error) { return getClones(c, bug) } +// GetAllClones gets all clones including its parents and children spanning multiple levels +func (c *Fake) GetAllClones(bug *Bug) ([]*Bug, error) { + if c.BugErrors.Has(bug.ID) { + return nil, errors.New("injected error getting subcomponents") + } + return getAllClones(c, bug) +} + +// GetRoot gets the original bug. +func (c *Fake) GetRootForClone(bug *Bug) (*Bug, error) { + if c.BugErrors.Has(bug.ID) { + return nil, errors.New("injected error getting bug") + } + return getRootForClone(c, bug) +} + // the Fake is a Client var _ Client = &Fake{} diff --git a/prow/bugzilla/types.go b/prow/bugzilla/types.go index 5a6f9b7e13c7..cd3241b09439 100644 --- a/prow/bugzilla/types.go +++ b/prow/bugzilla/types.go @@ -107,6 +107,8 @@ type Bug struct { Version []string `json:"version,omitempty"` // Whiteboard is he value of the "status whiteboard" field on the bug. Whiteboard string `json:"whiteboard,omitempty"` + // PRs holds the links to the pull requests associated with the bug. + PRs []ExternalBug `json:"external_bug,omitempty"` } // BugCreate holds the info needed to create a new bug From 72274580102441904a238fa25024e73d1a501762 Mon Sep 17 00:00:00 2001 From: geobk Date: Wed, 9 Sep 2020 12:13:17 -0400 Subject: [PATCH 2/4] Add login and name options for git commits --- experiment/autobumper/bumper/bumper.go | 79 ++++++++++++++++++++++++-- prow/flagutil/git.go | 19 ++++--- 2 files changed, 87 insertions(+), 11 deletions(-) diff --git a/experiment/autobumper/bumper/bumper.go b/experiment/autobumper/bumper/bumper.go index 413e98368435..bc72108971d3 100644 --- a/experiment/autobumper/bumper/bumper.go +++ b/experiment/autobumper/bumper/bumper.go @@ -107,6 +107,31 @@ type Options struct { SkipPullRequest bool } +// GitCommand is used to pass the various components of the git command which needs to be executed +type GitCommand struct { + baseCommand string + args []string + workingDir string +} + +// Call will execute the Git command and switch the working directory if specified +func (gc GitCommand) Call(stdout, stderr io.Writer) error { + return Call(stdout, stderr, gc.baseCommand, gc.buildCommand()...) +} + +func (gc GitCommand) buildCommand() []string { + args := []string{} + if gc.workingDir != "" { + args = append(args, "-C", gc.workingDir) + } + args = append(args, gc.args...) + return args +} + +func (gc GitCommand) getCommand() string { + return fmt.Sprintf("%s %s", gc.baseCommand, strings.Join(gc.buildCommand(), " ")) +} + func validateOptions(o *Options) error { if !o.SkipPullRequest && o.GitHubToken == "" { return fmt.Errorf("--github-token is mandatory when --skip-pull-request is false") @@ -507,7 +532,7 @@ func GitCommitAndPush(remote, remoteBranch, name, email, message string, stdout, // Avoid doing metadata-only pushes that re-trigger tests and remove lgtm if localTreeRef != remoteTreeRef { - if err := GitPush(forkRemoteName, remoteBranch, stdout, stderr); err != nil { + if err := GitPush(forkRemoteName, remoteBranch, stdout, stderr, ""); err != nil { return err } } else { @@ -518,14 +543,60 @@ func GitCommitAndPush(remote, remoteBranch, name, email, message string, stdout, } // GitPush push the changes to the given remote and branch. -func GitPush(remote, remoteBranch string, stdout, stderr io.Writer) error { +func GitPush(remote, remoteBranch string, stdout, stderr io.Writer, workingDir string) error { logrus.Info("Pushing to remote...") - if err := Call(stdout, stderr, "git", "push", "-f", remote, fmt.Sprintf("HEAD:%s", remoteBranch)); err != nil { - return fmt.Errorf("failed to git push: %w", err) + gitCmd := "git" + gc := GitCommand{ + baseCommand: gitCmd, + args: []string{"push", "-f", remote, fmt.Sprintf("HEAD:%s", remoteBranch)}, + workingDir: workingDir, + } + if err := gc.Call(stdout, stderr); err != nil { + return fmt.Errorf("failed to %s: %w", gc.getCommand(), err) } return nil } +// RunAndCommitIfNeeded makes a commit in the workingDir if there are +// any changes resulting from the command execution. Returns true if a commit is made +func RunAndCommitIfNeeded(stdout, stderr io.Writer, author, cmd string, args []string, workingDir string) (bool, error) { + fullCommand := fmt.Sprintf("%s %s", cmd, strings.Join(args, " ")) + + logrus.Infof("Running: %s", fullCommand) + if err := Call(stdout, stderr, cmd, args...); err != nil { + return false, fmt.Errorf("failed to run %s: %w", fullCommand, err) + } + + changed, err := HasChanges() + if err != nil { + return false, fmt.Errorf("error occurred when checking changes: %w", err) + } + + if !changed { + logrus.WithField("command", fullCommand).Info("No changes to commit") + return false, nil + } + gitCmd := "git" + gc := GitCommand{ + baseCommand: gitCmd, + args: []string{"add", "."}, + workingDir: workingDir, + } + if err := gc.Call(stdout, stderr); err != nil { + return false, fmt.Errorf("failed to %s: %w", gc.getCommand(), err) + } + gc = GitCommand{ + baseCommand: gitCmd, + args: []string{"commit", "-m", fullCommand, "--author", author}, + workingDir: workingDir, + } + if err := gc.Call(stdout, stderr); err != nil { + return false, fmt.Errorf("failed to %s: %w", gc.getCommand(), err) + } + + return true, nil +} + func tagFromName(name string) string { parts := strings.Split(name, ":") if len(parts) < 2 { diff --git a/prow/flagutil/git.go b/prow/flagutil/git.go index a7d956c9f1e8..6572f8324dbc 100644 --- a/prow/flagutil/git.go +++ b/prow/flagutil/git.go @@ -19,6 +19,7 @@ package flagutil import ( "errors" "flag" + "fmt" "k8s.io/test-infra/prow/git/v2" "k8s.io/test-infra/prow/github" @@ -28,8 +29,8 @@ import ( // GitOptions holds options for interacting with git. type GitOptions struct { host string - user string - email string + User string + Email string tokenPath string useSSH bool useGitHubUser bool @@ -38,8 +39,8 @@ type GitOptions struct { // AddFlags injects Git options into the given FlagSet. func (o *GitOptions) AddFlags(fs *flag.FlagSet) { fs.StringVar(&o.host, "git-host", "github.com", "host to contact for git operations.") - fs.StringVar(&o.user, "git-user", "", "User for git commits, optional. Can be derived from GitHub credentials.") - fs.StringVar(&o.email, "git-email", "", "Email for git commits, optional. Can be derived from GitHub credentials.") + fs.StringVar(&o.User, "git-user", "", "User for git commits, optional. Can be derived from GitHub credentials.") + fs.StringVar(&o.Email, "git-email", "", "Email for git commits, optional. Can be derived from GitHub credentials.") fs.StringVar(&o.tokenPath, "git-token-path", "", "Path to the file containing the git token for HTTPS operations, optional. Can be derived from GitHub credentials.") fs.BoolVar(&o.useSSH, "git-over-ssh", false, "Use SSH when pushing and pulling instead of HTTPS. SSH credentials should be present at ~/.ssh") fs.BoolVar(&o.useGitHubUser, "git-user-from-github", true, "Use GitHub credentials and user identity for git operations.") @@ -52,10 +53,13 @@ func (o *GitOptions) Validate(dryRun bool) error { } if !o.useGitHubUser { + if (o.Email == "") != (o.User == "") { + return fmt.Errorf("--git-user and --git-email must be specified together") + } switch { - case o.user == "": + case o.User == "": return errors.New("--git-user is required, or may be loaded by setting --git-user-from-github") - case o.email == "": + case o.Email == "": return errors.New("--git-email is required, or may be loaded by setting --git-user-from-github") } } @@ -63,13 +67,14 @@ func (o *GitOptions) Validate(dryRun bool) error { if !o.useSSH && !o.useGitHubUser && o.tokenPath == "" { return errors.New("--git-token-path must be provided or defaulted by setting --git-user-from-github or --git-over-ssh") } + return nil } // GitClient creates a new git client. func (o *GitOptions) GitClient(userClient github.UserClient, token func() []byte, censor func(content []byte) []byte, dryRun bool) (git.ClientFactory, error) { gitUser := func() (name, email string, err error) { - name, email = o.user, o.email + name, email = o.User, o.Email if o.useGitHubUser { user, err := userClient.BotUser() if err != nil { From 00472c8a46dcb6a5391c1c7b4b414df21a1d07f1 Mon Sep 17 00:00:00 2001 From: geobk Date: Fri, 11 Sep 2020 12:45:16 -0400 Subject: [PATCH 3/4] Move git author options to bumper.go --- experiment/autobumper/bumper/bumper.go | 21 +++++++++++++++++++++ prow/flagutil/git.go | 19 +++++++------------ 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/experiment/autobumper/bumper/bumper.go b/experiment/autobumper/bumper/bumper.go index bc72108971d3..70bc7613e9b9 100644 --- a/experiment/autobumper/bumper/bumper.go +++ b/experiment/autobumper/bumper/bumper.go @@ -20,6 +20,7 @@ import ( "bytes" "encoding/json" "errors" + "flag" "fmt" "io" "io/ioutil" @@ -107,6 +108,26 @@ type Options struct { SkipPullRequest bool } +// GitAuthorOptions is specifically to read the author info for a commit +type GitAuthorOptions struct { + GitName string + GitEmail string +} + +// AddFlags will read the author info from the command line parameters +func (o *GitAuthorOptions) AddFlags(fs *flag.FlagSet) { + fs.StringVar(&o.GitName, "git-name", "", "The name to use on the git commit.") + fs.StringVar(&o.GitEmail, "git-email", "", "The email to use on the git commit.") +} + +// Validate will validate the input GitAuthorOptions +func (o *GitAuthorOptions) Validate() error { + if (o.GitEmail == "") != (o.GitName == "") { + return fmt.Errorf("--git-name and --git-email must be specified together") + } + return nil +} + // GitCommand is used to pass the various components of the git command which needs to be executed type GitCommand struct { baseCommand string diff --git a/prow/flagutil/git.go b/prow/flagutil/git.go index 6572f8324dbc..a7d956c9f1e8 100644 --- a/prow/flagutil/git.go +++ b/prow/flagutil/git.go @@ -19,7 +19,6 @@ package flagutil import ( "errors" "flag" - "fmt" "k8s.io/test-infra/prow/git/v2" "k8s.io/test-infra/prow/github" @@ -29,8 +28,8 @@ import ( // GitOptions holds options for interacting with git. type GitOptions struct { host string - User string - Email string + user string + email string tokenPath string useSSH bool useGitHubUser bool @@ -39,8 +38,8 @@ type GitOptions struct { // AddFlags injects Git options into the given FlagSet. func (o *GitOptions) AddFlags(fs *flag.FlagSet) { fs.StringVar(&o.host, "git-host", "github.com", "host to contact for git operations.") - fs.StringVar(&o.User, "git-user", "", "User for git commits, optional. Can be derived from GitHub credentials.") - fs.StringVar(&o.Email, "git-email", "", "Email for git commits, optional. Can be derived from GitHub credentials.") + fs.StringVar(&o.user, "git-user", "", "User for git commits, optional. Can be derived from GitHub credentials.") + fs.StringVar(&o.email, "git-email", "", "Email for git commits, optional. Can be derived from GitHub credentials.") fs.StringVar(&o.tokenPath, "git-token-path", "", "Path to the file containing the git token for HTTPS operations, optional. Can be derived from GitHub credentials.") fs.BoolVar(&o.useSSH, "git-over-ssh", false, "Use SSH when pushing and pulling instead of HTTPS. SSH credentials should be present at ~/.ssh") fs.BoolVar(&o.useGitHubUser, "git-user-from-github", true, "Use GitHub credentials and user identity for git operations.") @@ -53,13 +52,10 @@ func (o *GitOptions) Validate(dryRun bool) error { } if !o.useGitHubUser { - if (o.Email == "") != (o.User == "") { - return fmt.Errorf("--git-user and --git-email must be specified together") - } switch { - case o.User == "": + case o.user == "": return errors.New("--git-user is required, or may be loaded by setting --git-user-from-github") - case o.Email == "": + case o.email == "": return errors.New("--git-email is required, or may be loaded by setting --git-user-from-github") } } @@ -67,14 +63,13 @@ func (o *GitOptions) Validate(dryRun bool) error { if !o.useSSH && !o.useGitHubUser && o.tokenPath == "" { return errors.New("--git-token-path must be provided or defaulted by setting --git-user-from-github or --git-over-ssh") } - return nil } // GitClient creates a new git client. func (o *GitOptions) GitClient(userClient github.UserClient, token func() []byte, censor func(content []byte) []byte, dryRun bool) (git.ClientFactory, error) { gitUser := func() (name, email string, err error) { - name, email = o.User, o.Email + name, email = o.user, o.email if o.useGitHubUser { user, err := userClient.BotUser() if err != nil { From 083b47fccb12a956984db317193ccddb3251ce33 Mon Sep 17 00:00:00 2001 From: geobk Date: Fri, 11 Sep 2020 13:20:36 -0400 Subject: [PATCH 4/4] Use constant instead of hardcoding "git" --- experiment/autobumper/bumper/bumper.go | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/experiment/autobumper/bumper/bumper.go b/experiment/autobumper/bumper/bumper.go index 70bc7613e9b9..c72c891db5d9 100644 --- a/experiment/autobumper/bumper/bumper.go +++ b/experiment/autobumper/bumper/bumper.go @@ -58,6 +58,8 @@ const ( errOncallMsgTempl = "An error occurred while finding an assignee: `%s`.\nFalling back to Blunderbuss." noOncallMsg = "Nobody is currently oncall, so falling back to Blunderbuss." + + gitCmd = "git" ) var ( @@ -264,7 +266,7 @@ func cdToRootDir() error { } return nil } - cmd := exec.Command("git", "rev-parse", "--show-toplevel") + cmd := exec.Command(gitCmd, "rev-parse", "--show-toplevel") output, err := cmd.Output() if err != nil { return fmt.Errorf("failed to get the repo's root directory: %w", err) @@ -482,13 +484,12 @@ func getNewProwVersion(images map[string]string) (string, error) { // HasChanges checks if the current git repo contains any changes func HasChanges() (bool, error) { - cmd := "git" args := []string{"status", "--porcelain"} - logrus.WithField("cmd", cmd).WithField("args", args).Info("running command ...") - combinedOutput, err := exec.Command(cmd, args...).CombinedOutput() + logrus.WithField("cmd", gitCmd).WithField("args", args).Info("running command ...") + combinedOutput, err := exec.Command(gitCmd, args...).CombinedOutput() if err != nil { - logrus.WithField("cmd", cmd).Debugf("output is '%s'", string(combinedOutput)) - return false, fmt.Errorf("error running command %s %s: %w", cmd, args, err) + logrus.WithField("cmd", gitCmd).Debugf("output is '%s'", string(combinedOutput)) + return false, fmt.Errorf("error running command %s %s: %w", gitCmd, args, err) } return len(strings.TrimSuffix(string(combinedOutput), "\n")) > 0, nil } @@ -519,22 +520,22 @@ func MakeGitCommit(remote, remoteBranch, name, email string, images map[string]s func GitCommitAndPush(remote, remoteBranch, name, email, message string, stdout, stderr io.Writer) error { logrus.Info("Making git commit...") - if err := Call(stdout, stderr, "git", "add", "-A"); err != nil { + if err := Call(stdout, stderr, gitCmd, "add", "-A"); err != nil { return fmt.Errorf("failed to git add: %w", err) } commitArgs := []string{"commit", "-m", message} if name != "" && email != "" { commitArgs = append(commitArgs, "--author", fmt.Sprintf("%s <%s>", name, email)) } - if err := Call(stdout, stderr, "git", commitArgs...); err != nil { + if err := Call(stdout, stderr, gitCmd, commitArgs...); err != nil { return fmt.Errorf("failed to git commit: %w", err) } - if err := Call(stdout, stderr, "git", "remote", "add", forkRemoteName, remote); err != nil { + if err := Call(stdout, stderr, gitCmd, "remote", "add", forkRemoteName, remote); err != nil { return fmt.Errorf("failed to add remote: %w", err) } fetchStderr := &bytes.Buffer{} var remoteTreeRef string - if err := Call(stdout, fetchStderr, "git", "fetch", forkRemoteName, remoteBranch); err != nil { + if err := Call(stdout, fetchStderr, gitCmd, "fetch", forkRemoteName, remoteBranch); err != nil { if !strings.Contains(fetchStderr.String(), fmt.Sprintf("couldn't find remote ref %s", remoteBranch)) { return fmt.Errorf("failed to fetch from fork: %w", err) } @@ -566,7 +567,6 @@ func GitCommitAndPush(remote, remoteBranch, name, email, message string, stdout, // GitPush push the changes to the given remote and branch. func GitPush(remote, remoteBranch string, stdout, stderr io.Writer, workingDir string) error { logrus.Info("Pushing to remote...") - gitCmd := "git" gc := GitCommand{ baseCommand: gitCmd, args: []string{"push", "-f", remote, fmt.Sprintf("HEAD:%s", remoteBranch)}, @@ -597,7 +597,6 @@ func RunAndCommitIfNeeded(stdout, stderr io.Writer, author, cmd string, args []s logrus.WithField("command", fullCommand).Info("No changes to commit") return false, nil } - gitCmd := "git" gc := GitCommand{ baseCommand: gitCmd, args: []string{"add", "."}, @@ -733,7 +732,7 @@ func getAssignment(oncallAddress string) string { func getTreeRef(stderr io.Writer, refname string) (string, error) { revParseStdout := &bytes.Buffer{} - if err := Call(revParseStdout, stderr, "git", "rev-parse", refname+":"); err != nil { + if err := Call(revParseStdout, stderr, gitCmd, "rev-parse", refname+":"); err != nil { return "", fmt.Errorf("failed to parse ref: %w", err) } fields := strings.Fields(revParseStdout.String())