Skip to content

CPP-990 Add brief section on OpenSSL 3.x to building doc#557

Merged
absurdfarce merged 4 commits intomasterfrom
cpp990
Sep 29, 2023
Merged

CPP-990 Add brief section on OpenSSL 3.x to building doc#557
absurdfarce merged 4 commits intomasterfrom
cpp990

Conversation

@absurdfarce
Copy link
Copy Markdown
Contributor

No description provided.

Comment thread topics/building/README.md
platforms.

Given these results we expect the driver to behave well with OpenSSL 3.x.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@emeliawilkinson24 and @weideng1: I'm not 💯 convinced this paragraph is actually useful. The same information is provided in more detail in the comments of CPP-990 and I'm not sure a paragraph like the one above gives the user any tools to make their experience with the driver better.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@absurdfarce I agree. I think the brief note on open SSL 3.x is largely speculative, the only part definitely needed there in my opinion is "Two officially supported platforms (Ubuntu 22.04 and Rocky Linux 9.2) come with OpenSSL 3.x by default and the unit and integration tests all pass on these platforms." We shouldn't say we think something will work well, we want to tell users we tested it and according to everything we know it definitely works.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me @emeliawilkinson24 . After the other edits you had on your review I think we can get to something I can live with if I just remove that last sentence. I'll give it a shot and see what it looks like.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, I liked how things look with that sentence out. Take another pass when you have some time @emeliawilkinson24 and let me know what you think!

Copy link
Copy Markdown
Contributor

@weideng1 weideng1 left a comment

Choose a reason for hiding this comment

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

Good explanation of the background with the additional paragraph you added. LGTM

Comment thread topics/building/README.md
* [libuv] 1.x
* Kerberos v5 ([Heimdal] or [MIT]) \*
* [OpenSSL] v1.0.x or v1.1.x \*\*
* [OpenSSL] v1.0.x, v1.1.x or v3.x \*\*
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* [OpenSSL] v1.0.x, v1.1.x or v3.x \*\*
* [OpenSSL] v1.0.x, v1.1.x, or v3.x \*\*

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wait, are we gonna have to duke it out about the Harvard comma now? :)

Comment thread topics/building/README.md

### A Brief Note on OpenSSL 3.x

Migrating from OpenSSL 1.1.x to 3.x largely involves avoiding the use of many functions which are now deprecated (consult
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Migrating from OpenSSL 1.1.x to 3.x largely involves avoiding the use of many functions which are now deprecated (consult
Migrating from OpenSSL 1.1.x to 3.x avoids using many functions which are now deprecated (consult

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think this one quite works. "Migrating from OpenSSL X to Y avoids using many functions which are now deprecated" seems wrong. The intent is to communicate what migration involves, not the effect of migration.

Comment thread topics/building/README.md Outdated
Comment thread topics/building/README.md Outdated
Comment thread topics/building/README.md
platforms.

Given these results we expect the driver to behave well with OpenSSL 3.x.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@absurdfarce I agree. I think the brief note on open SSL 3.x is largely speculative, the only part definitely needed there in my opinion is "Two officially supported platforms (Ubuntu 22.04 and Rocky Linux 9.2) come with OpenSSL 3.x by default and the unit and integration tests all pass on these platforms." We shouldn't say we think something will work well, we want to tell users we tested it and according to everything we know it definitely works.

absurdfarce and others added 3 commits September 28, 2023 16:53
Co-authored-by: Emelia <105240296+emeliawilkinson24@users.noreply.github.com>
Co-authored-by: Emelia <105240296+emeliawilkinson24@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@emeliawilkinson24 emeliawilkinson24 left a comment

Choose a reason for hiding this comment

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

LGTM

@absurdfarce absurdfarce merged commit f15ee23 into master Sep 29, 2023
@absurdfarce absurdfarce deleted the cpp990 branch September 29, 2023 15:35
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.

3 participants