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(client): allow to set read_timeout and connect_timeout #137

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

SuperTux88
Copy link
Contributor

Allow to set read_timeout and connect_timeout on the underlying client. Otherwise the pull will just hang forever if you have an unstable connection and there was an interruption.

I wasn't sure if it would be useful to set some defaults here, but decided against it, because there was no timeout before (so no change for existing implementations) and there is also no timeout set in the reqwest client by default. And I also wasn't sure what useful defaults would be (since that maybe depends on your connection?). But let me know if you want me to set a default value for these.

@SuperTux88
Copy link
Contributor Author

So how should I handle this failed build about the licenses? I think this has nothing to do with my change. I changed the reqwest version to 0.12.4, because the read_timeout was only just added to this release (see release notes). But since there is no commited Cargo.lock, this version should already have been used before, as this release is already 2 Month old. I just wanted to make the version requirement more explicit (so in case you still have an old Cargo.lock locally, it updates the version).

But as there is no commited Cargo.lock, it also just uses the newest version of all the other things. And it looks like there was a new version of idna which got updated from 0.5.0 to 1.0.0 3 days ago, and this new version has a whole bunch of new dependencies, one of which also brings in this new license, which isn't allowed yet?

So, should I just add the license to the allowed list of licenses, or should I pin idna to 0.5.0 again, so it uses the old version? Or should I just ignore the issue about the build and you'll deal with that outside of this PR, as it's not related?

@thomastaylor312
Copy link
Contributor

@SuperTux88 You beat me by 6 minutes 😆. I just pushed up a fix: #138

Copy link
Contributor

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

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

This is great! As soon as #138 merges, you should be able to rebase on top for passing tests and I'll merge!

Signed-off-by: Benjamin Neff <benjamin@coding4coffee.ch>
@SuperTux88
Copy link
Contributor Author

@thomastaylor312 Rebased, build is now green 🥳 Thanks for the fix.

@flavio flavio merged commit e48b479 into oras-project:main Jun 14, 2024
5 checks passed
@SuperTux88 SuperTux88 deleted the timeouts branch June 14, 2024 21:26
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.

3 participants