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

ci(NODE-6505): Setup CI #4

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

aditi-khare-mongoDB
Copy link

@aditi-khare-mongoDB aditi-khare-mongoDB commented Nov 21, 2024

Summary

  • Add scripting to setup CI for mongoose integration tests
  • Add basic integration testing to check the CI has been setup correctly

@aditi-khare-mongoDB aditi-khare-mongoDB changed the title Node 6505/ci setup ci(NODE-6505): Setup CI Nov 21, 2024
@aditi-khare-mongoDB aditi-khare-mongoDB marked this pull request as ready for review November 22, 2024 21:04
Copy link

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

This is a great start!

Along with the comments, we also need the ability to set up a cluster and run tests locally. Can we add a script that uses drivers-evergreen-tools to set up a cluster for local development as well as using the github action?


on:
push
#workflow_dispatch: {}

Choose a reason for hiding this comment

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

Intentionally commented out?

Choose a reason for hiding this comment

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

Just so any push triggers the action, I've added this branch name into the push config for clarity - will remove it when we're ready to merge.

test/encryption/encryption.test.js Outdated Show resolved Hide resolved
test/encryption/encryption.test.js Outdated Show resolved Hide resolved
test/encryption/encryption.test.js Show resolved Hide resolved
.github/workflows/encryption-tests.yml Outdated Show resolved Hide resolved
.github/workflows/encryption-tests.yml Outdated Show resolved Hide resolved
.github/scripts/run-kms-servers.sh Outdated Show resolved Hide resolved
Copy link

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

I think this was missed from my initial review:

Along with the comments, we also need the ability to set up a cluster and run tests locally. Can we add a script that uses drivers-evergreen-tools to set up a cluster for local development as well as using the github action?

@aditi-khare-mongoDB


on:
push:
branches: ['master', 'NODE-6505/ci-setup']

Choose a reason for hiding this comment

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

From the AC on the ticket:

Run GHA on PR open to main and commit to main, maybe remove the PR trigger after FLE work is done.

We're missing a trigger when PRs are opened against main (if we make that change, you can remove your branch from this list too)

Choose a reason for hiding this comment

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

This should be deleted. Should we perhaps added .pid files to the .gitignore? (also if we're using crypt_shared correctly, we shouldn't be spawning mongocryptds 🤔 )

@@ -22,6 +22,7 @@
"bson": "^6.7.0",
"kareem": "2.6.3",
"mongodb": "~6.10.0",
"mongodb-client-encryption": "^6.1.0",

Choose a reason for hiding this comment

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

This probably shouldn't be a production dependency of mongoose 🙂

'db.coll': {
bsonType: 'object',
encryptMetadata: {
keyId: [new mdb.UUID(dataKey)]

Choose a reason for hiding this comment

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

I think the return type of createDataKey is UUID

Suggested change
keyId: [new mdb.UUID(dataKey)]
keyId: [dataKey]

Comment on lines +79 to +83
await encryptedClient.db('db').collection('coll').insertOne({ a: 1 });

// a dummyClient not configured with autoEncryption, returns a encrypted binary type, meaning that encryption succeeded
const encryptedCursor = await dummyClient.db('db').collection('coll').find();
const encryptedResult = await encryptedCursor.next();

Choose a reason for hiding this comment

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

Suggested change
await encryptedClient.db('db').collection('coll').insertOne({ a: 1 });
// a dummyClient not configured with autoEncryption, returns a encrypted binary type, meaning that encryption succeeded
const encryptedCursor = await dummyClient.db('db').collection('coll').find();
const encryptedResult = await encryptedCursor.next();
const { insertedId } = await encryptedClient.db('db').collection('coll').insertOne({ a: 1 });
// a dummyClient not configured with autoEncryption, returns a encrypted binary type, meaning that encryption succeeded
const encryptedCursor = await dummyClient.db('db').collection('coll').findOne({ _id: insertedId });

(optional) Usually we'd just query for the document we inserted specifically, instead of creating a cursor and iterating it. This also applies below, to the encryptedClient.find()

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.

2 participants