Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VC-35738] Replace logs.Log with logr.Logger in the remaining code #612

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Nov 7, 2024

The aim of this PR is to remove the remaining references to the logs.Log variable and replace them with context derived logr.Logger calls.

I haven't given much thought to the log levels because I think we're going to be removing most of the messages about missing CRDs and permissions errors in another feature.

$ go test ./pkg/datagatherer/k8s/... -v -run TestNoneCache -count 1
=== RUN   TestNoneCache
    cache.go:53: E1107 17:57:50.964580] Cache update failure err="not a cacheResource type: *k8s.notCachable missing metadata/uid field" operation="add"
    cache.go:66: E1107 17:57:50.964680] Cache update failure err="not a cacheResource type: *k8s.notCachable missing metadata/uid field" operation="update"
    cache.go:80: E1107 17:57:50.964689] Cache update failure err="not a cacheResource type: *k8s.notCachable missing metadata/uid field" operation="delete"
--- PASS: TestNoneCache (0.00s)
PASS
ok      github.com/jetstack/preflight/pkg/datagatherer/k8s      0.025s
 ./hack/e2e/test.sh
{
  "ts": 1731002359611.0227,
  "caller": "cache/reflector.go:561",
  "msg": "k8s.io/[email protected]/tools/cache/reflector.go:243: failed to list firefly.venafi.com/v1, Resource=issuers: issuers.firefly.venafi.com is forbidden: User \"system:serviceaccount:venafi:venafi-kubernetes-agent\" cannot list resource \"issuers\" in API group \"firefly.venafi.com\" at the cluster scope",
  "v": 0
}
{
  "ts": 1731002359611.0823,
  "caller": "k8s/dynamic.go:278",
  "msg": "datagatherer informer has failed and is backing off",
  "v": 0,
  "groupVersionResource": "firefly.venafi.com/v1, Resource=issuers",
  "reason": "failed to list firefly.venafi.com/v1, Resource=issuers: issuers.firefly.venafi.com is forbidden: User \"system:serviceaccount:venafi:venafi-kubernetes-agent\" cannot list resource \"issuers\" in API group \"firefly.venafi.com\" at the cluster scope"
}
{
  "ts": 1731002360493.6667,
  "caller": "k8s/dynamic.go:276",
  "msg": "server missing resource for datagatherer",
  "v": 0,
  "groupVersionResource": "networking.istio.io/v1alpha3, Resource=gateways"
}
{
  "ts": 1731002360614.0635,
  "caller": "cache/reflector.go:561",
  "msg": "k8s.io/[email protected]/tools/cache/reflector.go:243: failed to list networking.istio.io/v1alpha3, Resource=virtualservices: the server could not find the requested resource",
  "v": 0
}
image

@wallrj wallrj mentioned this pull request Nov 7, 2024
12 tasks
@wallrj wallrj requested a review from maelvls November 7, 2024 18:07
@wallrj wallrj force-pushed the VC-35738/execute-context-logs branch from 2243d7f to 1f19391 Compare November 12, 2024 14:38
Base automatically changed from VC-35738/execute-context-logs to VC-35738/feature November 12, 2024 14:41
@wallrj wallrj force-pushed the VC-35738/remove-logs-dot-log-2 branch from 3842893 to e8c5eed Compare November 12, 2024 15:33
@wallrj wallrj changed the title WIP: [VC-35738] Remove the logs.Log variable and replace it with logr.Logger in the remaining code [VC-35738] Remove the logs.Log variable and replace it with logr.Logger in the remaining code Nov 12, 2024
@wallrj wallrj changed the title [VC-35738] Remove the logs.Log variable and replace it with logr.Logger in the remaining code [VC-35738] Replace logs.Log with logr.Logger in the remaining code Nov 12, 2024
@@ -136,3 +139,14 @@ func TestOnAddCache(t *testing.T) {
})
}
}

// TestNoneCache demonstrates that the cache helpers do not crash if passed a
// non-cachable object, but log an error with a reference to the object type.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It says that "it logs an error" but we don't assert that something is actually logged. Without seeing the actual log lines, it's hard to see what the test does.

I don't know if it was worth it, but I've opened a PR on top of yours to improve that: #619.

Copy link
Member

@maelvls maelvls left a comment

Choose a reason for hiding this comment

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

All good to me. I haven't tested manually nor run the smoke test on my laptop, but I am confident that nothing will break with these changes.

A small nit that we can deal with later: TestCacheNone talks about logs that should be printed, but doesn't assert these logs. I proposed a quick change to fix that in #619; feel free to merge your PR first, and I'll deal with #619 later on.

@wallrj wallrj merged commit 2797067 into VC-35738/feature Nov 14, 2024
2 checks passed
@wallrj wallrj deleted the VC-35738/remove-logs-dot-log-2 branch November 14, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants