fix: add Elasticsearch variable overrides - #6646
Conversation
|
Thanks for the pull request, @Agrendalath! I've created OSPR-6356 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
kaustavb12
left a comment
There was a problem hiding this comment.
👍
- I tested this: I have verified that the corresponding variables have been changed in the course-discovery and edx-notes-api repos for lilac
- I read through the code
- Includes documentation
… discovery The variable name has been changed in: edx/course-discovery@df5e75f Co-authored-by: @0x29a <demid@opencraft.com>
…es_api The variable name has been changed in: edx/edx-notes-api@e24a267 Co-authored-by: @0x29a <demid@opencraft.com>
aaa001b to
eee778f
Compare
|
@nizarmah, would you be able to add this to the OSPR list? It could be a good candidate for the CC review. |
|
@Agrendalath Thank you for your contribution. @pomegranited Are you interested in reviewing this? |
|
@natabene Yep! I'll review this next sprint (starts Tues 11 Jan). |
|
Yes @Agrendalath! I was going to ask @pomegranited as well, thank you so much @natabene! |
pomegranited
left a comment
There was a problem hiding this comment.
👍
- I tested this by deploying an Ocim appserver using this branch, with Notes and Discovery services enabled. I checked that the configured ES hostnames made it through to the Notes and Discovery .yml files.
- I read through the code, and checked the updated variable names against course_discovery.settings.production and notesserver.settings.common.
- Includes documentation
- Commit structure follows OEP-0051
-
I made sure any change in configuration variables is reflected in the corresponding client'sN/Aconfiguration-securerepository.
|
@Agrendalath Looks good to me. |
|
Thanks for reviewing this, @pomegranited and @nadeemshahzad. Given that this is already approved, and won't likely need any changes, I've opened #6659 and #6660 to backport it to Lilac and Maple. Would you mind reviewing these too (and merging them, if possible)? |
|
So who can merge this? |
|
@natabene, @pomegranited is going to merge this one on Monday. |
|
@Agrendalath 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Elasticsearch variables have been renamed for
discoveryandedx_notes_apiservices after the Koa release. This replaces the old names with the new ones.Reviewers
Follow-up
open-release/lilac.masterandopen-release/maple.masteronce we merge this.