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

write callback and termination API #160

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

edwinpjacques
Copy link
Contributor

@edwinpjacques edwinpjacques commented Oct 23, 2020

Description

  1. Allow write callback. Application was to use it to leverage the Kubernetes watch API which maintains a long-running connection.
  2. API to terminate connection from another thread.
  3. Fix all the disabled/broken CI tests that block the merge.

Checklist

Not all of these might apply to your change but the more you are able to check
the easier it will be to get your contribution merged.

@edwinpjacques edwinpjacques force-pushed the master branch 9 times, most recently from a326d80 to 518cc14 Compare October 27, 2020 07:45
@edwinpjacques
Copy link
Contributor Author

@mrtazz I am ready for your review, and hopefully your approval! I did a lot of work to get the CI working well again. I enabled all tests, even the OPTIONS test. I would very much appreciate if this can be approved, or if you can provide any necessary feedback for approval ASAP. Thank you.

@edwinpjacques edwinpjacques force-pushed the master branch 3 times, most recently from 43b5b9e to f98b8de Compare October 27, 2020 13:07
This was referenced Oct 27, 2020
@edwinpjacques
Copy link
Contributor Author

edwinpjacques commented Oct 27, 2020

I thought that my changing of the squid container was perhaps superfluous, as the old container works locally. However, it does not work in CI. I switched back to the container I provided and my custom configuration file, as that passed locally and in CI.

@edwinpjacques
Copy link
Contributor Author

@shawkinsl would you be able to provide any feedback on this merge request? It fixes CI and allows for stream processing of a GET response via write callback.

Enables streaming get, and out-of-band termination of connection.
Had to fix CI as well. Added necessary tests, and enabled a
previously disabled test.
Copy link
Contributor Author

@edwinpjacques edwinpjacques left a comment

Choose a reason for hiding this comment

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

squashed commits, ready for approval.

@edwinpjacques edwinpjacques requested a review from mrtazz October 29, 2020 19:40
@edwinpjacques edwinpjacques self-assigned this Oct 29, 2020
@edwinpjacques
Copy link
Contributor Author

@ElonDusk26 and @KaiPetzke would you mind to give a "thumbs up" on this review so I can get it merged with a clean conscience? Thanks!

@ElonDusk26
Copy link

Astounding work @edwinpjacques! Hopefully @mrtazz agrees with me.

@edwinpjacques
Copy link
Contributor Author

Got out-of-band permission to merge from @mrtazz based on trust. Thanks for the kudos @ElonDusk26 ! I hope you can help fix the issue #158. It would be nice to have that resolved in the next release of code. Thank you.

@edwinpjacques edwinpjacques merged commit 77646d9 into mrtazz:master Oct 30, 2020
@KaiPetzke
Copy link

Sorry to disagree on this one. The callback declaration:

auto writeCallback = [](void *data, size_t size, size_t nmemb, void *userdata) -> size_t

looks too much old-style C-ish to me. This is a C++ wrapper around libcurl, and we should use C++ APIs, not C APIs.

The C++11 style would be to use std::function.

The Pre-C++11-style would be to use a class with a virtual function callback

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