Add support for watch @ resourceVersion, and improve tests. #45
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
k8s-ci-robot
added
size/M
drubin
left a comment
drubin
left a comment
There was a problem hiding this comment.
Minor comment about style, but other than that looks good.
kubernetes/src/kubernetes/watch.rb
Outdated
| path + query | ||
| end | ||
|
|
||
| def connect(path, resource_version, &_block) |
There was a problem hiding this comment.
Maybe its worth setting this as resource_version = nil so it isn't a breaking change?
There was a problem hiding this comment.
done. (learn something new about Ruby every day :)
examples/watch/watch.rb
Outdated
| watch = Kubernetes::Watch.new(client) | ||
|
|
||
| watch.connect('/api/v1/namespaces') do |obj| | ||
| watch.connect('/api/v1/namespaces', nil) do |obj| |
There was a problem hiding this comment.
I think it makes sense to annotate these examples if we have more than one param and especially if the value is nil
There was a problem hiding this comment.
this is fixed by the comment above....
|
comments addressed, please take another look. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
removed
the
lgtm
brendandburns
added
the
lgtm
3 participants
@drubin @cben
Builds on #40, will rebase after #40 merges.