Repository navigation
test(vpn): add default render helm unit tests - #4347
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
added
size/M
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe VPN chart now includes a Helm unit test suite. The Makefile adds a ChangesVPN Helm chart tests
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Akanksha Trehun (@magic-peach) LGTM. The tests can go red: I broke the chart once per assertion, and each of the six assertions failed alone, the one I had predicted before the run.
Business context: vpn is one of the eight tenant-facing apps #3631 lists without helm-unittest coverage. This PR pins its default Deployment render and adds the test: target the CI sweep looks for.
The mutations were a second copy of the Deployment document (only hasDocuments fails), then the kind, name, replicas, image and containerPort. Each one failed only its own assertion.
The comment at the top of the suite is correct. Without templates/secret.yaml in templates:, the render errors at the checksum/config include on templates/deployment.yaml:19. Without the _namespace.host value, templates/secret.yaml:1 errors on a nil pointer. So both are needed.
CI will reach the suite. The Unit & controller tests job runs make unit-tests, which calls hack/helm-unit-tests.sh. That script runs every package where make -n test succeeds. At head the probe prints helm unittest ., and make -C packages/apps/vpn test prints the Test Suites: 1 passed line the script checks for. Against current main (41ef4a6) the branch merges cleanly, main has no test target for vpn, and the suite passes on the merged tree.
The image pin that blocked #4074 is not a problem here. quay.io/outline/shadowbox:stable is hardcoded at templates/deployment.yaml:45, outside every file the release tooling rewrites, and the test file has no rc version.
CI has not run the suite yet: Pre-Commit Checks and Pull Request wait for a maintainer to approve the fork run. I ran it locally with helm v4.3.0 and helm-unittest 1.1.1.
Two non-blocking notes:
- The description says the suite pins "the default render", but it asserts only the Deployment. The PR body becomes the merge commit message here, so "the default Deployment render" would be exact. The CodeRabbit summary in the same body also calls the pinned
containerPorta "service port". - Nothing pins the password carry-over in
templates/secret.yaml. On upgrade it reads the existing<release>-vpnSecret and keeps each user's password, so their clients keep working. Deleting line 10 leaves this suite green. A case withusers: {alice: {}}and akubernetesProviderfixture holding an existingshadowbox_config.jsonfails on exactly that; I tried it locally.
vpn was one of the remaining user-facing apps with no helm-unittest coverage. Signed-off-by: Akanksha Trehun <[email protected]>
2bcaddf to
422b6f6
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes: every asserted value is live and the target really runs in CI. The coverage is narrower than the title claims, though. Two of the chart's six templates get rendered, one gets asserted.
Findings
- [MINOR]
packages/apps/vpn/tests/vpn_test.yaml:6, the suite-leveltemplates:list scopes rendering, not just assertions
- [MINOR]
packages/apps/vpn/tests/vpn_test.yaml:20, the four assertions all read the Deployment's own fields, so the chart's cross-file couplings stay uncovered
Claim mismatches
[PARTIAL] "Helm unit test coverage for the default render": one of six templates is asserted, two of six are rendered.
Caveats
- I broke each asserted value in turn: the
replicasdefault 2 to 3, the image tag,containerPort, the Deployment name. Each one reddened the suite, so nothing here is theatre. The header comment aboutsecret.yamlis accurate too: drop it from the list anddeployment.yaml:19fails on itsinclude. - Rendered output is unchanged against the merge base on the four corners I ran. The only difference comes from
tls.yamlcallinggenCA/genSignedCert, which already differs between two renders of one revision. - The sweep does pick the new target up. The
Unit & controller testsjob on this head logsRunning tests in packages/apps/vpnand thenPASS vpn default render.
Follow-ups
- #4081, #4074 and #4093 ship the same single-suite shape, so whatever is decided here applies to all four.
- Something this suite could grow into, pre-existing and uncovered today:
usersmap keys carry no constraint invalues.schema.json(additionalProperties, no key pattern), and a key holding a double quote breaks the render outright. A tenant reaches that through the aggregated API. Worth a schema constraint plus a negative case here.
$ helm template vpn-test . --namespace tenant-test --set _namespace.host=example.com --set 'users.alice.password=hackme' | grep '"name"'
"name": "alice",
$ helm template vpn-test . --namespace tenant-test --set _namespace.host=example.com --set 'users.bad\"key.password=hackme'
Error: YAML parse error on vpn/templates/secret.yaml: error converting YAML to JSON: yaml: line 6: did not find expected key
| # Both templates are loaded so the Deployment's checksum/config annotation can | ||
| # resolve its include of secret.yaml; template: pins assertions to the | ||
| # Deployment document alone. | ||
| templates: |
There was a problem hiding this comment.
[MINOR] the suite-level templates: list scopes rendering, not just assertions
Only deployment.yaml and secret.yaml are listed, and helm-unittest does not render what is absent from that list. A render-breaking edit to service.yaml, tls.yaml, workloadmonitor.yaml or dashboard-resourcemap.yaml therefore passes. vpn has no bats or chainsaw suite, and the repo runs no chart-wide helm template / helm lint gate, so those four templates still have no automated render coverage after this PR.
# templates/service.yaml: internalTrafficPolicy: Cluster
# -> internalTrafficPolicy: {{ .Values.doesNotExist.field }}
$ cd packages/apps/vpn && helm unittest .
PASS vpn default render tests/vpn_test.yaml
Charts: 1 passed, 1 total
Listing all six templates fixes it, and I ran that: the suite stays green on the clean tree and the same edit then reddens.
$ cd packages/apps/vpn && helm unittest . # six templates listed, service.yaml broken
FAIL vpn default render tests/vpn_test.yaml
Error: template: vpn/templates/service.yaml:35:35: executing "vpn/templates/service.yaml" at <.Values.doesNotExist.field>: nil pointer evaluating interface {}.field
Charts: 1 failed, 0 passed, 1 total
Error: plugin "unittest" exited with error
|
|
||
| tests: | ||
| - it: renders a Deployment with the default replicas and image | ||
| template: templates/deployment.yaml |
There was a problem hiding this comment.
[MINOR] the four assertions all read the Deployment's own fields, so the chart's cross-file couplings stay uncovered
Widening templates: fixes the render blindness above, it does not fix this. Two values in this chart have to agree across files, and both are the kind of drift a tenant notices before CI does.
Port 40000 is written in templates/deployment.yaml:48 (containerPort, asserted), templates/service.yaml:27-32 (port and targetPort, TCP and UDP) and templates/secret.yaml:34 (portForNewAccessKeys, which also lands in the ss:// URL on line 64). Move the Service off it and traffic arrives where shadowbox is not listening, while the suite says nothing:
# templates/service.yaml:28,32: targetPort: 40000 -> targetPort: 40001
$ cd packages/apps/vpn && helm unittest .
PASS vpn default render tests/vpn_test.yaml
Charts: 1 passed, 1 total
Same shape for the Secret keys the Deployment mounts by name (templates/deployment.yaml:39-42 items: against templates/secret.yaml:37). Rename one and the pod cannot mount its config at runtime:
# templates/secret.yaml:37: shadowbox_config.json -> shadowbox_cfg.json
$ cd packages/apps/vpn && helm unittest .
PASS vpn default render tests/vpn_test.yaml
Charts: 1 passed, 1 total
That second run is also the one case where checksum/config (templates/deployment.yaml:19) should have moved: the Secret's content changed, the annotation changed with it, and nothing asserted it. Pinning the annotation is what makes listing secret.yaml earn its place in the suite.
3 participants
What this PR does
vpn was one of the eight user-facing apps in
packages/apps/called out in #3631 as shipping with zero helm-unittest coverage. This adds atests/suite pinning the default render and wires the standardtest: helm unittest .target the sibling charts already use, following the same pattern as #4081, #4074 and #4093.Ran
helm unittest .locally against the chart, it passes.make showneeds a live cluster connection throughcozyhr, which was not available in this environment, so I relied onhelm unittestfor local verification, same as the referenced sibling PRs.Screenshots
Not applicable, test-only change.
Downstream repositories
Walked the trigger map in docs/agents/contributing.md against the diff: this only adds a test file and a Makefile target inside an existing package, it does not add, rename or remove a package, and it does not touch
values.yaml,values.schema.json,Chart.yamlorREADME.md, so none of the listed triggers apply.Release note
Summary by CodeRabbit
Tests
Chores