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.

Sign up for GitHub

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

Jump to bottom

Fix issue 265 and 268 and improve logging #281

Merged
merged 9 commits into from
May 24, 2023
Merged

Fix issue 265 and 268 and improve logging #281

merged 9 commits into from
May 24, 2023

Conversation

mario-steinhoff-gcx
Copy link
Contributor

@mario-steinhoff-gcx mario-steinhoff-gcx commented May 17, 2023 •
edited
Loading

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.

@mario-steinhoff-gcx
feat: improve logging
58251fc
@mario-steinhoff-gcx
fix: use per-resource cache maps
ef8c047 
fixes kiwigrid#265
@mario-steinhoff-gcx
fix: do not set dest_folder when removing resources
52249ed 
we could use _get_destination_folder here but when is_removed is set to
True, dest_folder is not used

fixes kiwigrid#268
@tomrk-esteam8
Copy link
Collaborator

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
Copy link
Contributor Author

@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.

@jekkel jekkel requested a review from tomrk-esteam8 May 23, 2023 11:23
@mario-steinhoff-gcx
Copy link
Contributor Author

mario-steinhoff-gcx commented May 23, 2023 •
edited
Loading

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

tomrk-esteam8
tomrk-esteam8 previously approved these changes May 23, 2023
View reviewed changes
Copy link
Collaborator

@tomrk-esteam8 tomrk-esteam8 left a comment

Choose a reason for hiding this comment

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

Seems to be fine for me, when tests will work, we can merge @jekkel

@mario-steinhoff-gcx
Copy link
Contributor Author

mario-steinhoff-gcx commented May 23, 2023 •
edited
Loading

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
Copy link
Contributor Author

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

@mario-steinhoff-gcx
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

mario-steinhoff-gcx commented May 23, 2023 •
edited
Loading

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
Copy link
Collaborator

thanks for suggestion, we will discuss it

@tomrk-esteam8
Copy link
Collaborator

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

@jekkel
Copy link
Member

jekkel commented May 24, 2023

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?

@jekkel jekkel merged commit 13c64f6 into kiwigrid:master May 24, 2023
@jekkel jekkel added the bug Something isn't working label May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jekkel jekkel jekkel approved these changes

@tomrk-esteam8 tomrk-esteam8 tomrk-esteam8 left review comments

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

when listing secrets and configmaps, secrets fail with 'V1Secret' object has no attribute 'binary_data'
3 participants
@mario-steinhoff-gcx @tomrk-esteam8 @jekkel