Handle error response codes to HTTP requests. #61
Conversation
supriyagarg
left a comment
supriyagarg
left a comment
There was a problem hiding this comment.
The changes look good. However, the watch response in kubernetes.cc (L750) and the oauth response in oauth2.cc (L238) don't have the error handling code.
|
Thanks for the review. I've added the handling in oauth2.cc. |
igorpeshansky
force-pushed
the
igorp-handle-http-codes
branch
from
7bc2c05 to
32e3db1
Compare
igorpeshansky
changed the base branch from
igorp-docker-instance-id
to
master
|
Rebased off |
src/api_server.cc
Outdated
| LOG(INFO) << "Metadata request sent successfully"; | ||
| } | ||
| } catch (const boost::system::system_error& e) { | ||
| LOG(ERROR) << "Unsuccessful: " << e.what(); |
There was a problem hiding this comment.
s/Unsuccessful/Metadata request unsuccessful/
| http::client::response response = client.post(request); | ||
| if (status(response) != 200) { | ||
| throw boost::system::system_error( | ||
| boost::system::errc::make_error_code(boost::system::errc::not_connected), |
There was a problem hiding this comment.
I don't believe not_connected is the best exception to throw here, but if it's just a matter of getting "something" to throw, it's fine.
There was a problem hiding this comment.
Yes, that code is never examined.
src/api_server.cc
Outdated
| request << boost::network::header("Authorization", auth_header); | ||
| request << boost::network::body(request_body); | ||
| http::client::response response = client.post(request); | ||
| if (status(response) != 200) { |
There was a problem hiding this comment.
I would accept anything in the 2xx range, technically a POST can result in a 201 Created. Applies to other posts as well. I'm sure it's not our contract with the resource metadata api yet, but just something to be mindful of.
src/docker.cc
Outdated
| } | ||
| try { | ||
| http::local_client::response response = client.get(request); | ||
| if (status(response) != 200) { |
There was a problem hiding this comment.
Just as a note, I'm fine with just a 200 here as you have it.
There was a problem hiding this comment.
Changed for consistency.
|
|
||
| namespace { | ||
| template<class T> | ||
| std::string AsString(T v) { |
There was a problem hiding this comment.
I don't see this being used anywhere.
There was a problem hiding this comment.
Lines 32-34 below.
There was a problem hiding this comment.
Well, my eyes certainly failed me 🤣
igorpeshansky
force-pushed
the
igorp-handle-http-codes
branch
from
03e04f2 to
b64acd4
Compare
igorpeshansky
left a comment
igorpeshansky
left a comment
There was a problem hiding this comment.
Addressed feedback. PTAL.
src/api_server.cc
Outdated
| LOG(INFO) << "Metadata request sent successfully"; | ||
| } | ||
| } catch (const boost::system::system_error& e) { | ||
| LOG(ERROR) << "Unsuccessful: " << e.what(); |
src/api_server.cc
Outdated
| request << boost::network::header("Authorization", auth_header); | ||
| request << boost::network::body(request_body); | ||
| http::client::response response = client.post(request); | ||
| if (status(response) != 200) { |
| http::client::response response = client.post(request); | ||
| if (status(response) != 200) { | ||
| throw boost::system::system_error( | ||
| boost::system::errc::make_error_code(boost::system::errc::not_connected), |
There was a problem hiding this comment.
Yes, that code is never examined.
src/docker.cc
Outdated
| } | ||
| try { | ||
| http::local_client::response response = client.get(request); | ||
| if (status(response) != 200) { |
There was a problem hiding this comment.
Changed for consistency.
|
|
||
| namespace { | ||
| template<class T> | ||
| std::string AsString(T v) { |
There was a problem hiding this comment.
Lines 32-34 below.
3 participants
Depends on #59, but will rebase and merge to
masteronce that's in.