Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always enable experimental features #2774

Merged
merged 2 commits into from
Oct 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
23 changes: 23 additions & 0 deletions cli/cobra.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ func setupCommonRootCommand(rootCmd *cobra.Command) (*cliflags.ClientOptions, *p
cobra.AddTemplateFunc("vendorAndVersion", vendorAndVersion)
cobra.AddTemplateFunc("invalidPluginReason", invalidPluginReason)
cobra.AddTemplateFunc("isPlugin", isPlugin)
cobra.AddTemplateFunc("isExperimental", isExperimental)
cobra.AddTemplateFunc("decoratedName", decoratedName)

rootCmd.SetUsageTemplate(usageTemplate)
Expand Down Expand Up @@ -191,6 +192,19 @@ var helpCommand = &cobra.Command{
},
}

func isExperimental(cmd *cobra.Command) bool {
if _, ok := cmd.Annotations["experimentalCLI"]; ok {
return true
}
var experimental bool
cmd.VisitParents(func(cmd *cobra.Command) {
if _, ok := cmd.Annotations["experimentalCLI"]; ok {
experimental = true
}
})
return experimental
}

func isPlugin(cmd *cobra.Command) bool {
return cmd.Annotations[pluginmanager.CommandAnnotationPlugin] == "true"
}
Expand Down Expand Up @@ -286,7 +300,16 @@ var usageTemplate = `Usage:
{{- if .HasSubCommands}} {{ .CommandPath}}{{- if .HasAvailableFlags}} [OPTIONS]{{end}} COMMAND{{end}}

{{if ne .Long ""}}{{ .Long | trim }}{{ else }}{{ .Short | trim }}{{end}}
{{- if isExperimental .}}

EXPERIMENTAL:
{{.CommandPath}} is an experimental feature.
Experimental features provide early access to product functionality. These
features may change between releases without warning, or can be removed from a
future release. Learn more about experimental features in our documentation:
https://docs.docker.com/go/experimental/

{{- end}}
{{- if gt .Aliases 0}}

Aliases:
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
Loading