clientgo/examples: add ToC for examples #46883
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
size/M
k8s-ci-robot
added
release-note-none
hack/verify-staging-imports.sh
Outdated
| @@ -79,7 +79,7 @@ if grep -rq '// import "k8s.io/kubernetes/' 'staging/'; then | |||
| exit 1 | |||
| fi | |||
|
|
|||
| for EXAMPLE in vendor/k8s.io/client-go/examples/{in-cluster,out-of-cluster,third-party-resources}; do | |||
| for EXAMPLE in vendor/k8s.io/client-go/examples/{authenticate-in-cluster,authenticate-out-of-cluster,third-party-resources}; do | |||
There was a problem hiding this comment.
The server url and port is also set up differently, so it's not just authenticate. Maybe call them in/out-cluster-client-configuration?
There was a problem hiding this comment.
I think authenticating is fine. The fact that you're pointing server URL/port is an implementation detail of authentication. The name change I made in this case was actually suggested by Jago (on the proposal doc) and I agree that it makes the point clear.
It also helps with discovery as the letter A is listed first on GitHub.
There was a problem hiding this comment.
There are ways to configure the client without supplying the authentication information, so authentication doesn't always equal to configuration. So i think "configuration" is slightly better.
It also helps with discovery as the letter A is listed first on GitHub.
We have your ToC now so we don't depend on the alphabetical order anymore :)
| @@ -0,0 +1,36 @@ | |||
| # client-go Usage Examples | |||
|
|
|||
| This directory contains examples that cover examples of various use cases | |||
| # client-go Usage Examples | ||
|
|
||
| This directory contains examples that cover examples of various use cases | ||
| and functionality for Go client library for Kubernetes. |
There was a problem hiding this comment.
s/Go client library for Kubernetes/client-go/
| This directory contains examples that cover examples of various use cases | ||
| and functionality for Go client library for Kubernetes. | ||
|
|
||
| ### Authentication |
|
|
||
| - [**Third-party resources**](./third-party-resources): Register a custom | ||
| resource type with the API, create/update/query this custom type, and write a | ||
| controller to handle events for this custom resource type. |
There was a problem hiding this comment.
events could be misleading, because Kubernetes have an API type called event. How about "write a controller drives the cluster state based on the changes to the custom resources."
| - [**Third-party resources**](./third-party-resources): Register a custom | ||
| resource type with the API, create/update/query this custom type, and write a | ||
| controller to handle events for this custom resource type. | ||
| - [**Work queues**](./workqueue): Create a custom controller with a Watch for Pod events, |
There was a problem hiding this comment.
How about "Create a hotloop-free controller with the rate-limited workqueue and the informer framework", i don't think we need to mention "Watch for Pod".
And could you move this example before the tpr one? The latter is more complex.
There was a problem hiding this comment.
I don't understand the "Work queue" example just yet but the following 2 concepts are a bit too difficult to put here without any explanation:
- hotloop-free controller
- informer framework.
Is there a better way you think you can explain this example?
Create a hotloop-free controller with the rate-limited workqueue and the informer framework
I cannot infer any of this from workqueue/README.md which says:
This example demonstrates how to write a controller which follows the states
of watched resources.
It demonstrates how to:
* combine the workqueue with a cache to a full controller
* synchronize the controller on startup
:) Let me know if you have a better explanation and I will change/consolidate.
There was a problem hiding this comment.
tl;dr
Consider introduce informer in the description of the tpr example:
"The example demonstrates how to write a controller using the informer."
And update the description of workqueue example to:
"Combine the workqueue with the informer to write a controller. Workqueue makes sure the controller doesn't hot-loop on failed operations."
hotloop-free controller
I'm referring to this line, this makes sure the controller to exponential backoff before process the item again. Preventing hot-loop is a main feature of the workqueue.
IMO another important functionality of the workqueue is that it allow multiple workers to work in parallel, unfortunately the example doesn't show that.
informer framework
I mean the "k8s.io/client-go/tools/cache" package, which is also used in the thirdparty resource example, so we don't need to emphasis "informer framework" in the workqueue example.
combine the workqueue with a cache to a full controller
Here the "cache" means "k8s.io/client-go/tools/cache".
synchronize the controller on startup
It refers to this line. It's less important. Probably we should copy this line to the tpr example.
| - [**Work queues**](./workqueue): Create a custom controller with a Watch for Pod events, | ||
| a rate limited Work Queue and an Informer with cache. | ||
|
|
||
| ## How to run these examples |
There was a problem hiding this comment.
The in-cluster won't work with these steps. And the user needs a kubeconfig ready. I suggest that we direct the readers to follow each example's README to learn how to run them.
k8s-github-robot
added
the
needs-rebase
k8s-github-robot
removed
the
needs-rebase
|
@caesarxuchao PTAL again. I have this comment still: #46883 (comment) I think we need to do a better job summarizing what |
|
Also please note the recent changes to the examples. The tpr example is marked as deprecated, the successor is CRD. |
|
@caesarxuchao yeah I incorporated the relevant changes. feel free to take a look again. |
| ### Advanced Concepts | ||
|
|
||
| - [**Work queues**](./workqueue): Create a hotloop-free controller with the | ||
| rate-limited workqueue and the informer framework. |
There was a problem hiding this comment.
Could you make "informer framework" a hyperlink to https://godoc.org/k8s.io/client-go/tools/cache#NewInformer?
There was a problem hiding this comment.
Will do. But still my comment above stands. This sentence is very confusing to a beginner. We need to simplify.
|
One nit and lgtm. |
|
There is a standalone example on informer: #44446. I think we can explain informer better after that's merged, |
Also add authenticate- prefix to auth samples. Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
|
Done. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @deads2k for approval It's mostly doc improvement for client-go, can we add the 1.7 milestone? |
|
@caesarxuchao cutting it a little fine, but I think this is the last day of slush /approve no-issue |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, caesarxuchao, deads2k Associated issue requirement bypassed by: deads2k The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
@k8s-bot pull-kubernetes-unit test this |
marun
added
the
sig/api-machinery
|
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
|
@k8s-bot pull-kubernetes-federation-e2e-gce test this |
|
It seems like this is stuck at the |
|
@caesarxuchao it looks like it's getting blocked on the flakes for the past 2 days. how do we get this merged? Should I rebase? |
|
/retest |
|
No rebase needed. We just retry the test. Sigh. |
|
No luck :( |
|
Opened an issue: #47672 |
|
/retest |
|
Automatic merge from submit-queue |
Also add authenticate- prefix to auth samples. This patch could use some
improvement explaining workqueue and TPR examples as I'm not entirely sure.
/assign @caesarxuchao
Signed-off-by: Ahmet Alp Balkan ahmetb@google.com