fix: recursive push → pull round-trip (#1951 follow-up)#2045
Open
robertream wants to merge 8 commits intooras-project:mainfrom
Open
fix: recursive push → pull round-trip (#1951 follow-up)#2045robertream wants to merge 8 commits intooras-project:mainfrom
robertream wants to merge 8 commits intooras-project:mainfrom
Conversation
robertream
requested review from
TerryHowe,
Wwwsylvia,
sabre1041,
sajayantony and
shizhMSFT
as code owners
robertream
force-pushed
the
fix/recursive-push-pull-bug
branch
2 times, most recently
from
390f16c to
e154d89
Compare
Signed-off-by: Terry Howe <terrylhowe@gmail.com> Signed-off-by: Robert Ream <154010+robertream@users.noreply.github.com>
Signed-off-by: Terry Howe <terrylhowe@gmail.com> Signed-off-by: Robert Ream <154010+robertream@users.noreply.github.com>
Add comprehensive unit tests for the recursive push feature to improve test coverage of cmd/oras/root/push.go. Signed-off-by: Terry Howe <terrylhowe@gmail.com> Signed-off-by: Robert Ream <154010+robertream@users.noreply.github.com>
…e push When buildNode stored child Image Index descriptors in the parent index's manifests list, it called maps.Copy to attach directory annotations — but createIndex had already set ocispec.AnnotationTitle=<dirname> on the returned descriptor, so maps.Copy preserved it. oras pull treats any descriptor with AnnotationTitle as a named file and calls metadataHandler.OnFilePulled, which tries to open/create the path. When a directory with that name already exists on disk this fails with: "open <path>/<dirname>: is a directory" Fix: - createIndex no longer adds AnnotationTitle to its annotations or the returned descriptor - buildNode assigns (replaces) directory annotations instead of merging, ensuring no stale AnnotationTitle can survive from the inner call Adds TestBuildFromNode_noAnnotationTitleOnIndexDescriptors as a regression test that reproduces the exact failure mode reported in PR oras-project#1951 review. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Robert Ream <154010+robertream@users.noreply.github.com>
Adds three e2e specs under "pushing recursively with --recursive flag": 1. Push a nested directory tree and pull it back, verifying every file's content is preserved — this is the primary regression test for the "open <path>: is a directory" bug from PR oras-project#1951 review. 2. Error: --recursive with a file argument (not a directory). 3. Error: --recursive combined with --config. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Robert Ream <154010+robertream@users.noreply.github.com>
store.Add with an empty media type falls through to oras-go's default of application/vnd.oci.image.layer.v1.tar, which implies tar content. Files pushed with --recursive are raw blobs, not tarballs, so application/octet-stream is the correct media type. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Robert Ream <154010+robertream@users.noreply.github.com>
…exes Two related bugs caused oras pull to fail with "open <path>: is a directory" after a recursive push: 1. Image Index descriptors stored in parent indexes had AnnotationTitle set (fixed in previous commit). 2. When a directory has both files and subdirs, buildNode creates chunk manifests for the files and stores them in the parent index. These chunk manifest descriptors had AnnotationTitle = node.Name. oras pull's PostCopy calls content.Successors on each copied index, then calls OnFilePulled for every successor that has AnnotationTitle. This tried to create a file named "<dirname>" which fails when that directory already exists on disk. Fix: remove AnnotationTitle from chunk manifest annotations in buildNode. The individual file layers already carry AnnotationTitle values that drive actual file restoration during pull. Also skip path validation for --recursive push: the directory argument is never used as an annotation title, so the absolute-path check was incorrectly blocking temp-dir paths in e2e tests (and legitimate use of absolute paths in general). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Robert Ream <154010+robertream@users.noreply.github.com>
The --config+--recursive conflict and file-not-directory checks are pure flag validation with no registry interaction. They are already covered by Test_pushCmd_PreRunE_recursiveValidation in push_test.go. Remove the redundant e2e specs so the e2e suite only tests what requires a registry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Robert Ream <154010+robertream@users.noreply.github.com>
robertream
force-pushed
the
fix/recursive-push-pull-bug
branch
from
e154d89 to
c691d99
Compare
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
2 participants
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.
Summary
This is a follow-up to #1951 (recursive directory push), fixing a pull bug identified in that PR's review and adding a round-trip e2e test.
Changes
fix: don't set
AnnotationTitleon Image Index descriptors — child directory index descriptors stored in parent index manifests lists were carryingorg.opencontainers.image.title = <dirname>.oras pull'sPostCopycallsOnFilePulledfor any successor withAnnotationTitle, which tried to create a file named<dirname>— failing withopen <path>: is a directory.fix: don't set
AnnotationTitleon chunk manifest descriptors — when a directory has both files and subdirs, the chunk manifest for its files was stored in the parent index withAnnotationTitle = <dirname>, causing the same pull failure.fix: use
application/octet-streamfor file blobs — raw (non-tarred) files were being pushed withapplication/vnd.oci.image.layer.v1.tarwhich incorrectly implies tar format.fix: skip absolute path validation for
--recursive— the directory argument is never stored as an annotation title, so the absolute-path guard was incorrectly blocking valid usage.test(e2e): round-trip test that pushes a nested directory tree and verifies pulled file contents match exactly.
refactor(e2e): flag validation tests (
--configconflict, file-not-dir) moved to unit tests since they need no registry.Test plan
go test ./...passesoras push --recursive <ref> <dir>followed byoras pull <ref>reproduces original directory structure