temporalio / helm-charts

Temporal Helm charts
MIT License
294 stars 321 forks source link

FIX remove web ui config in values.yaml #394

Closed washanhanzi closed 2 months ago

washanhanzi commented 1 year ago

What was changed

  1. remove web ui config in values.yaml
  2. remove web config volume in web-deployment.yaml
  3. remove web-config config map
  4. remove line 280 since the install bash didn't configure web ui auth
  5. update the document for web ui configuration with env variable

Why?

393

Checklist

  1. Closes #393

  2. How was this tested: manually, 100% craft by hand. since the repo didn't provide any testing tool.

  3. Any docs updates needed? already updated with the PR.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

CLAassistant commented 1 year ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

samm-git commented 1 year ago

+1, spent ~hour trying to understand why OID does not work when finally found that config map is not mounted to any volume/file.

joshbranham commented 11 months ago

Can we get this merged? Also just spent a ton of time on auth that wasn't necessary.

washanhanzi commented 10 months ago

@tsurdilo

kamelj commented 2 months ago

I can't understand why this was not merged; we all suffered when we tried to change the web values, and nothing was affected!

washanhanzi commented 2 months ago

@robholland

washanhanzi commented 2 months ago

All done. By the way, is there any test I can run? The PR has been open for a really long time, and I haven't touched the temporal charts since then. I'm not as confident as I was when I submitted the PR.

robholland commented 2 months ago

Nothing automated. We internally use the helm charts to install the base app, but we don't modify the UI so wouldn't pick up on problems. I'll do a manual test before I merge it though.