Restore container_id local resource ID when present. #51
Conversation
|
Is there a link to the original PR that removes it? It would be nice to compare them. |
supriyagarg
left a comment
supriyagarg
left a comment
There was a problem hiding this comment.
Very minor comments. Otherwise LGTM.
src/kubernetes.cc
Outdated
| const std::string docker_id = | ||
| container_status->Get<json::String>("containerID"); | ||
| if (docker_id.compare(0, docker_prefix_end, kDockerIdPrefix) != 0) { | ||
| LOG(ERROR) << "ContainerID " |
There was a problem hiding this comment.
ContainerID -> containerID to match the field name?
Or simply "Container ID"
src/kubernetes.cc
Outdated
| }; | ||
|
|
||
| if (container_status) { | ||
| std::size_t docker_prefix_end = sizeof(kDockerIdPrefix) - 1; |
There was a problem hiding this comment.
perhaps use strlen here (instead of sizeof) so you don't have to subtract by 1.
Also, docker_prefix_end -> docker_prefix_len is easier to parse.
There was a problem hiding this comment.
sizeof makes it a compile-time constant... I'd declare this constexpr, anyway...
As for end vs len, "len" is an abbreviation, so maybe even "length" or "count" (see the signature of compare). I used end because it's the position of the '\0', which is the end of the C string...
There was a problem hiding this comment.
Should we go with docker_prefix_length? As for the constexpr request, are you just asking for me to prefix this line with that?
There was a problem hiding this comment.
docker_prefix_length SGTM. And yes, please prefix the variable declaration with constexpr.
There was a problem hiding this comment.
Updated, stayed away from constexpr due to a modification within the conditional statement.
igorpeshansky
changed the title
5 participants
No description provided.