Migrate progress logger functions to use cmdio directly #3818
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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: <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.
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.
pietern
requested review from
andrewnester,
anton-107,
denik and
shreyas-goenka
as code owners
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
commented
Oct 24, 2025
| // 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()) |
There was a problem hiding this comment.
No need for a dependency on cmdio here.
The previous implementation wrote directly to os.Stderr.
20 failing tests:
|
denik
approved these changes
Oct 24, 2025
pietern
requested review from
jefferycheng1,
kanterov and
lennartkats-db
as code owners
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
approved these changes
Oct 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
This continues the progress logger simplification by migrating from the
Logger.Log()event-based system to direct cmdio calls. This builds on #3811 and #3812, which made the progress logger effectively only write strings to stderr.Why
This simplifies the codebase by removing the (unused) event abstraction layer while maintaining functionality. The compatibility layer provides a clean migration path and will eventually be removed once the functionality in those functions have a dedicated new home.
Tests
Tests pass. Analysis of the diff suggests that before/after is functionally equivalent. A minor note is that we no longer have a mutex around
.Log()calls, but there are no concurrent calls to the function.