fix: output line with --output-logs=new-only should reflect actual behavior #982
Conversation
|
@chelkyl is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/vercel/turbo-site/GT97RyaMS1iPVJCF6awEZE9WqdBt |
|
@chelkyl awesome job with these PRs! Can you send me either a Twitter or Discord DM? |
|
Need to update e2e.ts tests with updated string output |
|
@jaredpalmer Thanks for the notice, fixed! |
cli/internal/run/run.go
Outdated
| //Writing to Stdout only the "cache hit, replaying output" line | ||
| scan.Scan() | ||
| prefixUi.Output(ui.StripAnsi(string(scan.Bytes()))) | ||
| prefixUi.Output(fmt.Sprintf("%scache hit, suppressing output %s", logPrefix, ui.Dim(hash))) |
There was a problem hiding this comment.
This line seems a little weird: we're outputting from something called prefixUi, but we're also manually adding a prefix.
I think we might want to refactor this a little to be less confusing. I think what's going on is the following:
- the calling code has two
Uiinstances, one prefixed, one not - we're not actually passing in the prefixed one, because we expect the prefix to be in the logs already
- we're then manually adding the prefix in the case where we are synthesizing a log line
Here's my proposal:
- check the
outputLogsModebefore callingreplayLogs. - If we're in 'NoLogs' mode, great, we're done
- create a separate function to just output the cache hit for
HashLogsmode. We're already not using the file contents in that case, so don't even open the file. Pass in the correctUiinstance that already has the prefix baked in (targetUi, I think). It doesn't even need to be in a goroutine, we could just call it directly. In fact, it might not even need to be function, it might even be just a single line - If we're in
FullLogsmode, use the existingreplayLogs - Remove the case for
NoLogsinreplayLogs
There was a problem hiding this comment.
That refactor really cleans it up. Thanks for the suggestion!
by extracting output logs mode logic out
05c132f to
329bae5
Compare
from feat(colors): add options for forcing color/no-color
cli/internal/run/run.go
Outdated
| case FullLogs: | ||
| if e.rs.Opts.stream && fs.FileExists(filepath.Join(e.rs.Opts.cwd, logFileName)) { | ||
| e.logReplayWaitGroup.Add(1) | ||
| go replayLogs(targetLogger, e.ui, e.rs.Opts, logFileName, hash, &e.logReplayWaitGroup, false) |
There was a problem hiding this comment.
I realize it was here earlier, but let's get rid of the false for silent too. I think this is the only place where replayLogs is called.
cli/internal/run/run.go
Outdated
| @@ -796,7 +796,7 @@ func (c *RunCommand) executeDryRun(engine *core.Scheduler, g *completeGraph, rs | |||
| } | |||
|
|
|||
| // Replay logs will try to replay logs back to the stdout | |||
| func replayLogs(logger hclog.Logger, prefixUi cli.Ui, runOptions *RunOptions, logFileName, hash string, wg *sync.WaitGroup, silent bool, outputLogsMode LogsMode) { | |||
| func replayLogs(logger hclog.Logger, prefixUi cli.Ui, runOptions *RunOptions, logFileName, hash string, wg *sync.WaitGroup, silent bool) { | |||
There was a problem hiding this comment.
I think we can drop silent, it's only ever hard-coded to false.
I think we also want to rename prefixUi, since it's not prefixed.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Closes #972