Repository: supabase/cli
Pull request #713 (2022-12-17 13:25:48)

fix: unify image pull with retry implementation
## What kind of change does this PR introduce?

regression from #709 

## What is the current behavior?

Rate limit bug reappeared https://github.com/supabase/setup-cli/actions/runs/3717908241/jobs/6305704807

## What is the new behavior?

Use a single pull with retry method

## Additional context

Add any other context or screenshots.


coveralls: ## Pull Request Test Coverage Report for [Build 3720249265](https://coveralls.io/builds/55251935)

* **17** of **23**   **(73.91%)**  changed or added relevant lines in **4** files are covered.
* **2** unchanged lines in **2** files lost coverage.
* Overall coverage increased (+**0.2%**) to **56.501%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [internal/db/remote/changes/changes.go](https://coveralls.io/builds/55251935/source?filename=internal%2Fdb%2Fremote%2Fchanges%2Fchanges.go#L102) | 0 | 3 | 0.0%
| [internal/db/remote/commit/commit.go](https://coveralls.io/builds/55251935/source?filename=internal%2Fdb%2Fremote%2Fcommit%2Fcommit.go#L143) | 0 | 3 | 0.0%
<!-- | **Total:** | **17** | **23** | **73.91%** | -->

|  Files with Coverage Reduction | New Missed Lines | % |
| :-----|--------------|--: |
| [internal/db/remote/changes/changes.go](https://coveralls.io/builds/55251935/source?filename=internal%2Fdb%2Fremote%2Fchanges%2Fchanges.go#L105) | 1 | 0% |
| [internal/db/remote/commit/commit.go](https://coveralls.io/builds/55251935/source?filename=internal%2Fdb%2Fremote%2Fcommit%2Fcommit.go#L146) | 1 | 0% |
<!-- | **Total:** | **2** |  | -->

|  Totals | [![Coverage Status](https://coveralls.io/builds/55251935/badge)](https://coveralls.io/builds/55251935) |
| :-- | --: |
| Change from base [Build 3719923634](https://coveralls.io/builds/55251619): |  0.2% |
| Covered Lines: | 3381 |
| Relevant Lines: | 5984 |

---
##### 💛  - [Coveralls](https://coveralls.io)


Patch:
diff --git a/internal/db/remote/changes/changes.go b/internal/db/remote/changes/changes.go
index 0bfc0306..ae5f4822 100644
--- a/internal/db/remote/changes/changes.go
+++ b/internal/db/remote/changes/changes.go
@@ -99,26 +99,9 @@ func run(p utils.Program, ctx context.Context, username, password, database stri
 	p.Send(utils.StatusMsg("Pulling images..."))
 
 	// Pull images.
-	{
-		dbImage := utils.GetRegistryImageUrl(utils.DbImage)
-		if _, _, err := utils.Docker.ImageInspectWithRaw(ctx, dbImage); err != nil {
-			out, err := utils.DockerImagePullWithRetry(ctx, dbImage, 2)
-			if err != nil {
-				return err
-			}
-			if err := utils.ProcessPullOutput(out, p); err != nil {
-				return err
-			}
-		}
-		diffImage := utils.GetRegistryImageUrl(utils.DifferImage)
-		if _, _, err := utils.Docker.ImageInspectWithRaw(ctx, diffImage); err != nil {
-			out, err := utils.DockerImagePullWithRetry(ctx, diffImage, 2)
-			if err != nil {
-				return err
-			}
-			if err := utils.ProcessPullOutput(out, p); err != nil {
-				return err
-			}
+	for _, image := range []string{utils.DbImage, utils.DifferImage} {
+		if err := utils.DockerPullImageIfNotCached(ctx, image); err != nil {
+			return err
 		}
 	}
 
diff --git a/internal/db/remote/commit/commit.go b/internal/db/remote/commit/commit.go
index afc76835..a57ac0d7 100644
--- a/internal/db/remote/commit/commit.go
+++ b/internal/db/remote/commit/commit.go
@@ -140,26 +140,9 @@ func run(p utils.Program, ctx context.Context, username, password, database stri
 	p.Send(utils.StatusMsg("Pulling images..."))
 
 	// Pull images.
-	{
-		dbImage := utils.GetRegistryImageUrl(utils.DbImage)
-		if _, _, err := utils.Docker.ImageInspectWithRaw(ctx, dbImage); err != nil {
-			out, err := utils.DockerImagePullWithRetry(ctx, dbImage, 2)
-			if err != nil {
-				return err
-			}
-			if err := utils.ProcessPullOutput(out, p); err != nil {
-				return err
-			}
-		}
-		diffImage := utils.GetRegistryImageUrl(utils.DifferImage)
-		if _, _, err := utils.Docker.ImageInspectWithRaw(ctx, diffImage); err != nil {
-			out, err := utils.DockerImagePullWithRetry(ctx, diffImage, 2)
-			if err != nil {
-				return err
-			}
-			if err := utils.ProcessPullOutput(out, p); err != nil {
-				return err
-			}
+	for _, image := range []string{utils.DbImage, utils.DifferImage} {
+		if err := utils.DockerPullImageIfNotCached(ctx, image); err != nil {
+			return err
 		}
 	}
 
diff --git a/internal/start/start.go b/internal/start/start.go
index 0b6336d9..d11ff57f 100644
--- a/internal/start/start.go
+++ b/internal/start/start.go
@@ -6,7 +6,6 @@ import (
 	_ "embed"
 	"errors"
 	"fmt"
-	"io"
 	"os"
 	"strconv"
 	"strings"
@@ -16,9 +15,7 @@ import (
 	"github.com/charmbracelet/bubbles/spinner"
 	tea "github.com/charmbracelet/bubbletea"
 	"github.com/charmbracelet/lipgloss"
-	"github.com/docker/cli/cli/streams"
 	"github.com/docker/docker/api/types/container"
-	"github.com/docker/docker/pkg/jsonmessage"
 	"github.com/docker/go-connections/nat"
 	"github.com/jackc/pgx/v4"
 	"github.com/muesli/reflow/wrap"
@@ -84,35 +81,12 @@ func Run(ctx context.Context, fsys afero.Fs, excludedContainers []string) error
 }
 
 var (
-	pullRetry = 2
 	// TODO: Unhardcode keys
 	//go:embed templates/kong_config
 	kongConfigEmbed    string
 	kongConfigTemplate = template.Must(template.New("kongConfig").Parse(kongConfigEmbed))
 )
 
-func pullImage(p utils.Program, ctx context.Context, image string) error {
-	imageUrl := utils.GetRegistryImageUrl(image)
-	_, _, err := utils.Docker.ImageInspectWithRaw(ctx, imageUrl)
-	for i := 0; i < 3; i++ {
-		if err == nil {
-			break
-		}
-		var out io.ReadCloser
-		out, err = utils.DockerImagePullWithRetry(ctx, imageUrl, pullRetry)
-		if err != nil {
-			break
-		}
-		defer out.Close()
-		err = jsonmessage.DisplayJSONMessagesToStream(out, streams.NewOut(os.Stderr), nil)
-		if err != nil {
-			break
-		}
-		_, _, err = utils.Docker.ImageInspectWithRaw(ctx, imageUrl)
-	}
-	return err
-}
-
 func run(p utils.Program, ctx context.Context, fsys afero.Fs, excludedContainers []string, options ...func(*pgx.ConnConfig)) error {
 	if err := utils.DockerNetworkCreateIfNotExists(ctx, utils.NetId); err != nil {
 		return err
@@ -127,7 +101,7 @@ func run(p utils.Program, ctx context.Context, fsys afero.Fs, excludedContainers
 	{
 		total := len(utils.ServiceImages) + 1
 		p.Send(utils.StatusMsg(fmt.Sprintf("Pulling images... (0/%d)", total)))
-		if err := pullImage(p, ctx, utils.DbImage); err != nil {
+		if err := utils.DockerPullImageIfNotCached(ctx, utils.DbImage); err != nil {
 			return err
 		}
 		p.Send(utils.StatusMsg(fmt.Sprintf("Pulling images... (1/%d)", total)))
@@ -136,7 +110,7 @@ func run(p utils.Program, ctx context.Context, fsys afero.Fs, excludedContainers
 				fmt.Fprintln(os.Stderr, "Excluding container:", image)
 				continue
 			}
-			if err := pullImage(p, ctx, image); err != nil {
+			if err := utils.DockerPullImageIfNotCached(ctx, image); err != nil {
 				return err
 			}
 			p.Send(utils.StatusMsg(fmt.Sprintf("Pulling images... (%d/%d)", i+1, total)))
diff --git a/internal/utils/docker.go b/internal/utils/docker.go
index dd8f1e21..38ebe948 100644
--- a/internal/utils/docker.go
+++ b/internal/utils/docker.go
@@ -223,23 +223,33 @@ func GetRegistryImageUrl(imageName string) string {
 	return registry + "/supabase/" + imageName
 }
 
-func DockerImagePullWithRetry(ctx context.Context, image string, retries int) (io.ReadCloser, error) {
+func DockerImagePull(ctx context.Context, image string, w io.Writer) error {
 	out, err := Docker.ImagePull(ctx, image, types.ImagePullOptions{
 		RegistryAuth: GetRegistryAuth(),
 	})
-	for i := time.Duration(1); retries > 0; retries-- {
+	if err != nil {
+		return err
+	}
+	defer out.Close()
+	return jsonmessage.DisplayJSONMessagesToStream(out, streams.NewOut(w), nil)
+}
+
+// Used by unit tests
+var timeUnit = time.Second
+
+func DockerImagePullWithRetry(ctx context.Context, image string, retries int) error {
+	err := DockerImagePull(ctx, image, os.Stderr)
+	for i := 0; i < retries; i++ {
 		if err == nil {
 			break
 		}
 		fmt.Fprintln(os.Stderr, err)
-		fmt.Fprintf(os.Stderr, "Retrying after %d seconds...\n", i)
-		time.Sleep(i * time.Second)
-		out, err = Docker.ImagePull(ctx, image, types.ImagePullOptions{
-			RegistryAuth: GetRegistryAuth(),
-		})
-		i *= 2
-	}
-	return out, err
+		period := time.Duration(2<<(i+1)) * timeUnit
+		fmt.Fprintf(os.Stderr, "Retrying after %v: %s\n", period, image)
+		time.Sleep(period)
+		err = DockerImagePull(ctx, image, os.Stderr)
+	}
+	return err
 }
 
 func DockerPullImageIfNotCached(ctx context.Context, imageName string) error {
@@ -249,12 +259,7 @@ func DockerPullImageIfNotCached(ctx context.Context, imageName string) error {
 	} else if !client.IsErrNotFound(err) {
 		return err
 	}
-	out, err := DockerImagePullWithRetry(ctx, imageUrl, 2)
-	if err != nil {
-		return err
-	}
-	defer out.Close()
-	return jsonmessage.DisplayJSONMessagesToStream(out, streams.NewOut(os.Stderr), nil)
+	return DockerImagePullWithRetry(ctx, imageUrl, 2)
 }
 
 func DockerStop(containerID string) {


Test patch:
diff --git a/internal/start/start_test.go b/internal/start/start_test.go
index b9610ec5..b1107be4 100644
--- a/internal/start/start_test.go
+++ b/internal/start/start_test.go
@@ -7,7 +7,6 @@ import (
 	"testing"
 
 	"github.com/docker/docker/api/types"
-	"github.com/docker/docker/pkg/jsonmessage"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -116,95 +115,6 @@ func TestStartCommand(t *testing.T) {
 	})
 }
 
-func TestPullImage(t *testing.T) {
-	const image = "postgres"
-	imageUrl := utils.GetRegistryImageUrl(image)
-	p := utils.NewProgram(model{})
-
-	t.Run("inspects image before pull", func(t *testing.T) {
-		// Setup mock docker
-		require.NoError(t, apitest.MockDocker(utils.Docker))
-		defer gock.OffAll()
-		gock.New(utils.Docker.DaemonHost()).
-			Get("/v" + utils.Docker.ClientVersion() + "/images/" + imageUrl + "/json").
-			Reply(http.StatusOK).
-			JSON(types.ImageInspect{})
-		// Run test
-		err := pullImage(p, context.Background(), image)
-		// Check error
-		assert.NoError(t, err)
-		assert.Empty(t, apitest.ListUnmatchedRequests())
-	})
-
-	t.Run("pulls missing image", func(t *testing.T) {
-		// Setup mock docker
-		require.NoError(t, apitest.MockDocker(utils.Docker))
-		defer gock.OffAll()
-		gock.New(utils.Docker.DaemonHost()).
-			Get("/v" + utils.Docker.ClientVersion() + "/images/" + imageUrl + "/json").
-			Reply(http.StatusNotFound)
-		gock.New(utils.Docker.DaemonHost()).
-			Post("/v"+utils.Docker.ClientVersion()+"/images/create").
-			MatchParam("fromImage", image).
-			MatchParam("tag", "latest").
-			Reply(http.StatusAccepted).
-			BodyString("")
-		gock.New(utils.Docker.DaemonHost()).
-			Get("/v" + utils.Docker.ClientVersion() + "/images/" + imageUrl + "/json").
-			Reply(http.StatusOK).
-			JSON(types.ImageInspect{})
-		// Run test
-		err := pullImage(p, context.Background(), image)
-		// Check error
-		assert.NoError(t, err)
-		assert.Empty(t, apitest.ListUnmatchedRequests())
-	})
-
-	t.Run("throws error on pull failure", func(t *testing.T) {
-		pullRetry = 0
-		// Setup mock docker
-		require.NoError(t, apitest.MockDocker(utils.Docker))
-		defer gock.OffAll()
-		gock.New(utils.Docker.DaemonHost()).
-			Get("/v" + utils.Docker.ClientVersion() + "/images/" + imageUrl + "/json").
-			Reply(http.StatusNotFound)
-		gock.New(utils.Docker.DaemonHost()).
-			Post("/v"+utils.Docker.ClientVersion()+"/images/create").
-			MatchParam("fromImage", image).
-			MatchParam("tag", "latest").
-			Reply(http.StatusServiceUnavailable)
-		// Run test
-		err := pullImage(p, context.Background(), image)
-		// Check error
-		assert.ErrorContains(t, err, "request returned Service Unavailable")
-		assert.Empty(t, apitest.ListUnmatchedRequests())
-	})
-
-	t.Run("throws error on no space left", func(t *testing.T) {
-		// Setup mock docker
-		require.NoError(t, apitest.MockDocker(utils.Docker))
-		defer gock.OffAll()
-		gock.New(utils.Docker.DaemonHost()).
-			Get("/v" + utils.Docker.ClientVersion() + "/images/" + imageUrl + "/json").
-			Reply(http.StatusNotFound)
-		gock.New(utils.Docker.DaemonHost()).
-			Post("/v"+utils.Docker.ClientVersion()+"/images/create").
-			MatchParam("fromImage", image).
-			MatchParam("tag", "latest").
-			Reply(http.StatusAccepted).
-			JSON(jsonmessage.JSONMessage{Error: &jsonmessage.JSONError{Message: "no space left on device"}})
-		gock.New(utils.Docker.DaemonHost()).
-			Get("/v" + utils.Docker.ClientVersion() + "/images/" + imageUrl + "/json").
-			Reply(http.StatusOK).
-			JSON(types.ImageInspect{})
-		// Run test
-		err := pullImage(p, context.Background(), image)
-		// Check error
-		assert.ErrorContains(t, err, "no space left on device")
-		assert.Empty(t, apitest.ListUnmatchedRequests())
-	})
-}
-
 func TestDatabaseStart(t *testing.T) {
 	p := utils.NewProgram(model{})
 
diff --git a/internal/utils/docker_test.go b/internal/utils/docker_test.go
index dff8c223..08e67fe9 100644
--- a/internal/utils/docker_test.go
+++ b/internal/utils/docker_test.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
+	"github.com/docker/docker/pkg/jsonmessage"
 	"github.com/docker/docker/pkg/stdcopy"
 	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
@@ -72,6 +73,7 @@ func TestPullImage(t *testing.T) {
 	})
 
 	t.Run("throws error on failure to pull image", func(t *testing.T) {
+		timeUnit = time.Duration(0)
 		// Setup mock docker
 		require.NoError(t, apitest.MockDocker(Docker))
 		defer gock.OffAll()
@@ -88,15 +90,18 @@ func TestPullImage(t *testing.T) {
 			Post("/v"+Docker.ClientVersion()+"/images/create").
 			MatchParam("fromImage", imageId).
 			MatchParam("tag", "latest").
-			Reply(http.StatusServiceUnavailable)
+			Reply(http.StatusAccepted).
+			JSON(jsonmessage.JSONMessage{Error: &jsonmessage.JSONError{Message: "toomanyrequests"}})
 		gock.New(Docker.DaemonHost()).
 			Post("/v"+Docker.ClientVersion()+"/images/create").
 			MatchParam("fromImage", imageId).
 			MatchParam("tag", "latest").
-			Reply(http.StatusServiceUnavailable)
+			Reply(http.StatusAccepted).
+			JSON(jsonmessage.JSONMessage{Error: &jsonmessage.JSONError{Message: "no space left on device"}})
 		// Run test
-		assert.Error(t, DockerPullImageIfNotCached(context.Background(), imageId))
+		err := DockerPullImageIfNotCached(context.Background(), imageId)
 		// Validate api
+		assert.ErrorContains(t, err, "no space left on device")
 		assert.Empty(t, apitest.ListUnmatchedRequests())
 	})
 }