Initial classes to make http requests. #507
Conversation
googlebot
added
the
cla: yes
The depedency is only important when linking statically, which the Windows build caught.
|
Saw some revisions to this over the weekend, and Travis looks like it's failing as-is. I've already looked over most of this, but please @-mention me when this PR is passing and you want my final review :) |
|
@houglum PTAL. Tests pass now. |
storage/CMakeLists.txt
Outdated
| @@ -56,16 +56,22 @@ google_cloud_cpp_add_common_options(storage_common_options) | |||
| # Enable unit tests | |||
| enable_testing() | |||
|
|
|||
| # Search for libcurl | |||
| # Search for libcurl, in CMake 3.5 this does not define a target, but it does in later versions. | |||
| # Define the target ourselves if it is missing. | |||
There was a problem hiding this comment.
Still getting acquainted with CMake... maybe I missed it, but I don't see anything different in the CMake docs for find_package between v3.4 and v3.6. Also, the comment below says that the find_package calls do define a target in CMake 3.5. I'm just confused as to which versions do what, and why. Are there docs or some explanation of this you can link?
There was a problem hiding this comment.
Just added a link, it is in development (use the git-stage pull down in the documentation). Generally, we are sticking to cmake-3.5 because it is easier for our users to find that one in the
platforms we support.
storage/CMakeLists.txt
Outdated
| find_package(CURL REQUIRED) | ||
| if (NOT TARGET CURL::CURL) | ||
| add_library(CURL::CURL UNKNOWN IMPORTED) | ||
| set_target_properties(CURL::CURL PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${CURL_INCLUDE_DIR}") | ||
| set_property(TARGET CURL::CURL APPEND PROPERTY IMPORTED_LOCATION "${CURL_LIBRARY}") | ||
| endif () | ||
|
|
||
| # Find OpenSSL and ZLIB, these define targets as-of CMake 3.5 and higher. | ||
| find_package(OpenSSL REQUIRED) | ||
| find_package(ZLIB REQUIRED) |
There was a problem hiding this comment.
Looks like this is only required for Windows -- could you add a comment to that effect?
There was a problem hiding this comment.
I think (but I might be wrong), that these are actually required when linking statically, it just happens that Windows in the only platform where we are forcing static linking of system-provided libraries. I added a comment saying that.
As to why Windows is linked statically: because grpc does not support shared libraries on Windows. And as to why not link statically in one of the Linux builds, that is a bug, created #510 to track it.
Thanks to @houglum for the feedback.
|
@houglum PTAL. |
| */ | ||
| std::string CurlRequest::MakeRequest(std::string const& payload) { | ||
| // Pre-compute and cache the user agent string: | ||
| static std::string const user_agent = [] { |
There was a problem hiding this comment.
Probably not super important for the initial version of this class, but users often want to use a custom user agent (IIRC, we tack their custom string on the end of our standardized user agent... but I'd have to check with the client libraries folks). Might be useful to allow setting a custom user agent segment elsewhere and include it in this string - we can make a TODO for that and shelve it for now though.
There was a problem hiding this comment.
Ack, created #511 to track this, and added a placeholder into the design document.
| connected=no | ||
| readonly ATTEMPTS=$(seq 1 8) | ||
| for attempt in $ATTEMPTS; do | ||
| if curl "https://nghttp2.org/httpbin/get" >/dev/null 2>&1; then |
There was a problem hiding this comment.
Any reason for picking this particular URL?
There was a problem hiding this comment.
Yes, you can get the contents of your request in the response, and simulate failures, and return blobs of data too:
Makes it convenient for small tests like this, but we should be nice and not abuse their site.
This includes an integration test to make a remote http request.