turbo icon indicating copy to clipboard operation
turbo copied to clipboard

Fixes #1993 and #2096 adds a client timeout and additional logging

Open tm1000 opened this issue 3 years ago • 12 comments

This fixes #1993 and 2 parts of #2096 by adding a client timeout config option and additionally logging any status that is not http.StatusOK.

This is my first attempt at go, FYI. So I'll take any feedback you can give me (and I'm also going to have my coworkers who work in Go daily give me some feedback too)

All tests passed and so did the e2e tests

cc: @mehulkar

tm1000 avatar Oct 27 '22 20:10 tm1000

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

A member of the Team first needs to authorize it.

vercel[bot] avatar Oct 27 '22 20:10 vercel[bot]

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

Name Status Preview Comments Updated
examples-designsystem-docs 🔄 Building (Inspect) Visit Preview Nov 17, 2022 at 5:27PM (UTC)
turbo-site ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Nov 17, 2022 at 5:27PM (UTC)
5 Ignored Deployments
Name Status Preview Comments Updated
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview Nov 17, 2022 at 5:27PM (UTC)
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview Nov 17, 2022 at 5:27PM (UTC)
examples-native-web ⬜️ Ignored (Inspect) Visit Preview Nov 17, 2022 at 5:27PM (UTC)
examples-nonmonorepo ⬜️ Ignored (Inspect) Visit Preview Nov 17, 2022 at 5:27PM (UTC)
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview Nov 17, 2022 at 5:27PM (UTC)

vercel[bot] avatar Oct 27 '22 20:10 vercel[bot]

Excellent first-pass at Go!

It's hard to balance how we should configure this with the reality of trying to migrate our cache syncing layer to the daemon. Below you'll see the chart I made while trying to rationalize what our behavior here is.

We're in a transition between "one-off" and "long-running" configuration as some of these things will be transitioned to the daemon. Environment vars are in some kind of dead zone between "one-off" and "permanent" ("permanent" on CI, weirdly mutable on local boxes so possibly one-off), and flags really only make sense in the one-off situation.

In a future story this is a daemon configuration in the generalized case, not a per-run configuration (though no-daemon's existence necessitates per-run configuration as well). And, timeout in particular isn't really a repository-level configuration, it's a user level configuration, or even (user + repository + remote cache) level configuration. (Your distance to the cache and speed of your box isn't a shared property.)

