Skip to content
This repository has been archived by the owner on Aug 19, 2019. It is now read-only.

Don't add labels to container metadata if the pod has no labels. #48

Merged
merged 2 commits into from
Jan 4, 2018

Conversation

bmoyles0117
Copy link
Contributor

No description provided.

try {
labels = metadata->Get<json::Object>("labels");
} catch (const json::Exception& e) {
labels = new json::Object({});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will leak. Let's instead handle it by not sending the "labels" component at all...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -263,7 +263,12 @@ MetadataUpdater::ResourceMetadata KubernetesReader::GetContainerMetadata(
const std::string created_str =
metadata->Get<json::String>("creationTimestamp");
Timestamp created_at = rfc3339::FromString(created_str);
const json::Object* labels = metadata->Get<json::Object>("labels");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well also delete this from line 199...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

…as labels.

Use a Has test instead of catching the exception.
@bmoyles0117
Copy link
Contributor Author

Your changes LGTM.

@igorpeshansky igorpeshansky changed the title Deal with exception if labels are not present in pod metadata. Don't add labels to container metadata if the pod has no labels. Jan 3, 2018
Copy link
Member

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels weird approving my own changes, but you've already LGTM'd them.

@@ -263,7 +263,12 @@ MetadataUpdater::ResourceMetadata KubernetesReader::GetContainerMetadata(
const std::string created_str =
metadata->Get<json::String>("creationTimestamp");
Timestamp created_at = rfc3339::FromString(created_str);
const json::Object* labels = metadata->Get<json::Object>("labels");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

try {
labels = metadata->Get<json::Object>("labels");
} catch (const json::Exception& e) {
labels = new json::Object({});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@dhrupadb dhrupadb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌

Copy link
Contributor

@supriyagarg supriyagarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@igorpeshansky igorpeshansky merged commit 274c52d into master Jan 4, 2018
@igorpeshansky igorpeshansky deleted the bmoyles0117-fix-labels branch January 4, 2018 18:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants