-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Described all router environment variables #3090
Described all router environment variables #3090
Conversation
@JacobTanenbaum @ramr PTAL |
d75018f
to
fff15b9
Compare
|`*ROUTER_SLOWLORIS_TIMEOUT*` | 10s | ||
|Variable | Default | Description | ||
|`*ROUTER_SERVICE_HTTP_PORT*` | 80 | What port to listen for http requests on | ||
|`*ROUTER_SERVICE_HTTPS_PORT*` | 443 | What port to listen for http requests on |
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.
should this be "What port to listen for https requests on"?
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.
It should!
fff15b9
to
0180a61
Compare
LGTM |
@adellape PTAL |
|`*ROUTER_LOG_LEVEL*` | warning | What log level to send to the above address | ||
|`*ROUTER_BACKEND_CHECK_INTERVAL*` | 5000ms | How frequently to check backends for "liveness" | ||
|`*ROUTER_DEFAULT_CONNECT_TIMEOUT*`| 5s | The maximum connect time | ||
|`*ROUTER_DEFAULT_CLIENT_TIMEOUT*`| 30s |How long a client has to acknowledge or send data |
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.
Minor nit space between '|' and 'How'.
|`*ROUTER_BACKEND_CHECK_INTERVAL*` | 5000ms | How frequently to check backends for "liveness" | ||
|`*ROUTER_DEFAULT_CONNECT_TIMEOUT*`| 5s | The maximum connect time | ||
|`*ROUTER_DEFAULT_CLIENT_TIMEOUT*`| 30s |How long a client has to acknowledge or send data | ||
|`*ROUTER_DEFAULT_SERVER_TIMEOUT*`| 30s |How long a server has to acknowledge or send data |
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.
Same as above: Minor nit space between '|' and 'How'.
@knobunc this LGTM - minor nits and there's a few others if you want to add 'em here or in another PR:
Thx |
300aecb
to
bb9d691
Compare
@ramr Updated per your suggestion. Thanks |
LGTM |
bb9d691
to
1ee0697
Compare
Could you also help add the following which I knowed: |
@zhaozhanqi thanks for the suggestion. By looking for those I found more to add as well. |
1ee0697
to
b2f23cb
Compare
@openshift/team-documentation this is ready to merge. |
@gaurav-nelson - PTAL and if no issues, please merge. |
|`*ROUTER_ENABLE_COMPRESSION*`| false | If "true", compress responses when possible. | ||
|`*ROUTER_LOG_LEVEL*` | warning | What log level to send to the above address. | ||
|`*ROUTER_OVERRIDE_HOSTNAME*`| | If set, override the spec.host value for a route with the template in ROUTER_SUBDOMAIN. | ||
|`*ROUTER_SERVICE_HTTPS_PORT*` | 443 | What port to listen for https requests on. |
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.
Suggestion: Please change "What port to listen for https requests on." to "Port to listen for https requests." OR "Port on which to listen for https requests."
|`*ROUTER_LOG_LEVEL*` | warning | What log level to send to the above address. | ||
|`*ROUTER_OVERRIDE_HOSTNAME*`| | If set, override the spec.host value for a route with the template in ROUTER_SUBDOMAIN. | ||
|`*ROUTER_SERVICE_HTTPS_PORT*` | 443 | What port to listen for https requests on. | ||
|`*ROUTER_SERVICE_HTTP_PORT*` | 80 | What port to listen for http requests on. |
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.
Suggestion: Please change "What port to listen for http requests on." to "Port to listen for http requests." OR "Port on which to listen for http requests."
|`*ROUTER_OVERRIDE_HOSTNAME*`| | If set, override the spec.host value for a route with the template in ROUTER_SUBDOMAIN. | ||
|`*ROUTER_SERVICE_HTTPS_PORT*` | 443 | What port to listen for https requests on. | ||
|`*ROUTER_SERVICE_HTTP_PORT*` | 80 | What port to listen for http requests on. | ||
|`*ROUTER_SERVICE_NAME*` | public | The name the router will identify itself with in route statuses. |
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.
Suggestion: Please change "The name the router will identify itself with in route statuses." to "The name that router will identify itself with in route statuses."
Everything looks good. I will do a merge and then update the language changes I suggested. |
Thank you @gaurav-nelson. Your suggestions look good. If this can go in 3.4 and 3.3 that would be nice, but really getting it in 3.4 alone is sufficient. |
@knobunc Can you also squash down to 1 commit? Will leave the merging to @gaurav-nelson though here. |
b2f23cb
to
9c10e1b
Compare
Added missing router environment variables to the documentation, and added descriptons to all of them. For 3.4 only.
9c10e1b
to
66a6d22
Compare
Squashed and addressed @gaurav-nelson's comments. |
Language changes for #3090
@knobunc Can you please confirm that all these options available in 3.3 now? Before it gets released this Monday. Thanks. |
@gaurav-nelson thank you for checking. I just ran through them all and there are two that are not:
|
Added missing router environment variables to the documentation, and
added descriptons to all of them.
For 3.3 and 3.4.