Unleash / helm-charts

Contains helm-charts for Unleash
Apache License 2.0
41 stars 56 forks source link

feat: Add support for modifying edge probe values #97

Closed I3uckwheat closed 10 months ago

I3uckwheat commented 10 months ago

About the changes

This allows BASE_PATH enviornment variable changes to be reflected in the values.yaml file. Without this, k8s will attempt to hit /internal-backstage/health, while the helm-edge application is reporting health values at /BASE_PATH/internal-backstage/health.

Closes #96

Important files

This change is small, important files are deployment.yaml and values.yaml

Discussion points

It could be worth discussing creating a true value for BASE_PATH in the values.yaml file instead of having to duplicate the basepath value in the helm values chart.

I3uckwheat commented 10 months ago

Should I bump the version in Chart.yaml for this change?

chriswk commented 10 months ago

Should I bump the version in Chart.yaml for this change?

Yes, please. That's what's missing here for all the checks succeeding. :)

I3uckwheat commented 10 months ago

Should I bump the version in Chart.yaml for this change?

Yes, please. That's what's missing here for all the checks succeeding. :)

Awesome, that's done now! Thank you for getting to this so quickly. We're really excited to use Unleash :)

I3uckwheat commented 10 months ago

Humm.. I must be doing something wrong here, but I am unsure why the checks are still failing

chriswk commented 10 months ago

Part of the deploy is to deploy to k8s with minimum set of values. You've added more values, and are assuming that they are set, whereas the minimum one does not have it set. The error message at least says

 1. Get the application URL by running these commands:
  http://chart-example.local/
>>> helm upgrade unleash-edge-l58ijbik8p charts/unleash-edge --namespace unleash-edge-l58ijbik8p --reuse-values --wait --timeout 300s
Error: UPGRADE FAILED: template: unleash-edge/templates/deployment.yaml:55:24: executing "unleash-edge/templates/deployment.yaml" at <.Values.livenessProbe.enabled>: nil pointer evaluating interface {}.enabled

So, it looks like you'll need to check that .Values.livenessProbe.enabled has a value, or setup a coalescing operator, I'd say having defaults here in the template is probably the way to go, but giving the opportunity to override them.

Sorry for not spotting this earlier, I've run into the same problem myself earlier.

I3uckwheat commented 10 months ago

Part of the deploy is to deploy to k8s with minimum set of values. You've added more values, and are assuming that they are set, whereas the minimum one does not have it set. The error message at least says

 1. Get the application URL by running these commands:
  http://chart-example.local/
>>> helm upgrade unleash-edge-l58ijbik8p charts/unleash-edge --namespace unleash-edge-l58ijbik8p --reuse-values --wait --timeout 300s
Error: UPGRADE FAILED: template: unleash-edge/templates/deployment.yaml:55:24: executing "unleash-edge/templates/deployment.yaml" at <.Values.livenessProbe.enabled>: nil pointer evaluating interface {}.enabled

So, it looks like you'll need to check that .Values.livenessProbe.enabled has a value, or setup a coalescing operator, I'd say having defaults here in the template is probably the way to go, but giving the opportunity to override them.

Sorry for not spotting this earlier, I've run into the same problem myself earlier.

Ok, I'll get that setup. It's interesting to note that this is inconsistent with the unleash deployment.yaml value, which I based this PR on. I'm not very familiar with building helm charts, apologies!

I3uckwheat commented 10 months ago

This should do it!

chriswk commented 10 months ago

I think you have a fair point about the Unleash deployment one. I should make another iteration in that one as well, making sure that we choose sane defaults if the property is missing.

chriswk commented 10 months ago

Hmmm, this is strange. The install works, but the upgrade fails. I'm going to have to check the helm manual to see what's different here.

chriswk commented 10 months ago

I'm going to merge this, and then make it work. Thanks for all your effort :)

I3uckwheat commented 10 months ago

I appreciate you @chriswk! Thank you for merging this. I'll keep an eye on the repo for the update