So, to get this landed:

  • Two naming things: --remote-cache-timeout and TURBO_REMOTE_CACHE_TIMEOUT. (Existing vars will be renamed to that strategy in some future change.)
  • We need tests to ensure that we prevent setting this property via the repository config. (I don't really care if the solution for preventing it is elegant, I just want to avoid creating an API we have to deprecate.)

Flags Env Config File Go: eventually read from where Comments
--no-daemon run.runOpts.noDaemon Output monitoring can cause turbo to skip restoration from a cache hit. This is related but not explicitly connected.
--force TURBO_FORCE runcache.Opts.SkipReads → runcache.RunCache.readsDisabled The Go names are far more expressive as to what actually happens.
--no-cache runcache.Opts.SkipWrites → runcache.RunCache.writesDisabled The Go names are far more expressive as to what actually happens.
--cache-dir cache.Opts.OverrideDir → cache.fsCache.cacheDirectory Either an absolute path, or a path relative to the repository root. Default: node_modules/.cache/turbo/
--remote-only TURBO_REMOTE_ONLY cache.Opts.SkipFilesystem Disables reading and writing from the filesystem cache. Does not guarantee that the remote cache is actually enabled. Turborepo still creates cache outputs, but possibly sends them to the no-op cache if remote caching isn’t available. This exists as a workaround for unbounded growth of the cache in CI which may do their own caching from the filesystem.
cache.Opts.SkipRemote Computed: user.token && (repo.teamid OR repo.teamslug) After those values are resolved from flags and env.
--cache-workers cache.Opts.Workers 10 by default. Makes cache.Put async. When async this enables turbo to proceed to the next build task prior to moving all files into the cache.
--preflight client.ApiClient.usePreflight These nine items are all “how do I talk to the server?” They’re inconsistently configurable.
--token TURBO_TOKEN VERCEL_ARTIFACTS_TOKEN user.token userConfig.Token() → repoConfig.GetRemoteConfig() -> client.remoteConfig -> client.ApiClient.token
TURBO_TEAMID VERCEL_ARTIFACTS_OWNER repo.teamid repoConfig.GetRemoteConfig() -> client.remoteConfig -> client.ApiClient.teamID TURBO_TEAMID was incidentally created during the Viper adoption; it is entirely undocumented.
--team TURBO_TEAM repo.teamslug repoConfig.GetRemoteConfig() -> client.remoteConfig -> client.ApiClient.teamSlug
--api TURBO_API repo.apiurl repoConfig.GetRemoteConfig() -> client.remoteConfig -> client.ApiClient.baseUrl
--login TURBO_LOGIN repo.loginurl config.RepoConfig.LoginUrl()
turbo.remoteCache.signature cache.Opts.RemoteCacheOpts.Signature This is 1:1 from the turbo.json parse.
turbo.remoteCache.teamId cache.Opts.RemoteCacheOpts.TeamID ❌ Parsed but unused. Not exposed in the schema.
TURBO_REMOTE_CACHE_SIGNATURE_KEY os.Getenv This is only grabbed at the moment of validation.

nathanhammond avatar Oct 31 '22 10:10 nathanhammond

@nathanhammond thanks!

Which file is the repository Config again? Just trying to keep all the Configs in mind. I think you're just saying don't allow the setting in the turbo.json Config file. Only allow it as a cli arg or environment variable

tm1000 avatar Nov 01 '22 18:11 tm1000

@tm1000

https://github.com/vercel/turbo/blob/6a0228125cafbbceed53ff17a7b25259422df568/cli/internal/config/config_file.go#L138

https://github.com/vercel/turbo/blob/6a0228125cafbbceed53ff17a7b25259422df568/cli/internal/config/config_file.go#L180-L182

So, we need to make sure that it is not read in from <repo-root>/.turbo/config.json.

nathanhammond avatar Nov 02 '22 06:11 nathanhammond

@nathanhammond changes applied as requested.

tm1000 avatar Nov 02 '22 20:11 tm1000

@nathanhammond @chris-olszewski @mehulkar hey team!

Any update on this? I know you're all extremely busy with the amazing things at Vercel but I hate having to build my own turborepo binary and distribute it with my packages. I'd love for this to be merged/included.

tm1000 avatar Nov 15 '22 17:11 tm1000

@tm1000 we're tracking this to land before the 1.7 release, but don't have a strong timeline on when that will be! In other words, even if we got this PR merged today, we don't have a strong sense of when the next release would be.

mehulkar avatar Nov 15 '22 18:11 mehulkar

@mehulkar hey that works for me! Good to know at least 1.7 :-D

tm1000 avatar Nov 15 '22 22:11 tm1000

@chris-olszewski completed as requested

tm1000 avatar Nov 16 '22 23:11 tm1000

@chris-olszewski so it's failing now due to massive changes that happened in config_file.go

tm1000 avatar Nov 17 '22 18:11 tm1000

@tm1000 I'll deal with the changes coming from #2382. You happen to have caught us in an awkward transition that should get a lot better once #2733 is ready and lands.

chris-olszewski avatar Nov 17 '22 18:11 chris-olszewski

Hey @chris-olszewski.

Any updates on this? Im currently bundling my own special build of turborepo to get around client timeouts.

tm1000 avatar Feb 07 '23 21:02 tm1000

@NicholasLYang I'm going to move to get this merged in, but it's going to have to be ported over to Rust land.

nathanhammond avatar Feb 28 '23 04:02 nathanhammond

@nathanhammond you're the best. Since it's all rust im going to close this and review the rust port. I <3 rust.

tm1000 avatar Feb 28 '23 18:02 tm1000