-
Notifications
You must be signed in to change notification settings - Fork 36
feat(helm): Allow multiple versions of the operator to be installed #736
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #736 +/- ##
==========================================
- Coverage 49.13% 48.97% -0.17%
==========================================
Files 96 96
Lines 10604 10604
==========================================
- Hits 5210 5193 -17
- Misses 5034 5048 +14
- Partials 360 363 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| deploy_multi_namespace: _deploy-check-env-vars docker-build manifests _helm_setup kind-load-image ## Deploy multiple copies of the controller to the K8s cluster specified in ~/.kube/config. | ||
| helm upgrade ngrok-operator-crds $(CRD_CHART_DIR) --install \ | ||
| --kube-context=kind-$(KIND_CLUSTER_NAME) \ | ||
| --namespace kube-system \ |
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.
Out of curiosity, is this even necessary? All the resources should be cluster scoped right?
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.
Yes, this is just the namespace where the helm release info gets stored. For multi-install, I figured putting it in kube-system was good so we can test deleting namespace-a or namespace-b. Since the CRDs extend the kubernetes API, it felt natural to me to put the helm release in kube-system.
The other option I see would be bypassing helm and just doing a kubectl apply -f of the ngrok-crds templates directory. I was leaning the helm direction so that we test the chart install works like our users would. Let me know what you think
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.
I'm going to merge since this was just a nit. Let me know what you think and its something we can change pretty easily
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.
ahh, i wasn't considering the helm release secret. that makes sense, and I don't think it matters much for the secret so this is good 👍
alex-bezek
left a 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 a couple of makefile nits, looks good
Signed-off-by: Jonathan Stacks <[email protected]>
585ae44 to
60dfae5
Compare
What
We've broken out the CRDs into their own chart. With that work, this allows multiple versions of the ngrok-operator to be installed in the same cluster by first installing the CRDs and then installing the ngrok-operator chart twice with
installCRDs: falsefor both the the helm installs.How
A few of the RBAC resources did not include the release name. This fixes it so that the
Roles,RoleBindings,ClusterRoles, andClusterRoleBindings have the release name included.I've tested this by using the new make command:
make deploy_multi_namespace. The eventual goal(not in this PR) is to have this path tested in CI as well.Breaking Changes
No.