这是indexloc提供的服务,不要输入任何密码
Skip to content

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

Merged
merged 24 commits into from
Jun 9, 2022
Merged

Added "outputLogs" Configuration Option #1332

merged 24 commits into from
Jun 9, 2022

Conversation

ObliviousHarmony
Copy link
Contributor

@ObliviousHarmony ObliviousHarmony commented Jun 3, 2022

Description

This pull request builds on #822 by adding an "outputLogs" option to turbo.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.

  • Defaults to "full" in order to maintain backward compatibility with the current behavior.
  • --output-logs overrides the configuration option.
  • Merged cacheHitLogsMode and cacheMissLogsMode 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.
  • Refactored LogsMode into the util package to avoid circular dependencies in run, runcache, and fs.
    • I went with the 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 the package.json task.
  • Added "outputLogs": "full" to root turbo.json for clarity.
  • Documented the new configuration option and override behavior for --output-logs.

Questions

  • Should we name this "taskOutputMode" for consistency and a better description of what it does?
  • Should this be a task configuration rather than a top-level configuration? It would be a trivial change to make in this PR and would support having a configuration where a "build" task has no output but another does. I'm not sure if there's any use to that though?

Testing Instructions

  1. Check ./turbow.sh run build, behavior should match the current
  2. Change "outputLogs" in turbo.json to "none"
  3. Check ./turbow.sh run build, behavior should match current with --output-logs=none
  4. Change "outputLogs" in turbo.json to "hash-only"
  5. Check ./turbow.sh run build, behavior should match current with --output-logs=hash-only
  6. Change "outputLogs" in turbo.json to "new-only"
  7. Check ./turbow.sh run build, behavior should match current with --output-logs=new-only
  8. Check ./turbow.sh run build --output-logs=none, behavior should match current with --output-logs=none
  9. Optionally check other --output-logs inputs and verify that they override the configuration and match the current

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.
@vercel
Copy link

vercel bot commented Jun 3, 2022

@ObliviousHarmony is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

@ObliviousHarmony
Copy link
Contributor Author

ObliviousHarmony commented Jun 3, 2022

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
Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@vercel
Copy link

vercel bot commented Jun 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
turbo-site ⬜️ Ignored (Inspect) Jun 9, 2022 at 10:31PM (UTC)

Copy link
Contributor

@gsoltis gsoltis left a 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, with FullTaskOutput as the zero value.
  • Implement UnmarshalJSON for TaskOutputMode. 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
Copy link
Contributor

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.

runcacheOpts: runcache.Opts{},
scopeOpts: scope.Opts{},
runcacheOpts: runcache.Opts{
TaskOutputMode: util.FullTaskOutput,
Copy link
Contributor

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.
@ObliviousHarmony
Copy link
Contributor Author

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!

@ObliviousHarmony ObliviousHarmony requested a review from gsoltis June 6, 2022 23:06
Copy link
Contributor

@gsoltis gsoltis left a 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
Copy link
Contributor

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.
@ObliviousHarmony ObliviousHarmony requested a review from gsoltis June 7, 2022 18:32
@ObliviousHarmony
Copy link
Contributor Author

Thanks, @gsoltis. I've changed the "--output-logs" handling to remove the override string in favor of adding the overridden enum into the TaskCache struct. I'm not sure I like the ambiguity of having the TaskOutputMode member in the Opts struct, but, it looks like viper will help deal with that in a cleaner way. With that forward-looking context, however, I agree this solution is preferable.

@gsoltis
Copy link
Contributor

gsoltis commented Jun 8, 2022

@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.

@ObliviousHarmony
Copy link
Contributor Author

ObliviousHarmony commented Jun 8, 2022

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.

I agree with your assessment @gsoltis; passing flags all the way through the execution chain and using flags.Changed() felt like a hack given that we're validating it and parsing it into Opts already. Since RunE is called after the input validation, it seems wrong continuing to check flags after entering run.run().

With that said, I don't mind the approach of getting the override state into the TaskCache construction. A better approach would be to instead use rs.Opts.runCacheOpts from executeTasks() so that we're making use of the validated/parsed flags.

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.

@ObliviousHarmony
Copy link
Contributor Author

ObliviousHarmony commented Jun 8, 2022

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?

@ObliviousHarmony
Copy link
Contributor Author

I just saw your PR @gsoltis, that answers my question 😄 I forgot that RunCache ends up with the options too. I've merged your PR, made a small change, and merged from main.

@gsoltis gsoltis merged commit 6f4bbdd into vercel:main Jun 9, 2022
@ObliviousHarmony ObliviousHarmony deleted the add/output-config branch June 9, 2022 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants