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

Documentation on Enabling SSL/TLS support for SQL Server. #21938

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Documentation on Enabling SSL/TLS support for SQL Server. #21938

merged 1 commit into from
Feb 22, 2024

Conversation

sabbasani
Copy link
Contributor

@sabbasani sabbasani commented Feb 15, 2024

Description

Enabling SSL support for SQL Server

Motivation and Context

Added security feature SSL/TLS where users can connect to SSL/TLS enabled Datasource.

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==
SQL Server Docs
* Add Connection security documentation for the SQL Server Connector.

Copy link

linux-foundation-easycla bot commented Feb 15, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot added the docs label Feb 15, 2024
@sabbasani sabbasani changed the title Update sqlserver.rst Enabling SSL/TLS support for SQL Server. Feb 15, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thank you for this docs contribution! I made some suggestions to shorten a few statements, a couple of punctuation changes, and to make this draft more consistent with the existing Presto docs, but this was great. If you have any issues with my suggestions, let's discuss and figure it out together.

presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
@steveburnett
Copy link
Contributor

Thanks for writing a release note entry! As a suggestion using the release notes guidelines, consider the following:

== RELEASE NOTES ==
General Changes
* Add Connection security documentation for the SQL Server Connector.

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pulled updated branch, new local build, everything looks good.

@steveburnett
Copy link
Contributor

  • Please squash merge commits as mentioned in the Pull Request Checklist of CONTRIBUTING.md.

  • Do you have more work you feel you need to do? If not, consider removing Draft/work in progress status by selecting the Ready for review button in the Checks section at the bottom of the PR.

@sabbasani sabbasani marked this pull request as ready for review February 19, 2024 06:15
@sabbasani sabbasani requested a review from a team as a code owner February 19, 2024 06:15
@sabbasani sabbasani requested a review from presto-oss February 19, 2024 06:15
Copy link

github-actions bot commented Feb 19, 2024

Codenotify: Notifying subscribers in CODENOTIFY files for diff 2bd3f5e...0f48cd0.

Notify File(s)
@steveburnett presto-docs/src/main/sphinx/connector/sqlserver.rst

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Two final nits, both intended to improve readability. Let me know what you think, please!

presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thanks for the very fast response today! I suggested a revision to your revision, and possibly consider adding an example of a connection-url for a truststore config.

presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
steveburnett
steveburnett previously approved these changes Feb 19, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)
Thanks for all your work!

Nit question about the new code example, everything else looks fine.

Pulled updated branch, new local build of docs, everything looks good.

presto-docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
@skairali skairali changed the title Enabling SSL/TLS support for SQL Server. Documentation on Enabling SSL/TLS support for SQL Server. Feb 20, 2024
Copy link
Member

@skairali skairali left a comment

Choose a reason for hiding this comment

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

LGTM

This is a required change and useful to all who want to enable TLS support in connections to SQL server from Presto

@tdcmeehan
Copy link
Contributor

Thanks for adding this documentation. Please fix the commit message per our guidelines linked in the checklist in your PR. Suggested:

Add TLS documentation for SQL Server

Remove all text in the body as it is superfluous.

@sabbasani
Copy link
Contributor Author

Thanks for adding this documentation. Please fix the commit message per our guidelines linked in the checklist in your PR. Suggested:

Add TLS documentation for SQL Server

Remove all text in the body as it is superfluous.

Thanks @tdcmeehan !! I have changed commit message as per your suggestion.

@skairali skairali merged commit 9d7b77c into prestodb:master Feb 22, 2024
57 checks passed
@wanglinsong wanglinsong mentioned this pull request May 1, 2024
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants