Add test for GetPodAndContainerMetadata #130

ACEmilG · 2018-04-04T22:38:38Z

No description provided.

supriyagarg · 2018-04-04T22:40:19Z

test/kubernetes_unittest.cc

+  Configuration config(std::stringstream(
+    "KubernetesClusterName: TestClusterName\n"
+    "KubernetesClusterLocation: TestClusterLocation\n"
+    "MetadataApiResourceTypePerarator: \".\"\n"


Please fix. TypeSeparator?

Sorry, fixed.

supriyagarg · 2018-04-04T22:42:10Z

test/kubernetes_unittest.cc

+    })},
+  });
+
+  auto metadatas = GetPodAndContainerMetadata(


why not m like in other places.
If you want plural, then metadata_list or something is better.

changed to metadata_list

Frankly, I prefer m for brevity. Any reason to not go with that? As @supriyagarg mentioned, this is a convention in other tests.

I guess I have a strong aversion to single-letter variable names. Changed.

supriyagarg · 2018-04-04T22:44:34Z

test/kubernetes_unittest.cc

+  EXPECT_EQ(time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"),
+            metadatas[1].metadata().created_at);
+  EXPECT_EQ(Timestamp(), metadatas[1].metadata().collected_at);
+  json::value metadata1 = json::object({


metadata1 -> container1

supriyagarg · 2018-04-04T22:45:35Z

test/kubernetes_unittest.cc

+  EXPECT_EQ(time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"),
+            metadatas[2].metadata().created_at);
+  EXPECT_EQ(Timestamp(), metadatas[2].metadata().collected_at);
+  json::value metadata2 = json::object({


metadata2 -> container2

igorpeshansky · 2018-04-05T04:01:02Z

test/kubernetes_unittest.cc

    "KubernetesClusterName: TestClusterName\n"
    "KubernetesClusterLocation: TestClusterLocation\n"
-    "MetadataApiResourceTypePerarator: \",\"\n"
+    "MetadataApiResourceTypeSeperator: \",\"\n"


Make that MetadataApiResourceTypeSeparator (here and below).
Sigh, the config revamp in #127 would allow catching invalid config options.

This still needs to be fixed.

Fixed, sorry about that

igorpeshansky · 2018-04-05T04:03:46Z

test/kubernetes_unittest.cc

+  EXPECT_EQ("gke_container.TestNamespace.TestPodUid.TestContainerName0",
+            metadata_list[0].ids()[0]);
+  EXPECT_EQ("gke_container.TestNamespace.TestPodName.TestContainerName0",
+            metadata_list[0].ids()[1]);


EXPECT_EQ(std::vector<std::string>{ "gke_container.TestNamespace.TestPodUid.TestContainerName0", "gke_container.TestNamespace.TestPodName.TestContainerName0", }, metadata_list[0].ids());

igorpeshansky · 2018-04-05T04:05:18Z

test/kubernetes_unittest.cc

+  EXPECT_EQ("k8s_container.TestPodUid.TestContainerName0",
+            metadata_list[1].ids()[0]);
+  EXPECT_EQ("k8s_container.TestNamespace.TestPodName.TestContainerName0",
+            metadata_list[1].ids()[1]);


EXPECT_EQ(std::vector<std::string>{ "k8s_container.TestPodUid.TestContainerName0", "k8s_container.TestNamespace.TestPodName.TestContainerName0", }, metadata_list[1].ids());

igorpeshansky · 2018-04-05T04:07:56Z

test/kubernetes_unittest.cc

+  EXPECT_EQ(time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"),
+            metadata_list[2].metadata().created_at);
+  EXPECT_EQ(Timestamp(), metadata_list[2].metadata().collected_at);
+  json::value container2 = json::object({


Should this not be pod metadata?

I actually meant that this looks like container metadata (with the "metadata", "spec", and "status" fields). Should this not be pod metadata (with one "api" field)?

Hmm, well reading the code, it does seem to me that a pod should be played at the end of the metadata list. Truthfully, I had a hard time following all the code paths closely, so its difficult for me to evaluate whether this is an actual bug.

Oh, silly me. This is pod metadata. I miscounted the nesting depth. Never mind.

igorpeshansky

There's one more comment about the typo in the option name...

igorpeshansky · 2018-04-05T19:04:34Z

test/kubernetes_unittest.cc

+  EXPECT_EQ(time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"),
+            metadata_list[2].metadata().created_at);
+  EXPECT_EQ(Timestamp(), metadata_list[2].metadata().collected_at);
+  json::value container2 = json::object({


I actually meant that this looks like container metadata (with the "metadata", "spec", and "status" fields). Should this not be pod metadata (with one "api" field)?

igorpeshansky

One remaining rename that still needs to happen.

igorpeshansky · 2018-04-10T14:34:27Z

test/kubernetes_unittest.cc

    "KubernetesClusterName: TestClusterName\n"
    "KubernetesClusterLocation: TestClusterLocation\n"
-    "MetadataApiResourceTypePerarator: \",\"\n"
+    "MetadataApiResourceTypeSeperator: \",\"\n"


This still needs to be fixed.

igorpeshansky · 2018-04-10T14:35:42Z

test/kubernetes_unittest.cc

+  EXPECT_EQ(time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"),
+            metadata_list[2].metadata().created_at);
+  EXPECT_EQ(Timestamp(), metadata_list[2].metadata().collected_at);
+  json::value container2 = json::object({


Oh, silly me. This is pod metadata. I miscounted the nesting depth. Never mind.

igorpeshansky

One more.

igorpeshansky · 2018-04-10T17:32:12Z

test/kubernetes_unittest.cc

                                       is_deleted);
  }

+  std::vector<MetadataUpdater::ResourceMetadata> GetPodAndContainerMetadata(


Any reason these accessors are not static?

igorpeshansky

LGTM

supriyagarg

LGTM

Add test for GetPodAndContainerMetadata

cb7efe2

supriyagarg reviewed Apr 4, 2018

View reviewed changes

Addressed code review comments

328bac6

igorpeshansky self-requested a review April 5, 2018 03:59

igorpeshansky suggested changes Apr 5, 2018

View reviewed changes

Addressed code review comments

6fed1a3

igorpeshansky suggested changes Apr 5, 2018

View reviewed changes

igorpeshansky suggested changes Apr 10, 2018

View reviewed changes

Fix spelling error

6302f96

igorpeshansky suggested changes Apr 10, 2018

View reviewed changes

Set fixture functions to static

1379398

igorpeshansky approved these changes Apr 10, 2018

View reviewed changes

supriyagarg approved these changes Apr 10, 2018

View reviewed changes

ACEmilG merged commit c17e8de into master Apr 11, 2018

ACEmilG deleted the ACEmilG-GetPodAndContainerMetadata-test branch April 11, 2018 12:57

Add test for GetPodAndContainerMetadata #130

Add test for GetPodAndContainerMetadata #130

Uh oh!

Conversation

ACEmilG commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

supriyagarg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone