Skip to content

Commit

Permalink
Always enable experimental features
Browse files Browse the repository at this point in the history
The CLI disabled experimental features by default, requiring users
to set a configuration option to enable them.

Disabling experimental features was a request from Enterprise users
that did not want experimental features to be accessible.

We are changing this policy, and now enable experimental features
by default. Experimental features may still change and/or removed,
and will be highlighted in the documentation and "usage" output.

For example, the `docker manifest inspect --help` output now shows:

    EXPERIMENTAL:
      docker manifest inspect is an experimental feature.

      Experimental features provide early access to product functionality. These features
      may change between releases without warning or can be removed entirely from a future
      release. Learn more about experimental features: https://docs.docker.com/go/experimental/

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
  • Loading branch information
thaJeztah committed Oct 2, 2020
1 parent d9c36c2 commit 977d3ae
Show file tree
Hide file tree
Showing 14 changed files with 48 additions and 126 deletions.
1 change: 0 additions & 1 deletion cli-plugins/examples/helloworld/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,5 @@ func main() {
SchemaVersion: "0.1.0",
Vendor: "Docker Inc.",
Version: "testing",
Experimental: os.Getenv("HELLO_EXPERIMENTAL") != "",
})
}
15 changes: 6 additions & 9 deletions cli-plugins/manager/candidate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@ import (
)

type fakeCandidate struct {
path string
exec bool
meta string
allowExperimental bool
path string
exec bool
meta string
}

