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

Avoid misleading text in Service concept #36672

Conversation

sftim
Copy link
Contributor

@sftim sftim commented Sep 7, 2022

Split out from #30817

The existing text implied that creating a node port or load balanced Service would actually add an extra Service object that you could see in the Kubernetes API. This isn't true, so avoid implying that it is.

/sig network

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 7, 2022
@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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 7, 2022
@sftim
Copy link
Contributor Author

sftim commented Sep 7, 2022

Helps with #14770

@netlify
Copy link

netlify bot commented Sep 7, 2022

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit fdceaa2
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6345de347467250008b1c8c3
😎 Deploy Preview https://deploy-preview-36672--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sftim sftim force-pushed the 20220411_remove_misleading_service_clusterip_detail branch 2 times, most recently from 1377390 to 0f7f162 Compare September 15, 2022 12:27
@sftim sftim force-pushed the 20220411_remove_misleading_service_clusterip_detail branch from 0f7f162 to 3b06176 Compare October 11, 2022 16:48
Copy link
Contributor

@chrisnegus chrisnegus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just a few small suggestion.

content/en/docs/concepts/services-networking/service.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/service.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/service.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/service.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/service.md Outdated Show resolved Hide resolved
The existing text implied that creating a node port or load balanced
Service would actually add an extra Service object that you could see in
the Kubernetes API. This isn't true, so avoid implying that it is.
@sftim sftim force-pushed the 20220411_remove_misleading_service_clusterip_detail branch from 3b06176 to fdceaa2 Compare October 11, 2022 21:20
@sftim
Copy link
Contributor Author

sftim commented Oct 12, 2022

Also, would this change need tech review from SIG Network?

@reylejano
Copy link
Member

Preview of updated page:
https://deploy-preview-36672--kubernetes-io-main-staging.netlify.app/docs/concepts/services-networking/service/

I don't think this needs a tech review as the technical aspects changed or added in this PR are currently in the docs:

  • "Kubernetes additionally allocates a port (TCP, UDP or SCTP to match the protocol of the Service)"
  • kube-proxy flag --nodeport-addresses, nodePortAddresses field, <NodeIP>:spec.ports[*].nodePort
    and .spec.clusterIP:spec.ports[*].port are discussed on the current page and moved / clarified in this PR
  • type: NodePort field and type: LoadBalancer already exists in the example Service manifests, this PR explains them more thoroughly

The changes look good
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: reylejano

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 Oct 12, 2022
@sftim
Copy link
Contributor Author

sftim commented Oct 12, 2022

Thanks Rey

@nate-double-u
Copy link
Contributor

Looks good, thanks @sftim!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5dabd31c511de19a2f0985e301e9805cfb0d1201

@k8s-ci-robot k8s-ci-robot merged commit b2f830f into kubernetes:main Oct 21, 2022
@sftim sftim deleted the 20220411_remove_misleading_service_clusterip_detail branch October 21, 2022 23:09
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. sig/network Categorizes an issue or PR as relevant to SIG Network. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants