Conversation
dacbd
requested review from
adityachoudhari26,
jsbroks and
zacharyblasczyk
as code owners
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a new Helm chart Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@charts/ctrlc-sync/templates/cronjob.yaml`:
- Around line 37-93: The pod template in the CronJob template is missing
restartPolicy which causes the API server to reject the manifest; update the pod
spec in charts/ctrlc-sync/templates/cronjob.yaml (inside the template -> spec
block where containers, volumes, nodeSelector, etc. are defined) to set
restartPolicy: {{ .Values.job.restartPolicy }} and add a corresponding default
in values.yaml (e.g., job.restartPolicy with a value of "OnFailure" or "Never");
ensure the restartPolicy is validated to only allow "OnFailure" or "Never" for
Job/CronJob pods.
In `@charts/ctrlc-sync/values.yaml`:
- Around line 94-102: The comment text in values.yaml incorrectly references
"Deployment"; update the two comment lines associated with the values keys
volumes and volumeMounts in the ctrlc-sync chart so they say "CronJob" (e.g.,
"Additional volumes on the CronJob definition" and "Additional volumeMounts on
the CronJob definition") to accurately reflect the resource created by this
chart.
- Around line 3-11: The Kubernetes API type in the docs comment is incorrect:
the example showing apiKey -> valueFrom -> secretKeyRef corresponds to
EnvVarSource (not EnvFromSource); update the reference/link and any mention in
the comment to point to EnvVarSource (and its docs URL) and keep the example
keys (apiKey, valueFrom, secretKeyRef) as-is so readers can find the correct
pattern.
🧹 Nitpick comments (2)
charts/ctrlc-sync/templates/cronjob.yaml (1)
54-63: Consider setting secure defaults for security contexts.Trivy flags that the CronJob uses a default security context, which allows root privileges and writable root filesystem. While the template supports overriding via
podSecurityContextandsecurityContextvalues, shipping secure defaults would be a better baseline.🔒 Suggested default values
In
values.yaml, consider adding:podSecurityContext: runAsNonRoot: true seccompProfile: type: RuntimeDefault securityContext: allowPrivilegeEscalation: false readOnlyRootFilesystem: true capabilities: drop: - ALLcharts/ctrlc-sync/values.yaml (1)
71-80: Consider enabling security defaults instead of leaving them commented out.For a production-ready chart, enabling
runAsNonRoot: trueand dropping all capabilities by default would be a stronger security posture. Currently these are only commented-out examples, so users who don't customize will run with no security context restrictions.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@charts/ctrlc-sync/templates/cronjob.yaml`:
- Around line 48-49: The restartPolicy key is placed as a sibling of spec under
the Pod template instead of inside the pod spec; move restartPolicy into the
PodSpec under the template (i.e., into template.spec.restartPolicy) and remove
the misplaced sibling entry so the CronJob/Job template reads template.metadata,
template.spec, and inside template.spec include restartPolicy: OnFailure
(ensuring it is under the same structure as containers/initContainers).
In `@charts/ctrlc-sync/values.yaml`:
- Around line 27-30: The Helm template guard is using truthiness so numeric zero
values (e.g., ttlSecondsAfterFinished: 0) are ignored; update the conditionals
in cronjob.yaml to check existence instead of truthiness by using hasKey (hasKey
.Values.job "ttlSecondsAfterFinished") or an explicit nil comparison (ne
.Values.job.ttlSecondsAfterFinished nil) for
.Values.job.ttlSecondsAfterFinished, and apply the same change for
.Values.job.activeDeadlineSeconds and .Values.job.startingDeadlineSeconds so
zero is rendered correctly.
🧹 Nitpick comments (2)
charts/ctrlc-sync/templates/cronjob.yaml (2)
59-68: Consider hardening default security context for the container.Trivy flags that the container runs without a read-only root filesystem and uses the default security context (KSV-0014, KSV-0118). Since values default to
{}, the chart ships with no security hardening. Consider providing secure defaults that users can override:# values.yaml securityContext: readOnlyRootFilesystem: true runAsNonRoot: true allowPrivilegeEscalation: falseThis is a common best practice for Helm charts to be secure-by-default.
9-15: Falsy-value guards will silently drop0values fortimeZone,startingDeadlineSeconds, etc.As noted in the values.yaml review,
{{- if .Values.cron.startingDeadlineSeconds }}will not render when the value is0. While0may not be a practical value forstartingDeadlineSeconds, this pattern is error-prone. Consider using{{- if not (kindIs "invalid" ...) }}or{{- if hasKey .Values.cron "startingDeadlineSeconds" }}for numeric optional fields.
2 participants
This is an initial release/setup.
I have plans to allow you to configure the other scanners, making more of the settings modular.
Summary by CodeRabbit