Send user agents with all our tools #1289
Conversation
lbonn
requested review from
jochenschneider,
pattivacek,
eu-siemann and
mike-sul
lbonn
force-pushed
the
feat/OTA-3550/tools-user-agent
branch
2 times, most recently
from
202efc2 to
ffc53f1
Compare
Codecov Report
@@ Coverage Diff @@
## master #1289 +/- ##
==========================================
- Coverage 79.32% 79.22% -0.11%
==========================================
Files 175 175
Lines 10367 10379 +12
==========================================
- Hits 8224 8223 -1
- Misses 2143 2156 +13
Continue to review full report at Codecov.
|
|
|
||
| const auto resp = http.get(server + "/user_agent", HttpInterface::kNoLimit); | ||
| const auto app = resp.body.substr(0, resp.body.find('/')); | ||
| EXPECT_EQ(app, "Aktualizr"); |
There was a problem hiding this comment.
Could you provide a brief comment to make it clear that you're testing the expected default here?
| curlEasySetoptWrapper(handle, CURLOPT_NOBODY, 1L); | ||
| curlEasySetoptWrapper(handle, CURLOPT_VERBOSE, 1L); | ||
| curlEasySetoptWrapper(handle, CURLOPT_SSL_VERIFYPEER, 0L); | ||
| CurlEasyWrapper curl; |
Signed-off-by: Laurent Bonnans <[email protected]>
lbonn
force-pushed
the
feat/OTA-3550/tools-user-agent
branch
from
ffc53f1 to
41afadf
Compare
| @@ -73,7 +61,7 @@ HttpClient::HttpClient(const std::vector<std::string>* extra_headers) | |||
| } | |||
| } | |||
| curlEasySetoptWrapper(curl, CURLOPT_HTTPHEADER, headers); | |||
| curlEasySetoptWrapper(curl, CURLOPT_USERAGENT, user_agent.c_str()); | |||
| curlEasySetoptWrapper(curl, CURLOPT_USERAGENT, Utils::getUserAgent()); | |||
There was a problem hiding this comment.
It might override the user agent header specified by an HttpClient user/client in extra_headers.
IMHO, statically predefined headers should be specified in a HTTP client factory configuration (which doesn't exist), so libaktualizr clients could specify desired mandatory headers in explicit way. Utils::set/getUserAgent() is an implicit API for libaktualizr clients/apps they need to be aware and set it before Aktualizr instantiation.
This is just my two cents and can be ignored since it will require more work :)
There was a problem hiding this comment.
Yes, I agree this is not especially elegant. The main problem I had is that different garage tools are using some common code that instantiates an HttpClient, that's why I opted for the less intrusive global setting.
I don't see much use case for HttpClient users redefining user agents dynamically though, it's maybe the (rare) kind of thing that can be setup globally.
But then I also question my choice of putting the API in utils.h. I'll try moving it to http client itself.
There was a problem hiding this comment.
No problem, I understand that we need to be practical and balance between an ideal but time consuming and a just enough but quicker solutions.
BTW, at first glance, this PR, IMHO, is related to this one #1273 (comment)
There was a problem hiding this comment.
lol - i've been watching this PR to see how things went before pushing my change which will at best produce a merge conflict.
There was a problem hiding this comment.
Haha well... This PR is not really urgent anyway. Would you also prefer that to be done another way?
There was a problem hiding this comment.
no. I was just wanted to see the approach that was chosen here since there's a good chance I could do the same thing in my change.
|
Should we go forward anyway? |
No description provided.