-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
fix: remove default usage of obsolete nginx directives without explictly being set #10029
Conversation
This issue is currently awaiting triage. If Ingress contributors determines this is a relevant issue, they will accept it by applying the The 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. |
Due to the following PR becoming stale: #9441 Ive opened this one to start trying to grep this |
c4f3a05
to
9fe8261
Compare
The resulting configuration does not have the directives set by default Nginx Default Conf
|
9fe8261
to
7440dde
Compare
7440dde
to
e054c10
Compare
e054c10
to
5923693
Compare
We use the PR title as a change log entry. Can you make the title more descriptive? |
@strongjz does that work? |
@Spazzy757 can you help and paste the link to the test that covers the deprecated directive and the replacement directive please |
Signed-off-by: Spazzy <[email protected]>
5923693
to
8e80b01
Compare
@longwuyuan there are no tests for Nginx Config as far as I can tell, how I tested this was I ran the environment with: make dev-env Then I dumped the nginx.conf kubectl -n ingress-nginx ingress-nginx conf > nginx.conf And saw that it doesn't have any of the directives anymore, I also logged out the containers to verify, without the above, when nginx is stopped:
with the changes:
|
@rikatz this is not breaking ingress but this is long long overdue so please comment when you get a chance |
/approve Removing the defaults lgtm, tho it is a breaking change yes, as people are waiting for this to be working without changes and now they need to force on ConfigMap to use it :) |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rikatz, Spazzy757 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 |
What this PR does / why we need it:
Nginx is giving warning about deprecated parameters being used.
This is a check if they are being used and will only send warnings in that case
More info can be found here on the original PR: #8073
Types of changes
Which issue/s this PR fixes
fixes: #7261
How Has This Been Tested?
Checklist: