From 36449632ee7e49abfba6cc30824a1b84cc9a1059 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 23 Oct 2025 15:49:46 +0200 Subject: [PATCH 1/5] 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/5] 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 From dc02f404b4c1418eac32bd67fdf31df182630fec Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 24 Oct 2025 10:44:25 +0200 Subject: [PATCH 3/5] Remove unused JSON progress logging mode The JSON mode for progress logging was designed to output structured JSON events for machine parsing. However, this mode had several limitations: it prevented interactive prompts (Ask/AskSelect methods would error), required --auto-approve for destroy commands, and added complexity with JSON marshaling in the logger. The feature provided minimal practical value as most CLI usage is interactive, and the default append mode is sufficient for both human and machine consumption. Since the mode added unnecessary complexity without clear benefits, it has been removed in favor of the simpler append-only mode. --- NEXT_CHANGELOG.md | 1 + cmd/bundle/destroy.go | 11 ------- cmd/pipelines/destroy.go | 11 ------- cmd/pipelines/root/progress_logger.go | 2 +- cmd/root/progress_logger.go | 2 +- libs/cmdio/logger.go | 41 ++------------------------- libs/cmdio/logger_test.go | 16 ----------- libs/flags/progress_format.go | 12 ++------ libs/flags/progress_format_test.go | 15 +--------- 9 files changed, 9 insertions(+), 102 deletions(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 8a1908225e..4ec7ffe14f 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -7,6 +7,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)) ### Dependency updates diff --git a/cmd/bundle/destroy.go b/cmd/bundle/destroy.go index 678f3bfb39..17ccf91ae2 100644 --- a/cmd/bundle/destroy.go +++ b/cmd/bundle/destroy.go @@ -12,9 +12,7 @@ import ( "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" "github.com/databricks/cli/cmd/root" - "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/flags" "github.com/databricks/cli/libs/logdiag" "github.com/spf13/cobra" "golang.org/x/term" @@ -68,15 +66,6 @@ Typical use cases: return errors.New("please specify --auto-approve to skip interactive confirmation checks for non tty consoles") } - // Check auto-approve is selected for json logging - logger, ok := cmdio.FromContext(ctx) - if !ok { - return errors.New("progress logger not found") - } - if logger.Mode == flags.ModeJson && !autoApprove { - return errors.New("please specify --auto-approve since selected logging format is json") - } - phases.Initialize(ctx, b) if logdiag.HasError(ctx) { return root.ErrAlreadyPrinted diff --git a/cmd/pipelines/destroy.go b/cmd/pipelines/destroy.go index 3b8dabd997..ff7ac502e9 100644 --- a/cmd/pipelines/destroy.go +++ b/cmd/pipelines/destroy.go @@ -12,9 +12,7 @@ import ( "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/cmd/bundle/utils" "github.com/databricks/cli/cmd/root" - "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/flags" "github.com/databricks/cli/libs/logdiag" "github.com/spf13/cobra" "golang.org/x/term" @@ -56,15 +54,6 @@ func destroyCommand() *cobra.Command { return errors.New("please specify --auto-approve to skip interactive confirmation checks for non tty consoles") } - // Check auto-approve is selected for json logging - logger, ok := cmdio.FromContext(ctx) - if !ok { - return errors.New("progress logger not found") - } - if logger.Mode == flags.ModeJson && !autoApprove { - return errors.New("please specify --auto-approve since selected logging format is json") - } - phases.Initialize(ctx, b) if logdiag.HasError(ctx) { return root.ErrAlreadyPrinted diff --git a/cmd/pipelines/root/progress_logger.go b/cmd/pipelines/root/progress_logger.go index 74ae01b71d..6e9e5da712 100644 --- a/cmd/pipelines/root/progress_logger.go +++ b/cmd/pipelines/root/progress_logger.go @@ -44,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, json)") + flags.Var(&f.ProgressLogFormat, "progress-format", "format for progress logs (append)") 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 f7185b92fd..a8b4a446e2 100644 --- a/cmd/root/progress_logger.go +++ b/cmd/root/progress_logger.go @@ -43,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, json)") + flags.Var(&f.ProgressLogFormat, "progress-format", "format for progress logs (append)") flags.MarkHidden("progress-format") cmd.RegisterFlagCompletionFunc("progress-format", f.Complete) return &f diff --git a/libs/cmdio/logger.go b/libs/cmdio/logger.go index 873f3be2c0..53a0ccc894 100644 --- a/libs/cmdio/logger.go +++ b/libs/cmdio/logger.go @@ -3,8 +3,6 @@ package cmdio import ( "bufio" "context" - "encoding/json" - "errors" "fmt" "io" "os" @@ -17,7 +15,7 @@ import ( // This is the interface for all io interactions with a user type Logger struct { - // Mode for the logger. One of (append, json). + // Mode for the logger. One of (append). Mode flags.ProgressLogFormat // Input stream (eg. stdin). Answers to questions prompted using the Ask() method @@ -121,10 +119,6 @@ func splitAtLastNewLine(s string) (string, string) { } func (l *Logger) AskSelect(question string, choices []string) (string, error) { - if l.Mode == flags.ModeJson { - return "", errors.New("question prompts are not supported in json mode") - } - // Promptui does not support multiline prompts. So we split the question. first, last := splitAtLastNewLine(question) _, err := l.Writer.Write([]byte(first)) @@ -150,10 +144,6 @@ func (l *Logger) AskSelect(question string, choices []string) (string, error) { } func (l *Logger) Ask(question, defaultVal string) (string, error) { - if l.Mode == flags.ModeJson { - return "", errors.New("question prompts are not supported in json mode") - } - // Add default value to question prompt. if defaultVal != "" { question += fmt.Sprintf(` [%s]`, defaultVal) @@ -180,34 +170,9 @@ func (l *Logger) Ask(question, defaultVal string) (string, error) { return ans, nil } -func (l *Logger) writeJson(event Event) { - b, err := json.MarshalIndent(event, "", " ") - if err != nil { - // we panic because there we cannot catch this in jobs.RunNowAndWait - panic(err) - } - _, _ = l.Writer.Write(b) - _, _ = l.Writer.Write([]byte("\n")) -} - -func (l *Logger) writeAppend(event Event) { - _, _ = l.Writer.Write([]byte(event.String())) - _, _ = l.Writer.Write([]byte("\n")) -} - func (l *Logger) Log(event Event) { l.mutex.Lock() defer l.mutex.Unlock() - switch l.Mode { - case flags.ModeJson: - l.writeJson(event) - - case flags.ModeAppend: - l.writeAppend(event) - - default: - // we panic because errors are not captured in some log sides like - // jobs.RunNowAndWait - panic("unknown progress logger mode: " + l.Mode.String()) - } + _, _ = l.Writer.Write([]byte(event.String())) + _, _ = l.Writer.Write([]byte("\n")) } diff --git a/libs/cmdio/logger_test.go b/libs/cmdio/logger_test.go index 2aecfbdac1..bf1802f5d9 100644 --- a/libs/cmdio/logger_test.go +++ b/libs/cmdio/logger_test.go @@ -1,27 +1,11 @@ package cmdio import ( - "context" "testing" - "github.com/databricks/cli/libs/flags" "github.com/stretchr/testify/assert" ) -func TestAskFailedInJsonMode(t *testing.T) { - l := NewLogger(flags.ModeJson) - _, err := l.Ask("What is your spirit animal?", "") - assert.ErrorContains(t, err, "question prompts are not supported in json mode") -} - -func TestAskChoiceFailsInJsonMode(t *testing.T) { - l := NewLogger(flags.ModeJson) - ctx := NewContext(context.Background(), l) - - _, err := AskSelect(ctx, "what is a question?", []string{"b", "c", "a"}) - assert.EqualError(t, err, "question prompts are not supported in json mode") -} - func TestSplitAtLastNewLine(t *testing.T) { first, last := splitAtLastNewLine("hello\nworld") assert.Equal(t, "hello\n", first) diff --git a/libs/flags/progress_format.go b/libs/flags/progress_format.go index bf6e393809..53adf2ca7a 100644 --- a/libs/flags/progress_format.go +++ b/libs/flags/progress_format.go @@ -11,7 +11,6 @@ type ProgressLogFormat string var ( ModeAppend = ProgressLogFormat("append") - ModeJson = ProgressLogFormat("json") ModeDefault = ProgressLogFormat("default") ) @@ -28,19 +27,13 @@ func (p *ProgressLogFormat) Set(s string) error { switch lower { case ModeAppend.String(): *p = ProgressLogFormat(ModeAppend.String()) - case ModeJson.String(): - *p = ProgressLogFormat(ModeJson.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(ModeJson.String()) + *p = ProgressLogFormat(ModeAppend.String()) default: - valid := []string{ - ModeAppend.String(), - ModeJson.String(), - } - return fmt.Errorf("accepted arguments are [%s]", strings.Join(valid, ", ")) + return fmt.Errorf("accepted arguments are [%s]", ModeAppend.String()) } return nil } @@ -53,6 +46,5 @@ func (p *ProgressLogFormat) Type() string { func (p *ProgressLogFormat) Complete(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return []string{ "append", - "json", }, cobra.ShellCompDirectiveNoFileComp } diff --git a/libs/flags/progress_format_test.go b/libs/flags/progress_format_test.go index 46014960e4..f2a5d738f1 100644 --- a/libs/flags/progress_format_test.go +++ b/libs/flags/progress_format_test.go @@ -16,20 +16,7 @@ func TestProgressFormatSet(t *testing.T) { // invalid arg err := p.Set("foo") - assert.ErrorContains(t, err, "accepted arguments are [append, json]") - - // set json - err = p.Set("json") - assert.NoError(t, err) - assert.Equal(t, "json", p.String()) - - err = p.Set("JSON") - assert.NoError(t, err) - assert.Equal(t, "json", p.String()) - - err = p.Set("Json") - assert.NoError(t, err) - assert.Equal(t, "json", p.String()) + assert.ErrorContains(t, err, "accepted arguments are [append]") // set append err = p.Set("append") From a48589f44ca5186bf9511c7a4dbd1daf574b5d88 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 24 Oct 2025 14:12:37 +0200 Subject: [PATCH 4/5] Migrate progress logger functions to use cmdio directly --- bundle/run/job.go | 24 +-- bundle/run/job_test.go | 3 - bundle/run/pipeline.go | 8 +- bundle/run/pipeline_test.go | 2 - bundle/statemgmt/state_push_test.go | 3 +- cmd/root/root.go | 5 +- .../ssh/internal/proxy/client_server_test.go | 5 +- experimental/ssh/internal/setup/setup_test.go | 13 +- libs/cmdio/compat.go | 137 ++++++++++++ libs/cmdio/compat_test.go | 195 ++++++++++++++++++ libs/cmdio/error_event.go | 9 - libs/cmdio/event.go | 6 - libs/cmdio/logger.go | 137 ------------ libs/cmdio/logger_test.go | 29 --- libs/cmdio/message_event.go | 9 - 15 files changed, 354 insertions(+), 231 deletions(-) create mode 100644 libs/cmdio/compat.go create mode 100644 libs/cmdio/compat_test.go delete mode 100644 libs/cmdio/error_event.go delete mode 100644 libs/cmdio/event.go delete mode 100644 libs/cmdio/logger_test.go delete mode 100644 libs/cmdio/message_event.go diff --git a/bundle/run/job.go b/bundle/run/job.go index 08a3459210..1978852daf 100644 --- a/bundle/run/job.go +++ b/bundle/run/job.go @@ -74,9 +74,7 @@ func (r *jobRunner) logFailedTasks(ctx context.Context, runId int64) { log.Errorf(ctx, "task %s failed. Unable to fetch error trace: %s", red(task.TaskKey), err) continue } - if progressLogger, ok := cmdio.FromContext(ctx); ok { - progressLogger.Log(progress.NewTaskErrorEvent(task.TaskKey, taskInfo.Error, taskInfo.ErrorTrace)) - } + cmdio.Log(ctx, progress.NewTaskErrorEvent(task.TaskKey, taskInfo.Error, taskInfo.ErrorTrace)) log.Errorf(ctx, "Task %s failed!\nError:\n%s\nTrace:\n%s", red(task.TaskKey), taskInfo.Error, taskInfo.ErrorTrace) } else { @@ -89,9 +87,8 @@ func (r *jobRunner) logFailedTasks(ctx context.Context, runId int64) { // jobRunMonitor tracks state for a single job run and provides callbacks // for monitoring progress. type jobRunMonitor struct { - ctx context.Context - prevState *jobs.RunState - progressLogger *cmdio.Logger + ctx context.Context + prevState *jobs.RunState } // onProgress is the single callback that handles all state tracking and logging. @@ -104,7 +101,7 @@ func (m *jobRunMonitor) onProgress(info *jobs.Run) { // First time we see this run. if m.prevState == nil { log.Infof(m.ctx, "Run available at %s", info.RunPageUrl) - m.progressLogger.Log(progress.NewJobRunUrlEvent(info.RunPageUrl)) + cmdio.Log(m.ctx, progress.NewJobRunUrlEvent(info.RunPageUrl)) } // No state change: do not log. @@ -125,7 +122,7 @@ func (m *jobRunMonitor) onProgress(info *jobs.Run) { RunName: info.RunName, State: *info.State, } - m.progressLogger.Log(event) + cmdio.Log(m.ctx, event) log.Info(m.ctx, event.String()) } @@ -151,15 +148,8 @@ func (r *jobRunner) Run(ctx context.Context, opts *Options) (output.RunOutput, e w := r.bundle.WorkspaceClient() - // callback to log progress events. Called on every poll request - progressLogger, ok := cmdio.FromContext(ctx) - if !ok { - return nil, errors.New("no progress logger found") - } - monitor := &jobRunMonitor{ - ctx: ctx, - progressLogger: progressLogger, + ctx: ctx, } waiter, err := w.Jobs.RunNow(ctx, *req) @@ -171,7 +161,7 @@ func (r *jobRunner) Run(ctx context.Context, opts *Options) (output.RunOutput, e details, err := w.Jobs.GetRun(ctx, jobs.GetRunRequest{ RunId: waiter.RunId, }) - progressLogger.Log(progress.NewJobRunUrlEvent(details.RunPageUrl)) + cmdio.Log(ctx, progress.NewJobRunUrlEvent(details.RunPageUrl)) return nil, err } diff --git a/bundle/run/job_test.go b/bundle/run/job_test.go index bf5ff79d54..5d6079a87c 100644 --- a/bundle/run/job_test.go +++ b/bundle/run/job_test.go @@ -9,7 +9,6 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/flags" "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/jobs" "github.com/stretchr/testify/mock" @@ -160,7 +159,6 @@ func TestJobRunnerRestart(t *testing.T) { b.SetWorkpaceClient(m.WorkspaceClient) ctx := cmdio.MockDiscard(context.Background()) - ctx = cmdio.NewContext(ctx, cmdio.NewLogger(flags.ModeAppend)) jobApi := m.GetMockJobsAPI() jobApi.EXPECT().ListRunsAll(mock.Anything, jobs.ListRunsRequest{ @@ -231,7 +229,6 @@ func TestJobRunnerRestartForContinuousUnpausedJobs(t *testing.T) { b.SetWorkpaceClient(m.WorkspaceClient) ctx := cmdio.MockDiscard(context.Background()) - ctx = cmdio.NewContext(ctx, cmdio.NewLogger(flags.ModeAppend)) jobApi := m.GetMockJobsAPI() diff --git a/bundle/run/pipeline.go b/bundle/run/pipeline.go index bc02e7afef..a3f243798c 100644 --- a/bundle/run/pipeline.go +++ b/bundle/run/pipeline.go @@ -106,13 +106,9 @@ func (r *pipelineRunner) Run(ctx context.Context, opts *Options) (output.RunOutp // setup progress logger and tracker to query events updateTracker := progress.NewUpdateTracker(pipelineID, updateID, w) - progressLogger, ok := cmdio.FromContext(ctx) - if !ok { - return nil, errors.New("no progress logger found") - } // Log the pipeline update URL as soon as it is available. - progressLogger.Log(progress.NewPipelineUpdateUrlEvent(w.Config.Host, updateID, pipelineID)) + cmdio.Log(ctx, progress.NewPipelineUpdateUrlEvent(w.Config.Host, updateID, pipelineID)) if opts.NoWait { return &output.PipelineOutput{ @@ -129,7 +125,7 @@ func (r *pipelineRunner) Run(ctx context.Context, opts *Options) (output.RunOutp return nil, err } for _, event := range events { - progressLogger.Log(&event) + cmdio.Log(ctx, &event) log.Info(ctx, event.String()) } diff --git a/bundle/run/pipeline_test.go b/bundle/run/pipeline_test.go index 4236fdd4f7..26ee850995 100644 --- a/bundle/run/pipeline_test.go +++ b/bundle/run/pipeline_test.go @@ -9,7 +9,6 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/flags" sdk_config "github.com/databricks/databricks-sdk-go/config" "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/pipelines" @@ -76,7 +75,6 @@ func TestPipelineRunnerRestart(t *testing.T) { b.SetWorkpaceClient(m.WorkspaceClient) ctx := cmdio.MockDiscard(context.Background()) - ctx = cmdio.NewContext(ctx, cmdio.NewLogger(flags.ModeAppend)) mockWait := &pipelines.WaitGetPipelineIdle[struct{}]{ Poll: func(time.Duration, func(*pipelines.GetPipelineResponse)) (*pipelines.GetPipelineResponse, error) { diff --git a/bundle/statemgmt/state_push_test.go b/bundle/statemgmt/state_push_test.go index 88bad10918..9b1624c034 100644 --- a/bundle/statemgmt/state_push_test.go +++ b/bundle/statemgmt/state_push_test.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" mockfiler "github.com/databricks/cli/internal/mocks/libs/filer" + "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/filer" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -51,7 +52,7 @@ func TestStatePush(t *testing.T) { identityFiler(mock), } - ctx := context.Background() + ctx := cmdio.MockDiscard(context.Background()) b := statePushTestBundle(t) // Write a stale local state file. diff --git a/cmd/root/root.go b/cmd/root/root.go index 9815d0288d..574b8c71c3 100644 --- a/cmd/root/root.go +++ b/cmd/root/root.go @@ -13,7 +13,6 @@ import ( "github.com/databricks/cli/internal/build" "github.com/databricks/cli/libs/cmdctx" - "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/dbr" "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/telemetry" @@ -140,9 +139,7 @@ Stack Trace: // Run the command cmd, err = cmd.ExecuteContextC(ctx) if err != nil && !errors.Is(err, ErrAlreadyPrinted) { - // If cmdio logger initialization succeeds, then this function logs with the - // initialized cmdio logger, otherwise with the default cmdio logger - cmdio.LogError(cmd.Context(), err) + fmt.Fprintf(cmd.ErrOrStderr(), "Error: %s\n", err.Error()) } // Log exit status and error diff --git a/experimental/ssh/internal/proxy/client_server_test.go b/experimental/ssh/internal/proxy/client_server_test.go index ec16bb3cb4..8aef0b7c53 100644 --- a/experimental/ssh/internal/proxy/client_server_test.go +++ b/experimental/ssh/internal/proxy/client_server_test.go @@ -14,13 +14,14 @@ import ( "testing" "time" + "github.com/databricks/cli/libs/cmdio" "github.com/gorilla/websocket" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func createTestServer(t *testing.T, maxClients int, shutdownDelay time.Duration) *httptest.Server { - ctx := t.Context() + ctx := cmdio.MockDiscard(t.Context()) connections := NewConnectionsManager(maxClients, shutdownDelay) proxyServer := NewProxyServer(ctx, connections, func(ctx context.Context) *exec.Cmd { // 'cat' command reads each line from stdin and sends it to stdout, so we can test end-to-end proxying. @@ -30,7 +31,7 @@ func createTestServer(t *testing.T, maxClients int, shutdownDelay time.Duration) } func createTestClient(t *testing.T, serverURL string, requestHandoverTick func() <-chan time.Time, errChan chan error) (io.WriteCloser, *testBuffer) { - ctx := t.Context() + ctx := cmdio.MockDiscard(t.Context()) clientInput, clientInputWriter := io.Pipe() clientOutput := newTestBuffer(t) wsURL := "ws" + serverURL[4:] diff --git a/experimental/ssh/internal/setup/setup_test.go b/experimental/ssh/internal/setup/setup_test.go index 478f20ab12..7c4cb20925 100644 --- a/experimental/ssh/internal/setup/setup_test.go +++ b/experimental/ssh/internal/setup/setup_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/databricks/cli/libs/cmdio" "github.com/databricks/databricks-sdk-go/experimental/mocks" "github.com/databricks/databricks-sdk-go/service/compute" "github.com/stretchr/testify/assert" @@ -16,7 +17,7 @@ import ( ) func TestValidateClusterAccess_SingleUser(t *testing.T) { - ctx := context.Background() + ctx := cmdio.MockDiscard(context.Background()) m := mocks.NewMockWorkspaceClient(t) clustersAPI := m.GetMockClustersAPI() @@ -29,7 +30,7 @@ func TestValidateClusterAccess_SingleUser(t *testing.T) { } func TestValidateClusterAccess_InvalidAccessMode(t *testing.T) { - ctx := context.Background() + ctx := cmdio.MockDiscard(context.Background()) m := mocks.NewMockWorkspaceClient(t) clustersAPI := m.GetMockClustersAPI() @@ -43,7 +44,7 @@ func TestValidateClusterAccess_InvalidAccessMode(t *testing.T) { } func TestValidateClusterAccess_ClusterNotFound(t *testing.T) { - ctx := context.Background() + ctx := cmdio.MockDiscard(context.Background()) m := mocks.NewMockWorkspaceClient(t) clustersAPI := m.GetMockClustersAPI() @@ -315,7 +316,7 @@ func TestUpdateSSHConfigFile_HandlesReadError(t *testing.T) { } func TestSetup_SuccessfulWithNewConfigFile(t *testing.T) { - ctx := context.Background() + ctx := cmdio.MockDiscard(context.Background()) tmpDir := t.TempDir() configPath := filepath.Join(tmpDir, "ssh_config") @@ -349,7 +350,7 @@ func TestSetup_SuccessfulWithNewConfigFile(t *testing.T) { } func TestSetup_SuccessfulWithExistingConfigFile(t *testing.T) { - ctx := context.Background() + ctx := cmdio.MockDiscard(context.Background()) tmpDir := t.TempDir() configPath := filepath.Join(tmpDir, "ssh_config") @@ -393,7 +394,7 @@ func TestSetup_SuccessfulWithExistingConfigFile(t *testing.T) { } func TestSetup_DoesNotOverrideExistingHost(t *testing.T) { - ctx := context.Background() + ctx := cmdio.MockDiscard(context.Background()) tmpDir := t.TempDir() configPath := filepath.Join(tmpDir, "ssh_config") diff --git a/libs/cmdio/compat.go b/libs/cmdio/compat.go new file mode 100644 index 0000000000..029cc5462d --- /dev/null +++ b/libs/cmdio/compat.go @@ -0,0 +1,137 @@ +package cmdio + +import ( + "context" + "fmt" + "io" + "strings" + + "github.com/manifoldco/promptui" +) + +/* +Temporary compatibility layer for the progress logger interfaces. +*/ + +// Log is a compatibility layer for the progress logger interfaces. +// It writes the string representation of the stringer to the error writer. +func Log(ctx context.Context, str fmt.Stringer) { + LogString(ctx, str.String()) +} + +// LogString is a compatibility layer for the progress logger interfaces. +// It writes the string to the error writer. +func LogString(ctx context.Context, str string) { + c := fromContext(ctx) + _, _ = io.WriteString(c.err, str) + _, _ = io.WriteString(c.err, "\n") +} + +// readLine reads a line from the reader and returns it without the trailing newline characters. +// It is unbuffered because cmdio's stdin is also unbuffered. +// If we were to add a [bufio.Reader] to the mix, we would need to update the other uses of the reader. +// Once cmdio's stdio is made to be buffered, this function can be removed. +func readLine(r io.Reader) (string, error) { + var b strings.Builder + buf := make([]byte, 1) + for { + n, err := r.Read(buf) + if n > 0 { + if buf[0] == '\n' { + break + } + if buf[0] != '\r' { + b.WriteByte(buf[0]) + } + } + if err != nil { + if b.Len() == 0 { + return "", err + } + break + } + } + return b.String(), nil +} + +// Ask is a compatibility layer for the progress logger interfaces. +// It prompts the user with a question and returns the answer. +func Ask(ctx context.Context, question, defaultVal string) (string, error) { + c := fromContext(ctx) + + // Add default value to question prompt. + if defaultVal != "" { + question += fmt.Sprintf(` [%s]`, defaultVal) + } + question += `: ` + + // Print prompt. + _, err := io.WriteString(c.err, question) + if err != nil { + return "", err + } + + // Read user input. Trim new line characters. + ans, err := readLine(c.in) + if err != nil { + return "", err + } + + // Return default value if user just presses enter. + if ans == "" { + return defaultVal, nil + } + + return ans, nil +} + +// AskYesOrNo is a compatibility layer for the progress logger interfaces. +// It prompts the user with a question and returns the answer. +func AskYesOrNo(ctx context.Context, question string) (bool, error) { + ans, err := Ask(ctx, question+" [y/n]", "") + if err != nil { + return false, err + } + return ans == "y", nil +} + +func splitAtLastNewLine(s string) (string, string) { + // Split at the newline character + if i := strings.LastIndex(s, "\n"); i != -1 { + return s[:i+1], s[i+1:] + } + // Return the original string if no newline found + return "", s +} + +// AskSelect is a compatibility layer for the progress logger interfaces. +// It prompts the user with a question and returns the answer. +func AskSelect(ctx context.Context, question string, choices []string) (string, error) { + c := fromContext(ctx) + + // Promptui does not support multiline prompts. So we split the question. + first, last := splitAtLastNewLine(question) + _, err := io.WriteString(c.err, first) + if err != nil { + return "", err + } + + // Note: by default this prompt uses os.Stdin and os.Stdout. + // This is contrary to the rest of the original progress logger + // functions that write to stderr. + prompt := promptui.Select{ + Label: last, + Items: choices, + HideHelp: true, + Templates: &promptui.SelectTemplates{ + Label: "{{.}}: ", + Selected: last + ": {{.}}", + }, + } + + _, ans, err := prompt.Run() + if err != nil { + return "", err + } + return ans, nil +} diff --git a/libs/cmdio/compat_test.go b/libs/cmdio/compat_test.go new file mode 100644 index 0000000000..1052e34d89 --- /dev/null +++ b/libs/cmdio/compat_test.go @@ -0,0 +1,195 @@ +package cmdio + +import ( + "errors" + "io" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCompat_readLine(t *testing.T) { + tests := []struct { + name string + reader io.Reader + want string + wantErr bool + errContain string + }{ + { + name: "basic line with LF", + reader: strings.NewReader("hello\n"), + want: "hello", + }, + { + name: "line with CRLF", + reader: strings.NewReader("hello\r\n"), + want: "hello", + }, + { + name: "line with only CR", + reader: strings.NewReader("hello\r"), + want: "hello", + }, + { + name: "empty line with LF", + reader: strings.NewReader("\n"), + want: "", + }, + { + name: "empty line with CRLF", + reader: strings.NewReader("\r\n"), + want: "", + }, + { + name: "line without newline at EOF", + reader: strings.NewReader("hello"), + want: "hello", + }, + { + name: "multi-line input stops at first newline", + reader: strings.NewReader("hello\nworld\n"), + want: "hello", + }, + { + name: "line with multiple CR characters", + reader: strings.NewReader("hello\r\rworld\n"), + want: "helloworld", + }, + { + name: "line with CR in middle", + reader: strings.NewReader("hello\rworld\n"), + want: "helloworld", + }, + { + name: "line with spaces and special chars", + reader: strings.NewReader("hello world!@#$%\n"), + want: "hello world!@#$%", + }, + { + name: "unicode characters", + reader: strings.NewReader("hello 世界\n"), + want: "hello 世界", + }, + { + name: "empty reader", + reader: strings.NewReader(""), + want: "", + wantErr: true, + errContain: "EOF", + }, + { + name: "reader with error", + reader: &errorReader{err: errors.New("read error")}, + want: "", + wantErr: true, + errContain: "read error", + }, + { + name: "reader with error after some data", + reader: &errorAfterNReader{data: []byte("hello"), n: 3, err: io.ErrUnexpectedEOF}, + want: "hel", + }, + { + name: "long line", + reader: strings.NewReader(strings.Repeat("a", 1000) + "\n"), + want: strings.Repeat("a", 1000), + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got, err := readLine(test.reader) + if test.wantErr { + require.Error(t, err) + if test.errContain != "" { + assert.Contains(t, err.Error(), test.errContain) + } + } else { + require.NoError(t, err) + } + assert.Equal(t, test.want, got) + }) + } +} + +// errorReader is a test helper that always returns an error +type errorReader struct { + err error +} + +func (e *errorReader) Read(p []byte) (n int, err error) { + return 0, e.err +} + +// errorAfterNReader is a test helper that returns data for n reads, then returns an error +type errorAfterNReader struct { + data []byte + n int + count int + err error +} + +func (e *errorAfterNReader) Read(p []byte) (n int, err error) { + if e.count >= e.n { + return 0, e.err + } + if len(p) == 0 { + return 0, nil + } + if e.count < len(e.data) { + p[0] = e.data[e.count] + e.count++ + return 1, nil + } + return 0, e.err +} + +func TestCompat_splitAtLastNewLine(t *testing.T) { + tests := []struct { + name string + input string + wantFirst string + wantLast string + }{ + { + name: "LF newline in middle", + input: "hello\nworld", + wantFirst: "hello\n", + wantLast: "world", + }, + { + name: "CRLF newline in middle", + input: "hello\r\nworld", + wantFirst: "hello\r\n", + wantLast: "world", + }, + { + name: "no newline", + input: "hello world", + wantFirst: "", + wantLast: "hello world", + }, + { + name: "newline at end", + input: "hello\nworld\n", + wantFirst: "hello\nworld\n", + wantLast: "", + }, + { + name: "newline at start", + input: "\nhello world", + wantFirst: "\n", + wantLast: "hello world", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + first, last := splitAtLastNewLine(tt.input) + assert.Equal(t, tt.wantFirst, first) + assert.Equal(t, tt.wantLast, last) + }) + } +} diff --git a/libs/cmdio/error_event.go b/libs/cmdio/error_event.go deleted file mode 100644 index 6a6dd8e795..0000000000 --- a/libs/cmdio/error_event.go +++ /dev/null @@ -1,9 +0,0 @@ -package cmdio - -type ErrorEvent struct { - Error string `json:"error"` -} - -func (event *ErrorEvent) String() string { - return "Error: " + event.Error -} diff --git a/libs/cmdio/event.go b/libs/cmdio/event.go deleted file mode 100644 index c3c09acbdc..0000000000 --- a/libs/cmdio/event.go +++ /dev/null @@ -1,6 +0,0 @@ -package cmdio - -type Event interface { - // convert event into human readable string - String() string -} diff --git a/libs/cmdio/logger.go b/libs/cmdio/logger.go index 53a0ccc894..66564a1c4a 100644 --- a/libs/cmdio/logger.go +++ b/libs/cmdio/logger.go @@ -2,15 +2,11 @@ package cmdio import ( "bufio" - "context" - "fmt" "io" "os" - "strings" "sync" "github.com/databricks/cli/libs/flags" - "github.com/manifoldco/promptui" ) // This is the interface for all io interactions with a user @@ -43,136 +39,3 @@ func Default() *Logger { Reader: *bufio.NewReader(os.Stdin), } } - -func Log(ctx context.Context, event Event) { - logger, ok := FromContext(ctx) - if !ok { - logger = Default() - } - logger.Log(event) -} - -func LogString(ctx context.Context, message string) { - logger, ok := FromContext(ctx) - if !ok { - logger = Default() - } - logger.Log(&MessageEvent{ - Message: message, - }) -} - -func LogError(ctx context.Context, err error) { - logger, ok := FromContext(ctx) - if !ok { - logger = Default() - } - logger.Log(&ErrorEvent{ - Error: err.Error(), - }) -} - -func Ask(ctx context.Context, question, defaultVal string) (string, error) { - logger, ok := FromContext(ctx) - if !ok { - logger = Default() - } - return logger.Ask(question, defaultVal) -} - -func AskYesOrNo(ctx context.Context, question string) (bool, error) { - logger, ok := FromContext(ctx) - if !ok { - logger = Default() - } - - // Add acceptable answers to the question prompt. - question += ` [y/n]` - - // Ask the question - ans, err := logger.Ask(question, "") - if err != nil { - return false, err - } - - if ans == "y" { - return true, nil - } - return false, nil -} - -func AskSelect(ctx context.Context, question string, choices []string) (string, error) { - logger, ok := FromContext(ctx) - if !ok { - logger = Default() - } - return logger.AskSelect(question, choices) -} - -func splitAtLastNewLine(s string) (string, string) { - // Split at the newline character - if i := strings.LastIndex(s, "\n"); i != -1 { - return s[:i+1], s[i+1:] - } - // Return the original string if no newline found - return "", s -} - -func (l *Logger) AskSelect(question string, choices []string) (string, error) { - // Promptui does not support multiline prompts. So we split the question. - first, last := splitAtLastNewLine(question) - _, err := l.Writer.Write([]byte(first)) - if err != nil { - return "", err - } - - prompt := promptui.Select{ - Label: last, - Items: choices, - HideHelp: true, - Templates: &promptui.SelectTemplates{ - Label: "{{.}}: ", - Selected: last + ": {{.}}", - }, - } - - _, ans, err := prompt.Run() - if err != nil { - return "", err - } - return ans, nil -} - -func (l *Logger) Ask(question, defaultVal string) (string, error) { - // Add default value to question prompt. - if defaultVal != "" { - question += fmt.Sprintf(` [%s]`, defaultVal) - } - question += `: ` - - // print prompt - _, err := l.Writer.Write([]byte(question)) - if err != nil { - return "", err - } - - // read user input. Trim new line characters - ans, err := l.Reader.ReadString('\n') - if err != nil { - return "", err - } - ans = strings.Trim(ans, "\n\r") - - // Return default value if user just presses enter - if ans == "" { - return defaultVal, nil - } - return ans, nil -} - -func (l *Logger) Log(event Event) { - l.mutex.Lock() - defer l.mutex.Unlock() - _, _ = l.Writer.Write([]byte(event.String())) - _, _ = l.Writer.Write([]byte("\n")) -} diff --git a/libs/cmdio/logger_test.go b/libs/cmdio/logger_test.go deleted file mode 100644 index bf1802f5d9..0000000000 --- a/libs/cmdio/logger_test.go +++ /dev/null @@ -1,29 +0,0 @@ -package cmdio - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestSplitAtLastNewLine(t *testing.T) { - first, last := splitAtLastNewLine("hello\nworld") - assert.Equal(t, "hello\n", first) - assert.Equal(t, "world", last) - - first, last = splitAtLastNewLine("hello\r\nworld") - assert.Equal(t, "hello\r\n", first) - assert.Equal(t, "world", last) - - first, last = splitAtLastNewLine("hello world") - assert.Equal(t, "", first) - assert.Equal(t, "hello world", last) - - first, last = splitAtLastNewLine("hello\nworld\n") - assert.Equal(t, "hello\nworld\n", first) - assert.Equal(t, "", last) - - first, last = splitAtLastNewLine("\nhello world") - assert.Equal(t, "\n", first) - assert.Equal(t, "hello world", last) -} diff --git a/libs/cmdio/message_event.go b/libs/cmdio/message_event.go deleted file mode 100644 index 19272ce071..0000000000 --- a/libs/cmdio/message_event.go +++ /dev/null @@ -1,9 +0,0 @@ -package cmdio - -type MessageEvent struct { - Message string `json:"message"` -} - -func (event *MessageEvent) String() string { - return event.Message -} From 50939cc4a796df3c038dfed329336ed12d0eed12 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 24 Oct 2025 14:26:09 +0200 Subject: [PATCH 5/5] Remove unused mutex field --- libs/cmdio/logger.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/libs/cmdio/logger.go b/libs/cmdio/logger.go index 66564a1c4a..679d31a935 100644 --- a/libs/cmdio/logger.go +++ b/libs/cmdio/logger.go @@ -4,7 +4,6 @@ import ( "bufio" "io" "os" - "sync" "github.com/databricks/cli/libs/flags" ) @@ -20,8 +19,6 @@ type Logger struct { // Output stream where the logger writes to Writer io.Writer - - mutex sync.Mutex } func NewLogger(mode flags.ProgressLogFormat) *Logger {