From a12f834136f923db1e6dabd44e2534c6a9ba23a7 Mon Sep 17 00:00:00 2001 From: Erik Westrup Date: Thu, 20 Jul 2023 15:08:46 +0200 Subject: [PATCH 01/20] Prototype --- go/porcelain/deploy.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/go/porcelain/deploy.go b/go/porcelain/deploy.go index d4a18508..baf883ab 100644 --- a/go/porcelain/deploy.go +++ b/go/porcelain/deploy.go @@ -538,10 +538,14 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl context.GetLogger(ctx).WithError(operationError).Errorf("Failed to upload file %v", f.Name) apiErr, ok := operationError.(apierrors.Error) - if ok && apiErr.Code() == 401 { - sharedErr.mutex.Lock() - sharedErr.err = operationError - sharedErr.mutex.Unlock() + if ok { + if apiErr.Code() == 401 { + sharedErr.mutex.Lock() + sharedErr.err = operationError + sharedErr.mutex.Unlock() + } else if apiErr.Code()/100 == 4 { + return nil + } } } From 6eaf7b26331372d74de331c49ea117216025c310 Mon Sep 17 00:00:00 2001 From: Erik Westrup Date: Thu, 20 Jul 2023 15:12:49 +0200 Subject: [PATCH 02/20] fix --- go/porcelain/deploy.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/go/porcelain/deploy.go b/go/porcelain/deploy.go index baf883ab..8012bbfe 100644 --- a/go/porcelain/deploy.go +++ b/go/porcelain/deploy.go @@ -543,7 +543,9 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl sharedErr.mutex.Lock() sharedErr.err = operationError sharedErr.mutex.Unlock() - } else if apiErr.Code()/100 == 4 { + } + + if apiErr.Code()/100 == 4 { return nil } } From 2f75b17dec49ef7d09c3126c28d69268ddba52f9 Mon Sep 17 00:00:00 2001 From: Erik Westrup Date: Thu, 20 Jul 2023 15:13:28 +0200 Subject: [PATCH 03/20] Comment --- go/porcelain/deploy.go | 1 + 1 file changed, 1 insertion(+) diff --git a/go/porcelain/deploy.go b/go/porcelain/deploy.go index 8012bbfe..f567caa2 100644 --- a/go/porcelain/deploy.go +++ b/go/porcelain/deploy.go @@ -545,6 +545,7 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl sharedErr.mutex.Unlock() } + // TODO this changes retry behaviour for the fileUpload case as well. OK? if apiErr.Code()/100 == 4 { return nil } From 7d7c6a701b59075de07e02fb232162357add887f Mon Sep 17 00:00:00 2001 From: Jenae Janzen Date: Wed, 23 Aug 2023 13:20:01 -0400 Subject: [PATCH 04/20] stop retrying on 400 and 422 status codes --- go/porcelain/deploy.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/go/porcelain/deploy.go b/go/porcelain/deploy.go index f567caa2..c39b87c9 100644 --- a/go/porcelain/deploy.go +++ b/go/porcelain/deploy.go @@ -546,7 +546,10 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl } // TODO this changes retry behaviour for the fileUpload case as well. OK? - if apiErr.Code()/100 == 4 { + if apiErr.Code() == 400 || apiErr.Code() == 422 { + sharedErr.mutex.Lock() + sharedErr.err = operationError + sharedErr.mutex.Unlock() return nil } } From 03e50c86accee7b67f995b4192936b3370e25125 Mon Sep 17 00:00:00 2001 From: Jenae Janzen Date: Wed, 23 Aug 2023 13:20:59 -0400 Subject: [PATCH 05/20] remove todo comment --- go/porcelain/deploy.go | 1 - 1 file changed, 1 deletion(-) diff --git a/go/porcelain/deploy.go b/go/porcelain/deploy.go index c39b87c9..f415af54 100644 --- a/go/porcelain/deploy.go +++ b/go/porcelain/deploy.go @@ -545,7 +545,6 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl sharedErr.mutex.Unlock() } - // TODO this changes retry behaviour for the fileUpload case as well. OK? if apiErr.Code() == 400 || apiErr.Code() == 422 { sharedErr.mutex.Lock() sharedErr.err = operationError From 279e58b2157a297894c8f012cccc5c66f893ff53 Mon Sep 17 00:00:00 2001 From: Jenae Janzen Date: Wed, 23 Aug 2023 15:52:14 -0400 Subject: [PATCH 06/20] add SkipRetry ff to options struct --- go/porcelain/deploy.go | 17 ++++++++++------- go/porcelain/deploy_test.go | 10 +++++----- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/go/porcelain/deploy.go b/go/porcelain/deploy.go index f415af54..190ef2fd 100644 --- a/go/porcelain/deploy.go +++ b/go/porcelain/deploy.go @@ -83,7 +83,8 @@ type DeployOptions struct { BuildDir string LargeMediaEnabled bool - IsDraft bool + IsDraft bool + SkipRetry bool Title string Branch string @@ -344,12 +345,14 @@ func (n *Netlify) DoDeploy(ctx context.Context, options *DeployOptions, deploy * return deploy, nil } - if err := n.uploadFiles(ctx, deploy, options.files, options.Observer, fileUpload, options.UploadTimeout); err != nil { + skipRetry := options.SkipRetry || false + + if err := n.uploadFiles(ctx, deploy, options.files, options.Observer, fileUpload, options.UploadTimeout, skipRetry); err != nil { return nil, err } if options.functions != nil { - if err := n.uploadFiles(ctx, deploy, options.functions, options.Observer, functionUpload, options.UploadTimeout); err != nil { + if err := n.uploadFiles(ctx, deploy, options.functions, options.Observer, functionUpload, options.UploadTimeout, options.SkipRetry); err != nil { return nil, err } } @@ -401,7 +404,7 @@ func (n *Netlify) WaitUntilDeployLive(ctx context.Context, d *models.Deploy) (*m return n.waitForState(ctx, d, "ready") } -func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *deployFiles, observer DeployObserver, t uploadType, timeout time.Duration) error { +func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *deployFiles, observer DeployObserver, t uploadType, timeout time.Duration, skipRetry bool) error { sharedErr := &uploadError{err: nil, mutex: &sync.Mutex{}} sem := make(chan int, n.uploadLimit) wg := &sync.WaitGroup{} @@ -431,7 +434,7 @@ func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *depl select { case sem <- 1: wg.Add(1) - go n.uploadFile(ctx, d, file, observer, t, timeout, wg, sem, sharedErr) + go n.uploadFile(ctx, d, file, observer, t, timeout, wg, sem, sharedErr, skipRetry) case <-ctx.Done(): log.Info("Context terminated, aborting file upload") return errors.Wrap(ctx.Err(), "aborted file upload early") @@ -451,7 +454,7 @@ func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *depl return sharedErr.err } -func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundle, c DeployObserver, t uploadType, timeout time.Duration, wg *sync.WaitGroup, sem chan int, sharedErr *uploadError) { +func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundle, c DeployObserver, t uploadType, timeout time.Duration, wg *sync.WaitGroup, sem chan int, sharedErr *uploadError, skipRetry bool) { defer func() { wg.Done() <-sem @@ -545,7 +548,7 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl sharedErr.mutex.Unlock() } - if apiErr.Code() == 400 || apiErr.Code() == 422 { + if skipRetry && (apiErr.Code() == 400 || apiErr.Code() == 422) { sharedErr.mutex.Lock() sharedErr.err = operationError sharedErr.mutex.Unlock() diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 440aee50..5158f9d1 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -287,7 +287,7 @@ func TestUploadFiles_Cancelation(t *testing.T) { for _, bundle := range files.Files { d.Required = append(d.Required, bundle.Sum) } - err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute) + err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false) require.ErrorIs(t, err, gocontext.Canceled) } @@ -317,7 +317,7 @@ func TestUploadFiles_Errors(t *testing.T) { for _, bundle := range files.Files { d.Required = append(d.Required, bundle.Sum) } - err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute) + err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false) require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][500] uploadDeployFile default &{Code:0 Message:}") } @@ -377,11 +377,11 @@ func TestUploadFiles_SkipEqualFiles(t *testing.T) { d.Required = []string{files.Sums["a.html"]} d.RequiredFunctions = []string{functions.Sums["a"]} - err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute) + err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false) require.NoError(t, err) assert.Equal(t, 1, serverRequests) - err = client.uploadFiles(ctx, d, functions, nil, functionUpload, time.Minute) + err = client.uploadFiles(ctx, d, functions, nil, functionUpload, time.Minute, false) require.NoError(t, err) assert.Equal(t, 2, serverRequests) } @@ -437,7 +437,7 @@ func TestUploadFunctions_RetryCountHeader(t *testing.T) { d.RequiredFunctions = append(d.RequiredFunctions, bundle.Sum) } - require.NoError(t, client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute)) + require.NoError(t, client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute, false)) } func TestBundle(t *testing.T) { From 7d176fb4d6c64d70e37b1f6f1958e782d14ea32a Mon Sep 17 00:00:00 2001 From: Jenae Janzen Date: Wed, 23 Aug 2023 15:55:59 -0400 Subject: [PATCH 07/20] small fix --- go/porcelain/deploy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/porcelain/deploy.go b/go/porcelain/deploy.go index 190ef2fd..c38812c2 100644 --- a/go/porcelain/deploy.go +++ b/go/porcelain/deploy.go @@ -352,7 +352,7 @@ func (n *Netlify) DoDeploy(ctx context.Context, options *DeployOptions, deploy * } if options.functions != nil { - if err := n.uploadFiles(ctx, deploy, options.functions, options.Observer, functionUpload, options.UploadTimeout, options.SkipRetry); err != nil { + if err := n.uploadFiles(ctx, deploy, options.functions, options.Observer, functionUpload, options.UploadTimeout, skipRetry); err != nil { return nil, err } } From de2b8c11e9c52d0515a84667d06703b697ef5995 Mon Sep 17 00:00:00 2001 From: Jenae Janzen Date: Tue, 29 Aug 2023 14:45:12 -0400 Subject: [PATCH 08/20] use permanent error --- go/porcelain/deploy.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/go/porcelain/deploy.go b/go/porcelain/deploy.go index c38812c2..9fba3111 100644 --- a/go/porcelain/deploy.go +++ b/go/porcelain/deploy.go @@ -345,7 +345,7 @@ func (n *Netlify) DoDeploy(ctx context.Context, options *DeployOptions, deploy * return deploy, nil } - skipRetry := options.SkipRetry || false + skipRetry := options.SkipRetry if err := n.uploadFiles(ctx, deploy, options.files, options.Observer, fileUpload, options.UploadTimeout, skipRetry); err != nil { return nil, err @@ -406,6 +406,7 @@ func (n *Netlify) WaitUntilDeployLive(ctx context.Context, d *models.Deploy) (*m func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *deployFiles, observer DeployObserver, t uploadType, timeout time.Duration, skipRetry bool) error { sharedErr := &uploadError{err: nil, mutex: &sync.Mutex{}} + permanentErr := &backoff.PermanentError{Err: nil} sem := make(chan int, n.uploadLimit) wg := &sync.WaitGroup{} @@ -434,7 +435,7 @@ func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *depl select { case sem <- 1: wg.Add(1) - go n.uploadFile(ctx, d, file, observer, t, timeout, wg, sem, sharedErr, skipRetry) + go n.uploadFile(ctx, d, file, observer, t, timeout, wg, sem, sharedErr, permanentErr, skipRetry) case <-ctx.Done(): log.Info("Context terminated, aborting file upload") return errors.Wrap(ctx.Err(), "aborted file upload early") @@ -454,7 +455,7 @@ func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *depl return sharedErr.err } -func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundle, c DeployObserver, t uploadType, timeout time.Duration, wg *sync.WaitGroup, sem chan int, sharedErr *uploadError, skipRetry bool) { +func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundle, c DeployObserver, t uploadType, timeout time.Duration, wg *sync.WaitGroup, sem chan int, sharedErr *uploadError, permanentErr *backoff.PermanentError, skipRetry bool) { defer func() { wg.Done() <-sem @@ -549,10 +550,7 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl } if skipRetry && (apiErr.Code() == 400 || apiErr.Code() == 422) { - sharedErr.mutex.Lock() - sharedErr.err = operationError - sharedErr.mutex.Unlock() - return nil + operationError = permanentErr } } } From 9b326af293c3efffb6db21a2764204b713c73d30 Mon Sep 17 00:00:00 2001 From: Jacq Whitmer Date: Wed, 30 Aug 2023 11:57:51 -0700 Subject: [PATCH 09/20] test --- go/porcelain/deploy_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 5158f9d1..66e07136 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -320,6 +320,35 @@ func TestUploadFiles_Errors(t *testing.T) { err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false) require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][500] uploadDeployFile default &{Code:0 Message:}") } +func TestUploadFiles400Errors(t *testing.T) { + ctx := gocontext.Background() + + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) { + rw.WriteHeader(http.StatusUnprocessableEntity) + })) + defer server.Close() + + hu, _ := url.Parse(server.URL) + tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient) + client := NewRetryable(tr, strfmt.Default, 1) + client.uploadLimit = 1 + ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token")) + + // Create some files to deploy + dir, err := ioutil.TempDir("", "deploy") + require.NoError(t, err) + defer os.RemoveAll(dir) + require.NoError(t, ioutil.WriteFile(filepath.Join(dir, "foo.html"), []byte("Hello"), 0644)) + + files, err := walk(dir, nil, false, false) + require.NoError(t, err) + d := &models.Deploy{} + for _, bundle := range files.Files { + d.Required = append(d.Required, bundle.Sum) + } + err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false) + require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message:}") +} func TestUploadFiles_SkipEqualFiles(t *testing.T) { ctx := gocontext.Background() From 6248a6208a86ef53d639fe47ad553e13f3453796 Mon Sep 17 00:00:00 2001 From: Jacq Whitmer Date: Wed, 30 Aug 2023 12:26:34 -0700 Subject: [PATCH 10/20] test part 3 --- go/porcelain/deploy_test.go | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 66e07136..1b695a94 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -334,19 +334,8 @@ func TestUploadFiles400Errors(t *testing.T) { client.uploadLimit = 1 ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token")) - // Create some files to deploy - dir, err := ioutil.TempDir("", "deploy") - require.NoError(t, err) - defer os.RemoveAll(dir) - require.NoError(t, ioutil.WriteFile(filepath.Join(dir, "foo.html"), []byte("Hello"), 0644)) - - files, err := walk(dir, nil, false, false) - require.NoError(t, err) d := &models.Deploy{} - for _, bundle := range files.Files { - d.Required = append(d.Required, bundle.Sum) - } - err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false) + err := client.uploadFiles(ctx, d, nil, nil, fileUpload, time.Minute, false) require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message:}") } From 49e8d719992d17b754648e4dcf9ba53bf9228758 Mon Sep 17 00:00:00 2001 From: Jacq Whitmer Date: Wed, 30 Aug 2023 12:32:36 -0700 Subject: [PATCH 11/20] ff on for test --- go/porcelain/deploy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 1b695a94..50e08588 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -335,7 +335,7 @@ func TestUploadFiles400Errors(t *testing.T) { ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token")) d := &models.Deploy{} - err := client.uploadFiles(ctx, d, nil, nil, fileUpload, time.Minute, false) + err := client.uploadFiles(ctx, d, nil, nil, fileUpload, time.Minute, true) require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message:}") } From 01161271d8d608211307a4893a163f0eecb46517 Mon Sep 17 00:00:00 2001 From: Jacq Whitmer Date: Wed, 30 Aug 2023 12:38:53 -0700 Subject: [PATCH 12/20] test part 4 --- go/porcelain/deploy_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 50e08588..49fd51b3 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -334,8 +334,19 @@ func TestUploadFiles400Errors(t *testing.T) { client.uploadLimit = 1 ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token")) + // Create some files to deploy + dir, err := ioutil.TempDir("", "deploy") + require.NoError(t, err) + defer os.RemoveAll(dir) + require.NoError(t, ioutil.WriteFile(filepath.Join(dir, "foo.html"), []byte("Hello"), 0644)) + + files, err := walk(dir, nil, false, false) + require.NoError(t, err) d := &models.Deploy{} - err := client.uploadFiles(ctx, d, nil, nil, fileUpload, time.Minute, true) + for _, bundle := range files.Files { + d.Required = append(d.Required, bundle.Sum) + } + err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, true) require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message:}") } From 047bf94372fd0bfc888a34708064cb9c9c0739fe Mon Sep 17 00:00:00 2001 From: Jacq Whitmer Date: Wed, 30 Aug 2023 12:46:13 -0700 Subject: [PATCH 13/20] test part 5 --- go/porcelain/deploy_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 49fd51b3..08aed92b 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -343,9 +343,6 @@ func TestUploadFiles400Errors(t *testing.T) { files, err := walk(dir, nil, false, false) require.NoError(t, err) d := &models.Deploy{} - for _, bundle := range files.Files { - d.Required = append(d.Required, bundle.Sum) - } err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, true) require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message:}") } From 2192c510763d2ec504aef6bb3157ef40cad2dd84 Mon Sep 17 00:00:00 2001 From: Jacq Whitmer Date: Wed, 30 Aug 2023 13:12:34 -0700 Subject: [PATCH 14/20] test part 6 --- go/porcelain/deploy_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 08aed92b..0906e0b1 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -332,7 +332,7 @@ func TestUploadFiles400Errors(t *testing.T) { tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient) client := NewRetryable(tr, strfmt.Default, 1) client.uploadLimit = 1 - ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token")) + ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("bad")) // Create some files to deploy dir, err := ioutil.TempDir("", "deploy") @@ -343,8 +343,11 @@ func TestUploadFiles400Errors(t *testing.T) { files, err := walk(dir, nil, false, false) require.NoError(t, err) d := &models.Deploy{} + for _, bundle := range files.Files { + d.Required = append(d.Required, bundle.Sum) + } err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, true) - require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message:}") + require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][401] uploadDeployFile default &{Code:401 Message: Unauthorized}") } func TestUploadFiles_SkipEqualFiles(t *testing.T) { From 136793553cc1dfb5600bf218ccd29e93bca61d46 Mon Sep 17 00:00:00 2001 From: Jenae Janzen Date: Thu, 31 Aug 2023 18:22:39 -0400 Subject: [PATCH 15/20] return permanent error + work on tests --- go/porcelain/deploy.go | 15 +++++-- go/porcelain/deploy_test.go | 82 +++++++++++++++++++++++++++++++++++-- 2 files changed, 89 insertions(+), 8 deletions(-) diff --git a/go/porcelain/deploy.go b/go/porcelain/deploy.go index 9fba3111..07b0de83 100644 --- a/go/porcelain/deploy.go +++ b/go/porcelain/deploy.go @@ -406,7 +406,6 @@ func (n *Netlify) WaitUntilDeployLive(ctx context.Context, d *models.Deploy) (*m func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *deployFiles, observer DeployObserver, t uploadType, timeout time.Duration, skipRetry bool) error { sharedErr := &uploadError{err: nil, mutex: &sync.Mutex{}} - permanentErr := &backoff.PermanentError{Err: nil} sem := make(chan int, n.uploadLimit) wg := &sync.WaitGroup{} @@ -435,7 +434,7 @@ func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *depl select { case sem <- 1: wg.Add(1) - go n.uploadFile(ctx, d, file, observer, t, timeout, wg, sem, sharedErr, permanentErr, skipRetry) + go n.uploadFile(ctx, d, file, observer, t, timeout, wg, sem, sharedErr, skipRetry) case <-ctx.Done(): log.Info("Context terminated, aborting file upload") return errors.Wrap(ctx.Err(), "aborted file upload early") @@ -455,7 +454,7 @@ func (n *Netlify) uploadFiles(ctx context.Context, d *models.Deploy, files *depl return sharedErr.err } -func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundle, c DeployObserver, t uploadType, timeout time.Duration, wg *sync.WaitGroup, sem chan int, sharedErr *uploadError, permanentErr *backoff.PermanentError, skipRetry bool) { +func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundle, c DeployObserver, t uploadType, timeout time.Duration, wg *sync.WaitGroup, sem chan int, sharedErr *uploadError, skipRetry bool) { defer func() { wg.Done() <-sem @@ -542,6 +541,14 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl context.GetLogger(ctx).WithError(operationError).Errorf("Failed to upload file %v", f.Name) apiErr, ok := operationError.(apierrors.Error) + // In testing, we repeatedly get to this line, see "Failed to upload file", but no matter + // what we try, ok is always `false`, and the code we're trying to test gets skipped. + // There isn't a test case for the original code (the 401 error) + + // The operation error we're receiving is "(*models.Error) is not supported by the TextConsumer, can be resolved by supporting TextUnmarshaler interface" + // which seems like a problem with how we're stubbing the response body from the server in the test, + // but it's similar to how it's stubbed in + if ok { if apiErr.Code() == 401 { sharedErr.mutex.Lock() @@ -550,7 +557,7 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl } if skipRetry && (apiErr.Code() == 400 || apiErr.Code() == 422) { - operationError = permanentErr + operationError = backoff.Permanent(operationError) } } } diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 0906e0b1..32270f07 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -320,19 +320,91 @@ func TestUploadFiles_Errors(t *testing.T) { err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false) require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][500] uploadDeployFile default &{Code:0 Message:}") } -func TestUploadFiles400Errors(t *testing.T) { + +func TestUploadFiles400Error_SkipsRetry(t *testing.T) { + attempts := 0 ctx := gocontext.Background() server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) { + defer func() { + attempts++ + }() + rw.WriteHeader(http.StatusUnprocessableEntity) + rw.Header().Set("Content-Type", "application/json; charset=utf-8") + rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 422 }`)) })) defer server.Close() + // // File upload: + // hu, _ := url.Parse(server.URL) + // tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient) + // client := NewRetryable(tr, strfmt.Default, 1) + // client.uploadLimit = 1 + // ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token")) + + // // Create some files to deploy + // dir, err := ioutil.TempDir("", "deploy") + // require.NoError(t, err) + // defer os.RemoveAll(dir) + // require.NoError(t, ioutil.WriteFile(filepath.Join(dir, "foo.html"), []byte("Hello"), 0644)) + + // files, err := walk(dir, nil, false, false) + // require.NoError(t, err) + // d := &models.Deploy{} + // for _, bundle := range files.Files { + // d.Required = append(d.Required, bundle.Sum) + // } + // // Set SkipRetry to true + // err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, true) + + // // Function upload: hu, _ := url.Parse(server.URL) tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient) client := NewRetryable(tr, strfmt.Default, 1) client.uploadLimit = 1 - ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("bad")) + apiCtx := context.WithAuthInfo(ctx, apiClient.BearerToken("token")) + + dir, err := ioutil.TempDir("", "deploy") + functionsPath := filepath.Join(dir, ".netlify", "functions") + os.MkdirAll(functionsPath, os.ModePerm) + require.NoError(t, err) + defer os.RemoveAll(dir) + require.NoError(t, ioutil.WriteFile(filepath.Join(functionsPath, "foo.js"), []byte("module.exports = () => {}"), 0644)) + + files, _, _, err := bundle(ctx, functionsPath, mockObserver{}) + require.NoError(t, err) + d := &models.Deploy{} + for _, bundle := range files.Files { + d.RequiredFunctions = append(d.RequiredFunctions, bundle.Sum) + } + // Set SkipRetry to true + require.NoError(t, client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute, true)) + require.Equal(t, err, "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message: Unprocessable Entity}") + require.Equal(t, attempts, 1) +} + +func TestUploadFiles400Error_NoSkipRetry(t *testing.T) { + attempts := 0 + ctx := gocontext.Background() + + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) { + defer func() { + attempts++ + }() + + rw.WriteHeader(http.StatusUnprocessableEntity) + rw.Header().Set("Content-Type", "application/json; charset=utf-8") + rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 422 }`)) + return + })) + defer server.Close() + + hu, _ := url.Parse(server.URL) + tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient) + client := NewRetryable(tr, strfmt.Default, 1) + client.uploadLimit = 1 + ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token")) // Create some files to deploy dir, err := ioutil.TempDir("", "deploy") @@ -346,8 +418,10 @@ func TestUploadFiles400Errors(t *testing.T) { for _, bundle := range files.Files { d.Required = append(d.Required, bundle.Sum) } - err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, true) - require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][401] uploadDeployFile default &{Code:401 Message: Unauthorized}") + // Set SkipRetry to false + err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false) + // require.Equal(t, err, "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message: Unprocessable Entity}") + require.Equal(t, attempts, 12) } func TestUploadFiles_SkipEqualFiles(t *testing.T) { From 75e7e4975724ba9d2e75ca5f14fb9ffc89681bc2 Mon Sep 17 00:00:00 2001 From: Jenae Janzen Date: Thu, 31 Aug 2023 18:54:18 -0400 Subject: [PATCH 16/20] move comment out of code --- go/porcelain/deploy.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/go/porcelain/deploy.go b/go/porcelain/deploy.go index 07b0de83..ec27025b 100644 --- a/go/porcelain/deploy.go +++ b/go/porcelain/deploy.go @@ -541,14 +541,6 @@ func (n *Netlify) uploadFile(ctx context.Context, d *models.Deploy, f *FileBundl context.GetLogger(ctx).WithError(operationError).Errorf("Failed to upload file %v", f.Name) apiErr, ok := operationError.(apierrors.Error) - // In testing, we repeatedly get to this line, see "Failed to upload file", but no matter - // what we try, ok is always `false`, and the code we're trying to test gets skipped. - // There isn't a test case for the original code (the 401 error) - - // The operation error we're receiving is "(*models.Error) is not supported by the TextConsumer, can be resolved by supporting TextUnmarshaler interface" - // which seems like a problem with how we're stubbing the response body from the server in the test, - // but it's similar to how it's stubbed in - if ok { if apiErr.Code() == 401 { sharedErr.mutex.Lock() From 107dbe6f6126bf4b9c1bd53bc52c841630de1368 Mon Sep 17 00:00:00 2001 From: Jenae Janzen Date: Mon, 18 Sep 2023 09:49:25 -0400 Subject: [PATCH 17/20] save changes --- go/porcelain/deploy_test.go | 70 +++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 26 deletions(-) diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 32270f07..61285498 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -330,35 +330,53 @@ func TestUploadFiles400Error_SkipsRetry(t *testing.T) { attempts++ }() + rw.Header().Set("Content-Type", "application/json; charset=utf-8") rw.WriteHeader(http.StatusUnprocessableEntity) + rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 422 }`)) + })) + defer server.Close() + + // File upload: + hu, _ := url.Parse(server.URL) + tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient) + client := NewRetryable(tr, strfmt.Default, 1) + client.uploadLimit = 1 + ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token")) + + // Create some files to deploy + dir, err := ioutil.TempDir("", "deploy") + require.NoError(t, err) + defer os.RemoveAll(dir) + require.NoError(t, ioutil.WriteFile(filepath.Join(dir, "foo.html"), []byte("Hello"), 0644)) + + files, err := walk(dir, nil, false, false) + require.NoError(t, err) + d := &models.Deploy{} + for _, bundle := range files.Files { + d.Required = append(d.Required, bundle.Sum) + } + // Set SkipRetry to true + err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, true) + require.Equal(t, err, "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message: Unprocessable Entity}") + require.Equal(t, attempts, 1) +} + +func TestUploadFunctions422Error_SkipsRetry(t *testing.T) { + attempts := 0 + ctx := gocontext.Background() + + server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) { + defer func() { + attempts++ + }() + rw.Header().Set("Content-Type", "application/json; charset=utf-8") + rw.WriteHeader(http.StatusUnprocessableEntity) rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 422 }`)) })) defer server.Close() - // // File upload: - // hu, _ := url.Parse(server.URL) - // tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient) - // client := NewRetryable(tr, strfmt.Default, 1) - // client.uploadLimit = 1 - // ctx = context.WithAuthInfo(ctx, apiClient.BearerToken("token")) - - // // Create some files to deploy - // dir, err := ioutil.TempDir("", "deploy") - // require.NoError(t, err) - // defer os.RemoveAll(dir) - // require.NoError(t, ioutil.WriteFile(filepath.Join(dir, "foo.html"), []byte("Hello"), 0644)) - - // files, err := walk(dir, nil, false, false) - // require.NoError(t, err) - // d := &models.Deploy{} - // for _, bundle := range files.Files { - // d.Required = append(d.Required, bundle.Sum) - // } - // // Set SkipRetry to true - // err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, true) - - // // Function upload: + // Function upload: hu, _ := url.Parse(server.URL) tr := apiClient.NewWithClient(hu.Host, "/api/v1", []string{"http"}, http.DefaultClient) client := NewRetryable(tr, strfmt.Default, 1) @@ -379,7 +397,7 @@ func TestUploadFiles400Error_SkipsRetry(t *testing.T) { d.RequiredFunctions = append(d.RequiredFunctions, bundle.Sum) } // Set SkipRetry to true - require.NoError(t, client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute, true)) + err = client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute, true) require.Equal(t, err, "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message: Unprocessable Entity}") require.Equal(t, attempts, 1) } @@ -393,8 +411,8 @@ func TestUploadFiles400Error_NoSkipRetry(t *testing.T) { attempts++ }() - rw.WriteHeader(http.StatusUnprocessableEntity) rw.Header().Set("Content-Type", "application/json; charset=utf-8") + rw.WriteHeader(http.StatusUnprocessableEntity) rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 422 }`)) return })) @@ -420,7 +438,7 @@ func TestUploadFiles400Error_NoSkipRetry(t *testing.T) { } // Set SkipRetry to false err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false) - // require.Equal(t, err, "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message: Unprocessable Entity}") + require.Equal(t, err, "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message: Unprocessable Entity}") require.Equal(t, attempts, 12) } From ff30a75bb082fe8aa6ae0d7957d84955709bcc88 Mon Sep 17 00:00:00 2001 From: Jenae Janzen Date: Tue, 19 Sep 2023 11:35:22 -0400 Subject: [PATCH 18/20] assert error --- go/porcelain/deploy_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 148ecca4..8e0aab01 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -398,7 +398,7 @@ func TestUploadFunctions422Error_SkipsRetry(t *testing.T) { } // Set SkipRetry to true err = client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute, true) - require.Equal(t, err, "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message: Unprocessable Entity}") + require.Error(t, err) require.Equal(t, attempts, 1) } @@ -413,7 +413,7 @@ func TestUploadFiles400Error_NoSkipRetry(t *testing.T) { rw.Header().Set("Content-Type", "application/json; charset=utf-8") rw.WriteHeader(http.StatusUnprocessableEntity) - rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 422 }`)) + rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 400 }`)) return })) defer server.Close() @@ -438,7 +438,7 @@ func TestUploadFiles400Error_NoSkipRetry(t *testing.T) { } // Set SkipRetry to false err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false) - require.Equal(t, err, "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message: Unprocessable Entity}") + require.Error(t, err) require.Equal(t, attempts, 12) } From 10097e758d24337e2957e3a36e3af4e62eac0aff Mon Sep 17 00:00:00 2001 From: Jenae Janzen Date: Tue, 19 Sep 2023 11:50:02 -0400 Subject: [PATCH 19/20] missed one --- go/porcelain/deploy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 8e0aab01..5ab0c7f2 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -357,7 +357,7 @@ func TestUploadFiles400Error_SkipsRetry(t *testing.T) { } // Set SkipRetry to true err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, true) - require.Equal(t, err, "[PUT /deploys/{deploy_id}/files/{path}][422] uploadDeployFile default &{Code:422 Message: Unprocessable Entity}") + require.Error(t, err) require.Equal(t, attempts, 1) } From 25f5b63b0dda80372215462ac26b381bfcd5a565 Mon Sep 17 00:00:00 2001 From: Jenae Janzen Date: Wed, 20 Sep 2023 07:31:06 -0400 Subject: [PATCH 20/20] fix tests --- go/porcelain/deploy_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/go/porcelain/deploy_test.go b/go/porcelain/deploy_test.go index 5ab0c7f2..3da66d4e 100644 --- a/go/porcelain/deploy_test.go +++ b/go/porcelain/deploy_test.go @@ -321,7 +321,7 @@ func TestUploadFiles_Errors(t *testing.T) { require.Equal(t, err.Error(), "[PUT /deploys/{deploy_id}/files/{path}][500] uploadDeployFile default &{Code:0 Message:}") } -func TestUploadFiles400Error_SkipsRetry(t *testing.T) { +func TestUploadFiles422Error_SkipsRetry(t *testing.T) { attempts := 0 ctx := gocontext.Background() @@ -357,7 +357,7 @@ func TestUploadFiles400Error_SkipsRetry(t *testing.T) { } // Set SkipRetry to true err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, true) - require.Error(t, err) + require.ErrorContains(t, err, "Code:422 Message:Unprocessable Entity") require.Equal(t, attempts, 1) } @@ -398,7 +398,7 @@ func TestUploadFunctions422Error_SkipsRetry(t *testing.T) { } // Set SkipRetry to true err = client.uploadFiles(apiCtx, d, files, nil, functionUpload, time.Minute, true) - require.Error(t, err) + require.ErrorContains(t, err, "Code:422 Message:Unprocessable Entity") require.Equal(t, attempts, 1) } @@ -412,8 +412,8 @@ func TestUploadFiles400Error_NoSkipRetry(t *testing.T) { }() rw.Header().Set("Content-Type", "application/json; charset=utf-8") - rw.WriteHeader(http.StatusUnprocessableEntity) - rw.Write([]byte(`{"message": "Unprocessable Entity", "code": 400 }`)) + rw.WriteHeader(http.StatusBadRequest) + rw.Write([]byte(`{"message": "Bad Request", "code": 400 }`)) return })) defer server.Close() @@ -438,8 +438,8 @@ func TestUploadFiles400Error_NoSkipRetry(t *testing.T) { } // Set SkipRetry to false err = client.uploadFiles(ctx, d, files, nil, fileUpload, time.Minute, false) - require.Error(t, err) - require.Equal(t, attempts, 12) + require.ErrorContains(t, err, "Code:400 Message:Bad Request") + require.Greater(t, attempts, 1) } func TestUploadFiles_SkipEqualFiles(t *testing.T) {