Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ociarchive: Add new ArchiveFileNotFoundError #2123

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

cgwalters
Copy link
Contributor

@cgwalters cgwalters commented Sep 18, 2023

This is for containers/skopeo#2114
which is in turn a dependency of coreos/rpm-ostree#4598

Basically I want to map ENOENT to a clear error, because the build
tooling wants to treat "target image is not present" differently
from "DNS lookup failed" or "we got EPERM".

There's a bit of code motion here because we need to move
the os.Open() call before creating a temporary directory.

Signed-off-by: Colin Walters walters@verbum.org


@cgwalters
Copy link
Contributor Author

What's the right place to unit test this?

@cgwalters
Copy link
Contributor Author

Or, to address containers/skopeo#2114 (comment) I guess we could create a new ImageStructureNotFound (or ImageContainerNotFound) error or so. But I'd still want to map that underneath OpenImageOptional in skopeo's API.

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, to address containers/skopeo#2114 (comment) I guess we could create a new ImageStructureNotFound (or ImageContainerNotFound) error or so.

I’d prefer that; ImageNotFound is explicitly documented not to match this situation

But I'd still want to map that underneath OpenImageOptional in skopeo's API.

Sure, that works for me.

oci/archive/oci_transport.go Outdated Show resolved Hide resolved
@cgwalters cgwalters changed the title ociarchive: Return ImageNotFound if the file isn't present ociarchive: Add new ImageStructureNotFound Sep 18, 2023
oci/archive/oci_src.go Outdated Show resolved Hide resolved
oci/archive/oci_transport.go Outdated Show resolved Hide resolved
@cgwalters cgwalters force-pushed the add-image-not-found branch 2 times, most recently from 627c94a to 6e50f30 Compare September 18, 2023 15:12
oci/archive/oci_src.go Outdated Show resolved Hide resolved
oci/archive/oci_src.go Outdated Show resolved Hide resolved
@mtrmac
Copy link
Collaborator

mtrmac commented Sep 18, 2023

What's the right place to unit test this?

I’d place it in a new oci/archive/oci_src.go:TestNewImageSource. (TestReferenceNewImage or a new unit test for createUntarTempDir would also work, but *_transport.go attract a bit more attention due to signature/identity concerns, so limiting the scope of those files comes with less concern.)

@cgwalters
Copy link
Contributor Author

OK, updated for latest review and added a unit test.

@cgwalters cgwalters changed the title ociarchive: Add new ImageStructureNotFound ociarchive: Add new ArchiveFileNotFoundError Sep 19, 2023
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

oci/archive/oci_src_test.go Outdated Show resolved Hide resolved
oci/archive/oci_src_test.go Outdated Show resolved Hide resolved
oci/archive/oci_src_test.go Outdated Show resolved Hide resolved
@cgwalters cgwalters force-pushed the add-image-not-found branch 2 times, most recently from f134ce2 to 3d335e8 Compare September 19, 2023 21:57
@cgwalters
Copy link
Contributor Author

(Hm, the "PRs must be targeting tip to merge" on this repo is a little surprising)

This is for containers/skopeo#2114
which is in turn a dependency of coreos/rpm-ostree#4598

Basically I want to map ENOENT to a clear error, because the build
tooling wants to treat "target image is not present" differently
from "DNS lookup failed" or "we got EPERM".

There's a bit of code motion here because we need to move
the `os.Open()` call before creating a temporary directory.

Signed-off-by: Colin Walters <walters@verbum.org>
@mtrmac mtrmac merged commit 5fb9820 into containers:main Sep 20, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants