fix(cli): prevent false deprecation warnings for renamed options (#23931)

Co-authored-by: dylanhuff-at-coder <dylan@coder.com>
2026-09-24 15:04:27 +08:00 · 2026-04-15 12:33:31 -05:00
parent 91446ac7ca
commit e3f2398343
2 changed files with 185 additions and 4 deletions
@@ -1624,8 +1624,8 @@ func headerTransport(ctx context.Context, serverURL *url.URL, header []string, h
 	return transport, nil
 }

-// printDeprecatedOptions loops through all command options, and prints
-// a warning for usage of deprecated options.
+// PrintDeprecatedOptions loops through all command options, and
+// prints a warning for usage of deprecated options.
 func PrintDeprecatedOptions() serpent.MiddlewareFunc {
 	return func(next serpent.HandlerFunc) serpent.HandlerFunc {
 		return func(inv *serpent.Invocation) error {
@@ -1640,11 +1640,22 @@ func PrintDeprecatedOptions() serpent.MiddlewareFunc {
 					continue
 				}

+				// Verify that this deprecated option was itself
+				// the source of the value. Serpent propagates
+				// ValueSource across all options that share the
+				// same Value pointer, so a new option being set
+				// can make a deprecated sibling appear set when
+				// it was not.
+				source := deprecatedOptionDirectSource(inv, opt)
+				if source == serpent.ValueSourceNone {
+					continue
+				}
+
 				var warnStr strings.Builder
-				_, _ = warnStr.WriteString(translateSource(opt.ValueSource, opt))
+				_, _ = warnStr.WriteString(translateSource(source, opt))
 				_, _ = warnStr.WriteString(" is deprecated, please use ")
 				for i, use := range opt.UseInstead {
-					_, _ = warnStr.WriteString(translateSource(opt.ValueSource, use))
+					_, _ = warnStr.WriteString(translateSource(source, use))
 					if i != len(opt.UseInstead)-1 {
 						_, _ = warnStr.WriteString(" and ")
 					}
@@ -1661,6 +1672,34 @@ func PrintDeprecatedOptions() serpent.MiddlewareFunc {
 	}
 }

+// deprecatedOptionDirectSource returns the source by which a deprecated
+// option was directly set, ignoring any propagated ValueSource from
+// sibling options that share the same Value pointer.
+func deprecatedOptionDirectSource(inv *serpent.Invocation, opt serpent.Option) serpent.ValueSource {
+	if opt.Flag != "" {
+		fl := inv.ParsedFlags().Lookup(opt.Flag)
+		if fl != nil && fl.Changed {
+			return serpent.ValueSourceFlag
+		}
+	}
+
+	if opt.Env != "" {
+		_, exists := inv.Environ.Lookup(opt.Env)
+		if exists {
+			return serpent.ValueSourceEnv
+		}
+	}
+
+	if opt.ValueSource == serpent.ValueSourceYAML {
+		// There is no straightforward way to check whether a
+		// specific YAML key was present in the config file, so
+		// we conservatively assume the deprecated key was used.
+		return serpent.ValueSourceYAML
+	}
+
+	return serpent.ValueSourceNone
+}
+
 // translateSource provides the name of the source of the option, depending on the
 // supplied target ValueSource.
 func translateSource(target serpent.ValueSource, opt serpent.Option) string {
@@ -238,6 +238,148 @@ func Test_wrapTransportWithTelemetryHeader(t *testing.T) {
 	require.Equal(t, ti.Command, "test")
 }

+//nolint:tparallel,paralleltest // This test modifies environment variables.
+func TestPrintDeprecatedOptions(t *testing.T) {
+	newValue := serpent.StringOf(new(string))
+
+	// Both the "new" option and the deprecated option point at the
+	// same Value, mirroring how codersdk/deployment.go wires the
+	// CODER_EMAIL_* / CODER_NOTIFICATIONS_EMAIL_* pairs.
+	newOpt := serpent.Option{
+		Name:  "new-option",
+		Flag:  "new-option",
+		Env:   "CODER_TEST_NEW_OPTION",
+		Value: newValue,
+	}
+	deprecatedOpt := serpent.Option{
+		Name:       "old-option",
+		Flag:       "old-option",
+		Env:        "CODER_TEST_OLD_OPTION",
+		Value:      newValue, // same pointer
+		UseInstead: serpent.OptionSet{newOpt},
+	}
+
+	makeCmd := func(opts serpent.OptionSet) *serpent.Command {
+		return &serpent.Command{
+			Use:        "test",
+			Options:    opts,
+			Middleware: PrintDeprecatedOptions(),
+			Handler: func(_ *serpent.Invocation) error {
+				return nil
+			},
+		}
+	}
+
+	t.Run("EnvOnlyNew_NoWarning", func(t *testing.T) {
+		t.Setenv("CODER_TEST_NEW_OPTION", "val")
+
+		cmd := makeCmd(serpent.OptionSet{newOpt, deprecatedOpt})
+		var stderr bytes.Buffer
+		inv := cmd.Invoke()
+		inv.Environ = serpent.ParseEnviron(os.Environ(), "")
+		inv.Stderr = &stderr
+		err := inv.Run()
+		require.NoError(t, err)
+		require.Empty(t, stderr.String(),
+			"setting only the new env var should not produce a deprecation warning")
+	})
+
+	t.Run("EnvOnlyOld_Warning", func(t *testing.T) {
+		t.Setenv("CODER_TEST_OLD_OPTION", "val")
+
+		cmd := makeCmd(serpent.OptionSet{newOpt, deprecatedOpt})
+		var stderr bytes.Buffer
+		inv := cmd.Invoke()
+		inv.Environ = serpent.ParseEnviron(os.Environ(), "")
+		inv.Stderr = &stderr
+		err := inv.Run()
+		require.NoError(t, err)
+		require.Contains(t, stderr.String(), "is deprecated",
+			"setting the deprecated env var should produce a warning")
+	})
+
+	t.Run("EnvBothSet_Warning", func(t *testing.T) {
+		t.Setenv("CODER_TEST_NEW_OPTION", "new")
+		t.Setenv("CODER_TEST_OLD_OPTION", "old")
+
+		cmd := makeCmd(serpent.OptionSet{newOpt, deprecatedOpt})
+		var stderr bytes.Buffer
+		inv := cmd.Invoke()
+		inv.Environ = serpent.ParseEnviron(os.Environ(), "")
+		inv.Stderr = &stderr
+		err := inv.Run()
+		require.NoError(t, err)
+		require.Contains(t, stderr.String(), "is deprecated",
+			"setting both env vars should still warn about the deprecated one")
+	})
+
+	t.Run("DeprecatedEnvAndNewFlag_Warning", func(t *testing.T) {
+		t.Setenv("CODER_TEST_OLD_OPTION", "val")
+
+		cmd := makeCmd(serpent.OptionSet{newOpt, deprecatedOpt})
+		var stderr bytes.Buffer
+		inv := cmd.Invoke("--new-option", "val")
+		inv.Environ = serpent.ParseEnviron(os.Environ(), "")
+		inv.Stderr = &stderr
+		err := inv.Run()
+		require.NoError(t, err)
+		require.Contains(t, stderr.String(), "`CODER_TEST_OLD_OPTION` is deprecated",
+			"setting the deprecated env var should still warn even if the replacement flag overrides the value")
+		require.NotContains(t, stderr.String(), "`--old-option` is deprecated",
+			"the deprecated environment variable should not be misreported as a deprecated flag")
+	})
+
+	t.Run("FlagOnlyNew_NoWarning", func(t *testing.T) {
+		cmd := makeCmd(serpent.OptionSet{newOpt, deprecatedOpt})
+		var stderr bytes.Buffer
+		inv := cmd.Invoke("--new-option", "val")
+		inv.Stderr = &stderr
+		err := inv.Run()
+		require.NoError(t, err)
+		require.Empty(t, stderr.String(),
+			"passing only the new flag should not produce a deprecation warning")
+	})
+
+	t.Run("FlagOnlyOld_Warning", func(t *testing.T) {
+		cmd := makeCmd(serpent.OptionSet{newOpt, deprecatedOpt})
+		var stderr bytes.Buffer
+		inv := cmd.Invoke("--old-option", "val")
+		inv.Stderr = &stderr
+		err := inv.Run()
+		require.NoError(t, err)
+		require.Contains(t, stderr.String(), "is deprecated",
+			"passing the deprecated flag should produce a warning")
+	})
+
+	t.Run("CODER_EMAIL_FROM_NoWarning", func(t *testing.T) {
+		t.Setenv("CODER_EMAIL_FROM", "noreply@example.com")
+
+		deploymentValues := new(codersdk.DeploymentValues)
+		cmd := makeCmd(deploymentValues.Options())
+		var stderr bytes.Buffer
+		inv := cmd.Invoke()
+		inv.Environ = serpent.ParseEnviron([]string{"CODER_EMAIL_FROM=noreply@example.com"}, "")
+		inv.Stderr = &stderr
+		err := inv.Run()
+		require.NoError(t, err)
+		require.NotContains(t, stderr.String(), "is deprecated",
+			"setting only CODER_EMAIL_FROM should not produce any deprecation warning")
+	})
+
+	t.Run("NothingSet_NoWarning", func(t *testing.T) {
+		t.Parallel()
+
+		cmd := makeCmd(serpent.OptionSet{newOpt, deprecatedOpt})
+		var stderr bytes.Buffer
+		inv := cmd.Invoke()
+		inv.Stderr = &stderr
+		err := inv.Run()
+		require.NoError(t, err)
+		require.Empty(t, stderr.String(),
+			"setting nothing should not produce a deprecation warning")
+	})
+}
+
 func Test_wrapTransportWithEntitlementsCheck(t *testing.T) {
 	t.Parallel()