From e3f239834307af0c4c2f100fbc6efda96ceef180 Mon Sep 17 00:00:00 2001 From: Stephen Kirby <58410745+stirby@users.noreply.github.com> Date: Wed, 15 Apr 2026 12:33:31 -0500 Subject: [PATCH] fix(cli): prevent false deprecation warnings for renamed options (#23931) Co-authored-by: dylanhuff-at-coder --- cli/root.go | 47 +++++++++++-- cli/root_internal_test.go | 142 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 185 insertions(+), 4 deletions(-) diff --git a/cli/root.go b/cli/root.go index 830f70ba76..5bda9a416f 100644 --- a/cli/root.go +++ b/cli/root.go @@ -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 { diff --git a/cli/root_internal_test.go b/cli/root_internal_test.go index f5353ed658..dd61a95a0e 100644 --- a/cli/root_internal_test.go +++ b/cli/root_internal_test.go @@ -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()