feat: Email notification when releasing by OpenAPI #5324
Conversation
dosubot
bot
added
the
size:L
WalkthroughThe changes in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
spaceluke
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java (2)
113-138: Duplication of event-publishing code
The same logic for creating and configuringConfigPublishEventis repeated. Consider extracting this into a helper method or utility class to enforce DRY (Don’t Repeat Yourself) and promote maintainability.
140-169:createGrayReleaseevent logic
Again, the event-publishing code is effectively duplicated. Centralizing this logic will reduce the risk of inconsistent event configurations across methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
🔇 Additional comments (7)
apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java (7)
33-33: New import forConfigPublishEventlooks correct
This import is required for the event-driven approach you have introduced.
38-38: IntroducingApplicationEventPublisher
Importing Spring’sApplicationEventPublisheris a solid approach for decoupled event publishing.
59-59: New field for event publishing
Declaring aprivate final ApplicationEventPublisher publisher;ensures that all publish operations remain consistent throughout the controller.
73-73: Assigning the publisher
Using the constructor argument to setthis.publisheradheres to best practices in immutability for final fields. Nice job.
218-226: Rollback event consistency
Publishing an event before rolling back aligns rollback operations with the rest of the event-driven design. This consistent approach ensures that release-related activities (including notifications) are traceable and maintainable.
91-103: Event publishing increateRelease
The creation and publication of theConfigPublishEventis consistent with the new event-driven architecture. Please verify that the mapped event listener actually sends the email notifications as intended.✅ Verification successful
Email notifications are properly implemented
The ConfigPublishListener correctly handles email notifications with a robust implementation that:
- Builds appropriate email content based on the release operation type (normal/gray/rollback/merge)
- Sends notifications through EmailService with proper error handling
- Respects environment-specific email configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search any event listener or handler for ConfigPublishEvent that triggers the email notification. rg -A 10 "implements.*ApplicationListener|@EventListener"Length of output: 13688
Script:
#!/bin/bash # Get the complete implementation of ConfigPublishListener echo "=== ConfigPublishListener Implementation ===" rg -l "class ConfigPublishListener" | xargs cat echo -e "\n=== Email Service Usage ===" # Search for email-related services and their usage rg -A 5 "EmailService|sendEmail|notify.*email"Length of output: 21293
66-67: Constructor injection
The constructor injection for thepublisheris correctly added and follows Spring’s recommended pattern. Confirm that the Spring bean wiring is properly configured at runtime.✅ Verification successful
Constructor injection verified
The ApplicationEventPublisher injection follows Spring's standard constructor injection pattern and will be properly autowired at runtime. The class is correctly annotated with @RestController, ensuring it's picked up by Spring's component scanning.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the bean wiring for ApplicationEventPublisher injection in ReleaseController. # Here we look for ReleaseController references to confirm that it's picked up by Spring. rg -A 5 $'@RestController(\"openapiReleaseController\")\\npublic class ReleaseController|ApplicationEventPublisher'Length of output: 305
Script:
#!/bin/bash # Search for ReleaseController definition and ApplicationEventPublisher usage rg -U --multiline '@RestController.*?\npublic class ReleaseController' -A 10 # Look for ApplicationEventPublisher usage within ReleaseController ast-grep --pattern 'class ReleaseController { $$$ publisher.$_($$$) $$$ }' # Check for any custom configuration that might affect bean wiring fd -e xml -e java -e properties -e yaml -e yml | xargs rg -l "ApplicationEventPublisher|ReleaseController"Length of output: 4730
| @PreAuthorize(value = "@consumerPermissionValidator.hasReleaseNamespacePermission(#request, #appId, #namespaceName, #env)") | ||
| @PostMapping(value = "/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName}/gray-del-releases") | ||
| public OpenReleaseDTO createGrayDelRelease(@PathVariable String appId, @PathVariable String env, | ||
| @PathVariable String clusterName, @PathVariable String namespaceName, | ||
| @PathVariable String branchName, @RequestBody NamespaceGrayDelReleaseDTO model, | ||
| HttpServletRequest request) { | ||
| RequestPrecondition.checkArguments( | ||
| !StringUtils.isContainEmpty(model.getReleasedBy(), model.getReleaseTitle()), | ||
| "Params(releaseTitle and releasedBy) can not be empty"); | ||
| RequestPrecondition.checkArguments(model.getGrayDelKeys() != null, | ||
| "Params(grayDelKeys) can not be null"); | ||
|
|
||
| if (userService.findByUserId(model.getReleasedBy()) == null) { | ||
| throw BadRequestException.userNotExists(model.getReleasedBy()); | ||
| } | ||
|
|
||
| NamespaceGrayDelReleaseModel releaseModel = BeanUtils.transform( | ||
| NamespaceGrayDelReleaseModel.class, model); | ||
| releaseModel.setAppId(appId); | ||
| releaseModel.setEnv(env.toUpperCase()); | ||
| releaseModel.setClusterName(branchName); | ||
| releaseModel.setNamespaceName(namespaceName); | ||
|
|
||
| return OpenApiBeanUtils.transformFromReleaseDTO( | ||
| releaseService.publish(releaseModel, releaseModel.getReleasedBy())); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing event publishing in createGrayDelRelease
Unlike the other methods (createRelease, merge, createGrayRelease), this method does not publish a ConfigPublishEvent. If the intent is to maintain consistent event-driven behavior (including email notifications), you may need to publish an event here as well.
@@ -193,6 +193,16 @@ public OpenReleaseDTO createGrayDelRelease(...) {
return OpenApiBeanUtils.transformFromReleaseDTO(
releaseService.publish(releaseModel, releaseModel.getReleasedBy()));
+
+ // Potential fix: Publish event similarly
+ // ConfigPublishEvent event = ConfigPublishEvent.instance();
+ // event.withAppId(appId)
+ // .withCluster(clusterName)
+ // .withNamespace(namespaceName)
+ // .withReleaseId(<INSERT RELEASE ID>)
+ // .setEnv(Env.valueOf(env))
+ // .setGrayPublishEvent(true);
+ // publisher.publishEvent(event);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
There was no event emit when createGrayDelRelease in Portal
There was a problem hiding this comment.
@BlackBear2003 Yes, that's correct. The event emission is missing in the createGrayDelRelease method, while other release-related methods (createRelease, merge, createGrayRelease) do emit events. This inconsistency should be addressed to maintain uniform behavior across all release operations.
✏️ Learnings added
Learnt from: BlackBear2003
PR: apolloconfig/apollo#5324
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java:171-197
Timestamp: 2025-01-24T14:43:58.530Z
Learning: The `createGrayDelRelease` method in Portal's ReleaseController should emit a ConfigPublishEvent, similar to other release-related methods (`createRelease`, `merge`, `createGrayRelease`), to maintain consistent event-driven behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
spaceluke
changed the title
...portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
changes/changes-2.4.0.md (1)
13-13: Improve the grammar in the changelog entry.The current wording could be clearer. Consider this revision:
-* [Feature support to notification by email when releasing by OpenApi also](https://github.com/apolloconfig/apollo/pull/5324) +* [Feature: Add email notification support for OpenAPI releases](https://github.com/apolloconfig/apollo/pull/5324)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
changes/changes-2.4.0.md(1 hunks)
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: BlackBear2003
PR: apolloconfig/apollo#5324
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java:171-197
Timestamp: 2025-01-24T14:43:58.631Z
Learning: The `createGrayDelRelease` method in Portal's ReleaseController should emit a ConfigPublishEvent, similar to other release-related methods (`createRelease`, `merge`, `createGrayRelease`), to maintain consistent event-driven behavior.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
changes/changes-2.4.0.md
Outdated
| @@ -9,6 +9,8 @@ Apollo 2.4.0 | |||
| * [Feature support portal restTemplate Client connection pool config](https://github.com/apolloconfig/apollo/pull/5200) | |||
| * [Feature added the ability for administrators to globally search for Value](https://github.com/apolloconfig/apollo/pull/5182) | |||
| * [Feature support the observe status access-key for pre-check and logging only](https://github.com/apolloconfig/apollo/pull/5236) | |||
| * [Feature support to assign users management authority by the cluster (modify, publish)](https://github.com/apolloconfig/apollo/pull/5302) | |||
There was a problem hiding this comment.
The changes-2.4.0.md will be updated when 2.4.0 is released. So please update the CHANGES.md instead.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGES.md (1)
29-30: LGTM! Consider enhancing the feature descriptions.The changelog entries follow the correct format and provide clear links to the corresponding PRs. However, the descriptions could be more detailed to better communicate the impact and scope of these features.
Consider expanding the descriptions:
-* [Feature: support to assign users management authority by the cluster (modify, publish)](https://github.com/apolloconfig/apollo/pull/5302) -* [Feature: notification by email when releasing by OpenApi also](https://github.com/apolloconfig/apollo/pull/5324) +* [Feature: Support fine-grained user management with cluster-level authority controls for modification and publishing](https://github.com/apolloconfig/apollo/pull/5302) +* [Feature: Extend release notifications to include email alerts for OpenAPI-triggered releases](https://github.com/apolloconfig/apollo/pull/5324)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGES.md(1 hunks)changes/changes-2.4.0.md(0 hunks)
💤 Files with no reviewable changes (1)
- changes/changes-2.4.0.md
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
Learnt from: BlackBear2003
PR: apolloconfig/apollo#5324
File: apollo-portal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java:171-197
Timestamp: 2025-01-24T14:43:58.631Z
Learning: The `createGrayDelRelease` method in Portal's ReleaseController should emit a ConfigPublishEvent, similar to other release-related methods (`createRelease`, `merge`, `createGrayRelease`), to maintain consistent event-driven behavior.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (8)
What's the purpose of this PR
Support email notification when releasing by OpenAPI
Fix wrong code style of openapi.v1.controller.ReleaseController
Which issue(s) this PR fixes:
https://github.com/orgs/apolloconfig/projects/2/views/1?pane=issue&itemId=77891785
Brief changelog
see commits
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
New Features
Improvements