client: make WithAPIVersion, WithAPIVersionFromEnv order-independent

Environment-variables are expected to override config / defaults, so
make sure that the DOCKER_API_VERSION env-var always takes priority.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

client/client.go

@@ -211,6 +211,12 @@ func New(ops ...Opt) (*Client, error) {
 		}
 	}
 
+	if cfg.envAPIVersion != "" {
+		cfg.version = cfg.envAPIVersion
+	} else if cfg.manualAPIVersion != "" {
+		cfg.version = cfg.manualAPIVersion
+	}
+
 	if tr, ok := c.client.Transport.(*http.Transport); ok {
 		// Store the base transport before we wrap it in tracing libs below
 		// This is used, as an example, to close idle connections when the client is closed

client/client_options.go

@@ -41,6 +41,22 @@ type clientConfig struct {
 	// manualOverride is set to true when the version was set by users.
 	manualOverride bool
 
+	// manualAPIVersion contains the API version set by users. This field
+	// will only be non-empty if a valid-formed version was set through
+	// [WithAPIVersion].
+	//
+	// If both manualAPIVersion and envAPIVersion are set, manualAPIVersion
+	// takes precedence. Either field disables API-version negotiation.
+	manualAPIVersion string
+
+	// envAPIVersion contains the API version set by users. This field
+	// will only be non-empty if a valid-formed version was set through
+	// [WithAPIVersionFromEnv].
+	//
+	// If both manualAPIVersion and envAPIVersion are set, manualAPIVersion
+	// takes precedence. Either field disables API-version negotiation.
+	envAPIVersion string
+
 	// negotiateVersion indicates if the client should automatically negotiate
 	// the API version to use when making requests. API version negotiation is
 	// performed on the first request, after which negotiated is set to "true"
@@ -250,6 +266,9 @@ func WithTLSClientConfigFromEnv() Opt {
 // WithAPIVersion does not validate if the client supports the given version,
 // and callers should verify if the version lower than the maximum supported
 // version as defined by [MaxAPIVersion].
+//
+// [WithAPIVersionFromEnv] takes precedence if [WithAPIVersion] and
+// [WithAPIVersionFromEnv] are both set.
 func WithAPIVersion(version string) Opt {
 	return func(c *clientConfig) error {
 		version = strings.TrimSpace(version)
@@ -258,7 +277,7 @@ func WithAPIVersion(version string) Opt {
 			if err != nil {
 				return fmt.Errorf("invalid API version (%s): %w", version, err)
 			}
-			c.version = ver
+			c.manualAPIVersion = ver
 			c.manualOverride = true
 		}
 		return nil
@@ -280,6 +299,9 @@ func WithVersion(version string) Opt {
 // WithAPIVersion does not validate if the client supports the given version,
 // and callers should verify if the version lower than the maximum supported
 // version as defined by [MaxAPIVersion].
+//
+// [WithAPIVersionFromEnv] takes precedence if [WithAPIVersion] and
+// [WithAPIVersionFromEnv] are both set.
 func WithAPIVersionFromEnv() Opt {
 	return func(c *clientConfig) error {
 		version := strings.TrimSpace(os.Getenv(EnvOverrideAPIVersion))
@@ -288,7 +310,7 @@ func WithAPIVersionFromEnv() Opt {
 			if err != nil {
 				return fmt.Errorf("invalid API version (%s): %w", version, err)
 			}
-			c.version = ver
+			c.envAPIVersion = ver
 			c.manualOverride = true
 		}
 		return nil

client/client_options_test.go

@@ -250,6 +250,49 @@ func TestOptionWithAPIVersionFromEnv(t *testing.T) {
 	}
 }
 
+// TestOptionOverridePriority validates that overriding the API version through
+// [WithAPIVersionFromEnv] takes precedence over other manual options, regardless
+// the order in which they're passed.
+func TestOptionOverridePriority(t *testing.T) {
+	t.Run("no env-var set", func(t *testing.T) {
+		client, err := New(WithAPIVersionFromEnv(), WithAPIVersion("1.50"))
+		assert.NilError(t, err)
+		assert.Check(t, is.Equal(client.ClientVersion(), "1.50"))
+		assert.Check(t, is.Equal(client.manualOverride, true))
+	})
+
+	const expected = "1.51"
+	t.Setenv(EnvOverrideAPIVersion, expected)
+
+	t.Run("WithAPIVersionFromEnv first", func(t *testing.T) {
+		client, err := New(WithAPIVersionFromEnv(), WithAPIVersion("1.50"))
+		assert.NilError(t, err)
+		assert.Check(t, is.Equal(client.ClientVersion(), expected))
+		assert.Check(t, is.Equal(client.manualOverride, true))
+	})
+
+	t.Run("WithAPIVersionFromEnv last", func(t *testing.T) {
+		client, err := New(WithAPIVersion("1.50"), WithAPIVersionFromEnv())
+		assert.NilError(t, err)
+		assert.Check(t, is.Equal(client.ClientVersion(), expected))
+		assert.Check(t, is.Equal(client.manualOverride, true))
+	})
+
+	t.Run("FromEnv first", func(t *testing.T) {
+		client, err := New(FromEnv, WithAPIVersion("1.50"))
+		assert.NilError(t, err)
+		assert.Check(t, is.Equal(client.ClientVersion(), expected))
+		assert.Check(t, is.Equal(client.manualOverride, true))
+	})
+
+	t.Run("FromEnv last", func(t *testing.T) {
+		client, err := New(WithAPIVersion("1.50"), FromEnv)
+		assert.NilError(t, err)
+		assert.Check(t, is.Equal(client.ClientVersion(), expected))
+		assert.Check(t, is.Equal(client.manualOverride, true))
+	})
+}
+
 func TestWithUserAgent(t *testing.T) {
 	const userAgent = "Magic-Client/v1.2.3"
 	t.Run("user-agent", func(t *testing.T) {

client/ping.go

@@ -89,6 +89,10 @@ func (cli *Client) Ping(ctx context.Context, options PingOptions) (PingResult, e
 	}
 
 	if cli.negotiated.Load() && !options.ForceNegotiate {
+		// API version was already negotiated or manually set.
+		//
+		// We check cli.negotiated again under lock, to account for race
+		// conditions with the check at the start of this function.
 		return ping, nil
 	}
 

vendor/github.com/moby/moby/client/client.go

@@ -211,6 +211,12 @@ func New(ops ...Opt) (*Client, error) {
 		}
 	}
 
+	if cfg.envAPIVersion != "" {
+		cfg.version = cfg.envAPIVersion
+	} else if cfg.manualAPIVersion != "" {
+		cfg.version = cfg.manualAPIVersion
+	}
+
 	if tr, ok := c.client.Transport.(*http.Transport); ok {
 		// Store the base transport before we wrap it in tracing libs below
 		// This is used, as an example, to close idle connections when the client is closed

vendor/github.com/moby/moby/client/client_options.go

@@ -41,6 +41,22 @@ type clientConfig struct {
 	// manualOverride is set to true when the version was set by users.
 	manualOverride bool
 
+	// manualAPIVersion contains the API version set by users. This field
+	// will only be non-empty if a valid-formed version was set through
+	// [WithAPIVersion].
+	//
+	// If both manualAPIVersion and envAPIVersion are set, manualAPIVersion
+	// takes precedence. Either field disables API-version negotiation.
+	manualAPIVersion string
+
+	// envAPIVersion contains the API version set by users. This field
+	// will only be non-empty if a valid-formed version was set through
+	// [WithAPIVersionFromEnv].
+	//
+	// If both manualAPIVersion and envAPIVersion are set, manualAPIVersion
+	// takes precedence. Either field disables API-version negotiation.
+	envAPIVersion string
+
 	// negotiateVersion indicates if the client should automatically negotiate
 	// the API version to use when making requests. API version negotiation is
 	// performed on the first request, after which negotiated is set to "true"
@@ -250,6 +266,9 @@ func WithTLSClientConfigFromEnv() Opt {
 // WithAPIVersion does not validate if the client supports the given version,
 // and callers should verify if the version lower than the maximum supported
 // version as defined by [MaxAPIVersion].
+//
+// [WithAPIVersionFromEnv] takes precedence if [WithAPIVersion] and
+// [WithAPIVersionFromEnv] are both set.
 func WithAPIVersion(version string) Opt {
 	return func(c *clientConfig) error {
 		version = strings.TrimSpace(version)
@@ -258,7 +277,7 @@ func WithAPIVersion(version string) Opt {
 			if err != nil {
 				return fmt.Errorf("invalid API version (%s): %w", version, err)
 			}
-			c.version = ver
+			c.manualAPIVersion = ver
 			c.manualOverride = true
 		}
 		return nil
@@ -280,6 +299,9 @@ func WithVersion(version string) Opt {
 // WithAPIVersion does not validate if the client supports the given version,
 // and callers should verify if the version lower than the maximum supported
 // version as defined by [MaxAPIVersion].
+//
+// [WithAPIVersionFromEnv] takes precedence if [WithAPIVersion] and
+// [WithAPIVersionFromEnv] are both set.
 func WithAPIVersionFromEnv() Opt {
 	return func(c *clientConfig) error {
 		version := strings.TrimSpace(os.Getenv(EnvOverrideAPIVersion))
@@ -288,7 +310,7 @@ func WithAPIVersionFromEnv() Opt {
 			if err != nil {
 				return fmt.Errorf("invalid API version (%s): %w", version, err)
 			}
-			c.version = ver
+			c.envAPIVersion = ver
 			c.manualOverride = true
 		}
 		return nil

vendor/github.com/moby/moby/client/ping.go

@@ -89,6 +89,10 @@ func (cli *Client) Ping(ctx context.Context, options PingOptions) (PingResult, e
 	}
 
 	if cli.negotiated.Load() && !options.ForceNegotiate {
+		// API version was already negotiated or manually set.
+		//
+		// We check cli.negotiated again under lock, to account for race
+		// conditions with the check at the start of this function.
 		return ping, nil
 	}