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

Bump gotenberg from 8.5.1 to 8.7.0 & add HTTPS (TLS) support #35

Merged
merged 5 commits into from
Jun 21, 2024

Conversation

jonasgeiler
Copy link
Contributor

@jonasgeiler jonasgeiler commented Jun 5, 2024

This PR bumps the gotenberg version from 8.5.1 to 8.6.0, which adds HTTPS (TLS) support so I added the new flags to the helm chart.

https://github.com/gotenberg/gotenberg/releases/tag/v8.6.0

Tasks:

  • I've made changes
  • I've bumped chart's version in Chart.yaml
  • I've added changes to charts CHANGELOG.md
  • I've run helm-docs

@jonasgeiler jonasgeiler changed the title Bump gotenberg from 8.5.1 to 8.6.0 & add HTTPS support Bump gotenberg from 8.5.1 to 8.6.0 & add HTTPS (TLS) support Jun 5, 2024
@MaikuMori
Copy link
Owner

Hey, I was going to add this. I want to mimic what other charts do. Usually in this situation I allow referencing a secret with the keys that is set outside the normal helm deployment, but I'll check how this is usually handled in similar charts.

I rebased and added attribution for now.

@jonasgeiler
Copy link
Contributor Author

jonasgeiler commented Jun 6, 2024

@MaikuMori wrote:
Hey, I was going to add this. I want to mimic what other charts do. Usually in this situation I allow referencing a secret with the keys that is set outside the normal helm deployment, but I'll check how this is usually handled in similar charts.

I rebased and added attribution for now.

I think most would just use a volume with the TLS files in it and attach use the volumes/volumeMounts values to attach them. Then you can just set the path to the correct files and you're done. But you could also go full-blown and do it like bitnami with auto-generated certificates and a bunch of other stuff 😅 I'll leave it up to you!

@jonasgeiler
Copy link
Contributor Author

@MaikuMori If you tell me how exactly you want to implement TLS support I could improve or rework this PR myself if you want.

@MaikuMori
Copy link
Owner

Hi, sorry, I've been slacking.

Basically, the 2 features I'd like are:

  • Support setting secret name and 2 related keys inside the secret. (The secret itself would be managed outside, this is my use case)
  • Bonus: Support setting annotations so this can be done via cert-manager

Regarding volumes:

  • If we use volumes then maybe make it more automatic
  • Do we use volumes if secrets are available? If we provide the keys via Helm values (I don't like it, but I understand people would use this) then why not just expose 2 values for that and behind the scenes generate the secret.

This way, all cases are Secret based:

  • Secret managed by Helm
  • External secret
  • cert-manager also makes external secret

What do you think? I could try to add these changes tomorrow.

@MaikuMori MaikuMori changed the title Bump gotenberg from 8.5.1 to 8.6.0 & add HTTPS (TLS) support Bump gotenberg from 8.5.1 to 8.7.0 & add HTTPS (TLS) support Jun 21, 2024
@MaikuMori
Copy link
Owner

It was easier than I expected. I think there is no disadvantage of not doing it via manual volume mounts. @jonasgeiler Let me know if there are issues doing it this way in your specific workflow. Personally, I think this is more idiomatic and simpler.

Also, this should support cert-manager certificates. It already supported TLS via ingress annotations previously.

@MaikuMori MaikuMori merged commit 2854221 into MaikuMori:master Jun 21, 2024
6 checks passed
@jonasgeiler
Copy link
Contributor Author

@MaikuMori sorry for not replying earlier but yeah sounds good! Thanks! I'll try it out.

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.

None yet

2 participants