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

feat: pull recursively to include subject references #132

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tumido
Copy link
Contributor

@tumido tumido commented May 2, 2024

Hi folks 👋

I've noticed the Python Oras client library has kind of rudimentary support for attached artifacts. I'd like to address a couple of UX caveats in several PRs. Please let me know if it's heading in the right direction or if you have a different perspective.

Provide means to pull referred artifacts similar to oras pull --include-subject:

While oras-py supports including a subject when pushing a manifest, it does not support pulling the referenced artifact. I've inspired myself in the oras CLI, which has a --include-subject flag. I've tried to inspire myself in their pull implementation as well however, this would require a quite substantial rewrite of the oras-py pull logic - oras CLI uses a graph approach to collect blobs to be pulled, while oras-py simply iterates through the layers list. Therefore I've resorted to a recursive pull call based on the subject digest. This may raise some potential issues in the future since, well... it's recursion and it's synchronous. 🤷

This PR depends on #131

Signed-off-by: Tomas Coufal <tcoufal@redhat.com>
@tumido tumido changed the title feat: simplify how subjects are created from existing manifest feat: pull recursively to include subject references May 2, 2024
Signed-off-by: Tomas Coufal <tcoufal@redhat.com>
@@ -489,6 +489,28 @@ def push(uri, root):

</details>

<details>

<summary>Example of basic artifact attachment</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks to be redundant with the other PR - let's do that one first, merge and rebase here, and then we can do the review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know... that's why I left this PR as a draft for now. 🙂 It builds on top of the other PR. I wanted to share the direction with you but wanted to keep PRs atomic at the same time. 🙂 Let's finish the first PR first and I'll rebase on top of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! Thank you for your work on both, and looking forward to coming back to this one.

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.

2 participants