SDCICD-51: Support arbitrary metrics derived from log data

[jeefy]

No description provided.

[mdwn]

One small thing, otherwise LGTM.

[jeefy]

I disagree. We're already hardcoding the Metadata we expect. On top of that, we're not actually writing JSON. We're writing a hand-crafted unmarshalled interface{} Functionally this is here to test whether writeToJSON, which besides json.Marshal/Unmarshal, has logic to place and write a file correctly. Therefore we shouldn't validate that json.Marshal/Unmarshal works correctly, but that the data placed and read is what/where we expect. Let the json library do its thing, let's just test what we care about: Where the data is placed and it being what we expect.

…

On Thu, Feb 6, 2020 at 3:28 PM Michael Wilson ***@***.***> wrote: ***@***.**** commented on this pull request. One small thing, otherwise LGTM. ------------------------------ In pkg/metadata/metadata_test.go <#218 (comment)>: > @@ -10,11 +10,31 @@ import ( "testing" ) +// generateExpected marshals/unmarshals a supplied metadata object. +// Rather than hand-curating our own expected data, this functionally +// achieves the same test result but it's cleaner. +func generateExpected(m *Metadata) map[string]interface{} { The advantage of hard coding the test strings in here is that it forces the developer to be a little more intentional about the design of the JSON. With marshalling the JSON programmatically, it won't catch things like misnamed fields or missed fields. I think I'd rather have the hard coding here rather than this function. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#218?email_source=notifications&email_token=AAKUO6HXDUY2I4OKKPR4IWDRBRXGPA5CNFSM4KRDA4X2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUSQVQA#pullrequestreview-354749120>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKUO6E5MCXHPBPM7T7GLSTRBRXGPANCNFSM4KRDA4XQ> .

[mdwn]

/lgtm
Fair enough, good counterpoint!

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeefy, meowfaceman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [jeefy,meowfaceman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment