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

resolves #787 add support for SSL #955

Merged
merged 11 commits into from
Oct 28, 2021

Conversation

amirabramovich
Copy link
Contributor

resolved issue #787

@ggrossetie
Copy link
Member

Looks good, left a few suggestions (style) and an open question.
Would you be able to write the documentation for this feature? I guess you can add a section in https://github.com/yuzutech/kroki/blob/main/docs/modules/setup/pages/configuration.adoc

I can tell you that the search term "ssl" and "https" are pretty popular on https://docs.kroki.io/ so it's definitely something users will be interested to read 😄

@amirabramovich
Copy link
Contributor Author

Thanks for your suggestions, I made all the adjustments and added a documentation :)

@ggrossetie
Copy link
Member

I made all the adjustments and added a documentation :)

Thanks!

I just noticed that we are using setKeyPath and setCertPath however I think it's easier to pass the contents of the private key and the certificate.

In my experience it makes things easier since you don't have a to mount a volume to provide the certificate and private key files. You can just pass the contents of the certificate and the private key as string using environment variables.

Taking this into account, I've made several suggestions.

amirabramovich and others added 6 commits October 22, 2021 15:16
Co-authored-by: Guillaume Grossetie <g.grossetie@gmail.com>
Co-authored-by: Guillaume Grossetie <g.grossetie@gmail.com>
Co-authored-by: Guillaume Grossetie <g.grossetie@gmail.com>
Co-authored-by: Guillaume Grossetie <g.grossetie@gmail.com>
Co-authored-by: Guillaume Grossetie <g.grossetie@gmail.com>
@amirabramovich
Copy link
Contributor Author

Thanks, I changed the path to the value itself. Please review and merge :)

Copy link
Member

@ggrossetie ggrossetie left a comment

Choose a reason for hiding this comment

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

It looks good, thanks!

I made tiny changes in ServerSSLTest to assert that the deployment is failing when the SSL is enabled but KROKI_SSL_KEY is missing.

Apart from that, great work 👌🏻

@ggrossetie ggrossetie changed the title added support for SSL resolves #787 add support for SSL Oct 28, 2021
@ggrossetie ggrossetie merged commit 3edbf7e into yuzutech:main Oct 28, 2021
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.

2 participants