✨ chore: log missing kubeconfig data as warning #87
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @crenshaw-dev! |
k8s-ci-robot
added
the
size/XS
providers/kubeconfig/provider.go
Outdated
| kubeconfigData, ok := secret.Data[p.opts.KubeconfigSecretKey] | ||
| if !ok || len(kubeconfigData) == 0 { | ||
| log.Info("Secret does not contain kubeconfig data, skipping", "key", p.opts.KubeconfigSecretKey) | ||
| log.Error(errors.New("secret does not contain kubeconfig data, skipping"), "key", p.opts.KubeconfigSecretKey) |
There was a problem hiding this comment.
Makes sense, just one nit, our log lines usually start with uppercase, e.g.:
| log.Error(errors.New("secret does not contain kubeconfig data, skipping"), "key", p.opts.KubeconfigSecretKey) | |
| log.Error(errors.New("Secret does not contain kubeconfig data, skipping"), "key", p.opts.KubeconfigSecretKey) |
There was a problem hiding this comment.
Or wait, now that I think about it, how does this log line look exactly? I guess log.Error might be wrapping it differently?
There was a problem hiding this comment.
Good catch. Looking closer, I think the correct way to do this is to provide the capitalized message in as the second arg and leave the first arg nil. I've added to the description what the log line now looks like.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: crenshaw-dev The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/M
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| // Verify the cluster count hasn't changed (secret was skipped) | ||
| Eventually(provider.ListClusters, "2s").Should(HaveLen(2)) |
There was a problem hiding this comment.
I'm not happy with doing Eventually then Consistently, but I found that provider.ListClusters was coming up empty and failing Consistently if I didn't add the Eventually first.
There was a problem hiding this comment.
It kind of makes sense since you might be checking the list before clusters have been processed and added. But I don't think we need the Consistently once Eventually has passed.
There was a problem hiding this comment.
I added Consistently to cover this case:
- Before the test starts, the provider processed the existing two secrets, so
provider.ListClustersis sitting at length 2 - We add the bad secret
- Before the provider has a chance to pick up the bad secret, we assert that
Eventuallywe have length 2 - The Eventually passes, and the test succeeds
- After the test passes, a bug in the provider allows the bad secret to be treated as a cluster, and
provider.ListClustersgoes to length 3
|
@embik I improved the log line and also added tests. Happy to drop the tests if review/iteration costs more time than its worth. :-) |
3 participants
I encountered a problem where the
kubeconfigfield hadn't been set, but the failure was too quiet for me to understand what went wrong.I think error level makes sense, because if I've labeled the Secret to be used but haven't finished setting it up, I've done something wrong.
Since it's a user error instead of controller error, maybe it should be warn instead? But I didn't see a warn level.
Here's what the logs look like: