Adds webhook format check for Slack

[Noir01]

Description

Adds webhook format check for Slack

Issues Resolved

fixes #787

Check List

• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[zhichao-aws]

We used mock data to perform integration test. And the mock data may can not pass the new validation. Please also fix the tests in this repo and dashboards-notifications.

[Noir01]

We used mock data to perform integration test. And the mock data may can not pass the new validation. Please also fix the tests in this repo and dashboards-notifications.

I've (hopefully) fixed the integtests in this repo.
I'll go through dashboard-notifications tests and open a PR there soon

[Noir01]

Also, I just noticed the 'Unverified' tags on my commits. I was messing around with the automatic signing off feature and I think I messed up some settings which is leading to all my commits appearing as unverified

[Hailong-am]

Also, I just noticed the 'Unverified' tags on my commits. I was messing around with the automatic signing off feature and I think I messed up some settings which is leading to all my commits appearing as unverified

@Noir01 please follow the guide in https://github.com/opensearch-project/notifications/pull/814/checks?check_run_id=18584633999 to fix DCO and there has a unit test failure, can you fix it as well.

[Noir01]

Also, I just noticed the 'Unverified' tags on my commits. I was messing around with the automatic signing off feature and I think I messed up some settings which is leading to all my commits appearing as unverified

@Noir01 please follow the guide in https://github.com/opensearch-project/notifications/pull/814/checks?check_run_id=18584633999 to fix DCO and there has a unit test failure, can you fix it as well.

Can I close this PR and re-open another with the correct signing-off email?

[Hailong-am]

Can I close this PR and re-open another with the correct signing-off email?

You don't have to. Follow the guide mentioned in link https://github.com/opensearch-project/notifications/pull/814/checks?check_run_id=18584633999

1. In your local branch, run: git rebase HEAD~8 --signoff
2. Force push your changes to overwrite the branch: git push --force-with-lease origin patch-2

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.89%. Comparing base (62b7b4f) to head (aa3b636).
Report is 23 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #814      +/-   ##
============================================
+ Coverage     66.85%   66.89%   +0.03%     
- Complexity      263      265       +2     
============================================
  Files            39       39              
  Lines          1738     1740       +2     
  Branches        165      165              
============================================
+ Hits           1162     1164       +2     
  Misses          453      453              
  Partials        123      123              

Flag	Coverage Δ
opensearch-notifications	66.89% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Noir01]

I was trying to update this branch, and rebased it, but it got rid of the Verified tag. How can I fix this? Also, how do I deal with the failed codecov tests?

[Hailong-am]

I was trying to update this branch, and rebased it, but it got rid of the Verified tag. How can I fix this? Also, how do I deal with the failed codecov tests?

The DCO failed with reason

Summary
Commit sha: 7e6b4cd, Author: Aniruddh, Committer: Aniruddh; Can not find "Aniruddh [email protected]", in ["Aniruddh Srivastava [email protected]", "Aniruddh Srivastava [email protected]"].
Commit sha: 0791276, Author: Aniruddh Srivastava, Committer: Aniruddh; Expected "Aniruddh Srivastava [email protected]", but got "Aniruddh Srivastava [email protected]".

1. can you check the email settings of your account https://github.com/settings/emails make sure Keep my email addresses private is not checked.
2. make sure you local git config user email is [email protected] with command git config user.email

[Noir01]

I was trying to update this branch, and rebased it, but it got rid of the Verified tag. How can I fix this? Also, how do I deal with the failed codecov tests?

The DCO failed with reason

Summary
Commit sha: 7e6b4cd, Author: Aniruddh, Committer: Aniruddh; Can not find "Aniruddh [email protected]", in ["Aniruddh Srivastava [email protected]", "Aniruddh Srivastava [email protected]"].
Commit sha: 0791276, Author: Aniruddh Srivastava, Committer: Aniruddh; Expected "Aniruddh Srivastava [email protected]", but got "Aniruddh Srivastava [email protected]".

