Skip to content

Conversation

@zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Jul 30, 2021

Description

  • remove the forced "Content" key for custom webhooks, as the custom webhook server can require any format. User may provide json which contains customized format.
  • remove title for custom webook message, since newline breaks JSON format input.
  • Fix the issue that some unit test using junit4 not being picked up, by replacing with jupiter
  • change common-utils branch name in CI to main
  • suppport http as url protocol in webhook

Issues Resolved

#245
#232

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • 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.

@zhongnansu zhongnansu force-pushed the fix-custom-webhook-message branch from 59f3d93 to ad1353d Compare July 30, 2021 00:54
@zhongnansu zhongnansu marked this pull request as draft July 30, 2021 01:39
@zhongnansu
Copy link
Member Author

zhongnansu commented Jul 30, 2021

CI is failing, but it passed locally. I also try re-run CI on HEAD of dev branch. It fails too
@dai-chen could you help take a look at the issue?

davidcui1225
davidcui1225 previously approved these changes Jul 30, 2021
@zhongnansu zhongnansu marked this pull request as ready for review July 31, 2021 18:32
@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2021

Codecov Report

Merging #248 (a612ae1) into develop (5cad6a6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #248   +/-   ##
========================================
  Coverage    80.53%   80.53%           
========================================
  Files           60       60           
  Lines         1634     1634           
  Branches       415      415           
========================================
  Hits          1316     1316           
  Misses         310      310           
  Partials         8        8           
Flag Coverage Δ
dashboards-notifications 80.53% <ø> (ø)

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


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cad6a6...a612ae1. Read the comment docs.

import org.apache.http.impl.client.CloseableHttpClient
import org.apache.http.message.BasicStatusLine
import org.easymock.EasyMock
import org.junit.Test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we changed the Test import?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are using junit5, but this one is still junit4, it doesn't throw error, but actually the tests are not picked up. We should use junit 5 anywhere

Copy link
Contributor

@davidcui1225 davidcui1225 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one question

@zhongnansu zhongnansu merged commit a8fa959 into opensearch-project:develop Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants