Repository: goreleaser/goreleaser
Pull request #904 (2018-12-28 21:07:53)

better retry upload
current implementation on the client does not work well, as well its not the best place to do it, since we'll need to re-implement it on other clients when needed.

moved impl to the release pipe itself, also adding better tests for it.

refs #900 

codecov-io: # [Codecov](https://codecov.io/gh/goreleaser/goreleaser/pull/904?src=pr&el=h1) Report
> Merging [#904](https://codecov.io/gh/goreleaser/goreleaser/pull/904?src=pr&el=desc) into [master](https://codecov.io/gh/goreleaser/goreleaser/commit/6e09a19a83ed4b643981800aa2ecf1143dcb23c9?src=pr&el=desc) will **increase** coverage by `0.26%`.
> The diff coverage is `92.3%`.

[![Impacted file tree graph](https://codecov.io/gh/goreleaser/goreleaser/pull/904/graphs/tree.svg?width=650&token=3bJvv6b7hN&height=150&src=pr)](https://codecov.io/gh/goreleaser/goreleaser/pull/904?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #904      +/-   ##
==========================================
+ Coverage   87.83%   88.09%   +0.26%     
==========================================
  Files          48       48              
  Lines        2375     2378       +3     
==========================================
+ Hits         2086     2095       +9     
+ Misses        229      223       -6     
  Partials       60       60
```


| [Impacted Files](https://codecov.io/gh/goreleaser/goreleaser/pull/904?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/client/github.go](https://codecov.io/gh/goreleaser/goreleaser/pull/904/diff?src=pr&el=tree#diff-aW50ZXJuYWwvY2xpZW50L2dpdGh1Yi5nbw==) | `6.25% <0%> (+0.36%)` | :arrow_up: |
| [internal/pipe/release/release.go](https://codecov.io/gh/goreleaser/goreleaser/pull/904/diff?src=pr&el=tree#diff-aW50ZXJuYWwvcGlwZS9yZWxlYXNlL3JlbGVhc2UuZ28=) | `88.57% <100%> (+1.68%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/goreleaser/goreleaser/pull/904?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/goreleaser/goreleaser/pull/904?src=pr&el=footer). Last update [6e09a19...c29ea78](https://codecov.io/gh/goreleaser/goreleaser/pull/904?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/internal/client/github.go b/internal/client/github.go
index 835aa84c..93052b3b 100644
--- a/internal/client/github.go
+++ b/internal/client/github.go
@@ -133,18 +133,6 @@ func (c *githubClient) Upload(
 	releaseID int64,
 	name string,
 	file *os.File,
-) error {
-	return c.uploadRetry(ctx, releaseID, name, file, 0)
-}
-
-const maxRetries = 10
-
-func (c *githubClient) uploadRetry(
-	ctx *context.Context,
-	releaseID int64,
-	name string,
-	file *os.File,
-	retry int,
 ) error {
 	_, _, err := c.client.Repositories.UploadReleaseAsset(
 		ctx,
@@ -156,11 +144,5 @@ func (c *githubClient) uploadRetry(
 		},
 		file,
 	)
-	if err != nil {
-		if retry > maxRetries {
-			return err
-		}
-		return err
-	}
-	return c.uploadRetry(ctx, releaseID, name, file, retry+1)
+	return err
 }
diff --git a/internal/pipe/release/release.go b/internal/pipe/release/release.go
index e41371dc..c868f260 100644
--- a/internal/pipe/release/release.go
+++ b/internal/pipe/release/release.go
@@ -89,18 +89,31 @@ func doPublish(ctx *context.Context, c client.Client) error {
 	).List() {
 		artifact := artifact
 		g.Go(func() error {
-			return upload(ctx, c, releaseID, artifact)
+			return upload(ctx, c, releaseID, artifact, 1)
 		})
 	}
 	return g.Wait()
 }
 
-func upload(ctx *context.Context, c client.Client, releaseID int64, artifact artifact.Artifact) error {
+const maxTries = 10
+
+func upload(ctx *context.Context, c client.Client, releaseID int64, artifact artifact.Artifact, try int) error {
 	file, err := os.Open(artifact.Path)
 	if err != nil {
 		return err
 	}
 	defer file.Close() // nolint: errcheck
 	log.WithField("file", file.Name()).WithField("name", artifact.Name).Info("uploading to release")
-	return c.Upload(ctx, releaseID, artifact.Name, file)
+	err = c.Upload(ctx, releaseID, artifact.Name, file)
+	if err != nil {
+		if try == maxTries {
+			return errors.Wrapf(err, "failed to upload %s after %d retries", artifact.Name, try)
+		}
+		log.WithFields(log.Fields{
+			"try":      try,
+			"artifact": artifact.Name,
+		}).Warnf("failed to upload artifact, will retry")
+		return upload(ctx, c, releaseID, artifact, try+1)
+	}
+	return nil
 }


Test patch:
diff --git a/internal/pipe/release/release_test.go b/internal/pipe/release/release_test.go
index ca73bde9..6702a5c5 100644
--- a/internal/pipe/release/release_test.go
+++ b/internal/pipe/release/release_test.go
@@ -2,7 +2,6 @@ package release
 
 import (
 	"bytes"
-	"errors"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -13,6 +12,7 @@ import (
 	"github.com/goreleaser/goreleaser/internal/testlib"
 	"github.com/goreleaser/goreleaser/pkg/config"
 	"github.com/goreleaser/goreleaser/pkg/context"
+	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -120,11 +120,39 @@ func TestRunPipeUploadFailure(t *testing.T) {
 	client := &DummyClient{
 		FailToUpload: true,
 	}
-	assert.Error(t, doPublish(ctx, client))
+	assert.EqualError(t, doPublish(ctx, client), "failed to upload bin.tar.gz after 10 retries: upload failed")
 	assert.True(t, client.CreatedRelease)
 	assert.False(t, client.UploadedFile)
 }
 
+func TestRunPipeUploadRetry(t *testing.T) {
+	folder, err := ioutil.TempDir("", "goreleasertest")
+	assert.NoError(t, err)
+	tarfile, err := os.Create(filepath.Join(folder, "bin.tar.gz"))
+	assert.NoError(t, err)
+	var config = config.Project{
+		Release: config.Release{
+			GitHub: config.Repo{
+				Owner: "test",
+				Name:  "test",
+			},
+		},
+	}
+	var ctx = context.New(config)
+	ctx.Git = context.GitInfo{CurrentTag: "v1.0.0"}
+	ctx.Artifacts.Add(artifact.Artifact{
+		Type: artifact.UploadableArchive,
+		Name: "bin.tar.gz",
+		Path: tarfile.Name(),
+	})
+	client := &DummyClient{
+		FailFirstUpload: true,
+	}
+	assert.NoError(t, doPublish(ctx, client))
+	assert.True(t, client.CreatedRelease)
+	assert.True(t, client.UploadedFile)
+}
+
 func TestPipeDisabled(t *testing.T) {
 	var ctx = context.New(config.Project{
 		Release: config.Release{
@@ -263,6 +291,7 @@ type DummyClient struct {
 	CreatedRelease      bool
 	UploadedFile        bool
 	UploadedFileNames   []string
+	FailFirstUpload     bool
 	Lock                sync.Mutex
 }
 
@@ -278,13 +307,22 @@ func (client *DummyClient) CreateFile(ctx *context.Context, commitAuthor config.
 	return
 }
 
-func (client *DummyClient) Upload(ctx *context.Context, releaseID int64, name string, file *os.File) (err error) {
+func (client *DummyClient) Upload(ctx *context.Context, releaseID int64, name string, file *os.File) error {
 	client.Lock.Lock()
 	defer client.Lock.Unlock()
+	// ensure file is read to better mimic real behavior
+	_, err := ioutil.ReadAll(file)
+	if err != nil {
+		return errors.Wrapf(err, "unexpected error")
+	}
 	if client.FailToUpload {
 		return errors.New("upload failed")
 	}
+	if client.FailFirstUpload {
+		client.FailFirstUpload = false
+		return errors.New("upload failed, should retry")
+	}
 	client.UploadedFile = true
 	client.UploadedFileNames = append(client.UploadedFileNames, name)
-	return
+	return nil
 }