Fix tarball publisher ctx handling #545
Conversation
Previously, we'd use the ctx from the Build invocation, but because remote stashes the remote.WithContext option, when we later go to fetch the actual base image to write it out to a tarball, it will fail because the Build ctx was cancelled. For whatever reason, NewGo takes a ctx, so this change propagates the badness from remote so we can use the NewGo ctx when we fetch base images.
| @@ -826,7 +830,9 @@ func updatePath(cf *v1.ConfigFile, appPath string) { | |||
| // Build implements build.Interface | |||
| func (g *gobuild) Build(ctx context.Context, s string) (Result, error) { | |||
| // Determine the appropriate base image for this import path. | |||
| baseRef, base, err := g.getBase(ctx, s) | |||
| // We use the overall gobuild.ctx because the Build ctx gets cancelled | |||
There was a problem hiding this comment.
Why does the Build ctx get cancelled early?
There was a problem hiding this comment.
https://pkg.go.dev/golang.org/x/sync/errgroup#WithContext
We want to cancel outstanding builds if we encounter an error, so we use errgroup.WithContext, but the ctx is also cancelled once this returns: https://github.com/google/ko/blob/a56047a2a1d500a646393b4a62d1d509bd5575e4/pkg/commands/resolver.go#L415
Codecov Report
@@ Coverage Diff @@
## main #545 +/- ##
=======================================
Coverage 48.16% 48.17%
=======================================
Files 42 42
Lines 2182 2186 +4
=======================================
+ Hits 1051 1053 +2
- Misses 954 955 +1
- Partials 177 178 +1
Continue to review full report at Codecov.
|
Previously, we'd use the ctx from the Build invocation, but because
remote stashes the remote.WithContext option, when we later go to fetch
the actual base image to write it out to a tarball, it will fail because
the Build ctx was cancelled.
For whatever reason, NewGo takes a ctx, so this change propagates the
badness from remote so we can use the NewGo ctx when we fetch base
images.