-
Notifications
You must be signed in to change notification settings - Fork 269
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
Allow NGINX additional config #393
Allow NGINX additional config #393
Conversation
Signed-off-by: Florent Poinsaut <[email protected]>
9ea6f2d
to
3b8d2ac
Compare
Signed-off-by: Florent Poinsaut <[email protected]>
Signed-off-by: Florent Poinsaut <[email protected]>
Signed-off-by: Florent Poinsaut <[email protected]>
4994471
to
ad01b48
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.
@FlorentPoinsaut can you please rebase your PR to get the latest changes and bump the helm chart version?
Signed-off-by: Florent Poinsaut <[email protected]>
796267e
to
884e8bb
Compare
dismissing stale review but can't approve till tests have run
Signed-off-by: Florent Poinsaut <[email protected]>
23cb721
to
ab1a782
Compare
Signed-off-by: Florent Poinsaut <[email protected]>
Signed-off-by: Florent Poinsaut <[email protected]>
Have you tested this already? |
Co-authored-by: JesseBot <[email protected]> Signed-off-by: Florent Poinsaut <[email protected]>
Signed-off-by: Florent Poinsaut <[email protected]>
Yes @jessebot, it's currently on production in one of my hosting. |
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.
Looks good to me, but tagging in a couple of other reviewers just so I didn't miss anything because I still don't have to time to test this 🙏 Seems valid though. @provokateurin or @tvories feel free to merge if you also approve.
@FlorentPoinsaut we merged #465 and #476 for linting prior to this, and so you may need to rebase again. I'm sorry :( |
Done @jessebot |
@tvories and @provokateurin this has been open a while, so I'd like to merge it. Letting you both know as this is a bigger change to nginx. It looks ok to me, but if it breaks anything, we'd probably get users reporting in issues fairly quickly and we'd need to roll back, as nginx is a very common parameter to enable. It does seem fine though. |
Thanks @jessebot! |
@FlorentPoinsaut thanks for all your patience on this! Feels good to finally get this merged :) Extermely minor note: In the future, please allow edits by maintainers on your PRs so that we can update them and move them forward. And again, sorry for this taking so long! |
default.conf: |- | ||
{{- template "default.conf" $ }} | ||
{{- end }} | ||
{{- if .Values.nginx.config.custom }} |
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.
indent golang like yaml + use with instatt of if
{{- if .Values.nginx.config.custom }} | |
{{- with .Values.nginx.config.custom }} | |
zz-custom.conf: |- | |
{{ . | nindent 4 }j | |
{{- end }] |
Pull Request
Description of the change
Use the original
nginx.conf
file to allow additional config..Values.nginx.config.custom
doesn't override the default value anymore, it adds.Benefits
Example: ability to enable NGINX status page.
Possible drawbacks
Applicable issues
Additional information
Checklist
Chart.yaml
according to semver.