Handle pod metadata with missing containerStatuses. #49
Conversation
| for (const json::value& c_spec : *container_specs) { | ||
| if (config_.VerboseLogging()) { | ||
| LOG(INFO) << "Container: " << *c_status; | ||
| LOG(INFO) << "Container: " << *c_spec; |
There was a problem hiding this comment.
Assuming the spec has a lot more in it than just the status -- might this make it too chatty?
There was a problem hiding this comment.
I agree with the comment, however, this is only being displayed if verbose logging is enabled, so I'm ok with keeping it.
There was a problem hiding this comment.
Heh, the status object isn't that small either. At some point the code logged both, so I figured I was just changing the order, but I guess the logging of the spec got lost.
Just pushed a commit that also logs the status. The config option says "verbose logging", so some chattiness should be expected, and it's helpful in debugging the processing of the incoming JSON.
| result.emplace_back(GetLegacyResource(pod, name)); | ||
| result.emplace_back( | ||
| GetContainerMetadata(pod, container_status, container_spec, | ||
| GetContainerMetadata(pod, container_spec, container_status, |
There was a problem hiding this comment.
Nit: generally not sure why you reversed the args here, but whatever the order ends up being, it'd be nice if it lined up with the definitions above. That way the code reads consistently throughout. Either reverse the order of the params, or the order of the definitions.
There was a problem hiding this comment.
It does, though -- container_spec is now declared in the loop header, and container_status is retrieved by name later. What am I missing?
There was a problem hiding this comment.
I didn't zoom out and realize this is being invoked inside of the loop. This looks good now that I better understand.
| continue; | ||
| const json::Object* container_spec = c_spec->As<json::Object>(); | ||
| const std::string name = container_spec->Get<json::String>("name"); | ||
| auto status_it = container_status_by_name.find(name); |
There was a problem hiding this comment.
What does "_it" mean here? "is there"?
There was a problem hiding this comment.
Short for "iterator". Fairly standard C++ convention.
| for (const json::value& c_spec : *container_specs) { | ||
| if (config_.VerboseLogging()) { | ||
| LOG(INFO) << "Container: " << *c_status; | ||
| LOG(INFO) << "Container: " << *c_spec; |
There was a problem hiding this comment.
I agree with the comment, however, this is only being displayed if verbose logging is enabled, so I'm ok with keeping it.
bmoyles0117
left a comment
bmoyles0117
left a comment
There was a problem hiding this comment.
Forgot to request changes.
| continue; | ||
| const json::Object* container_spec = c_spec->As<json::Object>(); | ||
| const std::string name = container_spec->Get<json::String>("name"); | ||
| auto status_it = container_status_by_name.find(name); |
There was a problem hiding this comment.
Short for "iterator". Fairly standard C++ convention.
| result.emplace_back(GetLegacyResource(pod, name)); | ||
| result.emplace_back( | ||
| GetContainerMetadata(pod, container_status, container_spec, | ||
| GetContainerMetadata(pod, container_spec, container_status, |
There was a problem hiding this comment.
It does, though -- container_spec is now declared in the loop header, and container_status is retrieved by name later. What am I missing?
| continue; | ||
| const json::Object* container_spec = c_spec->As<json::Object>(); | ||
| const std::string name = container_spec->Get<json::String>("name"); | ||
| auto status_it = container_status_by_name.find(name); |
| result.emplace_back(GetLegacyResource(pod, name)); | ||
| result.emplace_back( | ||
| GetContainerMetadata(pod, container_status, container_spec, | ||
| GetContainerMetadata(pod, container_spec, container_status, |
There was a problem hiding this comment.
I didn't zoom out and realize this is being invoked inside of the loop. This looks good now that I better understand.
4 participants
Also remove the unused container_id.