-
Notifications
You must be signed in to change notification settings - Fork 185
Making watch work with read_namespaced_pod_log #93
Conversation
/assign @mbohlool |
Codecov Report
@@ Coverage Diff @@
## master #93 +/- ##
==========================================
+ Coverage 92.11% 92.29% +0.18%
==========================================
Files 13 13
Lines 1217 1246 +29
==========================================
+ Hits 1121 1150 +29
Misses 96 96
Continue to review full report at Codecov.
|
please add a test case for this. |
resp.close() | ||
resp.release_conn() | ||
if self.resource_version is not None: | ||
kwargs['resource_version'] = self.resource_version | ||
else: |
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.
Do you need the else: break 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.
Yes. The log streaming does not have resource_version
. So once the response finishes, you have to stop.
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.
In fact that was not good because using break inside finally swallows up the exception. I change it to setting a flag.
34a4b58
to
8e6f043
Compare
As much as I want to merge this I don't have ownership of this repo :) |
It swallows exceptions.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mitar, roycaihw 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 |
Fixes kubernetes-client/python#199.