-
Notifications
You must be signed in to change notification settings - Fork 288
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
add support Tencent SMS #470
Conversation
Would you like to add the following settings to schema.yaml? ### Tencent SMS
tencent_sms_secret_id: {type: string}
tencent_sms_secret_key: {type: string}
tencent_sms_sdk_appid: {type: string}
tencent_sms_to_number: {type: array, items: {type: string}}
tencent_sms_sign_name: {type: string}
tencent_sms_region: {type: string}
tencent_sms_template_id: {type: string}
tencent_sms_template_parm: {type: array, items: {type: string}} Add https://github.com/jertel/elastalert2/blob/master/docs/source/elastalert.rst#overview |
↓
|
@nsano-rururu I have finished editing |
There are not enough test cases. Coverage is not 100%. 87% 121-124, 127 |
Is it difficult to implement the rest of the tests? |
@nsano-rururu The correct tencent_sms_secret_id is required to achieve 100% coverage |
100% coverage seems difficult, but some testing has been implemented. I think it's okay to merge, but are there any other points of concern? |
100% coverage is attainable with mocks but then you're merely testing that the implementation can handle a (potentially flawed) mock response, which is of limited value. So yes, I think it's good to approve and merge. @nsano-rururu Since you've been the primary reviewer I'll let you do that. Thank you. Thank you for the contribution @liuxingjun. |
@nsano-rururu How soon can I use the helm version |
@jertel This is my first time using python, this is my first time to participate in open source, I can try to improve test coverage later |
@liuxingjun That is fine. It would be a good exercise for you to learn how to use Python mocks. There are several examples in the existing alerter unit tests. You can use the existing Helm release for 2.2.1 and change the |
Description
Checklist
make test-docker
with my changes.Questions or Comments