add support for iso8601 timestamps

[ironmike-au]

Description of the change:
This change adds a new zap flag to enable the user to specify the timeformat to be used in the operator log output. By default this is "unix" and left unchanged, but the user can now specify --zap-timeformat=iso8601 and the zap encoder will be configured to output in the more human-friendly time format.

Motivation for the change:
During development, I wanted to be able to look at the logs output by operator and immediately understand the time that each log occurred. Having logs in unix format means that I couldn't do this, so I created this PR which will allow the user to specify their timeformat choice by flag.

[openshift-ci-robot]

Hi @otakumike. Thanks for your PR.

I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[lilic]

/ok-to-test

[lilic]

I can't seem to find this in any zap docs, the zap-timeformat, can you link the docs here thanks! :)

[joelanford]

Does this flag work in conjuction with the --zap-encoder flag?

Since we call jsonEncoder() and consoleEncoder() in the encoderValue.Set function, it seems like there could be a situation where we set the encoder before we set the time format.

If so, maybe we could change encoderValue to have a function that returns the encoder:

type encoderValue struct {
	set     bool
	getEncoder func(zapcore.TimeEncoder) zapcore.Encoder
	str     string
}

Then in getConfig() we can actually call that function and pass in timeformatVal.timeEncoder

WDYT?

[joelanford]

@otakumike Just following up to see if you think you'll be able to address the PR comments?

[ironmike-au]

Yes, I plan to have a look at it this week. I wasn't expecting anyone to come to me so quickly :)

[joelanford]

@otakumike Sounds good! No worries. This looks like a nice feature addition, so I just wanted to pop it to the top of the stack so we don't lose track of it.

[ironmike-au]

I can't seem to find this in any zap docs, the zap-timeformat, can you link the docs here thanks! :)

As far as I can tell it isn't documented anywhere. I've only been able to figure out the little bit I have by reviewing the undocumented timeformat stuff in their code.

[ironmike-au]

Thank you very much for all your feedback. I think everything is all set to go now, but please let me know if I can do anything else?

[joelanford]

This is shaping up very nicely. Thanks for taking this on!

[joelanford]

I haven't had time to look at the most recent round of updates, but I should be able to later this week. I'll review the current tests to see if I see an easy way to incorporate testing of this new flag.

If there isn't one, another team member or I could build on your existing PR to refactor the tests, or we could add some TODOs in the existing tests and come back around to them in a separate PR.

[ironmike-au]

Hey @joelanford just wondering if there's anything more I can do on this one?

[joelanford]

@otakumike Sorry for the delay on this. I've been on vacation and travelling basically all of this month. Let me take another look and get back to you. Hopefully I'll actually have time this week 🤞

[joelanford]

@otakumike I just pushed some updates to your PR that adds the extra encoder tests (and makes a couple of other minor improvements I ran across while I was at it). Thanks for the tip on using the encoders to actually encode an entry as part of the test. That seems to do the trick.

Let me know what you think. If it looks good to you, I'll ping some other folks on the team to get more reviews, but this should be very close now. Thanks again for the patience!

[joelanford]

LGTM as soon as CI passes

[ironmike-au]

@joelanford LGTM. Hope you had a good holiday :)

[estroz]

/lgtm

@otakumike thanks for the PR!