fix(jenkins): align template standards#677
Conversation
github-actions
Bot
added
area:docs
area:schema
area:templates
area:tests
area:values
chart:jenkins
Standards Check (GR-079) — PASSEvery changed chart fully passes standards-check. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds ChangesJenkins Chart NetworkPolicy, Validation, and Notes Updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🟢 Security Scan:
|
| Framework | Score |
|---|---|
| MITRE + NSA + SOC2 | 91.41415% |
✅ Security posture acceptable.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Keep Helm test hook under templates
This file is no longer part of the chart's rendered manifests because it was moved out of templates/. I checked Helm's CLI semantics (helm template --help says it renders chart templates, and helm test --help says tests are defined in the installed chart); in this revision helm template test charts/jenkins emits no helm.sh/hook: test, and --show-only tests/test-connection.yaml errors with could not find template. As a result, every Jenkins install loses its helm test smoke test even though NOTES/docs still instruct users to run it.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
charts/jenkins/tests/networkpolicy_test.yaml (1)
36-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIndex-based assertion is brittle against default egress-rule count changes.
spec.egress[3]implicitly depends on exactly 3 generated rules (cluster, DNS, internet) precedingextraEgress. If a default egress flag changes or a new generated rule is added, this index silently shifts and the test can pass/fail for the wrong reason. Using acontainsassertion matching the rule content is more robust.♻️ Suggested fix
- it: should append extra egress rules values: - networkpolicy-extra-egress-values.yaml asserts: - - equal: - path: spec.egress[3].to[0].ipBlock.cidr - value: 10.0.0.0/8 - - equal: - path: spec.egress[3].ports[0].port - value: 443 + - contains: + path: spec.egress + content: + to: + - ipBlock: + cidr: 10.0.0.0/8 + ports: + - protocol: TCP + port: 443🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/jenkins/tests/networkpolicy_test.yaml` around lines 36 - 45, The network policy test is asserting the extra egress rule by a hardcoded array index, which is brittle when the number or order of default egress rules changes. Update the assertions in the networkpolicy test to locate the extra egress entry by its rule contents instead of spec.egress[3], using a contains-style match against the egress rule fields so the test remains stable as defaults evolve.charts/jenkins/values.yaml (1)
191-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaming inconsistency:
extraEgresssits outsideegress:block.All other egress-related knobs (
enabled,allowCluster,allowDns,allowInternet,extraRules) live undernetworkPolicy.egress, butextraEgressis a sibling ofegressinstead of nested inside it. Since it only takes effect whenegress.enabledis true (per the template), placing it underegresswould better reflect its scope and avoid confusing chart consumers about where egress-related overrides belong.♻️ Suggested restructuring (requires matching changes in schema and template)
egress: ... extraRules: [] - # -- Additional egress rules appended after generated egress rules. - extraEgress: [] + # -- Additional egress rules appended after generated egress rules. + extraEgress: []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/jenkins/values.yaml` around lines 191 - 192, The network policy values are inconsistent because extraEgress is defined as a top-level sibling instead of being grouped under networkPolicy.egress like the other egress settings. Move extraEgress into the egress block in values.yaml, and update any matching schema and template references that read the current path so the namespaced egress configuration stays consistent with enabled, allowCluster, allowDns, allowInternet, and extraRules.charts/jenkins/templates/_helpers.tpl (1)
99-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate externalSecrets validation logic.
The externalSecrets checks in Lines 101-108 duplicate the
failchecks already present incharts/jenkins/templates/externalsecret.yaml. Maintaining the same validation in two places risks the checks drifting out of sync (e.g., one gets updated with a new field/message and the other doesn't).Consider extracting the externalSecrets checks into their own helper (e.g.
jenkins.validate.externalSecrets) and having bothjenkins.validateandexternalsecret.yamlcall it, so there is a single source of truth.♻️ Suggested restructuring
+{{- define "jenkins.validate.externalSecrets" -}} +{{- if and .Values.externalSecrets.enabled (not .Values.admin.existingSecret) -}} +{{- fail "externalSecrets.enabled requires admin.existingSecret to be set to prevent credential drift between the chart-managed Secret and the ExternalSecret." -}} +{{- end -}} +{{- $externalSecretData := .Values.externalSecrets.data | default list -}} +{{- $externalSecretDataFrom := .Values.externalSecrets.dataFrom | default list -}} +{{- if and .Values.externalSecrets.enabled (eq (add (len $externalSecretData) (len $externalSecretDataFrom)) 0) -}} +{{- fail "externalSecrets.data or externalSecrets.dataFrom must not be empty when externalSecrets.enabled=true" -}} +{{- end -}} +{{- end -}} + {{- define "jenkins.validate" -}} -{{- if and .Values.externalSecrets.enabled (not .Values.admin.existingSecret) -}} -{{- fail "externalSecrets.enabled requires admin.existingSecret to be set to prevent credential drift between the chart-managed Secret and the ExternalSecret." -}} -{{- end -}} -{{- $externalSecretData := .Values.externalSecrets.data | default list -}} -{{- $externalSecretDataFrom := .Values.externalSecrets.dataFrom | default list -}} -{{- if and .Values.externalSecrets.enabled (eq (add (len $externalSecretData) (len $externalSecretDataFrom)) 0) -}} -{{- fail "externalSecrets.data or externalSecrets.dataFrom must not be empty when externalSecrets.enabled=true" -}} -{{- end -}} +{{- include "jenkins.validate.externalSecrets" . -}} {{- if and .Values.jcasC.enabled (eq (len (.Values.jcasC.configScripts | default dict)) 0) -}}And in
externalsecret.yaml, replace the inline checks with{{- include "jenkins.validate.externalSecrets" . -}}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/jenkins/templates/_helpers.tpl` around lines 99 - 115, The externalSecrets validation in jenkins.validate is duplicated elsewhere and should be centralized. Extract the externalSecrets-related fail checks into a dedicated helper such as jenkins.validate.externalSecrets, keep the shared logic there, and have both jenkins.validate and externalsecret.yaml invoke that helper. This will ensure the validation rules stay consistent and prevent drift between the two templates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/jenkins/README.md`:
- Around line 13-16: The README sentence in the NetworkPolicy section
incorrectly says the chart generates a separate Jenkins egress control. Update
the wording in the documentation to match the actual NetworkPolicy behavior
exposed by the chart, referring to the generated allowCluster, allowDns, and
allowInternet rules instead of “Jenkins,” and keep the mention of
networkPolicy.extraEgress as additive only.
---
Nitpick comments:
In `@charts/jenkins/templates/_helpers.tpl`:
- Around line 99-115: The externalSecrets validation in jenkins.validate is
duplicated elsewhere and should be centralized. Extract the
externalSecrets-related fail checks into a dedicated helper such as
jenkins.validate.externalSecrets, keep the shared logic there, and have both
jenkins.validate and externalsecret.yaml invoke that helper. This will ensure
the validation rules stay consistent and prevent drift between the two
templates.
In `@charts/jenkins/tests/networkpolicy_test.yaml`:
- Around line 36-45: The network policy test is asserting the extra egress rule
by a hardcoded array index, which is brittle when the number or order of default
egress rules changes. Update the assertions in the networkpolicy test to locate
the extra egress entry by its rule contents instead of spec.egress[3], using a
contains-style match against the egress rule fields so the test remains stable
as defaults evolve.
In `@charts/jenkins/values.yaml`:
- Around line 191-192: The network policy values are inconsistent because
extraEgress is defined as a top-level sibling instead of being grouped under
networkPolicy.egress like the other egress settings. Move extraEgress into the
egress block in values.yaml, and update any matching schema and template
references that read the current path so the namespaced egress configuration
stays consistent with enabled, allowCluster, allowDns, allowInternet, and
extraRules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21f3de72-f977-44dc-80e2-9a47aa418b2e
📒 Files selected for processing (10)
charts/jenkins/README.mdcharts/jenkins/templates/NOTES.txtcharts/jenkins/templates/_helpers.tplcharts/jenkins/templates/networkpolicy.yamlcharts/jenkins/templates/statefulset.yamlcharts/jenkins/tests/networkpolicy-extra-egress-values.yamlcharts/jenkins/tests/networkpolicy_test.yamlcharts/jenkins/tests/test-connection.yamlcharts/jenkins/values.schema.jsoncharts/jenkins/values.yaml
mberlofa
force-pushed
the
fix/jenkins-template-standards
branch
from
be650e1 to
599a639
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/jenkins/tests/test_connection_test.yaml (1)
8-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting
hook-delete-policytoo.The template also sets metadata: name: "{{ include "jenkins.fullname" . }}-test-connection" labels: {{- include "jenkins.labels" . | nindent 4 }} annotations: "helm.sh/hook": test "helm.sh/hook-delete-policy": before-hook-creation,hook-succeeded. The new suite only asserts kind, name, and the
helm.sh/hookannotation, leaving the delete-policy annotation uncovered.♻️ Optional additional assertion
- equal: path: metadata.annotations["helm.sh/hook"] value: test + - equal: + path: metadata.annotations["helm.sh/hook-delete-policy"] + value: before-hook-creation,hook-succeeded🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/jenkins/tests/test_connection_test.yaml` around lines 8 - 18, The Helm test suite for the connection test hook only verifies the Pod kind, name, and helm.sh/hook annotation, so it misses the hook cleanup setting. Update the test in test_connection_test.yaml to also assert metadata.annotations["helm.sh/hook-delete-policy"] on the rendered test-connection Pod, using the existing jenkins test hook assertions so the template’s full annotation set is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@charts/jenkins/tests/test_connection_test.yaml`:
- Around line 8-18: The Helm test suite for the connection test hook only
verifies the Pod kind, name, and helm.sh/hook annotation, so it misses the hook
cleanup setting. Update the test in test_connection_test.yaml to also assert
metadata.annotations["helm.sh/hook-delete-policy"] on the rendered
test-connection Pod, using the existing jenkins test hook assertions so the
template’s full annotation set is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2914f1d7-f260-4ea1-9553-f3d8382da0c8
📒 Files selected for processing (2)
charts/jenkins/README.mdcharts/jenkins/tests/test_connection_test.yaml
✅ Files skipped from review due to trivial changes (1)
- charts/jenkins/README.md
mberlofa
force-pushed
the
fix/jenkins-template-standards
branch
from
599a639 to
f6e8688
Compare
|
Addressed the remaining CodeRabbit review-summary items for Jenkins. What changed:
Validation:
The README inline thread was already resolved. The remaining items were present in CodeRabbit review summaries rather than unresolved review threads, so there are no additional thread IDs to reply to or resolve. |
1 participant
Summary
templates/tests/sohelm testrenders and executes it, with unittest coverage for the hook pod.networkPolicy.extraEgresswith schema and unit coverage while preserving existing generated egress controls.Related
Validation
helm template test charts/jenkins | rg -n "helm.sh/hook|test-connection"(hook rendered fromtemplates/tests/test-connection.yaml)helm unittest charts/jenkins(24 tests, 7 suites)make template-standards-check CHART=jenkinsnode scripts/charts/validate-chart.js --chart jenkins --no-k3dmake validate-chart CHART=jenkins TIMEOUT=900(FULLY VALIDATED, 20 layers)make release-check REPO=chartsmake attribution-check REPO=chartsSummary by CodeRabbit
networkPolicy.egress.extraEgressto append custom egress rules to the chart-generated NetworkPolicy.extraEgressaugments default egress behavior.