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

Closed
wants to merge 29 commits into from
Closed

Conversation

aditi-khare-mongoDB
Copy link

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

Descriptions
This testing framework is being added in to allow for ease of encryption testing in PRs to come.

Add CI configuration to ensure that FLE can be end-to-end tested in the mongoose repo.
Add scripting so users can test locally and understand how the CI configuration works.

Summary of Changes

  • Add scripting to setup CI for mongoose integration tests locally and through GHA
  • Add script to setup encryption cluster locally for local user testing
  • 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.

Choose a reason for hiding this comment

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

Do we want to be able to trigger the action manually, or is on "push" / "pr to master" sufficient?

Choose a reason for hiding this comment

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

I think manually would be nice especially as we continue encryption testing development and if Val or another mongoose contributor adds any encryption functionality.

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 Outdated 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

.github/workflows/encryption-tests.yml Outdated Show resolved Hide resolved
mongocryptd.pid Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/encryption/encryption.test.js Outdated Show resolved Hide resolved
test/encryption/encryption.test.js 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.

@aditi-khare-mongoDB Along with the changes in this PR, can you also put more context about the PR into the PR description? if/when Val reviews this PR, he likely won't understand why we're making these changes without additional context

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 Outdated Show resolved Hide resolved
test/encryption/encryption.test.js Outdated Show resolved Hide resolved
scripts/start-encrypted-cluster.sh Outdated Show resolved Hide resolved
scripts/run-encryption-tests-local.sh Outdated Show resolved Hide resolved
scripts/run-encryption-tests-local.sh Outdated Show resolved Hide resolved
scripts/run-encryption-tests-local.sh Outdated Show resolved Hide resolved
package.json 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.

A few small additional comments!

mongocryptd.pid Outdated Show resolved Hide resolved
scripts/start-encrypted-cluster.sh Outdated Show resolved Hide resolved
scripts/run-encryption-tests-local.sh Outdated Show resolved Hide resolved
scripts/run-encryption-tests-local.sh Outdated Show resolved Hide resolved
- name: Install mongodb-client-encryption
run: npm install mongodb-client-encryption
- name: Set up cluster
id: setup-cluster

Choose a reason for hiding this comment

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

Now that we have tooling to run encryption tests using drivers-evergreen-tools - can we just use the same tooling in CI instead of using the github action?

Choose a reason for hiding this comment

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

Good call!

@aditi-khare-mongoDB aditi-khare-mongoDB force-pushed the NODE-6505/ci-setup branch 2 times, most recently from 072e852 to 12730ce Compare December 27, 2024 19:03
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