From 87338bf0fa97c905d2d707393c95f73aea398cad Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Mon, 18 Jan 2016 14:17:39 -0800 Subject: [PATCH] Revert reporting of multiple pull errors Revert the portions of #17617 that report all errors when a pull falls back, and go back to just reporting the last error. This was nice to have, but causes some UX issues because nonexistent images show additional "unauthorized" errors. Keep the part of the PR that handled ENOSPC, as this appears to work even without tracking multiple errors. Fixes #19419 Signed-off-by: Aaron Lehmann --- distribution/pull.go | 30 +++++++------------ integration-cli/docker_cli_pull_local_test.go | 12 -------- integration-cli/docker_cli_pull_test.go | 1 + 3 files changed, 12 insertions(+), 31 deletions(-) diff --git a/distribution/pull.go b/distribution/pull.go index 5f38a67673..ab8c14ce81 100644 --- a/distribution/pull.go +++ b/distribution/pull.go @@ -3,7 +3,6 @@ package distribution import ( "fmt" "os" - "strings" "github.com/Sirupsen/logrus" "github.com/docker/docker/api" @@ -97,13 +96,12 @@ func Pull(ctx context.Context, ref reference.Named, imagePullConfig *ImagePullCo } var ( - // use a slice to append the error strings and return a joined string to caller - errors []string + lastErr error // discardNoSupportErrors is used to track whether an endpoint encountered an error of type registry.ErrNoSupport - // By default it is false, which means that if a ErrNoSupport error is encountered, it will be saved in errors. + // By default it is false, which means that if a ErrNoSupport error is encountered, it will be saved in lastErr. // As soon as another kind of error is encountered, discardNoSupportErrors is set to true, avoiding the saving of - // any subsequent ErrNoSupport errors in errors. + // any subsequent ErrNoSupport errors in lastErr. // It's needed for pull-by-digest on v1 endpoints: if there are only v1 endpoints configured, the error should be // returned and displayed, but if there was a v2 endpoint which supports pull-by-digest, then the last relevant // error is the ones from v2 endpoints not v1. @@ -123,7 +121,7 @@ func Pull(ctx context.Context, ref reference.Named, imagePullConfig *ImagePullCo puller, err := newPuller(endpoint, repoInfo, imagePullConfig) if err != nil { - errors = append(errors, err.Error()) + lastErr = err continue } if err := puller.Pull(ctx, ref); err != nil { @@ -144,34 +142,28 @@ func Pull(ctx context.Context, ref reference.Named, imagePullConfig *ImagePullCo // Because we found an error that's not ErrNoSupport, discard all subsequent ErrNoSupport errors. discardNoSupportErrors = true // append subsequent errors - errors = append(errors, err.Error()) + lastErr = err } else if !discardNoSupportErrors { // Save the ErrNoSupport error, because it's either the first error or all encountered errors // were also ErrNoSupport errors. // append subsequent errors - errors = append(errors, err.Error()) + lastErr = err } continue } - errors = append(errors, err.Error()) - logrus.Debugf("Not continuing with error: %v", fmt.Errorf(strings.Join(errors, "\n"))) - if len(errors) > 0 { - return fmt.Errorf(strings.Join(errors, "\n")) - } + logrus.Debugf("Not continuing with error: %v", err) + return err } imagePullConfig.ImageEventLogger(ref.String(), repoInfo.Name(), "pull") return nil } - if len(errors) == 0 { - return fmt.Errorf("no endpoints found for %s", ref.String()) + if lastErr == nil { + lastErr = fmt.Errorf("no endpoints found for %s", ref.String()) } - if len(errors) > 0 { - return fmt.Errorf(strings.Join(errors, "\n")) - } - return nil + return lastErr } // writeStatus writes a status message to out. If layersDownloaded is true, the diff --git a/integration-cli/docker_cli_pull_local_test.go b/integration-cli/docker_cli_pull_local_test.go index 1037951d24..768bb8172e 100644 --- a/integration-cli/docker_cli_pull_local_test.go +++ b/integration-cli/docker_cli_pull_local_test.go @@ -279,18 +279,6 @@ func (s *DockerSchema1RegistrySuite) TestPullIDStability(c *check.C) { testPullIDStability(c) } -// TestPullFallbackOn404 tries to pull a nonexistent manifest and confirms that -// the pull falls back to the v1 protocol. -// -// Ref: docker/docker#18832 -func (s *DockerRegistrySuite) TestPullFallbackOn404(c *check.C) { - repoName := fmt.Sprintf("%v/does/not/exist", privateRegistryURL) - - out, _, _ := dockerCmdWithError("pull", repoName) - - c.Assert(out, checker.Contains, "v1 ping attempt") -} - func (s *DockerRegistrySuite) TestPullManifestList(c *check.C) { pushDigest, err := setupImage(c) c.Assert(err, checker.IsNil, check.Commentf("error setting up image")) diff --git a/integration-cli/docker_cli_pull_test.go b/integration-cli/docker_cli_pull_test.go index 0c06499e29..9d36296091 100644 --- a/integration-cli/docker_cli_pull_test.go +++ b/integration-cli/docker_cli_pull_test.go @@ -62,6 +62,7 @@ func (s *DockerHubPullSuite) TestPullNonExistingImage(c *check.C) { out, err := s.CmdWithError("pull", "-a", e.Alias) c.Assert(err, checker.NotNil, check.Commentf("expected non-zero exit status when pulling non-existing image: %s", out)) c.Assert(out, checker.Contains, fmt.Sprintf("Error: image %s not found", e.Repo), check.Commentf("expected image not found error messages")) + c.Assert(out, checker.Not(checker.Contains), "unauthorized", check.Commentf(`message should not contain "unauthorized"`)) } }