feat: add Helm chart scaffolding and core templates#155
Conversation
✅ Deploy Preview for mcp-lifecycle-operator ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @Venkatesh1505. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
e41c668 to
54d8880
Compare
|
@aliok Sending it for your review. TIA |
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aliok
left a comment
There was a problem hiding this comment.
Thanks @Venkatesh1505 !
I had a quick look and have some comments. I want to take a 2nd look later, possibly after comments are addressed.
There was a problem hiding this comment.
What would happen if we put the CRD under dist/chart/crds rather then dist/chart/templates?
I understand that keeping it upper level would mean no automatic CRD upgrades with helm install but when one does helm uninstall, CRD and thus all the instances are kept.
If we put it under templates, we would have automatic CRD upgrade but introduce this deletion risk.
I don't have that much Helm experience TBH.
There was a problem hiding this comment.
cc @ibm-adarsh @Cali0707 @matzew @koksay (Helm expert)?
There was a problem hiding this comment.
Helm does not automatically update CRDs during a helm update when they live in the crds directory.
If you expect frequent CRD updates, I suggest moving them to a separate chart. Otherwise, for a couple of updates a year, you can include in the upgrade doc that users update them manually.
There was a problem hiding this comment.
There was a problem hiding this comment.
IMO we should move the CRDs to a separate chart then. Thoughts @aliok ?
There was a problem hiding this comment.
We're gonna have frequent updates, as the project is new. So, let's do a separate chart for that.
There was a problem hiding this comment.
Done in f13ce27 — moved the CRD to a separate dist/chart-crds/ chart (mcp-lifecycle-operator-crds).
The CRD is now a regular template in the new chart, so helm upgrade will update it automatically. The main chart's crds/ directory has been removed, and NOTES.txt updated to mention installing the CRD chart first.
Tested on a kind cluster:
helm install mcp-lifecycle-operator-crds dist/chart-crds/— CRD createdhelm upgrade mcp-lifecycle-operator-crds dist/chart-crds/— CRD updated (revision 2)helm install mcp-lifecycle-operator dist/chart/— pod 1/1 Running, all RBAC resources createdhelm uninstall— both charts cleaned up
|
/ok-to-test |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Venkatesh1505 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
creydr
left a comment
There was a problem hiding this comment.
Left two comments.
But I am also wondering if there is a way to keep the chart manifests in sync with what we have actually in config/, so dist would only be the result of some kustomize patches for example 🤔
There was a problem hiding this comment.
This file seems to be out of sync with the CRD in config/crd/bases/mcp.x-k8s.io_mcpservers.yaml.
It seems to miss
- extraAnnotations field
- extraLabels field
- mcp section under spec
Can we make sure those are updated as part of make manifests too?
There was a problem hiding this comment.
FWIW: in #207 we added a Makefile target which checks if the generated files & manifests are up-to-date. This also runs then in a workflow.
There was a problem hiding this comment.
In #204, we added get and list permissions on pods. This seems to be missing here
Add initial Helm chart under dist/chart/ with core templates that produce the same resources as the kustomize deployment: - Chart.yaml, values.yaml, .helmignore - CRD (crds/mcp.x-k8s.io_mcpservers.yaml) - ServiceAccount (conditional via serviceAccount.create) - RBAC: manager ClusterRole/Binding, leader-election Role/Binding, metrics-auth ClusterRole/Binding, metrics-reader ClusterRole (conditional via rbac.create) - Controller manager Deployment with configurable image, resources, nodeSelector, tolerations, affinity, podAnnotations, podLabels - Metrics Service (port 8443) - NOTES.txt with post-install instructions - _helpers.tpl with standard naming/labeling helpers Part of kubernetes-sigs#132 Signed-off-by: Venkatesh1505 <venkyravi97@gmail.com>
Signed-off-by: Ali Ok <aliok@redhat.com>
Signed-off-by: Ali Ok <aliok@redhat.com>
Signed-off-by: Ali Ok <aliok@redhat.com>
Signed-off-by: Ali Ok <aliok@redhat.com>
Signed-off-by: Ali Ok <aliok@redhat.com>
Signed-off-by: Ali Ok <aliok@redhat.com>
Signed-off-by: Ali Ok <aliok@redhat.com>
Signed-off-by: Ali Ok <aliok@redhat.com>
f13ce27 to
a7ee27e
Compare
|
@creydr can you take a new look? |
Looks good from my side (but I can't LGTM because I am not in the org) |
| {{- include "mcp-lifecycle-operator.labels" . | nindent 4 }} | ||
| spec: | ||
| ports: | ||
| - name: metrics |
There was a problem hiding this comment.
The Helm chart has name: metrics, while the kustomize file has name: https ( see line 15), and the ServiceMonitor explicitly expects port: https (line 17, with a comment reinforcing it). A Helm-deployed instance would breakPrometheus scraping if someone applies the existing ServiceMonitor config.
What this PR does
Adds an initial Helm chart under
dist/chart/that produces the same resources as the current kustomize deployment with default values.Closes #132
Resources included
crds/mcp.x-k8s.io_mcpservers.yaml) — copied fromconfig/crd/bases/serviceAccount.createrbac.create)Configurability (values.yaml)
All values are documented with
# --comments. Key options:image.repositorymcp-lifecycle-operatorimage.tag""(uses appVersion)replicas1serviceAccount.createtruerbac.createtruenodeSelector{}tolerations[]affinity{}Validation
helm lint— passeshelm template— all 10 resources render correctlyhelm installon Kind cluster — pod 1/1 Running, controller healthyhelm install --dry-run— passesFollow-ups (separate issues)