Merge pull request #3790 from thaJeztah/context_cleanup

context: various cleanups and improvements

Author: thaJeztah

cli/command/cli.go

@@ -28,6 +28,7 @@ import (
 	"github.com/docker/docker/api/types/registry"
 	"github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/client"
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/go-connections/tlsconfig"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
@@ -462,8 +463,8 @@ func resolveContextName(opts *cliflags.CommonOptions, config *configfile.ConfigF
 	}
 	if config != nil && config.CurrentContext != "" {
 		_, err := contextstore.GetMetadata(config.CurrentContext)
-		if store.IsErrContextDoesNotExist(err) {
-			return "", errors.Errorf("Current context %q is not found on the file system, please check your config file at %s", config.CurrentContext, config.Filename)
+		if errdefs.IsNotFound(err) {
+			return "", errors.Errorf("current context %q is not found on the file system, please check your config file at %s", config.CurrentContext, config.Filename)
 		}
 		return config.CurrentContext, err
 	}

cli/command/context/create.go

@@ -10,6 +10,7 @@ import (
 	"github.com/docker/cli/cli/command/formatter/tabwriter"
 	"github.com/docker/cli/cli/context/docker"
 	"github.com/docker/cli/cli/context/store"
+	"github.com/docker/docker/errdefs"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 )
@@ -127,7 +128,7 @@ func checkContextNameForCreation(s store.Reader, name string) error {
 	if err := store.ValidateContextName(name); err != nil {
 		return err
 	}
-	if _, err := s.GetMetadata(name); !store.IsErrContextDoesNotExist(err) {
+	if _, err := s.GetMetadata(name); !errdefs.IsNotFound(err) {
 		if err != nil {
 			return errors.Wrap(err, "error while getting existing contexts")
 		}

cli/command/context/remove.go

@@ -40,7 +40,7 @@ func RunRemove(dockerCli command.Cli, opts RemoveOptions, names []string) error
 		if name == "default" {
 			errs = append(errs, `default: context "default" cannot be removed`)
 		} else if err := doRemove(dockerCli, name, name == currentCtx, opts.Force); err != nil {
-			errs = append(errs, fmt.Sprintf("%s: %s", name, err))
+			errs = append(errs, err.Error())
 		} else {
 			fmt.Fprintln(dockerCli.Out(), name)
 		}
@@ -54,7 +54,7 @@ func RunRemove(dockerCli command.Cli, opts RemoveOptions, names []string) error
 func doRemove(dockerCli command.Cli, name string, isCurrent, force bool) error {
 	if isCurrent {
 		if !force {
-			return errors.New("context is in use, set -f flag to force remove")
+			return errors.Errorf("context %q is in use, set -f flag to force remove", name)
 		}
 		// fallback to DOCKER_HOST
 		cfg := dockerCli.ConfigFile()

cli/command/context/remove_test.go

@@ -6,8 +6,9 @@ import (
 
 	"github.com/docker/cli/cli/config"
 	"github.com/docker/cli/cli/config/configfile"
-	"github.com/docker/cli/cli/context/store"
+	"github.com/docker/docker/errdefs"
 	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
 )
 
 func TestRemove(t *testing.T) {
@@ -18,7 +19,7 @@ func TestRemove(t *testing.T) {
 	_, err := cli.ContextStore().GetMetadata("current")
 	assert.NilError(t, err)
 	_, err = cli.ContextStore().GetMetadata("other")
-	assert.Check(t, store.IsErrContextDoesNotExist(err))
+	assert.Check(t, is.ErrorType(err, errdefs.IsNotFound))
 }
 
 func TestRemoveNotAContext(t *testing.T) {
@@ -38,7 +39,7 @@ func TestRemoveCurrent(t *testing.T) {
 	createTestContext(t, cli, "other")
 	cli.SetCurrentContext("current")
 	err := RunRemove(cli, RemoveOptions{}, []string{"current"})
-	assert.ErrorContains(t, err, "current: context is in use, set -f flag to force remove")
+	assert.ErrorContains(t, err, `context "current" is in use, set -f flag to force remove`)
 }
 
 func TestRemoveCurrentForce(t *testing.T) {

cli/command/context/use_test.go

@@ -11,8 +11,8 @@ import (
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/config"
 	"github.com/docker/cli/cli/config/configfile"
-	"github.com/docker/cli/cli/context/store"
 	"github.com/docker/cli/cli/flags"
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/pkg/homedir"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
@@ -47,7 +47,7 @@ func TestUse(t *testing.T) {
 func TestUseNoExist(t *testing.T) {
 	cli := makeFakeCli(t)
 	err := newUseCommand(cli).RunE(nil, []string{"test"})
-	assert.Check(t, store.IsErrContextDoesNotExist(err))
+	assert.Check(t, is.ErrorType(err, errdefs.IsNotFound))
 }
 
 // TestUseDefaultWithoutConfigFile verifies that the CLI does not create

cli/command/defaultcontextstore.go

@@ -1,11 +1,10 @@
 package command
 
 import (
-	"fmt"
-
 	"github.com/docker/cli/cli/context/docker"
 	"github.com/docker/cli/cli/context/store"
 	cliflags "github.com/docker/cli/cli/flags"
+	"github.com/docker/docker/errdefs"
 	"github.com/pkg/errors"
 )
 
@@ -107,15 +106,15 @@ func (s *ContextStoreWithDefault) List() ([]store.Metadata, error) {
 // CreateOrUpdate is not allowed for the default context and fails
 func (s *ContextStoreWithDefault) CreateOrUpdate(meta store.Metadata) error {
 	if meta.Name == DefaultContextName {
-		return errors.New("default context cannot be created nor updated")
+		return errdefs.InvalidParameter(errors.New("default context cannot be created nor updated"))
 	}
 	return s.Store.CreateOrUpdate(meta)
 }
 
 // Remove is not allowed for the default context and fails
 func (s *ContextStoreWithDefault) Remove(name string) error {
 	if name == DefaultContextName {
-		return errors.New("default context cannot be removed")
+		return errdefs.InvalidParameter(errors.New("default context cannot be removed"))
 	}
 	return s.Store.Remove(name)
 }
@@ -135,15 +134,15 @@ func (s *ContextStoreWithDefault) GetMetadata(name string) (store.Metadata, erro
 // ResetTLSMaterial is not implemented for default context and fails
 func (s *ContextStoreWithDefault) ResetTLSMaterial(name string, data *store.ContextTLSData) error {
 	if name == DefaultContextName {
-		return errors.New("The default context store does not support ResetTLSMaterial")
+		return errdefs.InvalidParameter(errors.New("default context cannot be edited"))
 	}
 	return s.Store.ResetTLSMaterial(name, data)
 }
 
 // ResetEndpointTLSMaterial is not implemented for default context and fails
 func (s *ContextStoreWithDefault) ResetEndpointTLSMaterial(contextName string, endpointName string, data *store.EndpointTLSData) error {
 	if contextName == DefaultContextName {
-		return errors.New("The default context store does not support ResetEndpointTLSMaterial")
+		return errdefs.InvalidParameter(errors.New("default context cannot be edited"))
 	}
 	return s.Store.ResetEndpointTLSMaterial(contextName, endpointName, data)
 }
@@ -176,29 +175,13 @@ func (s *ContextStoreWithDefault) GetTLSData(contextName, endpointName, fileName
 			return nil, err
 		}
 		if defaultContext.TLS.Endpoints[endpointName].Files[fileName] == nil {
-			return nil, &noDefaultTLSDataError{endpointName: endpointName, fileName: fileName}
+			return nil, errdefs.NotFound(errors.Errorf("TLS data for %s/%s/%s does not exist", DefaultContextName, endpointName, fileName))
 		}
 		return defaultContext.TLS.Endpoints[endpointName].Files[fileName], nil
-
 	}
 	return s.Store.GetTLSData(contextName, endpointName, fileName)
 }
 
-type noDefaultTLSDataError struct {
-	endpointName string
-	fileName     string
-}
-
-func (e *noDefaultTLSDataError) Error() string {
-	return fmt.Sprintf("tls data for %s/%s/%s does not exist", DefaultContextName, e.endpointName, e.fileName)
-}
-
-// NotFound satisfies interface github.com/docker/docker/errdefs.ErrNotFound
-func (e *noDefaultTLSDataError) NotFound() {}
-
-// IsTLSDataDoesNotExist satisfies github.com/docker/cli/cli/context/store.tlsDataDoesNotExist
-func (e *noDefaultTLSDataError) IsTLSDataDoesNotExist() {}
-
 // GetStorageInfo implements store.Store's GetStorageInfo
 func (s *ContextStoreWithDefault) GetStorageInfo(contextName string) store.StorageInfo {
 	if contextName == DefaultContextName {

cli/command/defaultcontextstore_test.go

@@ -8,8 +8,10 @@ import (
 	"github.com/docker/cli/cli/context/docker"
 	"github.com/docker/cli/cli/context/store"
 	cliflags "github.com/docker/cli/cli/flags"
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/go-connections/tlsconfig"
 	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
 	"gotest.tools/v3/golden"
 )
 
@@ -153,19 +155,21 @@ func TestErrCreateDefault(t *testing.T) {
 		Metadata: testContext{Bar: "baz"},
 		Name:     "default",
 	})
+	assert.Check(t, is.ErrorType(err, errdefs.IsInvalidParameter))
 	assert.Error(t, err, "default context cannot be created nor updated")
 }
 
 func TestErrRemoveDefault(t *testing.T) {
 	meta := testDefaultMetadata()
 	s := testStore(t, meta, store.ContextTLSData{})
 	err := s.Remove("default")
+	assert.Check(t, is.ErrorType(err, errdefs.IsInvalidParameter))
 	assert.Error(t, err, "default context cannot be removed")
 }
 
 func TestErrTLSDataError(t *testing.T) {
 	meta := testDefaultMetadata()
 	s := testStore(t, meta, store.ContextTLSData{})
 	_, err := s.GetTLSData("default", "noop", "noop")
-	assert.Check(t, store.IsErrTLSDataDoesNotExist(err))
+	assert.Check(t, is.ErrorType(err, errdefs.IsNotFound))
 }

cli/context/store/metadata_test.go

@@ -5,6 +5,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/docker/docker/errdefs"
 	"gotest.tools/v3/assert"
 	"gotest.tools/v3/assert/cmp"
 )
@@ -22,7 +23,7 @@ func testMetadata(name string) Metadata {
 func TestMetadataGetNotExisting(t *testing.T) {
 	testee := metadataStore{root: t.TempDir(), config: testCfg}
 	_, err := testee.get("noexist")
-	assert.Assert(t, IsErrContextDoesNotExist(err))
+	assert.ErrorType(t, err, errdefs.IsNotFound)
 }
 
 func TestMetadataCreateGetRemove(t *testing.T) {
@@ -41,22 +42,22 @@ func TestMetadataCreateGetRemove(t *testing.T) {
 	assert.NilError(t, err)
 	// create a new instance to check it does not depend on some sort of state
 	testee = metadataStore{root: testDir, config: testCfg}
-	meta, err := testee.get(contextdirOf("test-context"))
+	meta, err := testee.get("test-context")
 	assert.NilError(t, err)
 	assert.DeepEqual(t, meta, testMeta)
 
 	// update
 
 	err = testee.createOrUpdate(expected2)
 	assert.NilError(t, err)
-	meta, err = testee.get(contextdirOf("test-context"))
+	meta, err = testee.get("test-context")
 	assert.NilError(t, err)
 	assert.DeepEqual(t, meta, expected2)
 
-	assert.NilError(t, testee.remove(contextdirOf("test-context")))
-	assert.NilError(t, testee.remove(contextdirOf("test-context"))) // support duplicate remove
-	_, err = testee.get(contextdirOf("test-context"))
-	assert.Assert(t, IsErrContextDoesNotExist(err))
+	assert.NilError(t, testee.remove("test-context"))
+	assert.NilError(t, testee.remove("test-context")) // support duplicate remove
+	_, err = testee.get("test-context")
+	assert.ErrorType(t, err, errdefs.IsNotFound)
 }
 
 func TestMetadataRespectJsonAnnotation(t *testing.T) {
@@ -121,7 +122,7 @@ func TestWithEmbedding(t *testing.T) {
 		},
 	}
 	assert.NilError(t, testee.createOrUpdate(Metadata{Metadata: testCtxMeta, Name: "test"}))
-	res, err := testee.get(contextdirOf("test"))
+	res, err := testee.get("test")
 	assert.NilError(t, err)
 	assert.Equal(t, testCtxMeta, res.Metadata)
 }

cli/context/store/metadatastore.go

@@ -2,13 +2,14 @@ package store
 
 import (
 	"encoding/json"
-	"fmt"
 	"os"
 	"path/filepath"
 	"reflect"
 	"sort"
 
+	"github.com/docker/docker/errdefs"
 	"github.com/fvbommel/sortorder"
+	"github.com/pkg/errors"
 )
 
 const (
@@ -55,11 +56,21 @@ func parseTypedOrMap(payload []byte, getter TypeGetter) (interface{}, error) {
 	return reflect.ValueOf(typed).Elem().Interface(), nil
 }
 
-func (s *metadataStore) get(id contextdir) (Metadata, error) {
-	contextDir := s.contextDir(id)
-	bytes, err := os.ReadFile(filepath.Join(contextDir, metaFile))
+func (s *metadataStore) get(name string) (Metadata, error) {
+	m, err := s.getByID(contextdirOf(name))
 	if err != nil {
-		return Metadata{}, convertContextDoesNotExist(err)
+		return m, errors.Wrapf(err, "load context %q", name)
+	}
+	return m, nil
+}
+
+func (s *metadataStore) getByID(id contextdir) (Metadata, error) {
+	bytes, err := os.ReadFile(filepath.Join(s.contextDir(id), metaFile))
+	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			return Metadata{}, errdefs.NotFound(errors.Wrap(err, "context does not exist"))
+		}
+		return Metadata{}, err
 	}
 	var untyped untypedContextMetadata
 	r := Metadata{
@@ -80,9 +91,11 @@ func (s *metadataStore) get(id contextdir) (Metadata, error) {
 	return r, err
 }
 
-func (s *metadataStore) remove(id contextdir) error {
-	contextDir := s.contextDir(id)
-	return os.RemoveAll(contextDir)
+func (s *metadataStore) remove(name string) error {
+	if err := os.RemoveAll(s.contextDir(contextdirOf(name))); err != nil {
+		return errors.Wrapf(err, "failed to remove metadata")
+	}
+	return nil
 }
 
 func (s *metadataStore) list() ([]Metadata, error) {
@@ -95,9 +108,12 @@ func (s *metadataStore) list() ([]Metadata, error) {
 	}
 	var res []Metadata
 	for _, dir := range ctxDirs {
-		c, err := s.get(contextdir(dir))
+		c, err := s.getByID(contextdir(dir))
 		if err != nil {
-			return nil, err
+			if os.IsNotExist(err) {
+				continue
+			}
+			return nil, errors.Wrap(err, "failed to read metadata")
 		}
 		res = append(res, c)
 	}
@@ -131,20 +147,13 @@ func listRecursivelyMetadataDirs(root string) ([]string, error) {
 				return nil, err
 			}
 			for _, s := range subs {
-				result = append(result, fmt.Sprintf("%s/%s", fi.Name(), s))
+				result = append(result, filepath.Join(fi.Name(), s))
 			}
 		}
 	}
 	return result, nil
 }
 
-func convertContextDoesNotExist(err error) error {
-	if os.IsNotExist(err) {
-		return &contextDoesNotExistError{}
-	}
-	return err
-}
-
 type untypedContextMetadata struct {
 	Metadata  json.RawMessage            `json:"metadata,omitempty"`
 	Endpoints map[string]json.RawMessage `json:"endpoints,omitempty"`