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.

Sign up for GitHub

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

Jump to bottom

rewrite of ingress #10958

Merged
merged 2 commits into from
Nov 16, 2018
Merged

rewrite of ingress #10958

merged 2 commits into from
Nov 16, 2018

Conversation

heckj
Copy link
Contributor

@heckj heckj commented Nov 12, 2018 •
edited
Loading

@heckj
rewrite of ingress
c71bc0f 
- reset and rewritten to provide more clarity and directness on what
  ingress is and does.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 12, 2018
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Nov 12, 2018 •
edited
Loading

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

Built with commit ad98851

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

zacharysarah
zacharysarah previously requested changes Nov 12, 2018
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@heckj So glad to see you contributing again! 💛 This looks good, some fiddly edits for consistency and clarity then GTG.

content/en/docs/concepts/services-networking/ingress.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/ingress.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/ingress.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/ingress.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/ingress.md Show resolved Hide resolved
content/en/docs/concepts/services-networking/ingress.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/ingress.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/ingress.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/ingress.md Outdated Show resolved Hide resolved
content/en/docs/concepts/services-networking/ingress.md Outdated Show resolved Hide resolved
@heckj
cleanup from review
ad98851 
- also normalized Ingress -> ingress through-out
@heckj
Copy link
Contributor Author

heckj commented Nov 12, 2018

thanks @zacharysarah 👋 Updated across the board, including making the ingress resource consistently lower case where it was being used a a proper noun before. I found quite a few more through-out.

PTAL

kbhawkey
kbhawkey reviewed Nov 12, 2018
View reviewed changes
content/en/docs/concepts/services-networking/ingress.md

## The Ingress Resource

A minimal Ingress might look like:
A minimal ingress example:

Copy link
Contributor

Choose a reason for hiding this comment

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

possibly further clarify:
A minimal ingress configuration example or specification?

kbhawkey
kbhawkey reviewed Nov 12, 2018
View reviewed changes
content/en/docs/concepts/services-networking/ingress.md
Ideally, all ingress controllers should fulfill this specification, but the various ingress
controllers operate slightly differently. The Kubernetes project supports and maintain
[GCE](https://git.k8s.io/ingress-gce/README.md) and [nginx](https://git.k8s.io/ingress-nginx/README.md)
ingress controllers.
Copy link
Contributor

Choose a reason for hiding this comment

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

lines 147-149 are a duplicate of lines 62-63?

Bradamant3
Bradamant3 reviewed Nov 16, 2018
View reviewed changes
content/en/docs/concepts/services-networking/ingress.md
In order for the ingress resource to work, the cluster must have an ingress controller running. This is unlike other types of controllers, which run as part of the `kube-controller-manager` binary, and are typically started automatically with a cluster. Choose the ingress controller implementation that best fits your cluster.

* Kubernetes as a project currently supports and maintains [GCE](https://git.k8s.io/ingress-gce/README.md) and
[nginx](https://git.k8s.io/ingress-nginx/README.md) controllers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Karen's point below about repeating this line is a good one -- context makes the later one more useful, I think. Or else move this one earlier, since the previous section talks about GCE and nginx too.

@Bradamant3
Copy link
Contributor

@zacharysarah can you approve changes?
@heckj I'm not wedded to the one fix I suggest, and I think the rest looks good, so I'll merge when Zach approves unless you feel like fiddling further.

@zacharysarah
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zacharysarah

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 Nov 16, 2018
@Bradamant3 Bradamant3 dismissed zacharysarah’s stale review November 16, 2018 20:34

approved in a separate comment

@Bradamant3
Copy link
Contributor

lol not quite what I meant by approve but I'll take it ;)

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2018
@k8s-ci-robot k8s-ci-robot merged commit fe46040 into kubernetes:master Nov 16, 2018
@heckj heckj deleted the ingress-expansion branch November 17, 2018 21:02
@heckj
Copy link
Contributor Author

heckj commented Nov 17, 2018

@kbhawkey @Bradamant3 apologies - I didn't see the comments here until this weekend when I was circling back to my docs pieces, and then saw it had already been merged.

I'm still not entirely happy with the overall content here, I think it's missing some a few insights that I had to learn the hard way, so I'll see about another PR in the future

@heckj
Copy link
Contributor Author

heckj commented Nov 17, 2018

@kbhawkey and @Bradamant3 I made updates per your suggestions in a follow up PR: #11039

k8s-ci-robot pushed a commit that referenced this pull request Nov 25, 2018
@heckj @k8s-ci-robot
follow up from #10958 (#11039)
8a7254c 
* follow up from #10958

and a bit of additional example work

Signed-off-by: Joe Heck <heckj@mac.com>

* Update ingress.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bradamant3 Bradamant3 Bradamant3 left review comments

@kbhawkey kbhawkey kbhawkey left review comments

@zacharysarah zacharysarah zacharysarah left review comments

@bprashanth bprashanth Awaiting requested review from bprashanth

@Rajakavitha1 Rajakavitha1 Awaiting requested review from Rajakavitha1

Assignees

@Bradamant3 Bradamant3

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@heckj @k8sio-netlify-preview-bot @Bradamant3 @zacharysarah @k8s-ci-robot @kbhawkey