diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 03f2ca3975..bbbcc17f70 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -8,6 +8,7 @@ ### CLI * Remove `inplace` mode for the `--progress-format` flag. ([#3811](https://github.com/databricks/cli/pull/3811)) * Remove `json` mode for the `--progress-format` flag. ([#3812](https://github.com/databricks/cli/pull/3812)) +* Deprecate the `--progress-format` flag. ([#3819](https://github.com/databricks/cli/pull/3819)) ### Dependency updates diff --git a/cmd/pipelines/root/progress_logger.go b/cmd/pipelines/root/progress_logger.go index 6e9e5da712..765b08a02b 100644 --- a/cmd/pipelines/root/progress_logger.go +++ b/cmd/pipelines/root/progress_logger.go @@ -1,51 +1,12 @@ -// Copied from cmd/root/progress_logger.go and adapted for pipelines use. package root import ( - "context" - - "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/env" - "github.com/databricks/cli/libs/flags" "github.com/spf13/cobra" ) -const envProgressFormat = "DATABRICKS_CLI_PROGRESS_FORMAT" - -type progressLoggerFlag struct { - flags.ProgressLogFormat -} - -func (f *progressLoggerFlag) initializeContext(ctx context.Context) (context.Context, error) { - // No need to initialize the logger if it's already set in the context. This - // happens in unit tests where the logger is setup as a fixture. - if _, ok := cmdio.FromContext(ctx); ok { - return ctx, nil - } - - format := f.ProgressLogFormat - if format == flags.ModeDefault { - format = flags.ModeAppend - } - - progressLogger := cmdio.NewLogger(format) - return cmdio.NewContext(ctx, progressLogger), nil -} - -func initProgressLoggerFlag(cmd *cobra.Command, logFlags *logFlags) *progressLoggerFlag { - f := progressLoggerFlag{ - ProgressLogFormat: flags.NewProgressLogFormat(), - } - - // Configure defaults from environment, if applicable. - // If the provided value is invalid it is ignored. - if v, ok := env.Lookup(cmd.Context(), envProgressFormat); ok { - _ = f.Set(v) - } - +func initProgressLoggerFlag(cmd *cobra.Command, logFlags *logFlags) { flags := cmd.PersistentFlags() - flags.Var(&f.ProgressLogFormat, "progress-format", "format for progress logs (append)") + _ = flags.String("progress-format", "", "format for progress logs") flags.MarkHidden("progress-format") - cmd.RegisterFlagCompletionFunc("progress-format", f.Complete) - return &f + flags.MarkDeprecated("progress-format", "this flag is no longer functional") } diff --git a/cmd/pipelines/root/root.go b/cmd/pipelines/root/root.go index 05be9144b7..72d0b52664 100644 --- a/cmd/pipelines/root/root.go +++ b/cmd/pipelines/root/root.go @@ -36,11 +36,13 @@ func New(ctx context.Context) *cobra.Command { // Initialize flags logFlags := initLogFlags(cmd) - progressLoggerFlag := initProgressLoggerFlag(cmd, logFlags) outputFlag := initOutputFlag(cmd) initProfileFlag(cmd) initTargetFlag(cmd) + // Deprecated flag. Warn if it is specified. + initProgressLoggerFlag(cmd, logFlags) + cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() @@ -55,11 +57,6 @@ func New(ctx context.Context) *cobra.Command { slog.String("version", build.GetInfo().Version), slog.String("args", strings.Join(os.Args, ", "))) - // Configure progress logger - ctx, err = progressLoggerFlag.initializeContext(ctx) - if err != nil { - return err - } // set context, so that initializeIO can have the current context cmd.SetContext(ctx) diff --git a/cmd/root/progress_logger.go b/cmd/root/progress_logger.go index a8b4a446e2..765b08a02b 100644 --- a/cmd/root/progress_logger.go +++ b/cmd/root/progress_logger.go @@ -1,50 +1,12 @@ package root import ( - "context" - - "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/env" - "github.com/databricks/cli/libs/flags" "github.com/spf13/cobra" ) -const envProgressFormat = "DATABRICKS_CLI_PROGRESS_FORMAT" - -type progressLoggerFlag struct { - flags.ProgressLogFormat -} - -func (f *progressLoggerFlag) initializeContext(ctx context.Context) (context.Context, error) { - // No need to initialize the logger if it's already set in the context. This - // happens in unit tests where the logger is setup as a fixture. - if _, ok := cmdio.FromContext(ctx); ok { - return ctx, nil - } - - format := f.ProgressLogFormat - if format == flags.ModeDefault { - format = flags.ModeAppend - } - - progressLogger := cmdio.NewLogger(format) - return cmdio.NewContext(ctx, progressLogger), nil -} - -func initProgressLoggerFlag(cmd *cobra.Command, logFlags *logFlags) *progressLoggerFlag { - f := progressLoggerFlag{ - ProgressLogFormat: flags.NewProgressLogFormat(), - } - - // Configure defaults from environment, if applicable. - // If the provided value is invalid it is ignored. - if v, ok := env.Lookup(cmd.Context(), envProgressFormat); ok { - _ = f.Set(v) - } - +func initProgressLoggerFlag(cmd *cobra.Command, logFlags *logFlags) { flags := cmd.PersistentFlags() - flags.Var(&f.ProgressLogFormat, "progress-format", "format for progress logs (append)") + _ = flags.String("progress-format", "", "format for progress logs") flags.MarkHidden("progress-format") - cmd.RegisterFlagCompletionFunc("progress-format", f.Complete) - return &f + flags.MarkDeprecated("progress-format", "this flag is no longer functional") } diff --git a/cmd/root/progress_logger_test.go b/cmd/root/progress_logger_test.go deleted file mode 100644 index 2095976a37..0000000000 --- a/cmd/root/progress_logger_test.go +++ /dev/null @@ -1,40 +0,0 @@ -package root - -import ( - "context" - "testing" - - "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/flags" - "github.com/spf13/cobra" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -type progressLoggerTest struct { - *cobra.Command - *logFlags - *progressLoggerFlag -} - -func initializeProgressLoggerTest(t *testing.T) ( - *progressLoggerTest, - *flags.ProgressLogFormat, -) { - plt := &progressLoggerTest{ - Command: &cobra.Command{}, - } - plt.logFlags = initLogFlags(plt.Command) - plt.progressLoggerFlag = initProgressLoggerFlag(plt.Command, plt.logFlags) - return plt, &plt.ProgressLogFormat -} - -func TestDefaultLoggerModeResolution(t *testing.T) { - plt, progressFormat := initializeProgressLoggerTest(t) - require.Equal(t, *progressFormat, flags.ModeDefault) - ctx, err := plt.progressLoggerFlag.initializeContext(context.Background()) - require.NoError(t, err) - logger, ok := cmdio.FromContext(ctx) - assert.True(t, ok) - assert.Equal(t, logger.Mode, flags.ModeAppend) -} diff --git a/cmd/root/root.go b/cmd/root/root.go index 574b8c71c3..96fde20846 100644 --- a/cmd/root/root.go +++ b/cmd/root/root.go @@ -42,12 +42,14 @@ func New(ctx context.Context) *cobra.Command { // Initialize flags logFlags := initLogFlags(cmd) - progressLoggerFlag := initProgressLoggerFlag(cmd, logFlags) outputFlag := initOutputFlag(cmd) initProfileFlag(cmd) initEnvironmentFlag(cmd) initTargetFlag(cmd) + // Deprecated flag. Warn if it is specified. + initProgressLoggerFlag(cmd, logFlags) + cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() @@ -62,11 +64,6 @@ func New(ctx context.Context) *cobra.Command { slog.String("version", build.GetInfo().Version), slog.String("args", strings.Join(os.Args, ", "))) - // Configure progress logger - ctx, err = progressLoggerFlag.initializeContext(ctx) - if err != nil { - return err - } // set context, so that initializeIO can have the current context cmd.SetContext(ctx) diff --git a/integration/bundle/helpers_test.go b/integration/bundle/helpers_test.go index a42e3254c5..48ca44585b 100644 --- a/integration/bundle/helpers_test.go +++ b/integration/bundle/helpers_test.go @@ -69,8 +69,6 @@ func deployBundle(t testutil.TestingT, ctx context.Context, path string) { func runResource(t testutil.TestingT, ctx context.Context, path, key string) (string, error) { ctx = env.Set(ctx, "BUNDLE_ROOT", path) - ctx = cmdio.NewContext(ctx, cmdio.Default()) - c := testcli.NewRunner(t, ctx, "bundle", "run", key) stdout, _, err := c.Run() return stdout.String(), err diff --git a/internal/testcli/runner.go b/internal/testcli/runner.go index 1e2e2e40c2..f74e965ffd 100644 --- a/internal/testcli/runner.go +++ b/internal/testcli/runner.go @@ -16,7 +16,6 @@ import ( "github.com/databricks/cli/cmd/root" "github.com/databricks/cli/internal/testutil" "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/flags" ) // Helper for running the root command in the background. @@ -101,13 +100,8 @@ func (r *Runner) RunBackground() { var stdoutW, stderrW io.WriteCloser stdoutR, stdoutW = io.Pipe() stderrR, stderrW = io.Pipe() - ctx := cmdio.NewContext(r.ctx, &cmdio.Logger{ - Mode: flags.ModeAppend, - Reader: bufio.Reader{}, - Writer: stderrW, - }) - cli := cmd.New(ctx) + cli := cmd.New(r.ctx) cli.SetOut(stdoutW) cli.SetErr(stderrW) cli.SetArgs(r.args) @@ -116,7 +110,7 @@ func (r *Runner) RunBackground() { } errch := make(chan error) - ctx, cancel := context.WithCancel(ctx) + ctx, cancel := context.WithCancel(r.ctx) // Tee stdout/stderr to buffers. stdoutR = io.TeeReader(stdoutR, &r.stdout) @@ -183,13 +177,8 @@ func (r *Runner) RunBackground() { func (r *Runner) Run() (bytes.Buffer, bytes.Buffer, error) { r.Helper() var stdout, stderr bytes.Buffer - ctx := cmdio.NewContext(r.ctx, &cmdio.Logger{ - Mode: flags.ModeAppend, - Reader: bufio.Reader{}, - Writer: &stderr, - }) - cli := cmd.New(ctx) + cli := cmd.New(r.ctx) cli.SetOut(&stdout) cli.SetErr(&stderr) cli.SetArgs(r.args) @@ -198,7 +187,7 @@ func (r *Runner) Run() (bytes.Buffer, bytes.Buffer, error) { r.Logf(" args: %s", strings.Join(r.args, ", ")) } - err := root.Execute(ctx, cli) + err := root.Execute(r.ctx, cli) if err != nil { if r.Verbose { r.Logf(" error: %s", err) diff --git a/libs/cmdio/context.go b/libs/cmdio/context.go deleted file mode 100644 index 0ab2513b20..0000000000 --- a/libs/cmdio/context.go +++ /dev/null @@ -1,20 +0,0 @@ -package cmdio - -import ( - "context" -) - -type progressLogger int - -var progressLoggerKey progressLogger - -// NewContext returns a new Context that carries the specified progress logger. -func NewContext(ctx context.Context, logger *Logger) context.Context { - return context.WithValue(ctx, progressLoggerKey, logger) -} - -// FromContext returns the progress logger value stored in ctx, if any. -func FromContext(ctx context.Context) (*Logger, bool) { - u, ok := ctx.Value(progressLoggerKey).(*Logger) - return u, ok -} diff --git a/libs/cmdio/logger.go b/libs/cmdio/logger.go deleted file mode 100644 index 679d31a935..0000000000 --- a/libs/cmdio/logger.go +++ /dev/null @@ -1,38 +0,0 @@ -package cmdio - -import ( - "bufio" - "io" - "os" - - "github.com/databricks/cli/libs/flags" -) - -// This is the interface for all io interactions with a user -type Logger struct { - // Mode for the logger. One of (append). - Mode flags.ProgressLogFormat - - // Input stream (eg. stdin). Answers to questions prompted using the Ask() method - // are read from here - Reader bufio.Reader - - // Output stream where the logger writes to - Writer io.Writer -} - -func NewLogger(mode flags.ProgressLogFormat) *Logger { - return &Logger{ - Mode: mode, - Writer: os.Stderr, - Reader: *bufio.NewReader(os.Stdin), - } -} - -func Default() *Logger { - return &Logger{ - Mode: flags.ModeAppend, - Writer: os.Stderr, - Reader: *bufio.NewReader(os.Stdin), - } -} diff --git a/libs/flags/progress_format.go b/libs/flags/progress_format.go deleted file mode 100644 index 53adf2ca7a..0000000000 --- a/libs/flags/progress_format.go +++ /dev/null @@ -1,50 +0,0 @@ -package flags - -import ( - "fmt" - "strings" - - "github.com/spf13/cobra" -) - -type ProgressLogFormat string - -var ( - ModeAppend = ProgressLogFormat("append") - ModeDefault = ProgressLogFormat("default") -) - -func (p *ProgressLogFormat) String() string { - return string(*p) -} - -func NewProgressLogFormat() ProgressLogFormat { - return ModeDefault -} - -func (p *ProgressLogFormat) Set(s string) error { - lower := strings.ToLower(s) - switch lower { - case ModeAppend.String(): - *p = ProgressLogFormat(ModeAppend.String()) - case ModeDefault.String(): - // We include ModeDefault here for symmetry reasons so this flag value - // can be unset after test runs. We should not point this value in error - // messages though since it's internal only - *p = ProgressLogFormat(ModeAppend.String()) - default: - return fmt.Errorf("accepted arguments are [%s]", ModeAppend.String()) - } - return nil -} - -func (p *ProgressLogFormat) Type() string { - return "format" -} - -// Complete is the Cobra compatible completion function for this flag. -func (p *ProgressLogFormat) Complete(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - return []string{ - "append", - }, cobra.ShellCompDirectiveNoFileComp -} diff --git a/libs/flags/progress_format_test.go b/libs/flags/progress_format_test.go deleted file mode 100644 index f2a5d738f1..0000000000 --- a/libs/flags/progress_format_test.go +++ /dev/null @@ -1,33 +0,0 @@ -package flags - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestProgressFormatNonTtyDefault(t *testing.T) { - format := NewProgressLogFormat() - assert.Equal(t, format, ModeDefault) -} - -func TestProgressFormatSet(t *testing.T) { - p := NewProgressLogFormat() - - // invalid arg - err := p.Set("foo") - assert.ErrorContains(t, err, "accepted arguments are [append]") - - // set append - err = p.Set("append") - assert.NoError(t, err) - assert.Equal(t, "append", p.String()) - - err = p.Set("Append") - assert.NoError(t, err) - assert.Equal(t, "append", p.String()) - - err = p.Set("APPEND") - assert.NoError(t, err) - assert.Equal(t, "append", p.String()) -}