fix: fetch build status from jobs instead of run status (#5495)

Author: Googlom

pkg/orchestrator/github_actions.go

@@ -31,14 +31,15 @@ type githubActionsConfigProvider struct {
 
 type run struct {
 	fetched   bool
-	Status    string    `json:"status"`
 	StartedAt time.Time `json:"run_started_at"`
 }
 
+// used to unmarshal list jobs of the current workflow run into []job
 type job struct {
-	ID      int64  `json:"id"`
-	Name    string `json:"name"`
-	HtmlURL string `json:"html_url"`
+	ID         int64  `json:"id"`
+	Name       string `json:"name"`
+	HtmlURL    string `json:"html_url"`
+	Conclusion string `json:"conclusion"`
 }
 
 type fullLog struct {
@@ -75,19 +76,26 @@ func (g *githubActionsConfigProvider) OrchestratorType() string {
 	return "GitHubActions"
 }
 
-// BuildStatus returns current run status
+// BuildStatus returns current run status by looking at all jobs of the current workflow run
+// if any job has conclusion "failure" the whole run is considered failed
+// if any job has conclusion "cancelled" the whole run is considered aborted
+// otherwise the run is considered successful
 func (g *githubActionsConfigProvider) BuildStatus() string {
-	g.fetchRunData()
-	switch g.runData.Status {
-	case "success":
-		return BuildStatusSuccess
-	case "cancelled":
-		return BuildStatusAborted
-	case "in_progress":
-		return BuildStatusInProgress
-	default:
+	if err := g.fetchJobs(); err != nil {
+		log.Entry().Debugf("fetching jobs: %s", err)
 		return BuildStatusFailure
 	}
+
+	for _, j := range g.jobs {
+		switch j.Conclusion {
+		case "failure":
+			return BuildStatusFailure
+		case "cancelled":
+			return BuildStatusAborted
+		}
+	}
+
+	return BuildStatusSuccess
 }
 
 // FullLogs returns the whole logfile for the current pipeline run
@@ -261,12 +269,7 @@ func (g *githubActionsConfigProvider) fetchRunData() {
 		return
 	}
 
-	runId, err := g.runIdInt64()
-	if err != nil {
-		log.Entry().Errorf("fetchRunData: %s", err)
-	}
-
-	runData, resp, err := g.client.Actions.GetWorkflowRunByID(g.ctx, g.owner, g.repo, runId)
+	runData, resp, err := g.client.Actions.GetWorkflowRunByID(g.ctx, g.owner, g.repo, g.runIdInt64())
 	if err != nil || resp.StatusCode != 200 {
 		log.Entry().Errorf("failed to get API data: %s", err)
 		return
@@ -279,7 +282,6 @@ func (g *githubActionsConfigProvider) fetchRunData() {
 func convertRunData(runData *github.WorkflowRun) run {
 	startedAtTs := piperutils.SafeDereference(runData.RunStartedAt)
 	return run{
-		Status:    piperutils.SafeDereference(runData.Status),
 		StartedAt: startedAtTs.Time,
 	}
 }
@@ -289,12 +291,7 @@ func (g *githubActionsConfigProvider) fetchJobs() error {
 		return nil
 	}
 
-	runId, err := g.runIdInt64()
-	if err != nil {
-		return err
-	}
-
-	jobs, resp, err := g.client.Actions.ListWorkflowJobs(g.ctx, g.owner, g.repo, runId, nil)
+	jobs, resp, err := g.client.Actions.ListWorkflowJobs(g.ctx, g.owner, g.repo, g.runIdInt64(), nil)
 	if err != nil || resp.StatusCode != 200 {
 		return errors.Wrap(err, "failed to get API data")
 	}
@@ -312,22 +309,24 @@ func convertJobs(jobs []*github.WorkflowJob) []job {
 	result := make([]job, 0, len(jobs))
 	for _, j := range jobs {
 		result = append(result, job{
-			ID:      j.GetID(),
-			Name:    j.GetName(),
-			HtmlURL: j.GetHTMLURL(),
+			ID:         j.GetID(),
+			Name:       j.GetName(),
+			HtmlURL:    j.GetHTMLURL(),
+			Conclusion: j.GetConclusion(),
 		})
 	}
 	return result
 }
 
-func (g *githubActionsConfigProvider) runIdInt64() (int64, error) {
+func (g *githubActionsConfigProvider) runIdInt64() int64 {
 	strRunId := g.BuildID()
 	runId, err := strconv.ParseInt(strRunId, 10, 64)
 	if err != nil {
-		return 0, errors.Wrapf(err, "invalid GITHUB_RUN_ID value %s: %s", strRunId, err)
+		log.Entry().Debugf("invalid GITHUB_RUN_ID value %s: %s", strRunId, err)
+		return 0
 	}
 
-	return runId, nil
+	return runId
 }
 
 func getOwnerAndRepoNames() (string, string) {

pkg/orchestrator/github_actions_test.go

@@ -17,20 +17,20 @@ import (
 
 func TestGitHubActionsConfigProvider_GetBuildStatus(t *testing.T) {
 	tests := []struct {
-		name    string
-		runData run
-		want    string
+		name string
+		jobs []job
+		want string
 	}{
-		{"BuildStatusSuccess", run{fetched: true, Status: "success"}, BuildStatusSuccess},
-		{"BuildStatusAborted", run{fetched: true, Status: "cancelled"}, BuildStatusAborted},
-		{"BuildStatusInProgress", run{fetched: true, Status: "in_progress"}, BuildStatusInProgress},
-		{"BuildStatusFailure", run{fetched: true, Status: "qwertyu"}, BuildStatusFailure},
-		{"BuildStatusFailure", run{fetched: true, Status: ""}, BuildStatusFailure},
+		{"BuildStatusSuccess", []job{{Conclusion: "success"}, {Conclusion: "success"}, {Conclusion: "success"}}, BuildStatusSuccess},
+		{"BuildStatusAborted", []job{{Conclusion: "success"}, {Conclusion: "success"}, {Conclusion: "cancelled"}}, BuildStatusAborted},
+		{"BuildStatusFailure", []job{{Conclusion: "success"}, {Conclusion: "failure"}, {Conclusion: "cancelled"}}, BuildStatusFailure},
+		{"BuildStatusSuccess", []job{{Conclusion: "success"}, {Conclusion: "cancelled"}, {Conclusion: "failure"}}, BuildStatusAborted},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := &githubActionsConfigProvider{
-				runData: tt.runData,
+				jobsFetched: true,
+				jobs:        tt.jobs,
 			}
 			assert.Equalf(t, tt.want, g.BuildStatus(), "BuildStatus()")
 		})
@@ -113,7 +113,6 @@ func TestGitHubActionsConfigProvider_fetchRunData(t *testing.T) {
 	startedAt, _ := time.Parse(time.RFC3339, "2023-08-11T07:28:24Z")
 	wantRunData := run{
 		fetched:   true,
-		Status:    "completed",
 		StartedAt: startedAt,
 	}
 
@@ -285,7 +284,6 @@ func TestGitHubActionsConfigProvider_Others(t *testing.T) {
 	startedAt, _ := time.Parse(time.RFC3339, "2023-08-11T07:28:24Z")
 	p.runData = run{
 		fetched:   true,
-		Status:    "",
 		StartedAt: startedAt,
 	}