Fix issue 265 and 268 and improve logging

kiwigrid / k8s-sidecar

This is a docker container intended to run inside a kubernetes cluster to collect config maps with a specified label and store the included files in a local folder.

MIT License

580 stars 181 forks source link

Fix issue 265 and 268 and improve logging #281

Closed mario-steinhoff-gcx closed 1 year ago

mario-steinhoff-gcx commented 1 year ago

This PR fixes #265 and #268.

I also improved logging a bit because I had a hard time understanding the control flow.

an INFO log line is printed when a list sync is started
a DEBUG log line is printed when a watch sync is started (INFO seems way too spammy with a default timeout of 60s)
an INFO log line is printed when a file is written or removed to disk - previously only a debug log line was printed without the full absolute path and even if the actual file was not changed
an ERROR log line is printed instead of printf() when a file cannot be removed

I built changes in a local image and it seems to work again.

tomrk-esteam8 commented 1 year ago

Hi, thank you for contribution. That's great. Could you please add as well some tests including for example secret and config map with the same name?

mario-steinhoff-gcx commented 1 year ago

@tomrk-esteam8 I added some tests as requested, but I'm not sure how to run them locally and they need to be approved first to run in GHA.

Currently there are no tests for the LIST method, but I'm not sure how to design a test for that.

mario-steinhoff-gcx commented 1 year ago

@jekkel @tomrk-esteam8 I fixed the tests, can someone re-approve?

mario-steinhoff-gcx commented 1 year ago

No idea why the test fails.

To to pinpoint the issue, I modified the workflow to also upload artifacts with all the expected files after first sync. I also split up the tests by sidecar/sidecar-5xx/sidecar-pythonscript.

Can you approve the changes?

mario-steinhoff-gcx commented 1 year ago

I forgot an mkdir and pushed a fix, need approval again.

mario-steinhoff-gcx commented 1 year ago

It looks like the problem was in the pythonscript test. The test compares the number of Hello from python script! occurences and currently it expects 7 occurences but in the logs we have 9, probably because I added 2 more files. I fixed the test, lets see if it works now.

tomrk-esteam8 commented 1 year ago

ok, I am just approving tests today for you, will chec it more detail when it works tomorrow, thank you for your work here :)

mario-steinhoff-gcx commented 1 year ago

ok, I am just approving tests today for you

May I suggest to redesign the tests so they can be run on the local machine without a dependency on the GHA workflow? It makes sense to require approval for changes to the GHA workflow but right now it seems to be the only way to run the the tests. And because the tests are embedded in the GHA workflow config every little change needs a new approval.

Tests failed again but the good news is that the pythonscript test works now, and it seems to be just a copy-paste error I made. Pushed another fix.

tomrk-esteam8 commented 1 year ago

thanks for suggestion, we will discuss it

tomrk-esteam8 commented 1 year ago

@jekkel few things changed, would love to have your expertise here, thanks

jekkel commented 1 year ago

May I suggest to redesign the tests so they can be run on the local machine without a dependency on the GHA workflow? It makes sense to require approval for changes to the GHA workflow but right now it seems to be the only way to run the the tests. And because the tests are embedded in the GHA workflow config every little change needs a new approval.

That's something we wished for quite some time already. I was thinking about using some kind of regular python test framework such that there's some tool support at least. Do you have any specific suggestions? Can you imagine to contribute this?