-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add Helm Chart #51
Add Helm Chart #51
Conversation
Thanks for the PR! 🤩 I'll give it a thorough look as soon as I find the time. 👍 |
+1 for this feature |
There has not been any activity to this pull request in the last 14 days. It will automatically be closed after 7 more days. Remove the |
Hi @eumel8 - How's things with the Helm chart? At my current project I had to create own Helm charts to get Varnish installed. But it'd be great to get official Helm chart done. If this project is not going to be alive, I'm planning to create own (and hopefully more active) fork of this under https://github.com/TheProjectAurora/ |
Sorry for the inactivity -- this should not have been closed. |
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.
Sorry for the delay. This was a big one, and I kept putting it off until later. I'm hesitant to merge this in its current state, and left some (hopefully) detailed comments explaining my thinking. Let me know if you have any thoughs on this or need any further feedback.
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.
my two cents :) great work thanks!
# name of backend service | ||
backendService: backend-service | ||
# name of backend service namespace | ||
# backendServiceNamespace: backend-service-namespace |
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.
if this is used because your backend is on a different namespace from kube-httpcache.. the rbac defined in the chart is not enough to let the operator work...
as per issue #40 a clusterrolebinding is needed..
kubectl create clusterrolebinding kube-httpcache --clusterrole=kube-httpcache --serviceaccount=[NAMESPACE_HERE]:kube-httpcache
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.
well also the clusterrole
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.
Either that, or alternatively, a ClusterRole
and two separate RoleBindings
for each namespace might also do the trick. Although it might be surprising (as in "principle of least surprise") for a user when the Helm chart starts creating new RoleBindings in namespaces other than the release namespace.
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 is the last remaining discussion on this PR, and I don't have a strong opinion yet on how to best proceed. Maybe the easiest solution would be to test for .Values.cache.backendServiceNamespace
in the NOTES.txt
template and notify the user to create the required role bindings themselves.
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.
mm, I think that the NOTES.txt idea is a good first approach.
Later we could add the roles and rolebindings... and cluster and clusterrolenbinding by setting some options in the values.yaml file
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.
👍 Sounds good to me. Will adjust the NOTES.txt in a follow-up PR.
It would be nice to expose the pod lifecycle hooks like |
maybe the helm chart could also implement the readiness probes patterns described here #36 (comment) |
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.
Just two ideas.
Would it be possible to run varnishnsca as optional sidecar for logging?
I could implement some of those ideas on different PRs, so we don't block this one getting merged... |
👍 I think most of the remaining suggestions could be considered independent features (that shouldn't necessarily block this PR, which has already been in review limbo long enough) and might be added in follow-up PRs. Also, liveness/readiness probes alone are an entirely different beast altogether that definitely warrant their own PR. |
Thanks for merge and my apologies for not having had time to continue working on it in the past few days. It's good to see how many more interesting ideas have come together. 💪 |
I'd like this simple, straight-forward project to use Varnish cache in Kubernetes. Before we use this in a professional way, I introduce a Helm chart and want to share it here.