Fix auditor #193
Fix auditor #193
Conversation
This affects the flag -manifest-based-snapshot-of flag.
No functional change.
Instead of hardcoding the logging client tied to GCP resources, use a "LoggingFacility" wrapper so that we can in the future use a fake logging client for unit tests.
k8s-ci-robot
added
the
cncf-cla: yes
|
/hold |
k8s-ci-robot
added
approved
k8s-ci-robot
added
the
sig/release
|
/cc @justinsb @thockin @dims @bartsmykla Please take a look. |
|
For more context, I cannot currently add unit tests for the |
| // all local (stdout), with a FakeLogClient. | ||
| func NewFakeLoggingFacility() *LoggingFacility { | ||
|
|
||
| logInfo := log.New(os.Stdout, "FAKE-INFO", log.LstdFlags) |
There was a problem hiding this comment.
I know it's fake, but Stderr is normally the place for logs :-)
| // Creates a client. | ||
| client, err := logging.NewClient(ctx, projectID) | ||
| if err != nil { | ||
| klog.Fatalf("Failed to create client: %v", err) |
There was a problem hiding this comment.
Nit: Looks like you could return the error, at least with the current code structure. But I agree it's probably not worth it.
|
Two minor nits that you can ignore, but lgtm /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, listx The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thanks for the review @justinsb ! I will address those nits in the next PR; merging this for now. /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
address review comments from #193
This is first part of a couple more PRs to fix #191. I need to do this so that I can add unit tests for the
Audit()function. After this PR I need to do a similar change (introduction of a wrapper type) for theerrorreporting.Clientstuff we still have inAudit().