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

add stream methods #60

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

add stream methods #60

wants to merge 2 commits into from

Conversation

vaidikcode
Copy link

Signed-off-by: vaidikcode [email protected]

Description

  • Added streaming methods for blob operations:

    • pushBlobStream() for uploads
    • getBlobStream() for downloads
    • DigestUtils.sha256(InputStream) for streaming hash calculation
  • Updated artifact operations to use streams:

Closes #25 .

Testing done

No, but I will add tests if needed.

### Submitter checklist
- [ ] If an issue exists, it is well described and linked in the description
- [ ] The description of this pull request is detailed and explain why this pull request is needed
- [ ] The changeset is on a specific branch. Using `feature/` for new feature, or improvements ; Using `fix/` for bug fixes ; Using `docs/` for any documentation changes.
- [ ] If required, the documentation has been updated
- [ ] There is automated tests to cover the code change / addition or an explanation why there is no tests in the description.

Signed-off-by: vaidikcode <[email protected]>
@vaidikcode vaidikcode requested a review from jonesbusy as a code owner January 29, 2025 13:13
@jonesbusy
Copy link
Collaborator

Thanks

Test are failing

Error:    RegistryTest.testShouldPushAndPullMinimalArtifact:187 » Oras Response code: 400
Error:    RegistryTest.testShouldPushCompressedDirectory:226 » Oras Failed to calculate digest

Did you run test locally

Make sure that all new code is tested

@vaidikcode
Copy link
Author

I have not run the tests locally yet, but after fixing the problems, I will push those changes.

@jonesbusy
Copy link
Collaborator

I have not run the tests locally yet, but after fixing the problems, I will push those changes.

This is the minimum before opening a PR. Thanks for your understanding

I value high quality PR (Don't matter the size) with test implemented and that are valuable for future consumer.

While I appreciate contribution, my feeling is that we open PR as fast as possible hoping this gets merged (I suspect it's because for GsoC proposal or LFX mentorship). But it doesn't work like that.

I will only accept PR that

  • Are well documented
  • Have test (unit, integration, testcontainer, wiremock etc...)
  • Don't introduce regression
  • Explose clear APIs and don't break encapsulation
  • Introduce real value (for example did we test pushing a very large artifact on an OCI registry? Like 5Gi or 10Gi artifact?)

Note I was using also https://github.com/jonesbusy/oras-java-cli to perform some interactive test. Feel free to use it if you want to test SDK performance againts a local registry

@vaidikcode
Copy link
Author

vaidikcode commented Jan 29, 2025

Thank you for your detailed feedback and for setting clear expectations for contributions. I completely understand the importance of submitting high-quality PRs that are well-documented, properly tested
Before finalizing the PR, I wanted to check:

1- Would you prefer me to add specific types of tests or cover additional scenarios?
2- Are there any other changes or refinements you'd like to see in the code before I proceed?

@vaidikcode
Copy link
Author

I think adding new unit tests, particularly for these new methods, would be beneficial. What do you think? Should I work on that?

@jonesbusy
Copy link
Collaborator

Yes all new code must be tested if possible. Thanks

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.

Ensure memory consuption when pushing/pulling large artifact
2 participants