[WIP] feat: add helm unit tests - #167
Conversation
Signed-off-by: Sathvik <Sathvik.S@ibm.com>
✅ Deploy Preview for mcp-lifecycle-operator ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sats-23 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 |
k8s-ci-robot
added
size/XXL
|
/hold Let's keep this PR as draft until #155 is merged. Changing the title of the PR to WIP. |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
do-not-merge/work-in-progress
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
The tests themselves are in good shape — real helm-unittest suites with literal expected values, sensible per-resource organization, and a thorough README. The problem is everything bundled around them: this PR ships its own kubebuilder-helm-plugin chart (PROJECT change, new dist/chart templates, committed dist/install.yaml), which collides with #155 — the chart that's been through extensive review — in both layout and value paths (manager.image.* vs image.*, crd.enable/crd.keep vs the separate CRDs chart). Whichever merges first invalidates the other.
Suggest slimming this to: dist/chart/tests/, the helm-test Makefile targets, and a CI hook that runs them — rebased onto #155's chart once it lands (the template paths and value keys in the suites will need updating to match). The helm-deploy/rollback/status targets and the committed install.yaml read as separate concerns from #157.
Two Makefile issues inline. For the rebased version, assertions on the securityContext hardening fields and probe ports would round out the coverage nicely.
| .PHONY: helm-test-debug | ||
| helm-test-debug: install-helm-unittest ## Run helm-unittest tests with debug output and rendered templates. | ||
| $(HELM) unittest $(HELM_CHART_DIR) -d --debug | ||
| $(HELM) rollback $(HELM_RELEASE) --namespace $(HELM_NAMESPACE) |
There was a problem hiding this comment.
This helm rollback line belongs to the helm-rollback target up on line 353, which currently has no recipe at all. As written, make helm-rollback is a no-op and make helm-test-debug runs the unit tests and then rolls back a live Helm release — surprising side effect for a test target.
| install-helm: ## Install the latest version of Helm. | ||
| @command -v $(HELM) >/dev/null 2>&1 || { \ | ||
| echo "Installing Helm..." && \ | ||
| curl -fsSL https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-4 | bash; \ |
There was a problem hiding this comment.
Piping an unpinned script from helm's main branch into bash isn't great supply-chain posture for a kubernetes-sigs repo — pin a release tag (and ideally verify a checksum), or simply require helm as a prerequisite and fail with a message, like most targets do for kubectl. Same theme on line 364: --verify=false on the plugin install deserves at least a comment explaining why verification is off.
4 participants
Fixes #157