1. can you check the email settings of your account https://github.com/settings/emails make sure Keep my email addresses private is not checked.
2. make sure you local git config user email is [email protected] with command git config user.email

I just did both of those steps

[Hailong-am]

I just did both of those steps

@Noir01 after this, please follow the steps in https://github.com/opensearch-project/notifications/pull/814/checks?check_run_id=18694631981 to fix DCO

[zhichao-aws]

Good work, but I think we should add more tests cases. Can we add unit test cases to test more success cases and fail cases for url validation? And the integ test case for exception message?

[zhichao-aws]

Good work, but I think we should add more tests cases. Can we add unit test cases to test more success cases and fail cases for url validation? And the integ test case for exception message?

I'm not very experienced in this, so could you explain a bit more?

Please refer to these test cases:

notifications/notifications/notifications/src/test/kotlin/org/opensearch/notifications/index/ConfigIndexingActionsTests.kt

Line 17 in 62b7b4f

fun `test validate microsoft teams`() {

;

notifications/notifications/notifications/src/test/kotlin/org/opensearch/integtest/config/MicrosoftTeamsNotificationConfigCrudIT.kt

Line 132 in 62b7b4f

fun `test create config with wrong Microsoft Teams url and get error text`() {

Got it. Do you only want those two copied for slack?

You know we're adding tests to make sure the code behavior are as expected. And here we want to make sure the url check reject wrong url, and return correct message to help users fix it. You can write test cases just like this example, but I think you should modify these mock urls to adapt to you regex expression

[Noir01]

Good work, but I think we should add more tests cases. Can we add unit test cases to test more success cases and fail cases for url validation? And the integ test case for exception message?

I'm not very experienced in this, so could you explain a bit more?

Please refer to these test cases:

notifications/notifications/notifications/src/test/kotlin/org/opensearch/notifications/index/ConfigIndexingActionsTests.kt

Line 17 in 62b7b4f

fun `test validate microsoft teams`() {

;

notifications/notifications/notifications/src/test/kotlin/org/opensearch/integtest/config/MicrosoftTeamsNotificationConfigCrudIT.kt

Line 132 in 62b7b4f

fun `test create config with wrong Microsoft Teams url and get error text`() {

Got it. Do you only want those two copied for slack?

You know we're adding tests to make sure the code behavior are as expected. And here we want to make sure the url check reject wrong url, and return correct message to help users fix it. You can write test cases just like this example, but I think you should modify these mock urls to adapt to you regex expression

Oh yeah, of course.

[Noir01]

Good work, but I think we should add more tests cases. Can we add unit test cases to test more success cases and fail cases for url validation? And the integ test case for exception message?

Done.

[Hailong-am]

The validation happens when create new slack channel or update existing slack channel. To be compatible with existing data, we may want to add a condition for update case, only if webhook url has changed, we do the validation. we can have this in follow up PR and remove the backport 2.x label for now.

[zhichao-aws]

lgtm

[Noir01]

The validation happens when create new slack channel or update existing slack channel. To be compatible with existing data, we may want to add a condition for update case, only if webhook url has changed, we do the validation. we can have this in follow up PR and remove the backport 2.x label for now.

@Hailong-am Shall I open a new issue for this?

[Hailong-am]

The validation happens when create new slack channel or update existing slack channel. To be compatible with existing data, we may want to add a condition for update case, only if webhook url has changed, we do the validation. we can have this in follow up PR and remove the backport 2.x label for now.

@Hailong-am Shall I open a new issue for this?

sure, thanks

[zhichao-aws]

The validation happens when create new slack channel or update existing slack channel. To be compatible with existing data, we may want to add a condition for update case, only if webhook url has changed, we do the validation. we can have this in follow up PR and remove the backport 2.x label for now.

@Hailong-am Shall I open a new issue for this?

sure, thanks

We already have these issues now #818 #819 .