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

DRIVERS-2789 Convert CSE Spec to Markdown #1527

Merged
merged 45 commits into from
Mar 4, 2024

Conversation

blink1073
Copy link
Member

Please complete the following before merging:

  • Update changelog.
  • Make sure there are generated JSON files from the YAML test files.
  • Test changes in at least one language driver.
  • Test these changes against all server versions and topologies (including standalone, replica set, sharded
    clusters, and serverless).

@blink1073 blink1073 requested review from a team as code owners February 27, 2024 12:35
@blink1073 blink1073 requested review from jmikola, tom-selander and dariakp and removed request for a team, jmikola, tom-selander and dariakp February 27, 2024 12:35
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

I'll review again once you take a pass through the generated heading links. Also, you should probably replace @dariakp with another reviewer.

source/unified-test-format/unified-test-format.md Outdated Show resolved Hide resolved
source/unified-test-format/unified-test-format.rst Outdated Show resolved Hide resolved
source/unified-test-format/unified-test-format.md Outdated Show resolved Hide resolved
source/unified-test-format/unified-test-format.rst Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.md Outdated Show resolved Hide resolved
See also:

> - [Introduction on crypt_shared](#crypt_shared)
> - \[Enabling crypt_shared\](#Enabling crypt_shared)
Copy link
Member

Choose a reason for hiding this comment

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

The "Enabling crypt_shared" links are broken throughout this document.

Also, the block-quoting probably isn't necessary here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

Choose a reason for hiding this comment

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

This still appears outstanding.

Copy link
Member Author

Choose a reason for hiding this comment

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

Re-fixed

source/client-side-encryption/client-side-encryption.md Outdated Show resolved Hide resolved
blink1073 and others added 6 commits February 27, 2024 11:24
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073
Copy link
Member Author

I addressed some but not all comments, stopping here for today.

@blink1073
Copy link
Member Author

I believe I've addressed all review.

@blink1073 blink1073 requested a review from jmikola March 1, 2024 00:54
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

Feel free to take whatever suggestions you want. I don't think I need to come back and look through this a fourth time.

.gitignore Outdated
@@ -10,3 +10,4 @@ logo.png
codereview.rc
.idea/**
docs_build
.pytest_cache
Copy link
Member

Choose a reason for hiding this comment

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

do you want to add a trailing newline?

Comment on lines 35 to 36
- Addition of `\$db` to command in `command_started_event`
- Addition of `\$\$type` to command_started_event and outcome.
Copy link
Member

Choose a reason for hiding this comment

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

These dollar signs should not be escaped. Also applies to some content later in the file.

should ensure that `crypt_shared` will not be loaded. Refer to the client-side-encryption documentation for information
on "disabling" `crypt_shared` and setting library search paths.

> \[!NOTE\] The [crypt_shared](../client-side-encryption.rst#crypt_shared) dynamic library can be obtained using the
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary escaping for the "NOTE" element. Also applies later in the file.


KMS providers

> A map of KMS providers to credentials. Configured client-side. Example:
Copy link
Member

Choose a reason for hiding this comment

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

Is the blockquote intentional here?

An alternative approach to formatting the terminology lists would be to apply bold formatting to the leading terms and/or use a bulleted list. Doing both would probably be best for readability.


[crypt_shared](#crypt_shared) is a dynamically-loaded C++ library providing query analysis for auto-encryption. It
replaces [mongocryptd](#mongocryptd) for performing query analysis to -
Ref:`mark-up sensitive fields within a command <subtype6.intent-to-encrypt>`.
Copy link
Member

Choose a reason for hiding this comment

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

"- Ref:`mark-up..." looks like an artifact.

source/client-side-encryption/client-side-encryption.md Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.md Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.md Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.md Outdated Show resolved Hide resolved
source/client-side-encryption/client-side-encryption.md Outdated Show resolved Hide resolved
blink1073 and others added 17 commits March 4, 2024 06:28
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073
Copy link
Member Author

Thanks for the review both! As suggested, let's merge and iterate.

@blink1073 blink1073 merged commit fc360b2 into mongodb:master Mar 4, 2024
4 checks passed
@blink1073 blink1073 deleted the DRIVERS-2789-cse-2 branch March 4, 2024 12:45
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