From 36449632ee7e49abfba6cc30824a1b84cc9a1059 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 23 Oct 2025 15:49:46 +0200 Subject: [PATCH 1/2] Remove unused in-place progress logging mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In-place mode was designed to update job progress in place using ANSI escape codes (e.g., showing 'PENDING' → 'RUNNING' → 'TERMINATED' on the same line). However, acceptance tests show jobs typically output only a single state transition: 'Run URL: ' followed by '[TIMESTAMP] "job-name" TERMINATED', suggesting the job completes before multiple states can be observed. The default mode selection logic required log-file to not be stderr AND stderr to be a terminal to enable in-place mode, which is an uncommon configuration. Additionally, only JobProgressEvent supported in-place updates while all other events (URLs, errors, pipeline events) fell back to append mode, making the feature inconsistent. The implementation added complexity with ANSI escape codes, terminal detection, and an IsInplaceSupported() interface method across all event types. Since the feature provided minimal practical value and likely was rarely (if ever) enabled by default, it has been removed in favor of the simpler append mode. --- bundle/deployplan/action.go | 5 --- bundle/run/progress/job.go | 4 --- bundle/run/progress/job_events.go | 8 ----- bundle/run/progress/pipeline.go | 5 --- bundle/run/progress/pipeline_events.go | 4 --- cmd/pipelines/root/progress_logger.go | 24 ++------------ cmd/root/progress_logger.go | 24 ++------------ cmd/root/progress_logger_test.go | 33 ++----------------- libs/cmdio/error_event.go | 4 --- libs/cmdio/event.go | 3 -- libs/cmdio/logger.go | 44 ++++---------------------- libs/cmdio/message_event.go | 4 --- libs/flags/progress_format.go | 5 --- libs/flags/progress_format_test.go | 15 +-------- 14 files changed, 14 insertions(+), 168 deletions(-) diff --git a/bundle/deployplan/action.go b/bundle/deployplan/action.go index d27488f4bd..6cd4d079b4 100644 --- a/bundle/deployplan/action.go +++ b/bundle/deployplan/action.go @@ -19,11 +19,6 @@ func (a Action) String() string { return fmt.Sprintf(" %s %s", a.ActionType.StringShort(), key) } -// Implements cmdio.Event for cmdio.Log -func (a Action) IsInplaceSupported() bool { - return false -} - type ActionType int // Actions are ordered in increasing severity. diff --git a/bundle/run/progress/job.go b/bundle/run/progress/job.go index 309ba0d79d..6deee451ad 100644 --- a/bundle/run/progress/job.go +++ b/bundle/run/progress/job.go @@ -34,7 +34,3 @@ func (event *JobProgressEvent) String() string { return result.String() } - -func (event *JobProgressEvent) IsInplaceSupported() bool { - return true -} diff --git a/bundle/run/progress/job_events.go b/bundle/run/progress/job_events.go index 26a15a3015..80e4938a71 100644 --- a/bundle/run/progress/job_events.go +++ b/bundle/run/progress/job_events.go @@ -27,10 +27,6 @@ func (event *TaskErrorEvent) String() string { return result.String() } -func (event *TaskErrorEvent) IsInplaceSupported() bool { - return false -} - type JobRunUrlEvent struct { Type string `json:"type"` Url string `json:"url"` @@ -46,7 +42,3 @@ func NewJobRunUrlEvent(url string) *JobRunUrlEvent { func (event *JobRunUrlEvent) String() string { return fmt.Sprintf("Run URL: %s\n", event.Url) } - -func (event *JobRunUrlEvent) IsInplaceSupported() bool { - return false -} diff --git a/bundle/run/progress/pipeline.go b/bundle/run/progress/pipeline.go index 8adad2cf97..a98f074f78 100644 --- a/bundle/run/progress/pipeline.go +++ b/bundle/run/progress/pipeline.go @@ -39,11 +39,6 @@ func (event *ProgressEvent) String() string { return result.String() } -func (event *ProgressEvent) IsInplaceSupported() bool { - return false -} - -// TODO: Add inplace logging to pipelines. https://github.com/databricks/cli/issues/280 type UpdateTracker struct { UpdateId string PipelineId string diff --git a/bundle/run/progress/pipeline_events.go b/bundle/run/progress/pipeline_events.go index 173b6ecf76..22b56fac5f 100644 --- a/bundle/run/progress/pipeline_events.go +++ b/bundle/run/progress/pipeline_events.go @@ -21,7 +21,3 @@ func NewPipelineUpdateUrlEvent(host, updateId, pipelineId string) *PipelineUpdat func (event *PipelineUpdateUrlEvent) String() string { return fmt.Sprintf("Update URL: %s\n", event.Url) } - -func (event *PipelineUpdateUrlEvent) IsInplaceSupported() bool { - return false -} diff --git a/cmd/pipelines/root/progress_logger.go b/cmd/pipelines/root/progress_logger.go index f914626e1c..74ae01b71d 100644 --- a/cmd/pipelines/root/progress_logger.go +++ b/cmd/pipelines/root/progress_logger.go @@ -3,30 +3,17 @@ package root import ( "context" - "errors" - "os" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/flags" "github.com/spf13/cobra" - "golang.org/x/term" ) const envProgressFormat = "DATABRICKS_CLI_PROGRESS_FORMAT" type progressLoggerFlag struct { flags.ProgressLogFormat - - log *logFlags -} - -func (f *progressLoggerFlag) resolveModeDefault(format flags.ProgressLogFormat) flags.ProgressLogFormat { - if (f.log.level.String() == "disabled" || f.log.file.String() != "stderr") && - term.IsTerminal(int(os.Stderr.Fd())) { - return flags.ModeInplace - } - return flags.ModeAppend } func (f *progressLoggerFlag) initializeContext(ctx context.Context) (context.Context, error) { @@ -36,14 +23,9 @@ func (f *progressLoggerFlag) initializeContext(ctx context.Context) (context.Con return ctx, nil } - if f.log.level.String() != "disabled" && f.log.file.String() == "stderr" && - f.ProgressLogFormat == flags.ModeInplace { - return nil, errors.New("inplace progress logging cannot be used when log-file is stderr") - } - format := f.ProgressLogFormat if format == flags.ModeDefault { - format = f.resolveModeDefault(format) + format = flags.ModeAppend } progressLogger := cmdio.NewLogger(format) @@ -53,8 +35,6 @@ func (f *progressLoggerFlag) initializeContext(ctx context.Context) (context.Con func initProgressLoggerFlag(cmd *cobra.Command, logFlags *logFlags) *progressLoggerFlag { f := progressLoggerFlag{ ProgressLogFormat: flags.NewProgressLogFormat(), - - log: logFlags, } // Configure defaults from environment, if applicable. @@ -64,7 +44,7 @@ func initProgressLoggerFlag(cmd *cobra.Command, logFlags *logFlags) *progressLog } flags := cmd.PersistentFlags() - flags.Var(&f.ProgressLogFormat, "progress-format", "format for progress logs (append, inplace, json)") + flags.Var(&f.ProgressLogFormat, "progress-format", "format for progress logs (append, json)") flags.MarkHidden("progress-format") cmd.RegisterFlagCompletionFunc("progress-format", f.Complete) return &f diff --git a/cmd/root/progress_logger.go b/cmd/root/progress_logger.go index 0e3efa6afb..f7185b92fd 100644 --- a/cmd/root/progress_logger.go +++ b/cmd/root/progress_logger.go @@ -2,30 +2,17 @@ package root import ( "context" - "errors" - "os" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/flags" "github.com/spf13/cobra" - "golang.org/x/term" ) const envProgressFormat = "DATABRICKS_CLI_PROGRESS_FORMAT" type progressLoggerFlag struct { flags.ProgressLogFormat - - log *logFlags -} - -func (f *progressLoggerFlag) resolveModeDefault(format flags.ProgressLogFormat) flags.ProgressLogFormat { - if (f.log.level.String() == "disabled" || f.log.file.String() != "stderr") && - term.IsTerminal(int(os.Stderr.Fd())) { - return flags.ModeInplace - } - return flags.ModeAppend } func (f *progressLoggerFlag) initializeContext(ctx context.Context) (context.Context, error) { @@ -35,14 +22,9 @@ func (f *progressLoggerFlag) initializeContext(ctx context.Context) (context.Con return ctx, nil } - if f.log.level.String() != "disabled" && f.log.file.String() == "stderr" && - f.ProgressLogFormat == flags.ModeInplace { - return nil, errors.New("inplace progress logging cannot be used when log-file is stderr") - } - format := f.ProgressLogFormat if format == flags.ModeDefault { - format = f.resolveModeDefault(format) + format = flags.ModeAppend } progressLogger := cmdio.NewLogger(format) @@ -52,8 +34,6 @@ func (f *progressLoggerFlag) initializeContext(ctx context.Context) (context.Con func initProgressLoggerFlag(cmd *cobra.Command, logFlags *logFlags) *progressLoggerFlag { f := progressLoggerFlag{ ProgressLogFormat: flags.NewProgressLogFormat(), - - log: logFlags, } // Configure defaults from environment, if applicable. @@ -63,7 +43,7 @@ func initProgressLoggerFlag(cmd *cobra.Command, logFlags *logFlags) *progressLog } flags := cmd.PersistentFlags() - flags.Var(&f.ProgressLogFormat, "progress-format", "format for progress logs (append, inplace, json)") + flags.Var(&f.ProgressLogFormat, "progress-format", "format for progress logs (append, json)") flags.MarkHidden("progress-format") cmd.RegisterFlagCompletionFunc("progress-format", f.Complete) return &f diff --git a/cmd/root/progress_logger_test.go b/cmd/root/progress_logger_test.go index ed39ca59f0..2095976a37 100644 --- a/cmd/root/progress_logger_test.go +++ b/cmd/root/progress_logger_test.go @@ -19,8 +19,6 @@ type progressLoggerTest struct { func initializeProgressLoggerTest(t *testing.T) ( *progressLoggerTest, - *flags.LogLevelFlag, - *flags.LogFileFlag, *flags.ProgressLogFormat, ) { plt := &progressLoggerTest{ @@ -28,38 +26,11 @@ func initializeProgressLoggerTest(t *testing.T) ( } plt.logFlags = initLogFlags(plt.Command) plt.progressLoggerFlag = initProgressLoggerFlag(plt.Command, plt.logFlags) - return plt, &plt.level, &plt.file, &plt.ProgressLogFormat -} - -func TestInitializeErrorOnIncompatibleConfig(t *testing.T) { - plt, logLevel, logFile, progressFormat := initializeProgressLoggerTest(t) - require.NoError(t, logLevel.Set("info")) - require.NoError(t, logFile.Set("stderr")) - require.NoError(t, progressFormat.Set("inplace")) - _, err := plt.progressLoggerFlag.initializeContext(context.Background()) - assert.ErrorContains(t, err, "inplace progress logging cannot be used when log-file is stderr") -} - -func TestNoErrorOnDisabledLogLevel(t *testing.T) { - plt, logLevel, logFile, progressFormat := initializeProgressLoggerTest(t) - require.NoError(t, logLevel.Set("disabled")) - require.NoError(t, logFile.Set("stderr")) - require.NoError(t, progressFormat.Set("inplace")) - _, err := plt.progressLoggerFlag.initializeContext(context.Background()) - assert.NoError(t, err) -} - -func TestNoErrorOnNonStderrLogFile(t *testing.T) { - plt, logLevel, logFile, progressFormat := initializeProgressLoggerTest(t) - require.NoError(t, logLevel.Set("info")) - require.NoError(t, logFile.Set("stdout")) - require.NoError(t, progressFormat.Set("inplace")) - _, err := plt.progressLoggerFlag.initializeContext(context.Background()) - assert.NoError(t, err) + return plt, &plt.ProgressLogFormat } func TestDefaultLoggerModeResolution(t *testing.T) { - plt, _, _, progressFormat := initializeProgressLoggerTest(t) + plt, progressFormat := initializeProgressLoggerTest(t) require.Equal(t, *progressFormat, flags.ModeDefault) ctx, err := plt.progressLoggerFlag.initializeContext(context.Background()) require.NoError(t, err) diff --git a/libs/cmdio/error_event.go b/libs/cmdio/error_event.go index 62897995bc..6a6dd8e795 100644 --- a/libs/cmdio/error_event.go +++ b/libs/cmdio/error_event.go @@ -7,7 +7,3 @@ type ErrorEvent struct { func (event *ErrorEvent) String() string { return "Error: " + event.Error } - -func (event *ErrorEvent) IsInplaceSupported() bool { - return false -} diff --git a/libs/cmdio/event.go b/libs/cmdio/event.go index 6976ab3d88..c3c09acbdc 100644 --- a/libs/cmdio/event.go +++ b/libs/cmdio/event.go @@ -3,7 +3,4 @@ package cmdio type Event interface { // convert event into human readable string String() string - - // true if event supports inplace logging, return false otherwise - IsInplaceSupported() bool } diff --git a/libs/cmdio/logger.go b/libs/cmdio/logger.go index 7d369a8a08..873f3be2c0 100644 --- a/libs/cmdio/logger.go +++ b/libs/cmdio/logger.go @@ -17,7 +17,7 @@ import ( // This is the interface for all io interactions with a user type Logger struct { - // Mode for the logger. One of (append, inplace, json). + // Mode for the logger. One of (append, json). Mode flags.ProgressLogFormat // Input stream (eg. stdin). Answers to questions prompted using the Ask() method @@ -27,28 +27,22 @@ type Logger struct { // Output stream where the logger writes to Writer io.Writer - // If true, indicates no events have been printed by the logger yet. Used - // by inplace logging for formatting - isFirstEvent bool - mutex sync.Mutex } func NewLogger(mode flags.ProgressLogFormat) *Logger { return &Logger{ - Mode: mode, - Writer: os.Stderr, - Reader: *bufio.NewReader(os.Stdin), - isFirstEvent: true, + 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), - isFirstEvent: true, + Mode: flags.ModeAppend, + Writer: os.Stderr, + Reader: *bufio.NewReader(os.Stdin), } } @@ -201,34 +195,10 @@ func (l *Logger) writeAppend(event Event) { _, _ = l.Writer.Write([]byte("\n")) } -func (l *Logger) writeInplace(event Event) { - if l.isFirstEvent { - // save cursor location - _, _ = l.Writer.Write([]byte("\033[s")) - } - - // move cursor to saved location - _, _ = l.Writer.Write([]byte("\033[u")) - - // clear from cursor to end of screen - _, _ = l.Writer.Write([]byte("\033[0J")) - - _, _ = l.Writer.Write([]byte(event.String())) - _, _ = l.Writer.Write([]byte("\n")) - l.isFirstEvent = false -} - func (l *Logger) Log(event Event) { l.mutex.Lock() defer l.mutex.Unlock() switch l.Mode { - case flags.ModeInplace: - if event.IsInplaceSupported() { - l.writeInplace(event) - } else { - l.writeAppend(event) - } - case flags.ModeJson: l.writeJson(event) diff --git a/libs/cmdio/message_event.go b/libs/cmdio/message_event.go index 55dc136cd4..19272ce071 100644 --- a/libs/cmdio/message_event.go +++ b/libs/cmdio/message_event.go @@ -7,7 +7,3 @@ type MessageEvent struct { func (event *MessageEvent) String() string { return event.Message } - -func (event *MessageEvent) IsInplaceSupported() bool { - return false -} diff --git a/libs/flags/progress_format.go b/libs/flags/progress_format.go index a0c0a8d30b..bf6e393809 100644 --- a/libs/flags/progress_format.go +++ b/libs/flags/progress_format.go @@ -11,7 +11,6 @@ type ProgressLogFormat string var ( ModeAppend = ProgressLogFormat("append") - ModeInplace = ProgressLogFormat("inplace") ModeJson = ProgressLogFormat("json") ModeDefault = ProgressLogFormat("default") ) @@ -29,8 +28,6 @@ func (p *ProgressLogFormat) Set(s string) error { switch lower { case ModeAppend.String(): *p = ProgressLogFormat(ModeAppend.String()) - case ModeInplace.String(): - *p = ProgressLogFormat(ModeInplace.String()) case ModeJson.String(): *p = ProgressLogFormat(ModeJson.String()) case ModeDefault.String(): @@ -41,7 +38,6 @@ func (p *ProgressLogFormat) Set(s string) error { default: valid := []string{ ModeAppend.String(), - ModeInplace.String(), ModeJson.String(), } return fmt.Errorf("accepted arguments are [%s]", strings.Join(valid, ", ")) @@ -57,7 +53,6 @@ func (p *ProgressLogFormat) Type() string { func (p *ProgressLogFormat) Complete(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return []string{ "append", - "inplace", "json", }, cobra.ShellCompDirectiveNoFileComp } diff --git a/libs/flags/progress_format_test.go b/libs/flags/progress_format_test.go index 55d6da65b9..46014960e4 100644 --- a/libs/flags/progress_format_test.go +++ b/libs/flags/progress_format_test.go @@ -16,7 +16,7 @@ func TestProgressFormatSet(t *testing.T) { // invalid arg err := p.Set("foo") - assert.ErrorContains(t, err, "accepted arguments are [append, inplace, json]") + assert.ErrorContains(t, err, "accepted arguments are [append, json]") // set json err = p.Set("json") @@ -43,17 +43,4 @@ func TestProgressFormatSet(t *testing.T) { err = p.Set("APPEND") assert.NoError(t, err) assert.Equal(t, "append", p.String()) - - // set inplace - err = p.Set("inplace") - assert.NoError(t, err) - assert.Equal(t, "inplace", p.String()) - - err = p.Set("Inplace") - assert.NoError(t, err) - assert.Equal(t, "inplace", p.String()) - - err = p.Set("INPLACE") - assert.NoError(t, err) - assert.Equal(t, "inplace", p.String()) } From 8c3f4b769bd3ffdfe50e24668182e4d4e7b012f3 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 23 Oct 2025 17:09:26 +0200 Subject: [PATCH 2/2] Add entry to NEXT_CHANGELOG.md --- NEXT_CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 4f895598c2..8a1908225e 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -6,6 +6,8 @@ ### CLI +* Remove `inplace` mode for the `--progress-format` flag. ([#3811](https://github.com/databricks/cli/pull/3811)) + ### Dependency updates ### Bundles