-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fix: Custom Elasticsearch Host #49
Fix: Custom Elasticsearch Host #49
Conversation
Thanks for the pull request, @CodeWithEmad! 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. |
cdbfac6
to
b4958fc
Compare
@CodeWithEmad |
66ac1f9
to
7cd6980
Compare
Sure. Thanks for the reminder 🙂 |
Hey @CodeWithEmad, thank you for this contribution! The build is green so we should be good to move forward with engineering review. @gabor-boros When you get a minute, could you post an update about when you'd be able to review this PR? |
@itsjeyd I just allowed the workflow pipelines to run, but I had no intention to review the PR. If there is a need for a reviewer, I can take the review tho. |
FTR: Set myself as a reviewer. |
Thanks for taking the review @gabor-boros! For next time, are there other people with merge rights to this repo that I could have pinged? |
I guess MoisesGSalas, jfavellar90, bradenmacdonald, felipemontoya, cmltaWt0 all have merge rights though I didn't confirm |
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.
@CodeWithEmad Thank you for the PR! I left a minor comment which is rather a question.
I agree with your comment that the Palm+ requirement for the shared ES cluster should be mentioned in the docs. Although it is not strictly related to this PR, would you like/have time to add that not to the docs in the scope of this PR? If not, we can create a separate PR for that.
with this, we can use different elastic hosts in the plugin
Mark down warnings were fixed in README.md isort and black were used in python files
this was added to make sure all tutor plugins use the same pattern and file structure.
from olive version, this was fixed and was removed from documentation
Multi-tenant Elasticsearch prerequisites are not available till palm version
3fb82a3
to
a3a8606
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.
👍 🎉 Thank you for the changes again, @CodeWithEmad. The changes are approved and I'm going to merge them in a bit.
@CodeWithEmad 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
This will introduce:
K8S_HARMONY_ELASTIC_HOST
to have custom hosts.Makefile
: to follow other tutor plugins.tutor-mfe
on k8s removed)I guess we should mention in the documents that multi-tenant works on palm+