fix(sync): log a warning for empty pods #9599
Conversation
pkg/skaffold/sync/sync_test.go
Outdated
| @@ -921,58 +921,64 @@ var pod = &v1.Pod{ | |||
| } | |||
|
|
|||
| func TestPerform(t *testing.T) { | |||
| tests := []struct { | |||
| description string | |||
There was a problem hiding this comment.
nit: I would revert and keep description as a field instead of making this a map. It is easier to read the tests that way and is the general best practice.
pkg/skaffold/sync/sync_test.go
Outdated
| "pod not running": { | ||
| image: "gcr.io/k8s-skaffold:123", | ||
| files: syncMap{"test.go": {"/test.go"}}, | ||
| pod: nil, |
There was a problem hiding this comment.
in the code you're only logging a warning, but here the test should fail?
There was a problem hiding this comment.
it fails because no files were synced
pkg/skaffold/sync/sync.go
Outdated
| @@ -328,6 +328,10 @@ func Perform(ctx context.Context, image string, files syncMap, cmdFn func(contex | |||
| return nil | |||
| } | |||
|
|
|||
| if len(namespaces) == 0 { | |||
There was a problem hiding this comment.
is there any way to add a test for this? not sure if that's possible with the current unit test setup
pkg/skaffold/sync/sync.go
Outdated
| @@ -328,6 +328,10 @@ func Perform(ctx context.Context, image string, files syncMap, cmdFn func(contex | |||
| return nil | |||
| } | |||
|
|
|||
| if len(namespaces) == 0 { | |||
There was a problem hiding this comment.
were you able to reproduce the issue and confirm this fixes it? In the bug they filed, I didn't see anything in the logs to indicate what the issue was
There was a problem hiding this comment.
it's just an assumption, because according to the log if something went wrong, it returns error, but it doesn't return any specific error if there is no namespaces OR nor running pod was found
There was a problem hiding this comment.
I think these logs should be merged regardless of the issue, because in these cases(empty namespaces or no running pods) there is no way to know what went wrong
There was a problem hiding this comment.
So I would delete this line as it'll never be hit. Specifically here: https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/deploy/util/namespaces.go#L35C4-L35C26 if the namespace is empty, it gets the kube context's namespace. If nothing was set, the default namespace is used.
| if test.pod == nil { | ||
| return fake.NewSimpleClientset(), nil | ||
| } | ||
|
|
||
| return fake.NewSimpleClientset(test.pod), test.clientErr | ||
| }) | ||
|
|
There was a problem hiding this comment.
there are no namespaces being given below in Perform the arg is `[]string{""} - why does the first test case still work "no error"? Is there a default namespace being used?
instead of adding a new test (i.e. TestPerform_WithoutNamespaces) could you just add another test case here? First add a new value to the test struct "namespaces", add to existing tests and then pass that along when calling Perform. And in that test case you'd have an empty namespace.
There was a problem hiding this comment.
because it's []string{""} slice of 1 item and the item is an empty string, this is why the len returns a non-zero value and it works.
There was a problem hiding this comment.
I was thinking about adding one more test case to the old test, but it'll complicate the test, so decided to create a separate test
There was a problem hiding this comment.
See above comment it turns out we don't need to check for empty namespaces and can remove the separate test. Appreciate the explanation though.
menahyouyeah
changed the title
Fixes: #9598
Description