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.

Sign up for GitHub

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

Allow for filepaths to include #161

Merged
merged 4 commits into from
Oct 9, 2024
Merged

Allow for filepaths to include #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f86fb1
Allow for filepaths to include
rhatdan Oct 7, 2024
3d82f52
add tests from PR review
tarilabs Oct 9, 2024
2188a58
remove test case agreed likely a mistake
tarilabs Oct 9, 2024
c72f79f
bump the version in oras/version.py and add a line to the changelog
tarilabs Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ and **Merged pull requests**. Critical items to know are:
The versions coincide with releases on pip. Only major versions will be released as tags on Github.

## [0.0.x](https://github.com/oras-project/oras-py/tree/main) (0.0.x)
- allow for filepaths to include `:` (0.2.22)
- release request (0.2.21)
- add missing basic auth data for request token function in token auth backend (0.2.2)
- re-enable chunked upload (0.2.1)
Expand Down
65 changes: 59 additions & 6 deletions oras/tests/test_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,65 @@ def test_annotated_registry_push(tmp_path, registry, credentials, target):
)


@pytest.mark.with_auth(False)
def test_file_contains_column(tmp_path, registry, credentials, target):
"""
Test for file containing column symbol
"""
client = oras.client.OrasClient(hostname=registry, insecure=True)
artifact = os.path.join(here, "artifact.txt")
assert os.path.exists(artifact)

# file containing `:`
try:
contains_column = here / "some:file"
with open(contains_column, "w") as f:
f.write("hello world some:file")

res = client.push(files=[contains_column], target=target)
assert res.status_code in [200, 201]

files = client.pull(target, outdir=tmp_path / "download")
download = str(tmp_path / "download/some:file")
assert download in files
assert oras.utils.get_file_hash(
str(contains_column)
) == oras.utils.get_file_hash(download)
finally:
contains_column.unlink()

# file containing `:` as prefix, pushed with type
try:
contains_column = here / ":somefile"
with open(contains_column, "w") as f:
f.write("hello world :somefile")

res = client.push(files=[f"{contains_column}:text/plain"], target=target)
assert res.status_code in [200, 201]

files = client.pull(target, outdir=tmp_path / "download")
download = str(tmp_path / "download/:somefile")
assert download in files
assert oras.utils.get_file_hash(
str(contains_column)
) == oras.utils.get_file_hash(download)
finally:
contains_column.unlink()

# error: file does not exist
with pytest.raises(FileNotFoundError):
client.push(files=[".doesnotexist"], target=target)

with pytest.raises(FileNotFoundError):
client.push(files=[":doesnotexist"], target=target)

with pytest.raises(FileNotFoundError, match=r".*does:not:exists .*"):
client.push(files=["does:not:exists:text/plain"], target=target)

with pytest.raises(FileNotFoundError, match=r".*does:not:exists .*"):
client.push(files=["does:not:exists:text/plain+ext"], target=target)


@pytest.mark.with_auth(False)
def test_chunked_push(tmp_path, registry, credentials, target):
"""
Expand Down Expand Up @@ -130,12 +189,6 @@ def test_parse_manifest(registry):
assert ref == "path/to/config"
assert content_type == "application/vnd.oci.image.config.v1+json"

testref = "path/to/config:application/vnd.oci.image.config.v1+json:extra"
remote = oras.provider.Registry(hostname=registry, insecure=True)
ref, content_type = remote._parse_manifest_ref(testref)
assert ref == "path/to/config"
assert content_type == "application/vnd.oci.image.config.v1+json:extra"

testref = "/dev/null:application/vnd.oci.image.manifest.v1+json"
ref, content_type = remote._parse_manifest_ref(testref)
assert ref == "/dev/null"
Expand Down
5 changes: 3 additions & 2 deletions oras/utils/fileio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,8 @@ def split_path_and_content(ref: str) -> PathAndOptionalContent:
: return: A Tuple of the path in the reference, and the content-type if one found,
otherwise None.
"""
if ":" not in ref:

if os.path.exists(ref) or ":" not in ref:
Copy link

Choose a reason for hiding this comment

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

Another case to consider handling here... ref's that start with colon. I don't think they should be treated as a naked content type, right? Instead, I guess the entire ref should be considered as a path?

Copy link
Contributor

Choose a reason for hiding this comment

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

@halfline you mean like :<content-type> ? Wouldn't that akin to an empty / missing file and a content type (is that allowed / is there a use case for it)? I'm wondering if we should specifically not allow that (e.g., if the path doesn't exist but the string starts with : maybe we need to raise an error.

Copy link

Choose a reason for hiding this comment

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

so in my mind, if someone e.g. does

╎❯ omlmd push http://quay.io/foo/foo:latest '.file-that-doesnt-exist'

and

╎❯ omlmd push http://quay.io/foo/foo:latest ':file-that-doesnt-exist'

in both cases the error message should be the same and the overall behavior should be the same.

But that's just a driveby subjective opinion. I haven't looked deeply into how split_path_and_content is used or could be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the path doesn't exist and the colon is being used weirdly like that to not separate a filepath from a content type, I think an error should be raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree if the file does not exist with or with the colon removed then raise the ENOENT error.
If the file exists with the colon then use it. If the file exists with the colon and a legitimate option then just use the file and let the user deal with it. IE change the names of the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rhatdan the discussion below is getting out of scope. If you'd like to add any additional tweaks based on the discussion here, please do. You'll also need to bump the version in oras/version.py and add a line to the changelog. When those are done I'll review once more and we can merge, and follow up discussion about reference formats, etc. should go in an issue.

return PathAndOptionalContent(ref, None)

if pathlib.Path(ref).drive:
Expand All @@ -370,5 +371,5 @@ def split_path_and_content(ref: str) -> PathAndOptionalContent:
)
return PathAndOptionalContent(ref, None)
else:
path_content_list = ref.split(":", 1)
path_content_list = ref.rsplit(":", 1)
return PathAndOptionalContent(path_content_list[0], path_content_list[1])
2 changes: 1 addition & 1 deletion oras/version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
__copyright__ = "Copyright The ORAS Authors."
__license__ = "Apache-2.0"

__version__ = "0.2.21"
__version__ = "0.2.22"
AUTHOR = "Vanessa Sochat"
EMAIL = "[email protected]"
NAME = "oras"
Expand Down
Loading