-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Changes made in wacth.py to print Empty newlines that are skipped when watching pod logs. #2372
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.
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
Conversation
Changes made in wacth.py to print Empty newlines that are skipped when watching pod logs.
Welcome @p172913! |
how did you test it? please add a test to this PR |
@yliaog I installed the kubernetes client in editable mode on local, made changes that are mentioned in PR and ran the code given in the issue as an test. It was working fine. And I am seeing output as expected. If needed I can add the test I did in tests... |
This file is to test whether empty line are printed when watch function in the kubernetes python client is used.
@yliaog As per your request I commited a test case. Please review and approve my PR. |
thanks for the PR, could you add a unit test in kubernetes/base/watch/watch_test.py ? |
@yliaog I added it in kubernetes/test/test_pod_logs.py. If i is needed in kubernetes/base/watch/watch_test.py. I can add it there. |
please add a test in kubernetes/base/watch/watch_test.py |
Added a unit test name test_pod_log_empty_lines to check if watch is printing empty lines. And made changes in test_watch_with_interspersed_newlines as the watch is also printing empty line. Added a condition to check for empty lines in ogs to avoid the errors.
Deleting file with tests added in test/test_pod_los.py
@yliaog added unit test in kubernetes/base/watch/watch_test.py with name test_pod_log_empty_lines. And made changes in test_watch_with_interspersed_newlines test case to avoid empty line exception. |
kubernetes/base/watch/watch.py
Outdated
yield line # Normal non-empty line | ||
last_was_empty = False | ||
elif not last_was_empty: | ||
yield '/n' # Only yield one empty line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be '\n' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it has to be '' (an empty string) if yield an empty string. We need to change unmarshal_event function also. If needed I can change '/n' to '' and will change unmarshal_event function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace '/n' with '' (empty string) and adjusted unmarshal_event function in watch.py
Changes made in unmarshal_event for not having issues with empty lines.
kubernetes/base/watch/watch_test.py
Outdated
|
||
def test_pod_log_empty_lines(self): | ||
pod_name = "demo-bug" | ||
# Manifest with busybax to keep pod engaged for sometiem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: sometiem ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will adjust
kubernetes/base/watch/watch_test.py
Outdated
def test_pod_log_empty_lines(self): | ||
pod_name = "demo-bug" | ||
# Manifest with busybax to keep pod engaged for sometiem | ||
pod_manifest = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is pod_manifest used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used minikube before replacing it with mock function. After checking locally forgot to delete it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just deleted pod_manifest from watch_test.py.
Removed pod_manifest from watch_test.py.
kubernetes/base/watch/watch_test.py
Outdated
# Print outputs | ||
print(f"Captured logs: {log_output}") | ||
# self.assertTrue(any("Hello from Docker" in line for line in log_output)) | ||
self.assertTrue(any(line.strip() == "" for line in log_output), "No empty lines found in logs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add an assertion that ensures the log_output has exactly what is expected, the whole log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed assertive statement to make sure entire output logs are verified.
Changes made to check whether entire log is printed or not.
kubernetes/base/watch/watch_test.py
Outdated
#Mock logs used for this test | ||
w.stream = Mock(return_value=[ | ||
"Hello from Docker", | ||
"", # Empty line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add another empty line here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added few empty lines to the test case.
Changes made in watch.py to print multiple empty line if necessary.
As per request added few empty lines to test case in watch_test.py
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p172913, yliaog The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/release-note-none |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PRs print Empty newlines that are skipped when watching pod logs. Changed yield login in stream to print only single empty line between the logs.
Which issue(s) this PR fixes:
Fixes #2358
Special notes for your reviewer:
This is my first time contributing, so please let me know if I made the change successfully!
Does this PR introduce a user-facing change?
None
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
None