-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Fixup Service page #15082
Fixup Service page #15082
Conversation
Deploy preview for kubernetes-io-master-staging ready! Built with commit 4207c2f https://deploy-preview-15082--kubernetes-io-master-staging.netlify.com |
lgtm |
@@ -430,8 +430,7 @@ specifying `"None"` for the cluster IP (`.spec.clusterIP`). | |||
|
|||
You can use a headless Service to interface with other service discovery mechanisms, | |||
without being tied to Kubernetes' implementation. For example, you could implement | |||
a custom [Operator]( | |||
be built upon this API. | |||
a custom Operator upon this API. |
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.
@sftim, the change is okay, though, the semantics seem a bit off to me. Can you clarify what upon this API
vs `build an Operator to interface with [what API]?
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.
Maybe it's easier to remove the example? It looks like I saved some unfinished work here, and I don't remember what point I wanted to make.
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.
Here is the current version:
You can use a headless Service to interface with other service discovery mechanisms, without being tied to Kubernetes’ implementation. For example, you could implement a custom [Operator]( be built upon this API.
- I think the original idea is suggesting that the custom Operator handles: load balancing, proxying, DNS configuration for this service, but I may have misinterpreted the concept.
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 re-rewrote the text to clarify a bit.
If that's not helping, what do people think about chopping out the example instead?
0c62a3d
to
1cf7095
Compare
1cf7095
to
4207c2f
Compare
I opted for chopping out the example. |
without being tied to Kubernetes' implementation. For example, you could implement | ||
a custom {{< glossary_tooltip term_id="operator-pattern" text="Operator" >}} upon | ||
this API. | ||
without being tied to Kubernetes' implementation. | ||
|
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.
hello @sftim .
I reread the section. A few nits and suggestions.
- hyphenate load-balancing or not?
- What does this phrase mean,
without being tied to Kubernetes' implementation.
? Implementation of what? - line 454, Is it reasonable to double quote, "A record"? Also, link to https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#a-records
- Should
Services
be bold, if so, be consistent - bold
kube-proxy
? - I think line 436 could be broken down and made into a list:
Limitations of Headless Services:
- a cluster IP is not allocated
kube-proxy
is not in use- Kubernetes does not perform load balancing
- line 443, should it be
Endpoints Controller
instead of endpoints controller?
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.
Whilst that's legitimate feedback, I'd like to get this 3-line change done now and have another PR take care of bigger fixes.
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.
Okay -- your intention is to just clean up the reference to Operators.
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kbhawkey 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 |
Fixup for #14526 (unhelpful example on Service page). Thought I'd fixed this but hadn't.
Will also put a further tweak into PR #14458.