Compute tags in parallel#1820
Conversation
dgageot
requested review from
balopat,
nkubala,
priyawadhwa and
r2d4
as code owners
dgageot
force-pushed
the
tags-parallel
branch
2 times, most recently
from
4461448 to
0ee6e8b
Compare
Codecov Report
@@ Coverage Diff @@
## master #1820 +/- ##
==========================================
+ Coverage 47.71% 47.78% +0.07%
==========================================
Files 143 143
Lines 6438 6447 +9
==========================================
+ Hits 3072 3081 +9
Misses 3086 3086
Partials 280 280
Continue to review full report at Codecov.
|
dgageot
force-pushed
the
tags-parallel
branch
4 times, most recently
from
097d67a to
8f162b8
Compare
| for i := range artifacts { | ||
| tagErrs[i] = make(chan tagErr, 1) | ||
|
|
||
| i := i |
There was a problem hiding this comment.
am I missing something here?
There was a problem hiding this comment.
The variable needs to be captured otherwise every go routine will use the same value
There was a problem hiding this comment.
makes sense. just looks kinda funny lol
| tag, err := r.Tagger.GenerateFullyQualifiedImageName(artifact.Workspace, imageName) | ||
| case t := <-tagErrs[i]: | ||
| tag := t.tag | ||
| err := t.err |
There was a problem hiding this comment.
should we combine errors here in the case that multiple tag generation runs fail?
There was a problem hiding this comment.
We could do that. That's not what the current sequential code is doing so I didn't want to also add that complexity.
There was a problem hiding this comment.
that's fair. I don't think it's a big deal.
nkubala
left a comment
nkubala
left a comment
There was a problem hiding this comment.
red X for kokoro is good, right? ❓ ❌ ❓
|
@nkubala actually, I don't know about this one... |
|
@nkubala This one is a real timeout. The build took more than 10 minutes but there's no visible issue. Let me push a PR with a timeout set to 15 minutes |
Signed-off-by: David Gageot <david@gageot.net>
4 participants
Signed-off-by: David Gageot david@gageot.net