Skip to content
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

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

Closed
wants to merge 16 commits into from

Conversation

tm1000
Copy link
Contributor

@tm1000 tm1000 commented Oct 27, 2022

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 tm1000 requested a review from a team as a code owner October 27, 2022 20:14
@vercel
Copy link

vercel bot commented Oct 27, 2022

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

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Oct 27, 2022

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)

Copy link
Contributor

@mehulkar mehulkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but I'll want to run it by @nathanhammond as well since it's a new option!

@nathanhammond
Copy link
Contributor

nathanhammond commented Oct 31, 2022

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.

@tm1000
Copy link
Contributor Author

tm1000 commented Nov 1, 2022

@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 tm1000 requested a review from a team as a code owner November 2, 2022 20:06
@tm1000
Copy link
Contributor Author

tm1000 commented Nov 2, 2022

@nathanhammond changes applied as requested.

@tm1000 tm1000 changed the title Fixes #2280 adds a client timeout Fixes #1993 and #2096 adds a client timeout and additional logging Nov 2, 2022
@tm1000
Copy link
Contributor Author

tm1000 commented Nov 15, 2022

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

@mehulkar
Copy link
Contributor

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

@tm1000
Copy link
Contributor Author

tm1000 commented Nov 15, 2022

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

Copy link
Member

@chris-olszewski chris-olszewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, thanks for implementing this. I just have a few minor changes to keep the code consistent with the rest of the codebase.

cli/internal/client/client.go Outdated Show resolved Hide resolved
cli/internal/cmdutil/cmdutil.go Outdated Show resolved Hide resolved
cli/internal/cmdutil/cmdutil.go Outdated Show resolved Hide resolved
cli/internal/config/config_file.go Outdated Show resolved Hide resolved
tm1000 and others added 2 commits November 15, 2022 15:45
Co-authored-by: Chris Olszewski <chrisdolszewski@gmail.com>
Co-authored-by: Chris Olszewski <chrisdolszewski@gmail.com>
@chris-olszewski chris-olszewski self-assigned this Nov 16, 2022
@tm1000
Copy link
Contributor Author

tm1000 commented Nov 16, 2022

@chris-olszewski completed as requested

@tm1000
Copy link
Contributor Author

tm1000 commented Nov 17, 2022

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

@chris-olszewski
Copy link
Member

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

@tm1000
Copy link
Contributor Author

tm1000 commented Feb 7, 2023

Hey @chris-olszewski.

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

@nathanhammond
Copy link
Contributor

@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 nathanhammond mentioned this pull request Feb 28, 2023
@tm1000
Copy link
Contributor Author

tm1000 commented Feb 28, 2023

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

@tm1000 tm1000 closed this Feb 28, 2023
kodiakhq bot pushed a commit that referenced this pull request Mar 3, 2023
This commit authored in part by @tm1000.

Closes #1993
Closes #2096
Closes #2428

Notes:
- You may want to look at the original diff:
https://github.com/vercel/turbo/pull/2428/files?w=1
- I've completely removed `pflag` at this point. It was only being used
in tests.
- I've added a new "config" in Rust land, for the API client. It only
accepts `timeout` right now.
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.

4 participants