fix: bind media type detection to patch context#1587
Open
sozercan wants to merge 1 commit into
Open
Conversation
sozercan
requested review from
ashnamehrotra,
jeremyrickard and
robert-cronin
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR binds container image media type detection to the patch operation’s caller context so local Docker/Podman inspection and remote registry lookups can be canceled or timed out (preventing hangs that bypass patch timeouts).
Changes:
- Added
utils.GetMediaTypeWithContext(ctx, ...)and threadedcontext.Contextinto Docker inspect, Podman CLI execution, andremote.Get. - Updated patch export decision logic to call a context-aware
shouldExportAsOCI(ctx, ...). - Updated unit tests to use the new context-aware helper signatures.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/mediatype.go | Introduces context-aware media type resolution and threads ctx into local/remote inspection paths. |
| pkg/utils/mediatype_test.go | Updates unit tests to call the context-aware helper signatures. |
| pkg/patch/single.go | Ensures OCI-vs-Docker export decision uses media type detection bound to the patch context. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 2
Comment on lines
+33
to
+34
|
|
||
| func GetMediaTypeWithContext(ctx context.Context, imageRef, runtime string) (string, error) { |
Comment on lines
168
to
173
| func TestPodmanMediaType_ImageNotFound(t *testing.T) { | ||
| // This test covers the case where podman inspect fails because image doesn't exist | ||
| // Since podman is available in the test environment, we test with non-existent image | ||
|
|
||
| _, err := podmanMediaType("non-existent-image:test") | ||
| _, err := podmanMediaType(context.Background(), "non-existent-image:test") | ||
| require.Error(t, err) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1587 +/- ##
==========================================
+ Coverage 38.80% 41.18% +2.37%
==========================================
Files 57 58 +1
Lines 9561 10114 +553
==========================================
+ Hits 3710 4165 +455
- Misses 5581 5653 +72
- Partials 270 296 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
context.Background()and remote lookups without a caller context, allowing Docker daemon or registry calls to hang indefinitely and bypass the patch timeout.Description
GetMediaTypeWithContext(ctx, imageRef, runtime)and keepsGetMediaType(...)as a backward-compatible wrapper that calls the new helper withcontext.Background().ctxinto local and remote resolution:localMediaTypeandpodmanMediaTypenow accept acontext.Contextand use it forcli.ImageInspectandexec.CommandContext, andremoteMediaTypepasses the context toremote.Getviaremote.WithContext(ctx).shouldExportAsOCI(ctx, ...), which usesGetMediaTypeWithContextso media type detection is bound to the patch timeout/cancellation.pkg/utils/mediatype_test.goto use the new context-aware signatures.Testing
go test ./pkg/utils -run 'MediaType'and the media-type related unit tests passed.go test ./pkg/utils ./pkg/patch; some tests failed in this environment due to missing/blocked Docker/registry dependencies rather than the media-type change.go testfor a patch test (TestDoesNotExist) under a timeout; it timed out in this CI-like environment, indicating external integration dependencies rather than the fix itself.Codex Task