atharvalade commented on code in PR #2976:
URL: https://github.com/apache/iggy/pull/2976#discussion_r2958606810
##########
helm/charts/iggy/templates/deployment.yaml:
##########
@@ -168,11 +168,11 @@ spec:
protocol: TCP
livenessProbe:
httpGet:
- path: /
+ path: /auth/sign-in
port: http
readinessProbe:
httpGet:
- path: /
+ path: /auth/sign-in
Review Comment:
`/auth/sign-in` is an application route, not a health endpoint. If this
route is renamed or restructured in a future web UI release, the probes will
silently break. Could we open a follow-up issue to add a lightweight `/healthz
(or /ping)` endpoint to the web UI, similar to what the server already has?
That would decouple the probe from the SPA
##########
helm/charts/iggy/README.md:
##########
@@ -46,14 +53,16 @@ helm install iggy ./helm/charts/iggy \
```bash
git clone https://github.com/apache/iggy.git
cd iggy
-helm install iggy ./helm/charts/iggy
+helm install iggy ./helm/charts/iggy \
+ --set server.serviceMonitor.enabled=false
Review Comment:
Every install example in this README now includes `--set
server.serviceMonitor.enabled=false`. Rather than documenting a workaround in
every example, should we change the default in `values.yaml` from `enabled:
true to enabled: false?` That way a bare helm install works out of the box, and
users with Prometheus Operator can opt in explicitly. The existing
`values.yaml` comment already says "Enable this if you're using Prometheus
Operator", which implies it should be opt-in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
