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.

Sign up for GitHub

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

feat: Email notification when releasing by OpenAPI #5324

Merged
merged 4 commits into from
Jan 26, 2025
Merged

feat: Email notification when releasing by OpenAPI #5324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f0e9a75
feat(OpenApi): notification by email when release by OpenApi
spaceluke Jan 24, 2025
9c8ed0f
chore(OpenApi): code style fix
spaceluke Jan 24, 2025
8dd5122
chore(changes): update changes-2.4.0.md
spaceluke Jan 25, 2025
35fd433
minor(changes): update CHANGES.md
spaceluke Jan 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ Apollo 2.4.0
* [Feature: add JSON formatting function in apollo-portal](https://github.com/apolloconfig/apollo/pull/5287)
* [Fix: add missing url patterns for AdminServiceAuthenticationFilter](https://github.com/apolloconfig/apollo/pull/5291)
* [Fix: support java.time.Instant serialization with gson](https://github.com/apolloconfig/apollo/pull/5298)
* [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)

------------------
All issues and pull requests are [here](https://github.com/apolloconfig/apollo/milestone/15?closed=1)
176 changes: 108 additions & 68 deletions ...tal/src/main/java/com/ctrip/framework/apollo/openapi/v1/controller/ReleaseController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@
import com.ctrip.framework.apollo.openapi.util.OpenApiBeanUtils;
import com.ctrip.framework.apollo.portal.entity.model.NamespaceGrayDelReleaseModel;
import com.ctrip.framework.apollo.portal.entity.model.NamespaceReleaseModel;
import com.ctrip.framework.apollo.portal.listener.ConfigPublishEvent;
import com.ctrip.framework.apollo.portal.service.NamespaceBranchService;
import com.ctrip.framework.apollo.portal.service.ReleaseService;
import com.ctrip.framework.apollo.portal.spi.UserService;
import javax.servlet.http.HttpServletRequest;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.security.access.prepost.PreAuthorize;
import org.springframework.web.bind.annotation.GetMapping;
Expand All @@ -54,18 +56,21 @@ public class ReleaseController {
private final NamespaceBranchService namespaceBranchService;
private final ConsumerPermissionValidator consumerPermissionValidator;
private final ReleaseOpenApiService releaseOpenApiService;
private final ApplicationEventPublisher publisher;

public ReleaseController(
final ReleaseService releaseService,
final UserService userService,
final NamespaceBranchService namespaceBranchService,
final ConsumerPermissionValidator consumerPermissionValidator,
ReleaseOpenApiService releaseOpenApiService) {
ReleaseOpenApiService releaseOpenApiService,
ApplicationEventPublisher publisher) {
this.releaseService = releaseService;
this.userService = userService;
this.namespaceBranchService = namespaceBranchService;
this.consumerPermissionValidator = consumerPermissionValidator;
this.releaseOpenApiService = releaseOpenApiService;
this.publisher = publisher;
}

@PreAuthorize(value = "@consumerPermissionValidator.hasReleaseNamespacePermission(#request, #appId, #namespaceName, #env)")
Expand All @@ -83,7 +88,19 @@ public OpenReleaseDTO createRelease(@PathVariable String appId, @PathVariable St
throw BadRequestException.userNotExists(model.getReleasedBy());
}

return this.releaseOpenApiService.publishNamespace(appId, env, clusterName, namespaceName, model);
OpenReleaseDTO releaseDTO = this.releaseOpenApiService.publishNamespace(appId, env,
clusterName, namespaceName, model);

ConfigPublishEvent event = ConfigPublishEvent.instance();
event.withAppId(appId)
.withCluster(clusterName)
.withNamespace(namespaceName)
.withReleaseId(releaseDTO.getId())
.setNormalPublishEvent(true)
.setEnv(Env.valueOf(env));
publisher.publishEvent(event);

return releaseDTO;
}

@GetMapping(value = "/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/releases/latest")
Expand All @@ -93,78 +110,91 @@ public OpenReleaseDTO loadLatestActiveRelease(@PathVariable String appId, @PathV
return this.releaseOpenApiService.getLatestActiveRelease(appId, env, clusterName, namespaceName);
}

@PreAuthorize(value = "@consumerPermissionValidator.hasReleaseNamespacePermission(#request, #appId, #namespaceName, #env)")
@PostMapping(value = "/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName}/merge")
public OpenReleaseDTO merge(@PathVariable String appId, @PathVariable String env,
@PathVariable String clusterName, @PathVariable String namespaceName,
@PathVariable String branchName, @RequestParam(value = "deleteBranch", defaultValue = "true") boolean deleteBranch,
@RequestBody NamespaceReleaseDTO model, HttpServletRequest request) {
RequestPrecondition.checkArguments(!StringUtils.isContainEmpty(model.getReleasedBy(), model
.getReleaseTitle()),
"Params(releaseTitle and releasedBy) can not be empty");

if (userService.findByUserId(model.getReleasedBy()) == null) {
throw BadRequestException.userNotExists(model.getReleasedBy());
}

ReleaseDTO mergedRelease = namespaceBranchService.merge(appId, Env.valueOf(env.toUpperCase()), clusterName, namespaceName, branchName,
model.getReleaseTitle(), model.getReleaseComment(),
model.isEmergencyPublish(), deleteBranch, model.getReleasedBy());

return OpenApiBeanUtils.transformFromReleaseDTO(mergedRelease);
@PreAuthorize(value = "@consumerPermissionValidator.hasReleaseNamespacePermission(#request, #appId, #namespaceName, #env)")
@PostMapping(value = "/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName}/merge")
public OpenReleaseDTO merge(@PathVariable String appId, @PathVariable String env,
@PathVariable String clusterName, @PathVariable String namespaceName,
@PathVariable String branchName,
@RequestParam(value = "deleteBranch", defaultValue = "true") boolean deleteBranch,
@RequestBody NamespaceReleaseDTO model, HttpServletRequest request) {
RequestPrecondition.checkArguments(
!StringUtils.isContainEmpty(model.getReleasedBy(), model.getReleaseTitle()),
"Params(releaseTitle and releasedBy) can not be empty");

if (userService.findByUserId(model.getReleasedBy()) == null) {
throw BadRequestException.userNotExists(model.getReleasedBy());
}

@PreAuthorize(value = "@consumerPermissionValidator.hasReleaseNamespacePermission(#request, #appId, #namespaceName, #env)")
@PostMapping(value = "/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName}/releases")
public OpenReleaseDTO createGrayRelease(@PathVariable String appId,
@PathVariable String env, @PathVariable String clusterName,
@PathVariable String namespaceName, @PathVariable String branchName,
@RequestBody NamespaceReleaseDTO model,
HttpServletRequest request) {
RequestPrecondition.checkArguments(!StringUtils.isContainEmpty(model.getReleasedBy(), model
.getReleaseTitle()),
"Params(releaseTitle and releasedBy) can not be empty");

if (userService.findByUserId(model.getReleasedBy()) == null) {
throw BadRequestException.userNotExists(model.getReleasedBy());
}

NamespaceReleaseModel releaseModel = BeanUtils.transform(NamespaceReleaseModel.class, model);

releaseModel.setAppId(appId);
releaseModel.setEnv(Env.valueOf(env).toString());
releaseModel.setClusterName(branchName);
releaseModel.setNamespaceName(namespaceName);

return OpenApiBeanUtils.transformFromReleaseDTO(releaseService.publish(releaseModel));
ReleaseDTO mergedRelease = namespaceBranchService.merge(appId, Env.valueOf(env.toUpperCase()),
clusterName, namespaceName, branchName, model.getReleaseTitle(), model.getReleaseComment(),
model.isEmergencyPublish(), deleteBranch, model.getReleasedBy());

ConfigPublishEvent event = ConfigPublishEvent.instance();
event.withAppId(appId).withCluster(clusterName).withNamespace(namespaceName)
.withReleaseId(mergedRelease.getId()).setMergeEvent(true).setEnv(Env.valueOf(env));
publisher.publishEvent(event);

return OpenApiBeanUtils.transformFromReleaseDTO(mergedRelease);
}

@PreAuthorize(value = "@consumerPermissionValidator.hasReleaseNamespacePermission(#request, #appId, #namespaceName, #env)")
@PostMapping(value = "/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/branches/{branchName}/releases")
public OpenReleaseDTO createGrayRelease(@PathVariable String appId, @PathVariable String env,
@PathVariable String clusterName, @PathVariable String namespaceName,
@PathVariable String branchName, @RequestBody NamespaceReleaseDTO model,
HttpServletRequest request) {
RequestPrecondition.checkArguments(
!StringUtils.isContainEmpty(model.getReleasedBy(), model.getReleaseTitle()),
"Params(releaseTitle and releasedBy) can not be empty");

if (userService.findByUserId(model.getReleasedBy()) == null) {
throw BadRequestException.userNotExists(model.getReleasedBy());
}

@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()));
NamespaceReleaseModel releaseModel = BeanUtils.transform(NamespaceReleaseModel.class, model);

releaseModel.setAppId(appId);
releaseModel.setEnv(Env.valueOf(env).toString());
releaseModel.setClusterName(branchName);
releaseModel.setNamespaceName(namespaceName);

ReleaseDTO releaseDTO = releaseService.publish(releaseModel);

ConfigPublishEvent event = ConfigPublishEvent.instance();
event.withAppId(appId).withCluster(clusterName).withNamespace(namespaceName)
.withReleaseId(releaseDTO.getId()).setGrayPublishEvent(true).setEnv(Env.valueOf(env));
publisher.publishEvent(event);

return OpenApiBeanUtils.transformFromReleaseDTO(releaseDTO);
}

@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()));
}

Comment on lines +171 to +197
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

There was no event emit when createGrayDelRelease in Portal

Copy link
Contributor

Choose a reason for hiding this comment

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

@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!

@PutMapping(path = "/releases/{releaseId}/rollback")
public void rollback(@PathVariable String env,
@PathVariable long releaseId, @RequestParam String operator, HttpServletRequest request) {
Expand All @@ -185,6 +215,16 @@ public void rollback(@PathVariable String env,
throw new AccessDeniedException("Forbidden operation. you don't have release permission");
}

ConfigPublishEvent event = ConfigPublishEvent.instance();
event.withAppId(release.getAppId())
.withCluster(release.getClusterName())
.withNamespace(release.getNamespaceName())
.withReleaseId(release.getId())
.setRollbackEvent(true)
.setEnv(Env.valueOf(env));
publisher.publishEvent(event);


this.releaseOpenApiService.rollbackRelease(env, releaseId, operator);
}

Expand Down
1 change: 0 additions & 1 deletion changes/changes-2.4.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,5 @@ 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)

------------------
All issues and pull requests are [here](https://github.com/apolloconfig/apollo/milestone/15?closed=1)
Loading