func (c *fakeCandidate) Path() string {
Expand All @@ -30,7 +29,7 @@ func (c *fakeCandidate) Metadata() ([]byte, error) {
}

func TestValidateCandidate(t *testing.T) {
var (
const (
goodPluginName = NamePrefix + "goodplugin"

builtinName = NamePrefix + "builtin"
Expand Down Expand Up @@ -70,14 +69,12 @@ func TestValidateCandidate(t *testing.T) {
{name: "invalid schemaversion", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "xyzzy"}`}, invalid: `plugin SchemaVersion "xyzzy" is not valid`},
{name: "no vendor", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0"}`}, invalid: "plugin metadata does not define a vendor"},
{name: "empty vendor", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": ""}`}, invalid: "plugin metadata does not define a vendor"},
{name: "experimental required", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: metaExperimental}, invalid: "requires experimental CLI"},
// This one should work
{name: "valid", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing"}`}},
{name: "valid + allowing experimental", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing"}`, allowExperimental: true}},
{name: "experimental + allowing experimental", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: metaExperimental, allowExperimental: true}},
{name: "experimental + allowing experimental", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: metaExperimental}},
} {
t.Run(tc.name, func(t *testing.T) {
p, err := newPlugin(tc.c, fakeroot, tc.c.allowExperimental)
p, err := newPlugin(tc.c, fakeroot)
if tc.err != "" {
assert.ErrorContains(t, err, tc.err)
} else if tc.invalid != "" {
Expand Down
22 changes: 2 additions & 20 deletions cli-plugins/manager/manager.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package manager

import (
"fmt"
"io/ioutil"
"os"
"os/exec"
Expand Down Expand Up @@ -30,16 +29,6 @@ func (e errPluginNotFound) Error() string {
return "Error: No such CLI plugin: " + string(e)
}

type errPluginRequireExperimental string

// Note: errPluginRequireExperimental implements notFound so that the plugin
// is skipped when listing the plugins.
func (e errPluginRequireExperimental) NotFound() {}

func (e errPluginRequireExperimental) Error() string {
return fmt.Sprintf("plugin candidate %q: requires experimental CLI", string(e))
}

type notFound interface{ NotFound() }

// IsNotFound is true if the given error is due to a plugin not being found.
Expand Down Expand Up @@ -133,7 +122,7 @@ func ListPlugins(dockerCli command.Cli, rootcmd *cobra.Command) ([]Plugin, error
continue
}
c := &candidate{paths[0]}
p, err := newPlugin(c, rootcmd, dockerCli.ClientInfo().HasExperimental)
p, err := newPlugin(c, rootcmd)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -181,19 +170,12 @@ func PluginRunCommand(dockerCli command.Cli, name string, rootcmd *cobra.Command
}

c := &candidate{path: path}
plugin, err := newPlugin(c, rootcmd, dockerCli.ClientInfo().HasExperimental)
plugin, err := newPlugin(c, rootcmd)
if err != nil {
return nil, err
}
if plugin.Err != nil {
// TODO: why are we not returning plugin.Err?

err := plugin.Err.(*pluginError).Cause()
// if an experimental plugin was invoked directly while experimental mode is off
// provide a more useful error message than "not found".
if err, ok := err.(errPluginRequireExperimental); ok {
return nil, err
}
return nil, errPluginNotFound(name)
}
cmd := exec.Command(plugin.Path, args...)
Expand Down
2 changes: 1 addition & 1 deletion cli-plugins/manager/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@ type Metadata struct {
// URL is a pointer to the plugin's homepage.
URL string `json:",omitempty"`
// Experimental specifies whether the plugin is experimental.
// Experimental plugins are not displayed on non-experimental CLIs.
// Deprecated: experimental features are now always enabled in the CLI
Experimental bool `json:",omitempty"`
}
6 changes: 1 addition & 5 deletions cli-plugins/manager/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type Plugin struct {
// non-recoverable error.
//
// nolint: gocyclo
func newPlugin(c Candidate, rootcmd *cobra.Command, allowExperimental bool) (Plugin, error) {
func newPlugin(c Candidate, rootcmd *cobra.Command) (Plugin, error) {
path := c.Path()
if path == "" {
return Plugin{}, errors.New("plugin candidate path cannot be empty")
Expand Down Expand Up @@ -96,10 +96,6 @@ func newPlugin(c Candidate, rootcmd *cobra.Command, allowExperimental bool) (Plu
p.Err = wrapAsPluginError(err, "invalid metadata")
return p, nil
}
if p.Experimental && !allowExperimental {
p.Err = &pluginError{errPluginRequireExperimental(p.Name)}
return p, nil
}
if p.Metadata.SchemaVersion != "0.1.0" {
p.Err = NewPluginError("plugin SchemaVersion %q is not valid, must be 0.1.0", p.Metadata.SchemaVersion)
return p, nil
Expand Down
25 changes: 3 additions & 22 deletions cli/command/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,16 +152,6 @@ func (cli *DockerCli) ClientInfo() ClientInfo {
}

func (cli *DockerCli) loadClientInfo() error {
var experimentalValue string
// Environment variable always overrides configuration
if experimentalValue = os.Getenv("DOCKER_CLI_EXPERIMENTAL"); experimentalValue == "" {
experimentalValue = cli.ConfigFile().Experimental
}
hasExperimental, err := isEnabled(experimentalValue)
if err != nil {
return errors.Wrap(err, "Experimental field")
}

var v string
if cli.client != nil {
v = cli.client.ClientVersion()
Expand All @@ -170,7 +160,7 @@ func (cli *DockerCli) loadClientInfo() error {
}
cli.clientInfo = &ClientInfo{
DefaultVersion: v,
HasExperimental: hasExperimental,
HasExperimental: true,
}
return nil
}
Expand Down Expand Up @@ -358,17 +348,6 @@ func resolveDefaultDockerEndpoint(opts *cliflags.CommonOptions) (docker.Endpoint
}, nil
}

func isEnabled(value string) (bool, error) {
switch value {
case "enabled":
return true, nil
case "", "disabled":
return false, nil
default:
return false, errors.Errorf("%q is not valid, should be either enabled or disabled", value)
}
}

func (cli *DockerCli) initializeFromClient() {
ctx := context.Background()
if strings.HasPrefix(cli.DockerEndpoint().Host, "tcp://") {
Expand Down Expand Up @@ -471,6 +450,8 @@ type ServerInfo struct {

// ClientInfo stores details about the supported features of the client
type ClientInfo struct {
// Deprecated: experimental CLI features always enabled. This field is kept
// for backward-compatibility, and is always "true".
HasExperimental bool
DefaultVersion string
}
Expand Down
16 changes: 8 additions & 8 deletions cli/command/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,25 +167,24 @@ func TestInitializeFromClient(t *testing.T) {
}
}

// The CLI no longer disables/hides experimental CLI features, however, we need
// to verify that existing configuration files do not break
func TestExperimentalCLI(t *testing.T) {
defaultVersion := "v1.55"

var testcases = []struct {
doc string
configfile string
expectedExperimentalCLI bool
doc string
configfile string
}{
{
doc: "default",
configfile: `{}`,
expectedExperimentalCLI: false,
doc: "default",
configfile: `{}`,
},
{
doc: "experimental",
configfile: `{
"experimental": "enabled"
}`,
expectedExperimentalCLI: true,
},
}

Expand All @@ -205,7 +204,8 @@ func TestExperimentalCLI(t *testing.T) {
cliconfig.SetDir(dir.Path())
err := cli.Initialize(flags.NewClientOptions())
assert.NilError(t, err)
assert.Check(t, is.Equal(testcase.expectedExperimentalCLI, cli.ClientInfo().HasExperimental))
// For backward-compatibility, HasExperimental will always be "true"
assert.Check(t, is.Equal(true, cli.ClientInfo().HasExperimental))
})
}
}
Expand Down
2 changes: 1 addition & 1 deletion cli/command/stack/kubernetes/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func WrapCli(dockerCli command.Cli, opts Options) (*KubeCli, error) {
}

func (c *KubeCli) composeClient() (*Factory, error) {
return NewFactory(c.kubeNamespace, c.kubeConfig, c.clientSet, c.ClientInfo().HasExperimental)
return NewFactory(c.kubeNamespace, c.kubeConfig, c.clientSet)
}

func (c *KubeCli) checkHostsMatch() error {
Expand Down
6 changes: 2 additions & 4 deletions cli/command/stack/kubernetes/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,10 @@ type Factory struct {
coreClientSet corev1.CoreV1Interface
appsClientSet appsv1beta2.AppsV1beta2Interface
clientSet *kubeclient.Clientset
experimental bool
}

// NewFactory creates a kubernetes client factory
func NewFactory(namespace string, config *restclient.Config, clientSet *kubeclient.Clientset, experimental bool) (*Factory, error) {
func NewFactory(namespace string, config *restclient.Config, clientSet *kubeclient.Clientset) (*Factory, error) {
coreClientSet, err := corev1.NewForConfig(config)
if err != nil {
return nil, err
Expand All @@ -39,7 +38,6 @@ func NewFactory(namespace string, config *restclient.Config, clientSet *kubeclie
coreClientSet: coreClientSet,
appsClientSet: appsClientSet,
clientSet: clientSet,
experimental: experimental,
}, nil
}

Expand Down Expand Up @@ -85,7 +83,7 @@ func (s *Factory) DaemonSets() typesappsv1beta2.DaemonSetInterface {

// Stacks returns a client for Docker's Stack on Kubernetes
func (s *Factory) Stacks(allNamespaces bool) (StackClient, error) {
version, err := kubernetes.GetStackAPIVersion(s.clientSet.Discovery(), s.experimental)
version, err := kubernetes.GetStackAPIVersion(s.clientSet.Discovery())
if err != nil {
return nil, err
}
Expand Down
14 changes: 7 additions & 7 deletions cli/command/system/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ package system

import (
"context"
"fmt"
"runtime"
"sort"
"strconv"
"text/tabwriter"
"text/template"
"time"
Expand Down Expand Up @@ -83,7 +83,7 @@ type clientVersion struct {
Arch string
BuildTime string `json:",omitempty"`
Context string
Experimental bool
Experimental bool `json:",omitempty"` // Deprecated: experimental CLI features always enabled. This field is kept for backward-compatibility, and is always "true"
}

type kubernetesVersion struct {
Expand Down Expand Up @@ -157,7 +157,7 @@ func runVersion(dockerCli command.Cli, opts *versionOptions) error {
BuildTime: reformatDate(version.BuildTime),
Os: runtime.GOOS,
Arch: arch(),
Experimental: dockerCli.ClientInfo().HasExperimental,
Experimental: true,
Context: dockerCli.CurrentContext(),
},
}
Expand Down Expand Up @@ -199,7 +199,7 @@ func runVersion(dockerCli command.Cli, opts *versionOptions) error {
"Os": sv.Os,
"Arch": sv.Arch,
"BuildTime": reformatDate(vd.Server.BuildTime),
"Experimental": fmt.Sprintf("%t", sv.Experimental),
"Experimental": strconv.FormatBool(sv.Experimental),
},
})
}
Expand Down Expand Up @@ -274,13 +274,13 @@ func getKubernetesVersion(dockerCli command.Cli, kubeConfig string) *kubernetesV
logrus.Debugf("failed to get Kubernetes client: %s", err)
return &version
}
version.StackAPI = getStackVersion(kubeClient, dockerCli.ClientInfo().HasExperimental)
version.StackAPI = getStackVersion(kubeClient)
version.Kubernetes = getKubernetesServerVersion(kubeClient)
return &version
}

func getStackVersion(client *kubernetesClient.Clientset, experimental bool) string {
apiVersion, err := kubernetes.GetStackAPIVersion(client, experimental)
func getStackVersion(client *kubernetesClient.Clientset) string {
apiVersion, err := kubernetes.GetStackAPIVersion(client)
if err != nil {
logrus.Debugf("failed to get Stack API version: %s", err)
return "Unknown"
Expand Down
19 changes: 5 additions & 14 deletions cmd/docker/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,12 +336,11 @@ func hideSubcommandIf(subcmd *cobra.Command, condition func(string) bool, annota

func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) error {
var (
buildKitDisabled = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return !v }
buildKitEnabled = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return v }
notExperimental = func(_ string) bool { return !details.ServerInfo().HasExperimental }
notExperimentalCLI = func(_ string) bool { return !details.ClientInfo().HasExperimental }
notOSType = func(v string) bool { return v != details.ServerInfo().OSType }
versionOlderThan = func(v string) bool { return versions.LessThan(details.Client().ClientVersion(), v) }
buildKitDisabled = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return !v }
buildKitEnabled = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return v }
notExperimental = func(_ string) bool { return !details.ServerInfo().HasExperimental }
notOSType = func(v string) bool { return v != details.ServerInfo().OSType }
versionOlderThan = func(v string) bool { return versions.LessThan(details.Client().ClientVersion(), v) }
)

cmd.Flags().VisitAll(func(f *pflag.Flag) {
Expand All @@ -359,7 +358,6 @@ func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) error {
hideFlagIf(f, buildKitDisabled, "buildkit")
hideFlagIf(f, buildKitEnabled, "no-buildkit")
hideFlagIf(f, notExperimental, "experimental")
hideFlagIf(f, notExperimentalCLI, "experimentalCLI")
hideFlagIf(f, notOSType, "ostype")
hideFlagIf(f, versionOlderThan, "version")
})
Expand All @@ -368,7 +366,6 @@ func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) error {
hideSubcommandIf(subcmd, buildKitDisabled, "buildkit")
hideSubcommandIf(subcmd, buildKitEnabled, "no-buildkit")
hideSubcommandIf(subcmd, notExperimental, "experimental")
hideSubcommandIf(subcmd, notExperimentalCLI, "experimentalCLI")
hideSubcommandIf(subcmd, notOSType, "ostype")
hideSubcommandIf(subcmd, versionOlderThan, "version")
}
Expand Down Expand Up @@ -417,9 +414,6 @@ func areFlagsSupported(cmd *cobra.Command, details versionDetails) error {
if _, ok := f.Annotations["experimental"]; ok && !details.ServerInfo().HasExperimental {
errs = append(errs, fmt.Sprintf(`"--%s" is only supported on a Docker daemon with experimental features enabled`, f.Name))
}
if _, ok := f.Annotations["experimentalCLI"]; ok && !details.ClientInfo().HasExperimental {
errs = append(errs, fmt.Sprintf(`"--%s" is only supported on a Docker cli with experimental cli features enabled`, f.Name))
}
// buildkit-specific flags are noop when buildkit is not enabled, so we do not add an error in that case
})
if len(errs) > 0 {
Expand All @@ -441,9 +435,6 @@ func areSubcommandsSupported(cmd *cobra.Command, details versionDetails) error {
if _, ok := curr.Annotations["experimental"]; ok && !details.ServerInfo().HasExperimental {
return fmt.Errorf("%s is only supported on a Docker daemon with experimental features enabled", cmd.CommandPath())
}
if _, ok := curr.Annotations["experimentalCLI"]; ok && !details.ClientInfo().HasExperimental {
return fmt.Errorf("%s is only supported on a Docker cli with experimental cli features enabled", cmd.CommandPath())
}
}
return nil
}
Expand Down
Loading

0 comments on commit 977d3ae

Please sign in to comment.