Fixed TestGetCoreClient unit test failure on Windows #21737
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bobsira 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
the
needs-ok-to-test
|
Hi @bobsira. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/M
|
Can one of the admins verify this patch? |
| mockK8sConfigPath := filepath.Join(tmpDir, "kube_config") | ||
|
|
||
| // Only write the file if we have any content; an empty string still creates a 0-byte file, | ||
| // which is what we want to simulate "empty config". |
There was a problem hiding this comment.
This comment contradicts the code, we always write a file, even if we don't have any content.
| defer os.Remove(mockK8sConfigPath) | ||
| if err != nil { | ||
| t.Fatalf("Unexpected error when writing to file %v. Error: %v", test.kubeconfigPath, err) | ||
| // Each sub-test gets its own temp dir & file for isolation and portability. |
There was a problem hiding this comment.
This is standard practice, I don't think we ned to justify it. All tests should work like this.
|
|
||
| // Only write the file if we have any content; an empty string still creates a 0-byte file, | ||
| // which is what we want to simulate "empty config". | ||
| if err := os.WriteFile(mockK8sConfigPath, []byte(test.config), 0644); err != nil { |
There was a problem hiding this comment.
There is no reason to create the kubeconfig world readable. This is the default kubeconfig permissions on macOS:
% ls -lh ~/.kube/config
-rw------- 1 nir staff 106B Oct 10 11:37 /Users/nir/.kube/config
It will be better to use real world permissions. This will break the code if it has bad assumptions about being able to read the config from another user in the same group.
| } | ||
| if err == nil && test.err { | ||
| t.Fatal("GetCoreClient expected to return error but got nil") | ||
| t.Fatalf("GetCoreClient expected to return error but got nil") |
There was a problem hiding this comment.
Why t.Fatalf() if we don't format anything?
4 participants
In this simple fix: