-
Notifications
You must be signed in to change notification settings - Fork 5
Add READMEs and cleanup #33
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
Conversation
dacharyc
left a comment
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.
Back to you for some minor fixups but overall LGTM!
mflix/README-JAVA-SPRING.md
Outdated
| [open an issue](https://github.com/mongodb/docs-sample-apps/issues/new/choose) | ||
| on the source repository `mongodb/docs-sample-apps`. | ||
| [open an issue](https://github.com/mongodb/sample-app-java-mflix/issues/new/choose) | ||
| on the source repository `mongodb/sample-app-java-mflix`. |
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.
Nope, this was actually correct. We have disabled Issues on the artifact repos and want people to create issues directly on the source repo so we only have one place to watch instead of three of them.
| on the source repository `mongodb/sample-app-java-mflix`. | |
| on the source repository `mongodb/docs-sample-apps`. |
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.
Ah good to know, I thought we were keeping the source repo separated. Changed back
mflix/README-JAVASCRIPT-EXPRESS.md
Outdated
| ├── README.md | ||
| ├── client/ # Next.js frontend | ||
| └── server/ # Express.js backend | ||
| ├── README-JAVASCRIPT-EXPRESS.md |
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.
As this is the public-facing project, the README will be renamed by the copy tool to just README.md in the artifact repo.
| ├── README-JAVASCRIPT-EXPRESS.md | |
| ├── README.md |
| ```env | ||
| # MongoDB Configuration | ||
| # Replace with your MongoDB Atlas connection string | ||
| MONGODB_URI=mongodb+srv://<username>:<password>@<cluster>.mongodb.net/sample_mflix?retryWrites=true&w=majority |
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 added the Voyage API key to the Python and Java .env.example files when I moved them - we should probably do this for JS too and update this README to match.
|
|
||
| From the `server/js-express` directory, run: | ||
|
|
||
| ```bash |
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 recommend making these separate bash blocks so users can just execute them in their IDE if they want to.
mflix/README-JAVASCRIPT-EXPRESS.md
Outdated
| [open an issue](https://github.com/mongodb/docs-sample-apps/issues/new/choose) | ||
| on the source repository `mongodb/docs-sample-apps`. | ||
| [open an issue](https://github.com/mongodb/sample-app-nodejs-mflix/issues/new/choose) | ||
| on the source repository `mongodb/sample-app-nodejs-mflix`. |
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.
| on the source repository `mongodb/sample-app-nodejs-mflix`. | |
| on the source repository `mongodb/docs-sample-apps`. |
mflix/README-JAVASCRIPT-EXPRESS.md
Outdated
| If you have verified the above and still have issues, please | ||
| [open an issue](https://github.com/mongodb/docs-sample-apps/issues/new/choose) | ||
| on the source repository `mongodb/docs-sample-apps`. | ||
| [open an issue](https://github.com/mongodb/sample-app-nodejs-mflix/issues/new/choose) |
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.
| [open an issue](https://github.com/mongodb/sample-app-nodejs-mflix/issues/new/choose) | |
| [open an issue](https://github.com/mongodb/docs-sample-apps/issues/new/choose) |
mflix/README-JAVA-SPRING.md
Outdated
| If you have verified the above and still have issues, please | ||
| [open an issue](https://github.com/mongodb/docs-sample-apps/issues/new/choose) | ||
| on the source repository `mongodb/docs-sample-apps`. | ||
| [open an issue](https://github.com/mongodb/sample-app-java-mflix/issues/new/choose) |
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.
| [open an issue](https://github.com/mongodb/sample-app-java-mflix/issues/new/choose) | |
| [open an issue](https://github.com/mongodb/docs-sample-apps/issues/new/choose) |
| ```bash | ||
| # Run all integration tests | ||
| ./mvnw test -Dtest=AtlasSearchIntegrationTest | ||
| ./mvnw test -Dtest=MongoDBSearchIntegrationTest |
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.
If we're changing this command, we need to change the corresponding test files.
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.
The files were already changed as far as I can see on my branch
|
|
||
| # Run a specific test | ||
| ./mvnw test -Dtest=AtlasSearchIntegrationTest#testSearchMoviesByPlot_Success | ||
| ./mvnw test -Dtest=MongoDBSearchIntegrationTest#testSearchMoviesByPlot_Success |
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.
Same here.
| "dotenv": "^17.2.3", | ||
| "express": "^5.1.0", | ||
| "mongodb": "^6.20.0", | ||
| "mongodb": "^7.0.0", |
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.
Awesome! 🎉
mflix/README-JAVASCRIPT-EXPRESS.md
Outdated
| └── server/ # Express.js backend | ||
| ├── README-JAVASCRIPT-EXPRESS.md | ||
| ├── client/ # Next.js frontend (TypeScript) | ||
| └── server/js-express/ # Express.js backend |
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.
Oops, missed this - the copier tool will rename this to just server in the artifact repo.
| └── server/js-express/ # Express.js backend | |
| └── server/ # Express.js backend |
mflix/README-JAVASCRIPT-EXPRESS.md
Outdated
|
|
||
| ### 2. Install Backend Dependencies | ||
|
|
||
| From the `server/js-express` directory, run: |
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.
| From the `server/js-express` directory, run: | |
| From the `server/` directory, run: |
mflix/README-JAVASCRIPT-EXPRESS.md
Outdated
|
|
||
| ### 3. Start the Backend Server | ||
|
|
||
| From the `server/js-express` directory, run: |
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.
| From the `server/js-express` directory, run: | |
| From the `server/` directory, run: |
mflix/README-JAVASCRIPT-EXPRESS.md
Outdated
| Navigate to the Express server directory: | ||
|
|
||
| ```bash | ||
| cd server/js-express |
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.
| cd server/js-express | |
| cd server/ |
dacharyc
left a comment
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.
LGTM! ✅
This PR does the following:
numCandidatesparameter for Vector Search to be limit * 20 (as recommended in our docs)