Move build to use sync WaitGroups (#371)

buildkite · Oct 10, 2024 · 0585748 · 0585748
1 parent 93d4825
commit 0585748
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 45 deletions.
diff --git a/pkg/cmd/build/download.go b/pkg/cmd/build/download.go
@@ -1,10 +1,11 @@
 package build
 
 import (
-	"context"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
+	"sync"
 
 	"github.com/buildkite/cli/v3/internal/build"
 	buildResolver "github.com/buildkite/cli/v3/internal/build/resolver"
@@ -13,7 +14,6 @@ import (
 	"github.com/buildkite/cli/v3/pkg/cmd/factory"
 	"github.com/charmbracelet/huh/spinner"
 	"github.com/spf13/cobra"
-	"golang.org/x/sync/errgroup"
 )
 
 func NewCmdBuildDownload(f *factory.Factory) *cobra.Command {
@@ -70,7 +70,7 @@ func NewCmdBuildDownload(f *factory.Factory) *cobra.Command {
 			spinErr := spinner.New().
 				Title("Downloading build resources").
 				Action(func() {
-					dir, err = download(cmd.Context(), *bld, f)
+					dir, err = download(*bld, f)
 				}).
 				Run()
 			if spinErr != nil {
@@ -96,7 +96,9 @@ func NewCmdBuildDownload(f *factory.Factory) *cobra.Command {
 	return &cmd
 }
 
-func download(ctx context.Context, build build.Build, f *factory.Factory) (string, error) {
+func download(build build.Build, f *factory.Factory) (string, error) {
+	var wg sync.WaitGroup
+	var mu sync.Mutex
 	b, _, err := f.RestAPIClient.Builds.Get(build.Organization, build.Pipeline, fmt.Sprint(build.BuildNumber), nil)
 	if err != nil {
 		return "", err
@@ -111,30 +113,37 @@ func download(ctx context.Context, build build.Build, f *factory.Factory) (strin
 		return "", err
 	}
 
-	eg, _ := errgroup.WithContext(ctx)
-
 	for _, job := range b.Jobs {
 		job := job
 		// only script (command) jobs will have logs
 		if job == nil || *job.Type != "script" {
 			continue
 		}
 
-		eg.Go(func() error {
-			log, _, err := f.RestAPIClient.Jobs.GetJobLog(build.Organization, build.Pipeline, *b.ID, *job.ID)
+		go func() {
+			defer wg.Done()
+			wg.Add(1)
+			log, _, apiErr := f.RestAPIClient.Jobs.GetJobLog(build.Organization, build.Pipeline, *b.ID, *job.ID)
 			if err != nil {
-				return err
+				mu.Lock()
+				err = apiErr
+				mu.Unlock()
+				return
 			}
-			if log == nil {
-				return fmt.Errorf("could not get logs for job %s", *job.ID)
+			if log == nil || log.Content == nil {
+				mu.Lock()
+				err = errors.New("empty log found")
+				mu.Unlock()
+				return
 			}
 
-			err = os.WriteFile(filepath.Join(directory, *job.ID), []byte(*log.Content), 0o644)
-			if err != nil {
-				return err
+			fileErr := os.WriteFile(filepath.Join(directory, *job.ID), []byte(*log.Content), 0o644)
+			if fileErr != nil {
+				mu.Lock()
+				err = fileErr
+				mu.Unlock()
 			}
-			return nil
-		})
+		}()
 	}
 
 	artifacts, _, err := f.RestAPIClient.Artifacts.ListByBuild(build.Organization, build.Pipeline, fmt.Sprint(build.BuildNumber), nil)
@@ -144,20 +153,21 @@ func download(ctx context.Context, build build.Build, f *factory.Factory) (strin
 
 	for _, artifact := range artifacts {
 		artifact := artifact
-		eg.Go(func() error {
-			out, err := os.Create(filepath.Join(directory, fmt.Sprintf("artifact-%s-%s", *artifact.ID, *artifact.Filename)))
+		go func() {
+			defer wg.Done()
+			wg.Add(1)
+			out, fileErr := os.Create(filepath.Join(directory, fmt.Sprintf("artifact-%s-%s", *artifact.ID, *artifact.Filename)))
 			if err != nil {
-				return err
+				err = fileErr
 			}
-			_, err = f.RestAPIClient.Artifacts.DownloadArtifactByURL(*artifact.DownloadURL, out)
+			_, apiErr := f.RestAPIClient.Artifacts.DownloadArtifactByURL(*artifact.DownloadURL, out)
 			if err != nil {
-				return err
+				err = apiErr
 			}
-			return nil
-		})
+		}()
 	}
 
-	err = eg.Wait()
+	wg.Wait()
 	if err != nil {
 		return "", err
 	}

diff --git a/pkg/cmd/build/view.go b/pkg/cmd/build/view.go
@@ -2,6 +2,7 @@ package build
 
 import (
 	"fmt"
+	"sync"
 
 	"github.com/MakeNowJust/heredoc"
 	"github.com/buildkite/cli/v3/internal/annotation"
@@ -17,7 +18,6 @@ import (
 	"github.com/charmbracelet/lipgloss"
 	"github.com/pkg/browser"
 	"github.com/spf13/cobra"
-	"golang.org/x/sync/errgroup"
 )
 
 func NewCmdBuildView(f *factory.Factory) *cobra.Command {
@@ -54,6 +54,7 @@ func NewCmdBuildView(f *factory.Factory) *cobra.Command {
 			$ bk build view -p deploy-pipeline -u "greg"
 		`),
 		RunE: func(cmd *cobra.Command, args []string) error {
+			var err error
 			// we find the pipeline based on the following rules:
 			// 1. an explicit flag is passed
 			// 2. a configured pipeline for this directory
@@ -103,29 +104,47 @@ func NewCmdBuildView(f *factory.Factory) *cobra.Command {
 			var buildArtifacts []buildkite.Artifact
 			var buildAnnotations []buildkite.Annotation
 
-			group, _ := errgroup.WithContext(cmd.Context())
+			var wg sync.WaitGroup
+			var mu sync.Mutex
+
 			spinErr := spinner.New().
 				Title("Loading build information").
 				Action(func() {
-					group.Go(func() error {
-						var err error
-						b, _, err = f.RestAPIClient.Builds.Get(bld.Organization, bld.Pipeline, fmt.Sprint(bld.BuildNumber), &buildkite.BuildsListOptions{})
-						return err
-					})
-
-					group.Go(func() error {
-						var err error
-						buildArtifacts, _, err = f.RestAPIClient.Artifacts.ListByBuild(bld.Organization, bld.Pipeline, fmt.Sprint(bld.BuildNumber), &buildkite.ArtifactListOptions{})
-						return err
-					})
-
-					group.Go(func() error {
-						var err error
-						buildAnnotations, _, err = f.RestAPIClient.Annotations.ListByBuild(bld.Organization, bld.Pipeline, fmt.Sprint(bld.BuildNumber), &buildkite.AnnotationListOptions{})
-						return err
-					})
-
-					err = group.Wait()
+					wg.Add(3)
+					go func() {
+						defer wg.Done()
+						var apiErr error
+						b, _, apiErr = f.RestAPIClient.Builds.Get(bld.Organization, bld.Pipeline, fmt.Sprint(bld.BuildNumber), &buildkite.BuildsListOptions{})
+						if apiErr != nil {
+							mu.Lock()
+							err = apiErr
+							mu.Unlock()
+						}
+					}()
+
+					go func() {
+						defer wg.Done()
+						var apiErr error
+						buildArtifacts, _, apiErr = f.RestAPIClient.Artifacts.ListByBuild(bld.Organization, bld.Pipeline, fmt.Sprint(bld.BuildNumber), &buildkite.ArtifactListOptions{})
+						if apiErr != nil {
+							mu.Lock()
+							err = apiErr
+							mu.Unlock()
+						}
+					}()
+
+					go func() {
+						defer wg.Done()
+						var apiErr error
+						buildAnnotations, _, apiErr = f.RestAPIClient.Annotations.ListByBuild(bld.Organization, bld.Pipeline, fmt.Sprint(bld.BuildNumber), &buildkite.AnnotationListOptions{})
+						if apiErr != nil {
+							mu.Lock()
+							err = apiErr
+							mu.Unlock()
+						}
+					}()
+
+					wg.Wait()
 				}).
 				Run()
 			if spinErr != nil {