-
Notifications
You must be signed in to change notification settings - Fork 3k
Make the Mailer extension trigger the SSL support #3377
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
Make the Mailer extension trigger the SSL support #3377
Conversation
== Using SSL with native executables | ||
|
||
Note that if you enable SSL for the mailer and you want to build a native executable, you will need to enable the SSL support. | ||
Please refer to the native-and-ssl-guide.html[Using SSL With Native Executables] guide for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is still interesting as you might have to bundle the SSL libraries in your Docker image for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we keep this part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would keep it.
|
||
Note that if you enable SSL for the mailer and you want to build a native executable, you will need to enable the SSL support. | ||
Please refer to the native-and-ssl-guide.html[Using SSL With Native Executables] guide for more information. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also update the list of extensions enabling SSL automatically here: https://quarkus.io/guides/native-and-ssl-guide ?
It's just after the sentence As SSL is ipso facto the standard nowadays, we decided to enable its support automatically for some of our extensions:
.
I think we miss some of the recently added (Neo4j comes to mind). We should s/SmallRye REST client/REST client/ too. And maybe we should add the name of the extensions in parentheses (quarkus-mailer
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I’ll update that part.
b062781
to
4007717
Compare
==== | ||
|
||
=== Working with containers | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want these changes, do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, my IDE was not synced with recent changes. I'll revert this.
When working with containers, the idea is to bundle both the SunEC library and the certificates in the container and to point your binary to them using the system properties mentioned above. | ||
|
||
You can for example modify your `Dockerfile.native` as follows to copy the required files to your final image: | ||
You can for example modify your `Dockerfile.native` as following to allow copying the required lib in your final image: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you reverted some changes I made recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Sorry for the noise. I'll rebase and apply the change I need to make.
* the Agroal connection pooling extension (`quarkus-agroal`), | ||
* the Jaeger extension (`quarkus-jaeger`), | ||
* the mailer extension (`quarkus-mailer`) | ||
* the Neo4j extension (`quarkus-neo4j`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neo4j was an example, there are a lot more missing. Just look for references to ExtensionSslNativeSupportBuildItem
.
As for the Elasticsearch client extension, better name the Hibernate Search + Elasticsearch one because that's the one we want to advertise.
* the REST client extension (`quarkus-rest-client`), | ||
* the Agroal connection pooling extension (`quarkus-agroal`), | ||
* the Jaeger extension (`quarkus-jaeger`), | ||
* the mailer extension (`quarkus-mailer`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* the mailer extension (`quarkus-mailer`) | |
* the Mailer extension (`quarkus-mailer`) |
4007717
to
438897e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're nearly there :).
* the Agroal connection pooling extension, | ||
* the Jaeger extension. | ||
* the Agroal connection pooling extension (`quarkus-agroal`), | ||
* the DynamoDB extension (`quarkus-amazon-dynamodb`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should talk about Amazon DynamoDB
* the Jaeger extension. | ||
* the Agroal connection pooling extension (`quarkus-agroal`), | ||
* the DynamoDB extension (`quarkus-amazon-dynamodb`), | ||
* the Elasticsearch Rest client extension (`quarkus-elasticsearch-rest-client`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said in another comment, here I would advertise the Hibernate Search + Elasticsearch extension instead of the client.
For now, we don't really want the users to include this extension directly.
---- | ||
|
||
== Using SSL with native executables | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better with a new line.
438897e
to
4367824
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool thanks!
Merged, thanks! |
Fixes #3221