Skip to content

ci(NODE-4698): test csfle with mongocryptd #3684

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

Merged

Conversation

W-A-James
Copy link
Contributor

@W-A-James W-A-James commented May 25, 2023

Description

What is changing?

Summary
Adding new CI variants to run CSFLE tests with mongocryptd instead of crypt_shared, which we already had coverage for.

changes to config.in.yml
  • script in "run tests" now conditionally executes .evergreen/prepare-crypt-shared-lib.sh
  • script in "run tests" now sets the TEST_NPM_SCRIPT env variable
changes to generate_evergreen_tasks.js
  • Added new tasks that only run the csfle integration suite on relevant server versions and topologies using mongocryptd
Is there new documentation needed for these changes?

No

What is the motivation for this change?

Spec compliance

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@W-A-James W-A-James marked this pull request as ready for review May 25, 2023 14:37
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

one small comment, otherwise looks great

Comment on lines 466 to 469
.filter((mongoVersion) => { // Only run on server versions >= 4.2
const numericVersion = Number(mongoVersion);
return !Number.isNaN(numericVersion) && numericVersion >= 4.2;
})
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually have a package called semver that we use for version comparison:

also, I think we should test on latest / rapid as well

Suggested change
.filter((mongoVersion) => { // Only run on server versions >= 4.2
const numericVersion = Number(mongoVersion);
return !Number.isNaN(numericVersion) && numericVersion >= 4.2;
})
.filter((version) => ['latest', 'rapid'].includes(version) || semver.gte(version, '4.2'))

@W-A-James W-A-James requested a review from baileympearson May 26, 2023 13:31
@baileympearson baileympearson self-assigned this May 26, 2023
@baileympearson baileympearson added the Team Review Needs review from team label May 26, 2023
@baileympearson baileympearson merged commit da12eb9 into main May 26, 2023
@baileympearson baileympearson deleted the NODE-4698/test_crypt_shared_with_older_server_versions branch May 26, 2023 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants