Add nginx debug flag #401
Merged
Add nginx debug flag #401
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dean-Coakley
added
the
enhancement
pleshakov
suggested changes
Oct 22, 2018
cmd/nginx-ingress/main.go
Outdated
| @@ -91,6 +91,9 @@ The external address of the service is used when reporting the status of Ingress | |||
|
|
|||
| nginxStatus = flag.Bool("nginx-status", true, | |||
| "Enable the NGINX stub_status, or the NGINX Plus API.") | |||
|
|
|||
| nginxDebug = flag.Bool("nginx-debug", false, | |||
| "Enable debugging for NGINX. Uses the `nginx-debug` binary. Requires 'error-log-level: debug' in the ConfigMap") | |||
cmd/nginx-ingress/main.go
Outdated
| @@ -91,6 +91,9 @@ The external address of the service is used when reporting the status of Ingress | |||
|
|
|||
| nginxStatus = flag.Bool("nginx-status", true, | |||
| "Enable the NGINX stub_status, or the NGINX Plus API.") | |||
|
|
|||
| nginxDebug = flag.Bool("nginx-debug", false, | |||
| "Enable debugging for NGINX. Uses the `nginx-debug` binary. Requires 'error-log-level: debug' in the ConfigMap") | |||
deployments/helm-chart/README.md
Outdated
| @@ -61,6 +61,7 @@ Parameter | Description | Default | |||
| `controller.kind` | The kind of the Ingress controller installation - deployment or daemonset. | deployment | |||
| `controller.nginxplus` | Deploys the Ingress controller for NGINX Plus. | false | |||
| `controller.hostNetwork` | Enables the Ingress controller pods to use the host's network namespace. | false | |||
| `controller.nginxDebug` | Enables debugging for NGINX. Uses the `nginx-debug` binary. Requires `error-log-level: debug` in the ConfigMap via `controller.config.entries` | false | |||
There was a problem hiding this comment.
Add . after controller.config.entries
isaachawley
approved these changes
Oct 23, 2018
Dean-Coakley
force-pushed
the
add-nginx-debug
branch
from
1c1ac17
to
ffbe338
Compare
pleshakov
suggested changes
Oct 25, 2018
internal/nginx/nginx.go
Outdated
| @@ -202,6 +204,7 @@ func NewNginxController(nginxConfPath string, local bool) *Controller { | |||
| nginxConfdPath: path.Join(nginxConfPath, "conf.d"), | |||
| nginxSecretsPath: path.Join(nginxConfPath, "secrets"), | |||
| local: local, | |||
| nginxBinaryPath: path.Join("/usr/sbin/", nginxBinary), | |||
There was a problem hiding this comment.
why not to set the absolute path in main.go? similar to nginxConfPath parameter.
* Add unit test for getNginxCommand * Fix cli-args documentation * nginxBinaryPath is now the second arg in NewNginxController
Dean-Coakley
force-pushed
the
add-nginx-debug
branch
from
56629d3
to
14867b2
Compare
pleshakov
suggested changes
Oct 25, 2018
internal/nginx/configurator_test.go
Outdated
| @@ -852,3 +851,29 @@ func TestUpdateEndpointsMergeableIngressFailsWithInvalidTemplate(t *testing.T) { | |||
| t.Errorf("UpdateEndpointsMergeableIngress returned \n%v, but expected \n%v", nil, "template execution error") | |||
| } | |||
| } | |||
|
|
|||
| func TestGetNginxCommand(t *testing.T) { | |||
There was a problem hiding this comment.
this test must be in nginx_test.go
you don't need to create a configurator, only the Controller
internal/nginx/configurator_test.go
Outdated
| expected string | ||
| }{ | ||
| {"reload", "/usr/sbin/nginx -s reload"}, | ||
| {"start", "/usr/sbin/nginx -s start"}, |
There was a problem hiding this comment.
please remove the start one
* Improved test by allowing it to take an argument to specify which nginx binary you want to use in the configurator
pleshakov
approved these changes
Oct 29, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
nginx-debugflagnginxDebugoption to helm chartChecklist
Before creating a PR, run through this checklist and mark each as complete.