-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Minor documentation cleanup #7826
Conversation
Hi @jsoref. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@@ -66,8 +66,8 @@ Accept-Ranges: bytes | |||
``` | |||
|
|||
In the example above, you can see that the response contains a `Set-Cookie` header with the settings we have defined. | |||
This cookie is created by NGINX, it contains a randomly generated key corresponding to the upstream used for that request (selected using [consistent hashing][consistent-hashing]) and has an `Expires` directive. | |||
If the user changes this cookie, NGINX creates a new one and redirects the user to another upstream. | |||
This cookie is created by the NGINX Ingress Controller, it contains a randomly generated key corresponding to the upstream used for that request (selected using [consistent hashing][consistent-hashing]) and has an `Expires` directive. |
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.
Can we use the phrase "ingress-nginx 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.
Certainly. Note that a whole bunch of phrases are used, if that's your preferred one, I'll try to apply it consistently across the markdown files.
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.
Please do. It was talked about and I am certain we want consistent "ingress-NGINX controller" everywhere. "NGINX" in caps or not is left to you. Thanks for the improvement. Helps.
@@ -6,7 +6,7 @@ The `auth-url` and `auth-signin` annotations allow you to use an external | |||
authentication provider to protect your Ingress resources. | |||
|
|||
!!! Important | |||
This annotation requires `nginx-ingress-controller v0.9.0` or greater.) |
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.
Can we change this to "ingress-nginx-controller" ?
kubectl create -f https://raw.githubusercontent.com/kubernetes/kops/master/addons/kubernetes-dashboard/v1.10.1.yaml | ||
``` | ||
```console | ||
kubectl create -f https://raw.githubusercontent.com/kubernetes/kops/master/ddons/kubernetes-dashboard/v1.10.1.yaml |
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.
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.
Hmm, not sure how that got damaged
@@ -1,5 +1,18 @@ | |||
# Custom Headers | |||
|
|||
## Caveats | |||
|
|||
ingress-nginx-controllers do not live apply changes made to customer header config maps. |
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.
This phrasing is a little odd to me. Can we try something, like "Changes to the customer header config maps do not force a reload of the ingress-nginx-controllers"
|
||
In order to workaround this limitation, you can: | ||
|
||
* make a change to some other tracked configuration field and then revert it |
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.
Unnecessary changes to force a reload of the config to me seem not optimal. A rolling restart of the deployment would be better advice.
/kind documentation |
…ffic and not some other part of the backend application
`for` adds nothing here
`;` is generally not the right punctuation...
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.
/lgtm
We can fix the other things in followups, overall looks great.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsoref, rikatz 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 |
* clarify link * Add section headers * console blocks * grpc example json was not valid * multi-tls update text The preceding point 1 related to https://github.com/kubernetes-retired/contrib/blob/4f2cb51ef82b4dddb625f6053ad132c1faf07aa1/ingress/controllers/nginx/examples/ingress.yaml and the deployments referenced in https://github.com/kubernetes-retired/contrib/blob/4f2cb51ef82b4dddb625f6053ad132c1faf07aa1/ingress/controllers/nginx/examples/README.md They are not relevant to the current instructions. * add whitespace around parens * grammar setup would be a proper noun, but it is not the intended concept, which is a state * grammar * is-only * via * Use bullets for choices * ingress-controller nginx is a distinct brand. generally this repo talks about ingress-controller, although it is quite inconsistent about how... * drop stray paren * OAuth is a brand and needs an article here also GitHub is a brand * Indent text under numbered lists * use e.g. * Document that customer header config maps changes do not trigger updates This should be removed if kubernetes#5238 is fixed. * article * period * infinitive verb + period * clarify that the gRPC server is responsible for listening for TCP traffic and not some other part of the backend application * avoid using ; and reword * whitespace * brand: gRPC * only-does is the right form `for` adds nothing here * spelling: GitHub * punctuation `;` is generally not the right punctuation... * drop stray `to` * sentence * backticks * fix link * Improve readability of compare/vs * Renumber list * punctuation * Favor Ingress-NGINX and Ingress NGINX * Simplify custom header restart text * Undo typo damage Co-authored-by: Josh Soref <[email protected]>
I'm happy to split or fold this as requested. My initial work is designed such that each change tries to explain what it is doing individually.
What this PR does / why we need it:
addresses brands
adds section headers
fixes indentation
fixes links
fixes random punctuation
fixes whitespace
improves console block tags/style
improves grammar
improves link text
fixes errant carryover from the old contrib repository
fixes gprc example output (?)
documents the current state of Update configuration on add-headers and proxy-set-headers ConfigMap change #5238
Types of changes
Which issue/s this PR fixes
How Has This Been Tested?
Checklist: