-
Notifications
You must be signed in to change notification settings - Fork 94
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
make probes on streaming and web deployments user configurable #10
base: main
Are you sure you want to change the base?
Conversation
…xisting values as defaults
… can define common failureThreshold and periodSeconds
…ers start quicker and check more frequently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be all, sorry for the feature creep and being unclear about what to change, next time, I'll do a separate PR. Hope this helps :)
@SISheogorath all requested changes made, please verify/review. |
@SISheogorath @renchap rebased, please review |
(Sorry for the huge delay in getting around to this) Generally I think this is a great idea. The values look good to me, I just want to ping @renchap to take a look as well before I approve. |
So we had a discussion internally about this. Generally it's a good idea to allow the customization of healthcheck endpoints. But there are two concerns with this particular implementation:
To clarify the second point, consider the possibility that we change the healthcheck endpoint in a future version. The onus to update the values file is then on everyone who uses the chart, and will lead to things breaking if people don't know they have to update it. I would instead suggest that the probes be constructed out of whatever values the user chooses to define, and fills in the rest of the probes' parameters with default values. Hopefully that makes sense! |
Implements #9