Skip to content
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

Merged
merged 12 commits into from
Nov 29, 2023

Conversation

FlorentPoinsaut
Copy link
Contributor

@FlorentPoinsaut FlorentPoinsaut commented May 11, 2023

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

  • fixes #

Additional information

Checklist

@FlorentPoinsaut FlorentPoinsaut marked this pull request as draft May 11, 2023 08:51
Signed-off-by: Florent Poinsaut <[email protected]>
Signed-off-by: Florent Poinsaut <[email protected]>
Signed-off-by: Florent Poinsaut <[email protected]>
@FlorentPoinsaut FlorentPoinsaut marked this pull request as ready for review May 11, 2023 10:21
Signed-off-by: Florent Poinsaut <[email protected]>
@jessebot jessebot self-requested a review September 5, 2023 13:06
Copy link
Collaborator

@jessebot jessebot left a 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?

@jessebot jessebot dismissed their stale review September 9, 2023 07:38

dismissing stale review but can't approve till tests have run

Signed-off-by: Florent Poinsaut <[email protected]>
@jessebot
Copy link
Collaborator

Have you tested this already?

FlorentPoinsaut and others added 2 commits November 13, 2023 15:56
Co-authored-by: JesseBot <[email protected]>
Signed-off-by: Florent Poinsaut <[email protected]>
@FlorentPoinsaut
Copy link
Contributor Author

Yes @jessebot, it's currently on production in one of my hosting.

Copy link
Collaborator

@jessebot jessebot left a 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.

@jessebot
Copy link
Collaborator

@FlorentPoinsaut we merged #465 and #476 for linting prior to this, and so you may need to rebase again. I'm sorry :(

@FlorentPoinsaut
Copy link
Contributor Author

Done @jessebot

@jessebot
Copy link
Collaborator

@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.

@jessebot jessebot merged commit b62953a into nextcloud:main Nov 29, 2023
2 checks passed
@FlorentPoinsaut
Copy link
Contributor Author

Thanks @jessebot!

@jessebot
Copy link
Collaborator

@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 }}
Copy link
Collaborator

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

Suggested change
{{- if .Values.nginx.config.custom }}
{{- with .Values.nginx.config.custom }}
zz-custom.conf: |-
{{ . | nindent 4 }j
{{- end }]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants