-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: ignore concurrency if not specified #7182
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7182 +/- ##
==========================================
- Coverage 70.48% 68.47% -2.01%
==========================================
Files 515 560 +45
Lines 23150 26461 +3311
==========================================
+ Hits 16317 18119 +1802
- Misses 5776 7090 +1314
- Partials 1057 1252 +195
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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.
WDYT about extracting the minConcurrency logic into a separately testable function?
pkg/skaffold/build/builder_mux.go
Outdated
} | ||
concurrency := *b.Concurrency() | ||
|
||
// set mux concurrency to be the minimum of all builders' concurrency. (concurrency = 0 means unlimited) |
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.
Add that 0
merits special treatment: if there are two configs A and B, and A has concurrency 0 and B has concurrency 1, then the "minimum" should be 1 since it's the most stringent.
pkg/skaffold/build/builder_mux.go
Outdated
if b.Concurrency() == nil { | ||
continue | ||
} |
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 doesn't seem to be equivalent. Consider if I have two modules, A and B. Both have local build-envs. A does not specify a build concurrency. B specified concurrency 0. I believe that prior to this change, A would have had a default concurrency of 1 set which would have resulted in the minConcurrency==1 (serialized). Whereas now A would be skippedI think we will have minConcurrency 0.
Honestly, I think we should separate this out into a function whose results can be tested for different scenarios.
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.
@briandealwis I verified via this test here that if B specified concurrency 0 and A does not set the concurrency, then it default to 1.
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.
don't have much to add that brian didn't already catch
Fixes: #7181
Description
The build concurrency should be set to the minimum defined concurrency across all configs. Previously, imported configs that didn't define concurrency were defaulting to the value of 1, thereby lowering the overall concurrency to 1.
We remove setting this default value in the config, rather if the resolved build concurrency is defaulted to 1 at the end.
Testing instructions
examples/multi-config-microservices
set the rootskaffold.yaml
config's build concurrency.skaffold build -v DEBUG
. The output should have the following log lines showing that the build concurrency was updated to that value: