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

feat: add a Helm chart #512

Merged
merged 14 commits into from
May 5, 2021
Merged

feat: add a Helm chart #512

merged 14 commits into from
May 5, 2021

Conversation

dunglas
Copy link
Owner

@dunglas dunglas commented Apr 30, 2021

Add a new Helm chart for the Caddy build. Fixes #357.

@sagikazarmark, I hesitated to use your Caddy chart, but as the hub has some specificities, I don't think that it's worth it, so I created a more specific one. However, I'll be glad to collaborate with you to share everything that can be shared.

@sagikazarmark
Copy link
Contributor

@dunglas sure, a specific chart is better in my opinion.

I think using helm-docs is a good way to generate documentation from the values file, so I'd consider adding that.

I'd also consider grouping application specific configuration under a config key, just to make it more separated from the rest of the chart.

Updating the ingress template is also worth doing: https://github.com/sagikazarmark/helm-charts/blob/master/charts/caddy/templates/ingress.yaml
I already have PR that will hopefully will be merged in Helm 3.6: helm/helm#9621

Copy link
Contributor

@nitneuk nitneuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

helm/mercure/templates/_helpers.tpl Outdated Show resolved Hide resolved
helm/mercure/templates/deployment.yaml Outdated Show resolved Hide resolved
- type: Resource
resource:
name: cpu
targetAverageUtilization: {{ .Values.autoscaling.targetCPUUtilizationPercentage }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I was facing this issue when using HPA with average cpu utilization (but with autoscaling/v2beta2)
kubernetes/kubernetes#56165

helm/mercure/templates/deployment.yaml Outdated Show resolved Hide resolved
helm/mercure/templates/ingress.yaml Outdated Show resolved Hide resolved
helm/mercure/templates/ingress.yaml Outdated Show resolved Hide resolved
@dunglas dunglas force-pushed the feat/helm branch 3 times, most recently from e689d3d to 2c54fb8 Compare May 4, 2021 15:24
key: extra-directives
{{- if .Values.persistence.enabled }}
volumeMounts:
- mountPath: /data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more conservative on these issues, so I'd vote for /var/mercure.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our Docker image and the official Caddy image already work like that.

charts/mercure/templates/ingress.yaml Outdated Show resolved Hide resolved
Comment on lines 5 to 25
# -- enable the debug mode
debug: false
# -- enable the development mode, including the debug UI and the demo
dev: false
# -- the URL representation of the transport to use
transportUrl: bolt:///data/mercure.db
# -- extra Mercure directives to include in the Caddyfile
extraDirectives: ""

# -- the JWT key to use for publishers, a random key will be generated if empty
publisherJwtKey: ""
# -- the JWT algorithm to use for publishers
publisherJwtAlg: HS256

# -- the JWT key to use for subscribers, a random key will be generated if empty
subscriberJwtKey: ""
# -- JWT algorithm to use for subscribers
subscriberJwtAlg: HS256

# -- license key for [the High Availability version](https://mercure.rocks/docs/hub/cluster) (not necessary is you use the FOSS version):
license: ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think these should be under a config key to better separate them from the rest of the chart values.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a common practice? AFAIK most charts don't do that (and it looks in contractions with Helm's best practices: https://helm.sh/docs/chart_best_practices/).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a contradiction with the best practices. I don't think the claim about the mandatory check in case of nesting is true.

I honestly can't tell whether most charts do it or not. It certainly proved useful for me. Take it with a grain of salt.

dunglas and others added 3 commits May 4, 2021 17:44
Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com>
* 'feat/helm' of github.com:dunglas/mercure:
  Update charts/mercure/templates/ingress.yaml
@dunglas dunglas merged commit 4a9a18b into main May 5, 2021
@dunglas dunglas deleted the feat/helm branch May 5, 2021 15:37
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.

Move the Helm chart to this repo
4 participants