Skip to content

Making Nginx optional #32

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 26, 2020
Merged

Making Nginx optional #32

merged 2 commits into from
Jul 26, 2020

Conversation

khaines
Copy link
Contributor

@khaines khaines commented Jul 2, 2020

Instances of cortex that have their own frontdoor/proxy can turn off nginx now.

Addresses #29

Signed-off-by: Ken Haines [email protected]

@rsteneteg
Copy link
Contributor

Hi Ken, thanks for making this change

Since the ingress resource is using the .Values.nginx.service.port it makes sense to also disable the ingress resource if nginx is disabled. I added this in my local copy of ingress,yaml

{{- if and .Values.ingress.enabled .Values.nginx.enabled -}}

@khaines
Copy link
Contributor Author

khaines commented Jul 4, 2020

Great callout Roger. I've updated the if statement

@rsteneteg
Copy link
Contributor

Thanks Ken, LGTM

@khaines khaines merged commit 965804d into master Jul 26, 2020
@khaines khaines deleted the khaines/optional-nginx branch August 2, 2020 23:17
Skaronator pushed a commit to Skaronator/cortex-helm-chart that referenced this pull request Apr 28, 2021
@khaines
Making Nginx optional (cortexproject#32)
5faef94 
* making nginx an optional component based on an enabled flag in config.yaml

Signed-off-by: Ken Haines <[email protected]>

* updating ingress to only be enabled if nginx is, in addition to the previous check

Signed-off-by: Ken Haines <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@khaines @rsteneteg