Skip to content
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

Merged
merged 1 commit into from
Aug 1, 2019

Conversation

sftim
Copy link
Contributor

@sftim sftim commented Jun 24, 2019

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 24, 2019
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jun 24, 2019
@netlify
Copy link

netlify bot commented Jun 24, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 4207c2f

https://deploy-preview-15082--kubernetes-io-master-staging.netlify.com

@poothia
Copy link
Contributor

poothia commented Jun 24, 2019

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.
Copy link
Contributor

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]?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

@sftim sftim force-pushed the 20190624_fix_service_concept branch from 0c62a3d to 1cf7095 Compare June 25, 2019 15:32
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2019
@sftim sftim changed the title Fixup Service page [WIP] Fixup Service page Jul 11, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2019
@sftim sftim force-pushed the 20190624_fix_service_concept branch from 1cf7095 to 4207c2f Compare July 28, 2019 16:51
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2019
@sftim
Copy link
Contributor Author

sftim commented Jul 28, 2019

I opted for chopping out the example.

@sftim sftim changed the title [WIP] Fixup Service page Fixup Service page Jul 28, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2019
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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@kbhawkey
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2019
@kbhawkey
Copy link
Contributor

kbhawkey commented Aug 1, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2019
@k8s-ci-robot k8s-ci-robot merged commit 3dea3db into kubernetes:master Aug 1, 2019
@sftim sftim deleted the 20190624_fix_service_concept branch August 6, 2019 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants