Skip to content

DRIVERS-2549: add search index management helpers #1423

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
May 25, 2023
Merged

DRIVERS-2549: add search index management helpers #1423

merged 21 commits into from
May 25, 2023

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented May 17, 2023 •
edited
Loading

This PR adds a new search management API to the index management spec.

Node POC: mongodb/node-mongodb-native#3672

E2E testing of the new helpers against atlas will be implemented in DRIVERS-2630.

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).

@baileympearson baileympearson requested a review from durran May 17, 2023 20:38
baileympearson
baileympearson commented May 18, 2023
View reviewed changes
.github/workflows/check_schema_version.sh
@@ -5,11 +5,7 @@ exitCode=0

# $1 - takes a single argument of the path to the JSON file containing a schemaVersion key at the top level.
Copy link
Contributor Author

@baileympearson baileympearson May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

happy to revert these changes. the changes here and in the workflow file make validating UTR schemas as simple as npm i -g ys-yaml ajv-cli && bash .github/workflows/check_schema_version.sh. Before, js-yaml needed to be installed as an npm library and ajv-cli as a runnable npm package.

@baileympearson baileympearson marked this pull request as ready for review May 18, 2023 19:22
@baileympearson baileympearson requested review from a team as code owners May 18, 2023 19:22
@baileympearson baileympearson requested review from alcaeus and removed request for a team May 18, 2023 19:22
@baileympearson baileympearson mentioned this pull request May 18, 2023
Merged
5 tasks
durran
durran requested changes May 19, 2023
View reviewed changes
@baileympearson baileympearson requested a review from durran May 19, 2023 19:50
durran
durran requested changes May 22, 2023
View reviewed changes
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

Just last 2 open comments.

@baileympearson baileympearson requested a review from durran May 22, 2023 15:24
jmikola
jmikola reviewed May 22, 2023
View reviewed changes
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.

Just leaving some drive-by feedback, which you can do with as you wish.

source/index-management/tests/createSearchIndex.yml Outdated
@@ -0,0 +1,62 @@
description: "createSearchIndex convenience helper"
schemaVersion: "1.10"
Copy link
Member

Choose a reason for hiding this comment

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

What about these test files require 1.10? That was when the thread entity was introduced (see: changelog).

Per schemaVersion:

Test files SHOULD be conservative when specifying a schema version. For example, if the latest schema version is 1.1 but the test file complies with schema version 1.0, the test file should specify 1.0.

This plays nicely with CI, which uses the test file's schemaVersion field for validation.

kevinAlbs
kevinAlbs reviewed May 23, 2023
View reviewed changes
@baileympearson @jmikola @kevinAlbs
Apply suggestions from code review
84f4152 
Apply changes from review to RST files.

Co-authored-by: Jeremy Mikola <[email protected]>
Co-authored-by: Kevin Albertson <[email protected]>
@baileympearson baileympearson requested a review from a team as a code owner May 24, 2023 14:33
@baileympearson baileympearson requested review from katcharov and removed request for a team May 24, 2023 14:33
@baileympearson baileympearson requested a review from kevinAlbs May 24, 2023 14:36
@alcaeus alcaeus removed their request for review May 25, 2023 12:12
kevinAlbs
kevinAlbs approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM with one suggested rename. Tests pass in C driver.

source/index-management/tests/dropSearchIndexes.yml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest renaming file to dropSearchIndex.yml to match operation name.

@durran durran merged commit ae35908 into master May 25, 2023
@durran durran deleted the DRIVERS-2549-search-index-management branch May 25, 2023 15:25
@jmikola jmikola mentioned this pull request Jun 1, 2023
Merged
@joshbsiegel joshbsiegel mentioned this pull request Jul 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmikola jmikola jmikola left review comments

@durran durran durran approved these changes

@kevinAlbs kevinAlbs kevinAlbs approved these changes

@katcharov katcharov Awaiting requested review from katcharov katcharov is a code owner automatically assigned from mongodb/dbx-spec-owners-fle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@baileympearson @durran @jmikola @kevinAlbs