Skip to content
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

[ISSUE #4420] Add Feishu Sink connector #4522

Merged
merged 11 commits into from
Nov 19, 2023

Conversation

SunnyBoy-WYH
Copy link
Contributor

@SunnyBoy-WYH SunnyBoy-WYH commented Oct 30, 2023

Fixes #4420.

Motivation

[ISSUE #4420] Add Feishu Sink connector

Modifications

add sink connector for feishu:

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@xwm1992
Copy link
Contributor

xwm1992 commented Nov 13, 2023

@SunnyBoy-WYH ping, please take a look for the reveiw and fix the ci check error, thanks.

@SunnyBoy-WYH
Copy link
Contributor Author

@SunnyBoy-WYH ping, please take a look for the reveiw and fix the ci check error, thanks.

ok,i will solve it later. thanks

Copy link
Contributor

@pandaapo pandaapo left a comment

Choose a reason for hiding this comment

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

It seems that you haven't fixed the previous CI error.

@SunnyBoy-WYH
Copy link
Contributor Author

It seems that you haven't fixed the previous CI error.

please approve the ci pipeline.

# Conflicts:
#	eventmesh-common/src/main/java/org/apache/eventmesh/common/Constants.java
@pandaapo
Copy link
Contributor

CI errors:

[ant:checkstyle] [WARN] /home/runner/work/eventmesh/eventmesh/eventmesh-connectors/eventmesh-connector-feishu/src/main/java/org/apache/eventmesh/connector/feishu/sink/connector/FeishuSinkConnector.java:105:108: '.' should be on a new line. [SeparatorWrapDot]

[ant:checkstyle] [WARN] /home/runner/work/eventmesh/eventmesh/eventmesh-connectors/eventmesh-connector-feishu/src/main/java/org/apache/eventmesh/connector/feishu/sink/connector/FeishuSinkConnector.java:106:51: ',' is not followed by whitespace. [WhitespaceAfter]
[ant:checkstyle] [WARN] /home/runner/work/eventmesh/eventmesh/eventmesh-connectors/eventmesh-connector-feishu/src/main/java/org/apache/eventmesh/connector/feishu/sink/connector/FeishuSinkConnector.java:141:63: WhitespaceAround: '{' is not preceded with whitespace. [WhitespaceAround]
[ant:checkstyle] [WARN] /home/runner/work/eventmesh/eventmesh/eventmesh-connectors/eventmesh-connector-feishu/src/main/java/org/apache/eventmesh/connector/feishu/sink/config/FeishuSinkConfig.java:19:1: 'import' should be separated from previous line. [EmptyLineSeparator]
> Task :eventmesh-connectors:eventmesh-connector-feishu:checkstyleMain FAILED

You can refer to https://eventmesh.apache.org/community/contribute/contribute#code-style to help you perform a checkstyle check locally.

Copy link

codecov bot commented Nov 18, 2023

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (fc900e5) 16.80% compared to head (0cf93d9) 16.87%.
Report is 2 commits behind head on master.

Files Patch % Lines
...tor/feishu/sink/connector/FeishuSinkConnector.java 56.81% 17 Missing and 2 partials ⚠️
...h/connector/feishu/server/FeishuConnectServer.java 0.00% 5 Missing ⚠️
...nector/feishu/sink/config/SinkConnectorConfig.java 33.33% 4 Missing ⚠️
...connector/feishu/sink/config/FeishuSinkConfig.java 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4522      +/-   ##
============================================
+ Coverage     16.80%   16.87%   +0.06%     
- Complexity     1626     1638      +12     
============================================
  Files           749      753       +4     
  Lines         28683    28740      +57     
  Branches       2487     2489       +2     
============================================
+ Hits           4821     4850      +29     
- Misses        23408    23434      +26     
- Partials        454      456       +2     

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

@SunnyBoy-WYH
Copy link
Contributor Author

CI errors:

[ant:checkstyle] [WARN] /home/runner/work/eventmesh/eventmesh/eventmesh-connectors/eventmesh-connector-feishu/src/main/java/org/apache/eventmesh/connector/feishu/sink/connector/FeishuSinkConnector.java:105:108: '.' should be on a new line. [SeparatorWrapDot]

[ant:checkstyle] [WARN] /home/runner/work/eventmesh/eventmesh/eventmesh-connectors/eventmesh-connector-feishu/src/main/java/org/apache/eventmesh/connector/feishu/sink/connector/FeishuSinkConnector.java:106:51: ',' is not followed by whitespace. [WhitespaceAfter]
[ant:checkstyle] [WARN] /home/runner/work/eventmesh/eventmesh/eventmesh-connectors/eventmesh-connector-feishu/src/main/java/org/apache/eventmesh/connector/feishu/sink/connector/FeishuSinkConnector.java:141:63: WhitespaceAround: '{' is not preceded with whitespace. [WhitespaceAround]
[ant:checkstyle] [WARN] /home/runner/work/eventmesh/eventmesh/eventmesh-connectors/eventmesh-connector-feishu/src/main/java/org/apache/eventmesh/connector/feishu/sink/config/FeishuSinkConfig.java:19:1: 'import' should be separated from previous line. [EmptyLineSeparator]
> Task :eventmesh-connectors:eventmesh-connector-feishu:checkstyleMain FAILED

You can refer to https://eventmesh.apache.org/community/contribute/contribute#code-style to help you perform a checkstyle check locally.

Done

import org.junit.jupiter.api.Test;

@Disabled
public class FeishuSinkConnectorTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you modify the code using Mockito stubbing or other mocking techniques to remove the annotation and make the test class runnable?

能否使用Mockito的打桩或者其他模拟的方式修改下代码,使该测试类可以移除掉该注解,并能实际运行?

}

@Spy
private FeishuSinkConnector feishuSinkConnector;
Copy link
Contributor

@pandaapo pandaapo Nov 18, 2023

Choose a reason for hiding this comment

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

The @Spy annotation is not very meaningful here, because you are not stubbing the object at all. You are basically constructing it manually.

这个@Spy注解意义不大,因为您没有对该对象进行任何打桩,基本上是手动构建的。


@Test
public void testFeishuSinkConnector() {
assertDoesNotThrow(() -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you are using Mockito for mocking, you can change the verification method of assertDoesNotThrow() to verify the number of calls to the feishuClient that you are mocking. Otherwise, the introduction of Mockito is meaningless.

现在您已经使用Mockito进行模拟了,就可以将assertDoesNotThrow()的验证方式改成验证您所模拟的feishuClient的调用次数。否则引入的Mockito就没有意义了。

final int times = 3;
List<ConnectRecord> connectRecords = new ArrayList<>();
for (int i = 0; i < times; i++) {
feishuSinkConnector.start();
Copy link
Contributor

Choose a reason for hiding this comment

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

One startup is enough. Additionally, it would be better to call stop () at the end of the test, although it is currently an empty method.

启动一次就够了。另外,测试结束时最好调用下stop(),虽然目前是个空方法。

@xwm1992 xwm1992 merged commit 5a8cd37 into apache:master Nov 19, 2023
13 checks passed
connectorConfig:
connectorName: feishuSink
reciveId: reciveIdValue
reciveType: open_id
Copy link
Contributor

Choose a reason for hiding this comment

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

The reciveId and recoverType fields in the sink-config.yml are different from the field names in SinkConnectorConfig, which will cause the configuration to fail to be imported correctly.

@SunnyBoy-WYH 配置文件中reciveId和reciveType字段与SinkConnectorConfig中字段名不同,这将导致配置无法正确导入。

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you willing to fix this bug? @hhuang1231 .
Additionally, this bug makes me worried about the correctness and effectiveness of this Connector in the absence of this bug.

您是否愿意修改这个bug?@hhuang1231 。另外,该bug也让我有点担心,当没有该bug时,该Connector的正确性和有效性。

Copy link
Contributor

Choose a reason for hiding this comment

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

@pandaapo ok, I'll try to perfect it and make it work.

hhuang1231 added a commit to hhuang1231/eventmesh that referenced this pull request Dec 8, 2023
# Conflicts:
#	eventmesh-examples/src/main/resources/application.properties
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.

[Feature] Add Feishu sink connector
5 participants