search

neondatabase / helm-charts

neondatabase helm charts
Apache License 2.0
38 stars 3 forks source link

Add neon-broker chart. #15

Closed arssher closed 1 year ago

arssher commented 1 year ago

Similar to neon-proxy, but a bit simpler: single replica, single port for service itself and metrics, internal service.

arssher commented 1 year ago

CI fails because neon image doesn't have neon_broker yet. I hoped to commit it along with helm install job, but this is a deadlock, so apparently better have one more PR. Or just ignore CI here, I tested manually & deployed chart on staging.

lassizci commented 1 year ago

Thanks for working on this!

CI fails because neon image doesn't have neon_broker yet. I hoped to commit it along with helm install job, but this is a deadlock, so apparently better have one more PR. Or just ignore CI here, I tested manually & deployed chart on staging.

Yeah, I think it often is so we need to have the application distributed first and then build a chart for it.

arssher commented 1 year ago

On default type and annotations: I realize common practice it to have ClusterIP and not vendor (aws)-specific defaults, but in this particular case this is our internal service, and we'd need to duplicate what we need in each env's values file. If that's ok, I'll adjust.

lassizci commented 1 year ago

On default type and annotations: I realize common practice it to have ClusterIP and not vendor (aws)-specific defaults, but in this particular case this is our internal service, and we'd need to duplicate what we need in each env's values file. If that's ok, I'll adjust.

I would rather use ingress also in our use case. The broker is open source and so are these charts, so from that perspective I see it would be nice to have more generic defaults.

cicdteam commented 1 year ago

On default type and annotations: I realize common practice it to have ClusterIP and not vendor (aws)-specific defaults, but in this particular case this is our internal service, and we'd need to duplicate what we need in each env's values file. If that's ok, I'll adjust.

Is it part of storage ? pageservers and safekepers wouldn't work without it ? if so then it's not internal service and helm chart shouldn't has domain specific settings by default.

lassizci commented 1 year ago

@cicdteam could you have a quick look too, as I commited bunch of stuff here too. Would be good to have some sanity check for my changes too

lassizci commented 1 year ago

I guess we can merge the chart already. Tests pass too. Although I did ninja-add the extraManifests support, since need to support two types of metrics definitions for a while.

arssher commented 1 year ago

Thanks!