Fix parsing concurrency as a number or percent #1371
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
Also fixes an invalid memory access when rendering the usage text for |
There was a problem hiding this comment.
Only thing intended to block is the typo in code you didn't touch and adding a few more edge case tests.
Any percentage value as default concurrency is also better than "10" so I don't have strong opinions about what it should be, but I see "10" as wrong (we're always shelling out, so it's not goroutine vs. threads; they're all threads underneath the hood).
| flags.AddFlag(&pflag.Flag{ | ||
| Name: "concurrency", | ||
| Usage: "Limit the concurrency of task execution. Use 1 for serial (i.e. one-at-a-time) execution.", | ||
| DefValue: "10", |
There was a problem hiding this comment.
I'm tempted to make this 50% or 100%... 10 is such an arbitrary number and has totally different outcomes on CI vs. local dev boxes.
| }, | ||
| cacheOpts: cache.Opts{ | ||
| Dir: defaultCacheFolder, | ||
| Workers: 10, |
There was a problem hiding this comment.
Relatedly, we don't even give you a choice here, which should probably be mapped to the same calculated concurrency value:
turborepo/cli/internal/config/config.go
Line 210 in 2419e2f
There was a problem hiding this comment.
Agreed, we should wire it up at some point. It also probably should not live in a top-level config, since it's not relevant to every command.
| @@ -389,3 +408,26 @@ func Test_taskSelfRef(t *testing.T) { | |||
| t.Fatalf("expected to failed to build task graph: %v", err) | |||
| } | |||
| } | |||
|
|
|||
| func TestUsageText(t *testing.T) { | |||
There was a problem hiding this comment.
Thanks for catching and testing this. ❤️
696088c to
d2d16d7
Compare
Fixes #1336
Implemented a custom value to accept either a number or percentage.