-
Notifications
You must be signed in to change notification settings - Fork 2k
Added "outputLogs" Configuration Option #1332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This commit adds a new configuration option "outputLogs" which will be used to set the default value of the `--output-logs` run option.
This commit refactors the task log output to use the new `util.TaskOutputMode`and removes the `LogMode` type used previously.
@ObliviousHarmony is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
As an aside, this is my first contribution to a project written in Go. While I've played around with it in the past, I have never really done anything meaningful with it. Whether this pull request moves forward or not, please let me know where I've gone wrong and what I should improve next time! Critical feedback is the path to quality code after all. |
@@ -0,0 +1,34 @@ | |||
package util |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with the util
package for this file because the status.go
file feels similar in nature. Does that sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be better off in runcache
alongside the flag-parsing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I touched on this in the PR description, but, I needed to put this in another package to avoid the circular dependency in the turbo_json.go
validation.
From a structural perspective, it also feels better elsewhere since it's not necessarily about runcache
output so much as it is about the display of task output. It's true all of that output currently lives in runcache
, but, ideologically this feels distinct enough to live in another package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I missed the circular dependency issue. I guess util
is probably fine for now. I expect the actual fix will involve moving turbo_json.go
out of the fs
package, but I think that is beyond the scope of this PR.
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! I think this is a good idea, and I like the idea of getting to per-task configuration as well.
As a general structural comment on the code, I think we can do the following:
- Change
TaskOutputMode
to a Go-style enum, withFullTaskOutput
as the zero value. - Implement
UnmarshalJSON
forTaskOutputMode
. This is where you can do error checking that a valid string has been provided and convert from the string to the enum value. You can see an example of this pattern here which gets called for each task definition when we are deserializing the pipeline.
I think that will let us skip the validation step as well as specifying a bunch of default value.
@@ -0,0 +1,34 @@ | |||
package util |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be better off in runcache
alongside the flag-parsing code.
cli/internal/run/run_test.go
Outdated
runcacheOpts: runcache.Opts{}, | ||
scopeOpts: scope.Opts{}, | ||
runcacheOpts: runcache.Opts{ | ||
TaskOutputMode: util.FullTaskOutput, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Go, whenever possible, we try to make the zero-value for a variable or field in a struct meaningful. You can see this in practice with CacheHitLogsMode
and CacheMissLogsMode
in this struct. This is using Go's version of an enum
, which is defined here. iota
maps to 0, so that when a LogsMode
is not specified, we default to FullLogs
.
I think we want the same to apply to this new field. That likely means following a similar pattern of using an enum and translating the string inputs to enum values.
That will let us avoid all the changes setting a default value of FullTaskOutput
, as that would be the zero value for the new field.
This commit replaces the TaskOutputMode strings with an appropriate enum and related parsing. It now uses UnmarshalJSON to parse as well and so we can remove `ValidateTurboJSON` entirely.
This commit moves the root "outputLogs" configuration option into the pipeline config. It also renames it to "outputMode" to better reflect the nature of the option.
After scrolling through all of the changed files in GitHub, I found other instances of unnecessary changes. Since this PR went through a few iterations, some things were left behind that weren't necessary.
Thanks for the review and the comprehensive information @gsoltis. Originally I had avoided using an enum because I didn't want to convert back and forth between representations. I've gone ahead and renamed "outputLogs" to "outputMode" and moved it into the pipeline configuration. I played around with it and I like that it's more expressive. I'm much happier with the PR now and it feels like some of those rougher edges are cleaned up. Thanks again for the guidance here! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely coming along. Can you add to turbo_json_test.go
with the new values?
I also included a note about trying to resolve the output mode by the time it's passed to runcache
, so we aren't worrying about overrides by that point, but it's probably not a requirement to get this merged. Once we've finished our cobra
migration, we'll likely tackle viper
which should hopefully help us properly account for precedence among flags, config files, and environment variables. So, either way, the resolution of the output mode is likely to get revisited in the nearish future.
@@ -0,0 +1,34 @@ | |||
package util |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I missed the circular dependency issue. I guess util
is probably fine for now. I expect the actual fix will involve moving turbo_json.go
out of the fs
package, but I think that is beyond the scope of this PR.
This removes the need to deal with strings outside of the "--output-logs" value handling.
Thanks, @gsoltis. I've changed the |
…s plumbing from run
@ObliviousHarmony going through this, I think I may have given you a bad suggestion. We do in fact need to know whether or not the passed-in logs mode is an override, because when we go to make the decision about the logs mode for a particular task, we need to know if there has been a global override or not, otherwise the task's configuration should take precedence. I think the consequence of having to plumb the flags throughout the execution path makes it fairly clear. Since I sent you down the wrong path, I'm going to send a PR to your branch shortly with my proposed fix for the situation. You can accept or adapt as you see fit. The rest of the PR looks great. |
I agree with your assessment @gsoltis; passing With that said, I don't mind the approach of getting the override state into the Sorry, I think my desire to land this PR and inexperience with Go has made me a bit more agreeable than I normally am 😄 In the future, I'll be more outspoken about any disagreements or friction I feel with a suggestion. |
As for knowing whether it was overridden or not @gsoltis, is using a pointer to a scalar type an acceptable way to have an optional member? You let the flag setting handle validation while still knowing whether the option is an override or not. I'm not sure what kind of memory allocation implications come with that though. You're allocating the memory for the struct but when you set it, you need to allocate memory for the enum too. Does that matter? |
Mark TaskOutputModeOverride as optional in runcache.Opts
I just saw your PR @gsoltis, that answers my question 😄 I forgot that |
Description
This pull request builds on #822 by adding an
"outputLogs"
option toturbo.json
. We've found that the default behavior is very noisy and doesn't provide any value to us. Rather than appending--output-logs
every time we use a command, it would be nice to set the configuration to do this by default.--output-logs
overrides the configuration option.cacheHitLogsMode
andcacheMissLogsMode
into a single type. Given the logic around this behavior, it seemed unnecessary to maintain two separate internal options when they provided no clear benefit.LogsMode
into theutil
package to avoid circular dependencies inrun
,runcache
, andfs
.TaskOutputMode
naming because it feels like we're talking more about the task output than the log output. As far as the end-user is concerned, whether it's cached or not, this is the output of thepackage.json
task."outputLogs": "full"
to rootturbo.json
for clarity.--output-logs
.Questions
"taskOutputMode"
for consistency and a better description of what it does?"build"
task has no output but another does. I'm not sure if there's any use to that though?Testing Instructions
./turbow.sh run build
, behavior should match the current"outputLogs"
inturbo.json
to"none"
./turbow.sh run build
, behavior should match current with--output-logs=none
"outputLogs"
inturbo.json
to"hash-only"
./turbow.sh run build
, behavior should match current with--output-logs=hash-only
"outputLogs"
inturbo.json
to"new-only"
./turbow.sh run build
, behavior should match current with--output-logs=new-only
./turbow.sh run build --output-logs=none
, behavior should match current with--output-logs=none
--output-logs
inputs and verify that they override the configuration and match the current