fix concurrency issue in multi-config #5646
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5646 +/- ##
=======================================
Coverage 70.53% 70.53%
=======================================
Files 410 410
Lines 15624 15631 +7
=======================================
+ Hits 11020 11026 +6
- Misses 3791 3794 +3
+ Partials 813 811 -2
Continue to review full report at Codecov.
|
| if len(b.Artifacts) > 0 { | ||
| b.LocalBuild.Concurrency = &constants.DefaultLocalConcurrency | ||
| } |
There was a problem hiding this comment.
What if we instead pushed this to the local builder itself, so that it determined the default concurrency at build time?
(And isn't this duplicating what is being done in the call to withLocalBuild() in defaults.go?)
There was a problem hiding this comment.
this is a function in the test file that sets up the expected config
There was a problem hiding this comment.
thought about pushing this to local.Builder but it doesn't maintain the []*latest.Artifact slice and it seemed more convoluted to first set the default to 1 but later ignore it in the method.
There was a problem hiding this comment.
Couldn't we just change this method in the local builder to return 1?
skaffold/pkg/skaffold/build/local/local.go
Lines 60 to 65 in 89b8315
There was a problem hiding this comment.
If we don't set it in default.Set then we'll need a reference to the []*latest.Artifact per config in local.Builder to decide if it should return 0 or 1 by default (0 if len(artifacts) == 0, otherwise 1). That plumbing seems more convoluted that this fix.
There was a problem hiding this comment.
Oh I didn't clue in that this definition was in a test — I thought it was the implementation of withLocalBuilder in pkg/skaffold/schema/defaults/ 🤦
| if len(b.Artifacts) > 0 { | ||
| b.LocalBuild.Concurrency = &constants.DefaultLocalConcurrency | ||
| } |
There was a problem hiding this comment.
Oh I didn't clue in that this definition was in a test — I thought it was the implementation of withLocalBuilder in pkg/skaffold/schema/defaults/ 🤦
Fixes #5645