-
Notifications
You must be signed in to change notification settings - Fork 370
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
ci: use bzlmod module for curl #14467
ci: use bzlmod module for curl #14467
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14467 +/- ##
==========================================
- Coverage 93.59% 93.58% -0.01%
==========================================
Files 2315 2316 +1
Lines 207140 207146 +6
==========================================
- Hits 193871 193864 -7
- Misses 13269 13282 +13 ☔ View full report in Codecov by Sentry. |
0dc9cdd
to
3db9092
Compare
@@ -41,18 +41,26 @@ extern "C" size_t CurlOnWriteData(char* ptr, size_t size, size_t nmemb, | |||
return size * nmemb; | |||
} | |||
|
|||
struct CurlPtrCleanup { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include "google/cloud/internal/curl_wrappers.h"
instead of redefining. Same goes for CurlSListFreeAll
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not want to use google::cloud::internal
in an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake. I didn't notice the file.
@@ -60,13 +60,20 @@ nlohmann::json MakeChatPayload(BuildStatus const& bs) { | |||
return nlohmann::json{{"text", std::move(text)}}; | |||
} | |||
|
|||
struct CurlPtrCleanup { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These do not link google::cloud::*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, sorry.
using Headers = std::unique_ptr<curl_slist, CurlSListFreeAll>; | ||
auto const headers = Headers{curl_slist_append(nullptr, kContentType)}; | ||
using CurlHandle = std::unique_ptr<CURL, CurlPtrCleanup>; | ||
auto curl = CurlHandle(curl_easy_init()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto const headers = CurlHeaders{curl_slist_append(nullptr, kContentType)};
auto curl = CurlPtr(curl_easy_init());
using Headers = std::unique_ptr<curl_slist, CurlSListFreeAll>; | ||
auto const headers = | ||
Headers{curl_slist_append(nullptr, authorization.c_str())}; | ||
using CurlHandle = std::unique_ptr<CURL, CurlPtrCleanup>; | ||
auto curl = CurlHandle(curl_easy_init()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. I do not think we should use internal types in an example.
Part of the work for #11485
This change is