AlfrescoArchive / activiti-cloud-charts

Helm Charts for Activiti cloud Apps
Apache License 2.0
29 stars 28 forks source link

feat: expose notifications service /actuator endpoint #65

Closed igdianov closed 5 years ago

igdianov commented 5 years ago

This PR adds ingress path to expose Notifications service /actuator endpoint. However, in order to avoid conflict with the Gateway /actuator, the base path for Notifications ingress has been changed to /notifications.

Part of https://github.com/Activiti/Activiti/issues/2720

salaboy commented 5 years ago

@igdianov why do we need an ingress path for the actuator? I want to be sure that I understand why is that a requirement in the templates.

igdianov commented 5 years ago

@mteodori I thought it would make sense to make Ingress endpoints configurable, i.e. disable | enable /graphiql or /actuator if someone wants. I think it is a good practice to make all configuration values explicit, but simplify configuration using templates with feature toggles. I like to have opinionated configuration but without hard coding the templates.

igdianov commented 5 years ago

@mteodori @salaboy I realized that we cannot expose Notifications /actuator endpoint because of the conflict with Gateway /actuator, so I propose to expose Notifications service endpoints under /notifications default base path. The base path can overridden using /{{.Release.Name}}/notifications template in AAE.

We will need to update the NOTES.txt in activiti-full-example-chart and documentation to reflect this breaking change.

salaboy commented 5 years ago

@igdianov yeah.. I totally like that /notifications is the way to go.. so we can finally map endpoints to building blocks in the website :)

igdianov commented 5 years ago

Also, the acceptance tests should be changed to support new /notifications endpoint url.

mteodori commented 5 years ago

All good for me