-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
There was a problem hiding this 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: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentionally commented out?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
53e268c
to
270d151
Compare
There was a problem hiding this 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?
There was a problem hiding this 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
There was a problem hiding this 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!
- name: Install mongodb-client-encryption | ||
run: npm install mongodb-client-encryption | ||
- name: Set up cluster | ||
id: setup-cluster |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call!
072e852
to
12730ce
Compare
lint typo lint
3c6deed
to
955cedf
Compare
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