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: allow to specify ssl: true in connection options #3387

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pkuczynski
Copy link
Contributor

Follow up for #3384, inspired by #3028

Copy link

codecov bot commented Feb 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.78%. Comparing base (a446fac) to head (b5e3a78).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3387   +/-   ##
=======================================
  Coverage   88.77%   88.78%           
=======================================
  Files          85       85           
  Lines       13448    13453    +5     
  Branches     1535     1537    +2     
=======================================
+ Hits        11939    11944    +5     
  Misses       1509     1509           
Flag Coverage Δ
compression-0 88.78% <100.00%> (+<0.01%) ⬆️
compression-1 88.78% <100.00%> (+<0.01%) ⬆️
static-parser-0 86.34% <100.00%> (+<0.01%) ⬆️
static-parser-1 87.17% <100.00%> (+<0.01%) ⬆️
tls-0 88.19% <100.00%> (+<0.01%) ⬆️
tls-1 88.55% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wellwelwel referenced this pull request Feb 9, 2025
* Update 00-index.mdx

* Create ssl.mdx

* Update ssl.mdx

* Update ssl.mdx

* Update ssl.mdx

* add default cert example

* Update website/docs/documentation/ssl.mdx

Co-authored-by: Weslley Araújo <[email protected]>

* Update website/docs/documentation/ssl.mdx

Co-authored-by: Weslley Araújo <[email protected]>

* Update website/docs/documentation/ssl.mdx

Co-authored-by: Weslley Araújo <[email protected]>

* Update website/docs/documentation/ssl.mdx

Co-authored-by: Weslley Araújo <[email protected]>

* Update website/docs/documentation/ssl.mdx

Co-authored-by: Weslley Araújo <[email protected]>

* some changes

* lint

* Update website/docs/documentation/ssl.mdx

---------

Co-authored-by: Weslley Araújo <[email protected]>
@wellwelwel
Copy link
Collaborator

wellwelwel commented Feb 10, 2025

Thanks, @pkuczynski!

I'll review your PR as soon as possible. @sidorares, feel free to review it first 🤝


@pkuczynski, the website (docs) has some customized components. Normally I don't suggest it to the contributors so as not to make it unnecessarily complicated (I put it in a personal queue to adjust it later). But to prevent confusion, I'm adding them as optional tips here 🙋🏻‍♂️

ExternalCodeEmbed

https://sidorares.github.io/node-mysql2/docs/contributing/website#externalcodeembed

Tip

With this component, plus the y tip in a446fac#commitcomment-152332163, we can dynamically demonstrate all the options in the documentation.

You can see a practical example by clicking in ConnectionOptions Specification in:

History

https://sidorares.github.io/node-mysql2/docs/contributing/website#history

Tip

Here we can refer to the next semver version (3.13.0), which will make it possible to use the ssl property with boolean values.

You can see a practical example by clicking in History and see the ResultSetHeader change log in:

Stability

https://sidorares.github.io/node-mysql2/docs/contributing/website#stability

Tip

In the documentation, instead of SSL Profile (deprecated), we can remove the parenthesis and replace it with a stability badge, for example: <Stability level={0} />.

You can see a practical example in:

All these suggestions are optional and you can find the documentation contribution guide at sidorares.github.io/node-mysql2/docs/contributing/website 💡

@wellwelwel wellwelwel changed the title feat: allow to specify ssl: true in connection options feat: allow to specify ssl: true in connection options Feb 10, 2025
@wellwelwel
Copy link
Collaborator

@pkuczynski, sorry for the delay.

The ssl value is set in:

this.ssl =
typeof options.ssl === 'string'
? ConnectionConfig.getSSLProfile(options.ssl)
: options.ssl || false;

I think it would be beneficial to keep the logic contained in the same place (especially as it's easier to maintain in the long-term), what do you think?

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