Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support for modifying edge probe values #97

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

I3uckwheat
Copy link
Contributor

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.

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.
@I3uckwheat
Copy link
Contributor Author

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

Copy link
Member

@chriswk chriswk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this. Good spot

@chriswk
Copy link
Member

chriswk commented Oct 19, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@chriswk
Copy link
Member

chriswk commented Oct 19, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

This should do it!

@chriswk
Copy link
Member

chriswk commented Oct 20, 2023

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
Copy link
Member

chriswk commented Oct 20, 2023

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
Copy link
Member

chriswk commented Oct 20, 2023

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

@chriswk chriswk merged commit 2c2bf04 into Unleash:main Oct 20, 2023
7 of 11 checks passed
@I3uckwheat
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helm values for health checks not configurable in unleash-edge
3 participants