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

Added support DingTalk notifier. #1653

Merged
merged 5 commits into from
Mar 26, 2021
Merged

Added support DingTalk notifier. #1653

merged 5 commits into from
Mar 26, 2021

Conversation

mask616
Copy link

@mask616 mask616 commented Mar 5, 2021

No description provided.

@SteKoe
Copy link
Contributor

SteKoe commented Mar 9, 2021

Hi @mask616! Thanks for your contribution. Could you please change the property webhookUrl to webhook-url as it is in the Slack and MS Teams notifier to have a unique naming? If done, your PR can be merged.

@mask616
Copy link
Author

mask616 commented Mar 9, 2021

Hi @mask616! Thanks for your contribution. Could you please change the property webhookUrl to webhook-url as it is in the Slack and MS Teams notifier to have a unique naming? If done, your PR can be merged.

Done. Opened to any feedback.

Copy link
Contributor

@SteKoe SteKoe left a comment

Choose a reason for hiding this comment

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

LG2M! But tests are missing.

public DingTalkNotifier(InstanceRepository repository, RestTemplate restTemplate) {
super(repository);
this.restTemplate = restTemplate;
this.message = parser.parseExpression(DEFAULT_MESSAGE, ParserContext.TEMPLATE_EXPRESSION);
Copy link
Member

Choose a reason for hiding this comment

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

This is always using DEFAULT_MESSAGE, right? I cannot find any place where the text from property spring.boot.admin.notify.dingtalk.message is being used.

Copy link
Author

Choose a reason for hiding this comment

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

It is used on line 106.

Copy link
Member

Choose a reason for hiding this comment

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

yes the property message from this class is used, but it is always initialized with DEFAULT_MESSAGE
(being passed to parseExpression(), the configuration spring.boot.admin.notify.dingtalk.message is not used (never passed to parseExpression())

Copy link
Author

Choose a reason for hiding this comment

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

This is a reference to other notifiers(eg. TelegramNotifier, SlackNotifier, PagerdutyNotifier, OpsGenieNotifier, LetsChatNotifier...).In oreder to be consistent with others notifiers, So I did.

Copy link
Author

Choose a reason for hiding this comment

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

For example :
In TelegramNotifier

public TelegramNotifier(InstanceRepository repository, RestTemplate restTemplate) {
super(repository);
this.restTemplate = restTemplate;
this.message = parser.parseExpression(DEFAULT_MESSAGE, ParserContext.TEMPLATE_EXPRESSION);
}

In SlackNotifier

public SlackNotifier(InstanceRepository repository, RestTemplate restTemplate) {
super(repository);
this.restTemplate = restTemplate;
this.message = parser.parseExpression(DEFAULT_MESSAGE, ParserContext.TEMPLATE_EXPRESSION);
}

Copy link
Member

Choose a reason for hiding this comment

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

Hi, sorry for answering so late. These other notifiers have a setter for the description where it is parsed:

public void setMessage(String message) {
	this.message = parser.parseExpression(message, ParserContext.TEMPLATE_EXPRESSION);
}

Copy link
Member

Choose a reason for hiding this comment

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

@mask616 if you add this piece we could merge your PR

Copy link
Author

Choose a reason for hiding this comment

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

@mask616 if you add this piece we could merge your PR

@erikpetzold I had add it just now.

Copy link
Contributor

@SteKoe SteKoe left a comment

Choose a reason for hiding this comment

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

Please add some tests as well.

@mask616
Copy link
Author

mask616 commented Mar 12, 2021

Please add some tests as well.

Hi, I've added DingTalk tests.

@codecov-io
Copy link

codecov-io commented Mar 26, 2021

Codecov Report

Merging #1653 (dd67462) into master (94d9d89) will increase coverage by 6.59%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1653      +/-   ##
============================================
+ Coverage     83.72%   90.32%   +6.59%     
+ Complexity     1203      147    -1056     
============================================
  Files           151       17     -134     
  Lines          3508      341    -3167     
  Branches        251       34     -217     
============================================
- Hits           2937      308    -2629     
+ Misses          441       18     -423     
+ Partials        130       15     -115     
Impacted Files Coverage Δ Complexity Δ
...server/domain/events/InstanceInfoChangedEvent.java
...rver/ui/config/AdminServerUiAutoConfiguration.java
...r/utils/jackson/InstanceInfoChangedEventMixin.java
.../config/AdminServerHazelcastAutoConfiguration.java
...oot/admin/server/services/ApplicationRegistry.java
...java/de/codecentric/boot/admin/CustomEndpoint.java
...oot/admin/server/notify/AbstractEventNotifier.java
...fig/AdminServerInstanceWebClientConfiguration.java
.../discovery/KubernetesServiceInstanceConverter.java
.../boot/admin/server/ui/extensions/UiExtensions.java
... and 123 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 94d9d89...dd67462. Read the comment docs.

@mask616 mask616 requested review from erikpetzold and SteKoe March 26, 2021 10:07
@SteKoe SteKoe merged commit a7d41b5 into codecentric:master Mar 26, 2021
@mask616
Copy link
Author

mask616 commented Mar 31, 2021

@SteKoe @erikpetzold Thank you for reviewing my code. I am very happy that my PR can be merged. This is the first time I have participated in open source software. I have learned a lot from this practice. This has also helped me participate in more open source software improvements Gained courage and confidence. Thank you again.

@erikpetzold
Copy link
Member

Hey @mask616, great to hear 🎉
Again thanks for your effort and have fun with future contributions

SteKoe added a commit to ParkerM/spring-boot-admin that referenced this pull request Apr 16, 2021
…-relpaths

* origin/master:
  Add retry on failing subscription (codecentric#1697)
  chore(deps): update dependency pl.project13.maven:git-commit-id-plugin to v4.0.4 (codecentric#1663)
  chore(deps): update dependency com.puppycrawl.tools:checkstyle to v8.41.1 (codecentric#1648)
  Fix for codecentric#1638 (codecentric#1673)
  Improve npm ci build times (see codecentric#1688) (codecentric#1689)
  Improved execution of grouped assertions (codecentric#1674)
  Use maven repo cache during publish snapshots (codecentric#1687)
  chore(deps): update metcalfc/changelog-generator action to v1 (codecentric#1669)
  chore(deps): update dependency org.codehaus.mojo:flatten-maven-plugin to v1.2.7 (codecentric#1686)
  Fix file formating by applying spring-javaformat:apply (codecentric#1685)
  Cache mvn repo in main build (see codecentric#1677 ) (codecentric#1678)
  Added support DingTalk notifier. (codecentric#1653)
  Simplify some code (codecentric#1670)
  Bugfix/1646 (codecentric#1661)
  Upgrade spring cloud dependencies (codecentric#1645)
  chore(deps): update dependency com.github.eirslett:frontend-maven-plugin to v1.11.2 (codecentric#1635)
  chore(deps): update spring boot to v2.4.3 (codecentric#1637)
  Bump version to 2.4.1-SNAPSHOT
  chore(deps): update testcontainers.version to v1.15.2 (codecentric#1629)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants