tryretool / retool-helm

MIT License
45 stars 57 forks source link

Support granular UI annotations and labels #132

Closed woz5999 closed 8 months ago

woz5999 commented 9 months ago

This PR adds the ability to specifically target the UI pods with annotations. The current available annotations configurations apply to multiple deployments, including the UI pods. In order to expose the service via our service mesh, we need the ability to target the UI pods with annotations. This change allows us to inject and expose the UI pods only, without inadvertently annotated undesired pods.

if you need me to make any changes like bumping versions or updating changelogs, etc, just let me know and I'm happy to do so.

bdjohnson529 commented 8 months ago

@woz5999 Are the UI pods the pods within the backend deployment? Those pods would be:

I can see that the backend annotation is shared with the workflows and workflows worker deployments.

Is your primary concern having a separate annotation which exclusively targets pods within the backend deployment?

woz5999 commented 8 months ago

@bdjohnson529 That's correct. Our service mesh leverages annotations for registration and routing. Currently, if we set the annotation for the "backend" is applies to the workflow pods as well, meaning that UI requests regularly get round robin'ed to non-UI pods, resulting in HTTP errors. By being able to target the UI pods individually, the issue is totally resolved.

jjlgao commented 8 months ago

Hey @woz5999 , Jason from Retool's infra team -- the PR makes sense to me, but we don't internally call these non-workflows pods "ui", so checking with our team on what we'd like that naming to be.

woz5999 commented 8 months ago

Sure thing. Naming's hard. Just let me know when you're ready and I can happily update.

jjlgao commented 8 months ago

Hey @woz5999 , we talked internally and we're good with using ui, but I just realized that we have a requirement for the two values.yaml files to match in the repo for legacy reasons. There's this topmost values.yaml file if you could copy your change over as well: https://github.com/tryretool/retool-helm/blob/main/values.yaml

Once you do that I'll merge and release this change for you.

Thanks!

woz5999 commented 8 months ago

@jjlgao done. let me know if you need anything else

woz5999 commented 8 months ago

The linter is failing because the chart version needs to be updated. I'm happy to do that. ✖︎ retool => (version: "6.0.8", path: "charts/retool") > chart version not ok. Needs a version bump!

I'm guessing 6.1.0 should be the new version?

jjlgao commented 8 months ago

The linter is failing because the chart version needs to be updated. I'm happy to do that. ✖︎ retool => (version: "6.0.8", path: "charts/retool") > chart version not ok. Needs a version bump!

I'm guessing 6.1.0 should be the new version?

6.0.9, thanks!

woz5999 commented 8 months ago

Done

woz5999 commented 8 months ago

Thanks everyone. When do new releases get published to your helm repository?

jjlgao commented 8 months ago

They're done manually. I'm going to publish it in a bit.

jjlgao commented 8 months ago

Just published, try pulling now!