-
Notifications
You must be signed in to change notification settings - Fork 515
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
Ability to Customize HAProxy 2.x Error Page #843
Ability to Customize HAProxy 2.x Error Page #843
Conversation
9445ba7
to
9fcd610
Compare
@Miciah PTAL |
9fcd610
to
ba8c5b1
Compare
operator/v1/types_ingress.go
Outdated
//This used to sync the configmap having customizable error code page created by cluster admin in openshift-config namespace. | ||
//to openshift-ingress which will be used by the router to serve appropriate error code pages. | ||
//If this field is empty, the default values i.e default shipped error code pages will be served by the haproxy router. |
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.
Some suggestions:
- Add a space after
//
on each line. - Start with the field's name.
- Focus on the usage rather than the implementation.
- Describe the content of the configmap (fill in the "[...]" in the suggestion below).
- Make the field optional.
- Should the field name be plural ("pages" rather than "page")?
//This used to sync the configmap having customizable error code page created by cluster admin in openshift-config namespace. | |
//to openshift-ingress which will be used by the router to serve appropriate error code pages. | |
//If this field is empty, the default values i.e default shipped error code pages will be served by the haproxy router. | |
// httpErrorCodePage specifies a configmap with customizable error pages in the openshift-config namespace. | |
// This configmap should have keys [...]. | |
// If this field is empty, the ingress controller uses the default error pages. | |
// | |
// +optional |
ba8c5b1
to
0f1eb64
Compare
0f1eb64
to
254b9bb
Compare
/lgtm |
operator/v1/types_ingress.go
Outdated
// Currently only error pages for 503 and 404 responses can be customized. | ||
// Each value in the configmap should be the full response, including HTTP headers. | ||
// If this field is empty, the ingress controller uses the default error pages. | ||
HttpErrorCodePages string `json:"httpErrorCodePages,omitempty"` |
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.
Sorry, I missed this:
HttpErrorCodePages string `json:"httpErrorCodePages,omitempty"` | |
HttpErrorCodePages configv1.ConfigMapNameReference `json:"httpErrorCodePages,omitempty"` |
/lgtm cancel
5c95da1
to
32ad9ee
Compare
@Miciah The make verify locally is passing not sure why it fails in the CI with Generating deepcopy funcs
|
32ad9ee
to
57f726a
Compare
Thanks! |
if this is openshift/enhancements#680, why isn't the enhancement merged? Merging API before enhancement is backwards. |
// where <error code> is an HTTP error code. | ||
// For example, "error-page-503.http" defines an error page for HTTP 503 responses. | ||
// Currently only error pages for 503 and 404 responses can be customized. | ||
// Each value in the configmap should be the full response, including HTTP headers. |
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.
how big a sample format? it's not obvious to me what I would place in this configmap value.
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.
https://github.com/openshift/router/blob/master/images/router/haproxy/conf/error-page-503.http (the default HTTP 503 error page) is a representative sample, and it's 3.34 KiB.
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.
https://github.com/openshift/router/blob/master/images/router/haproxy/conf/error-page-503.http (the default HTTP 503 error page) is a representative sample, and it's 3.34 KiB.
Can you create a stable link to this and add it here please? I suspect many users will have similar questions/requests.
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.
https://github.com/openshift/enhancements/blob/master/enhancements/ingress/customize-error-code-pages.md#proposal has the details on the configmap.
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.
@deads2k have worked with @Miciah on this. We will merge it soon. |
Add a stable link to the example since it is so large and this lgtm. |
@deads2k Permanent link- |
57f726a
to
ed57d80
Compare
ed57d80
to
c5d5f72
Compare
@deads2k added the changes. PTAL. |
[miheer@localhost api]$ make verify
|
c5d5f72
to
9695836
Compare
/retest |
9695836
to
71064b8
Compare
|
@deads2k can you please approve this PR ? |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, Miciah, miheer 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 |
https://issues.redhat.com/browse/NE-379