-
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
Add feature toggle for custom error pages /metrics #10984
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for kubernetes-ingress-nginx canceled.
|
Welcome @ricardoapl! |
Hi @ricardoapl. 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. |
This is still missing changes to the Helm chart, but I would really appreciate some feedback before the implementation is complete |
/assign @tao12345666333 |
/ok-to-test |
@tao12345666333 @strongjz please take another look, I believe I have resolved all of the issues |
Thanks! I will have a test today. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ricardoapl The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4e69af7
to
86e89ed
Compare
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.
I basically like the idea, but it's changing a lot of stuff in places it shouldn't.
Please first make sure you rebased correctly as you're currently re-adding stuff we already removed some time ago.
images/Makefile
Outdated
PLATFORMS?=linux/amd64,linux/arm,linux/arm64 | ||
OUTPUT= | ||
PROGRESS=plain | ||
BUILDX_PLATFORMS ?= linux/amd64,linux/arm,linux/arm64,linux/s390x |
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.
We have dropped s390x
. I think you rebased your PR wrongly.
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 be resolved with 9781d2b
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
Signed-off-by: Ricardo Lopes <[email protected]>
82c6e42
to
077675d
Compare
Signed-off-by: Ricardo Lopes <[email protected]>
077675d
to
9781d2b
Compare
Thank you for taking the time to look at this @Gacko, I really appreciate it
Can you please share an example or two of where this is happening?
I believe I've resolved this with 9781d2b, let me know if I missed anything |
What this PR does / why we need it:
Adds feature toggle to expose /metrics and /debug/vars endpoints from custom error pages default backend
Types of changes
Which issue/s this PR fixes
Fixes #9152
How Has This Been Tested?
Tested manually on local machine by running the binary with different environment variable combinations such as
Checklist: