feat(chart): streamline the default Kubernetes deployment - #2476
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (10)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe Helm chart now supports standalone and distributed deployment modes with strict value validation, configurable persistence, component-based naming, Secret-backed authentication, scheduling settings, updated documentation, and comprehensive CI render checks. ChangesHelm chart deployment configuration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Values as Helm Values
participant ConfigMap as ConfigMap template
participant UI as UI Deployment
participant Workloads as Distributed workloads
Values->>ConfigMap: Authentication and deployment settings
ConfigMap->>UI: Server, CORS, OIDC, and secret configuration
ConfigMap->>Workloads: Distributed component configuration
Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
🟢 Ready to approve
The chart values, schema validation, templating, and CI coverage changes are consistent and correctly scope secrets to the UI pod without exposing them via the ConfigMap.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds first-class Helm values (with schema validation and template guardrails) for configuring OIDC authentication and license injection in the Dagu Helm chart, while keeping secrets scoped to the UI/server pod and improving latest image freshness via pull policy defaults.
Changes:
- Default
image.pullPolicyset toAlways(tag remainslatest) and documentation updated accordingly. - New
license.*values + validation + UI-only Secret env injection (DAGU_LICENSE_KEY), with conflicts blocked viaextraEnvchecks. - New typed
auth.oidc.*values + schema + render intodagu.yamlConfigMap (non-secrets only), with UI-only Secret env injection (DAGU_AUTH_OIDC_CLIENT_SECRET) and CI render coverage.
File summaries
| File | Description |
|---|---|
| charts/dagu/values.yaml | Adds defaults for license.*, auth.oidc.*, and changes default pull policy to Always. |
| charts/dagu/values.schema.json | Introduces schema validation for license.* and auth.oidc.* plus conditional requirements when OIDC is enabled. |
| charts/dagu/templates/ui-deployment.yaml | Injects license and OIDC client secret from existing Secrets into the UI pod only. |
| charts/dagu/templates/NOTES.txt | Documents OIDC callback URL and setup note when OIDC is enabled. |
| charts/dagu/templates/configmap.yaml | Adds render-time validation guardrails and renders non-secret OIDC config into dagu.yaml. |
| charts/dagu/README.md | Documents license Secret wiring, OIDC configuration, and the Always pull policy behavior. |
| charts/dagu/Chart.yaml | Bumps chart version to 1.0.12. |
| .github/workflows/chart-ci.yaml | Adds a CI render step for the OIDC configuration path. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/chart-ci.yaml">
<violation number="1" location=".github/workflows/chart-ci.yaml:117">
P3: The added `Render OIDC configuration` CI step only covers the default (empty) role/workspace mapping path. Because auth.oidc.roleMapping.groupMappings/workspaceMappings are not set, configmap.yaml always renders the `workspace_mappings: {}` fallback branch, so the new non-empty global/workspace mapping rendering (and OIDC+proxy coexistence) added by this PR is never validated in chart CI. Consider setting a workspace/group mapping and optionally proxy.enabled=true in this step to actually exercise those render paths.</violation>
<violation number="2" location=".github/workflows/chart-ci.yaml:118">
P3: The new CI step renders the OIDC config but discards all output with `>/dev/null`, so it only catches hard render failures. It won't detect a regression where the OIDC block stops being emitted into `dagu.yaml` (e.g. the `auth.oidc.enabled` guard or `client_id`/`client_url` lines going missing), even though the PR describes this step as covering the OIDC render path. Consider grepping the rendered output for an OIDC marker so the step actually validates the OIDC content it claims to cover.</violation>
</file>
<file name="charts/dagu/templates/ui-deployment.yaml">
<violation number="1" location="charts/dagu/templates/ui-deployment.yaml:51">
P2: Malformed Secret names pass the chart schema and make the rendered Deployment invalid; constraining `license.existingSecret` and `auth.oidc.clientSecret.existingSecret` to DNS-1123 subdomain names (while still allowing the empty disabled value) would surface this during Helm validation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
🟢 Ready to approve
The changes are coherent and include schema + template-time guardrails; only a minor NOTES clarification was suggested.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
charts/dagu/templates/NOTES.txt:20
- When OIDC is enabled, the server can block OIDC login until a license with the SSO feature is active (it redirects with an error like “SSO requires an active Dagu license”). The install NOTES currently don’t mention the license prerequisite, which can confuse users who enable OIDC but haven’t configured a license/activation yet.
OIDC authentication is enabled.
Register this callback URL with the provider:
{{ trimSuffix "/" .Values.auth.oidc.clientUrl }}/oidc-callback
Complete builtin administrator setup before the first OIDC login.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
5 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="charts/dagu/templates/NOTES.txt">
<violation number="1" location="charts/dagu/templates/NOTES.txt:15">
P3: The new OIDC reminder only renders when the typed `auth.oidc.enabled` is true. Since this release deliberately keeps the legacy extraEnv-based OIDC path working, a cluster configured that way has OIDC active at runtime yet never prints the callback-URL registration or builtin-admin setup reminder that this note is meant to surface. Consider rendering the callback hint when OIDC is in effect through either path (e.g., also detect the legacy extraEnv markers), so users on the supported backward-compat path still get discoverability.</violation>
</file>
<file name=".github/workflows/chart-ci.yaml">
<violation number="1" location=".github/workflows/chart-ci.yaml:117">
P2: A regression in OIDC client-secret injection can pass this CI case because it never renders the UI Deployment. Rendering and asserting the UI Deployment as well would make the documented UI-only Secret coverage effective.</violation>
<violation number="2" location=".github/workflows/chart-ci.yaml:134">
P3: A regression that drops the workspace role can pass this CI case because it only asserts `workspace: platform`. Including `role: developer` in the expected output would cover the required workspace mapping completely.</violation>
<violation number="3" location=".github/workflows/chart-ci.yaml:136">
P3: The render guard can accept structurally incorrect OIDC YAML because `grep -Fq` does not require an exact line match. Using an anchored exact-line check such as `grep -Fxq` would make the indentation-sensitive assertions meaningful.</violation>
</file>
<file name="charts/dagu/values.schema.json">
<violation number="1" location="charts/dagu/values.schema.json:368">
P2: Enabling auth.oidc validates successfully without any license configuration, but the chart requires an active license for OIDC to work (README: 'OIDC ... plus active license'; DAGU_LICENSE_KEY is only injected when license.existingSecret is set). A user who enables OIDC without configuring the license block gets a green-light install that silently has non-functional OIDC. Consider extending the oidc.enabled 'then' clause to also require a non-empty license.existingSecret (mirroring the clientSecret.existingSecret check) so the schema rejects the broken combination.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
🟢 Ready to approve
The chart changes are consistent across values/schema/templates/docs, maintain secret scoping to the UI pod, and CI now asserts the critical OIDC/license render paths.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="charts/dagu/templates/ui-deployment.yaml">
<violation number="1" location="charts/dagu/templates/ui-deployment.yaml:48">
P2: Rotating either referenced Secret will not roll the UI pod, so OIDC authentication can keep using an invalid client secret and license changes remain unapplied until an operator restarts the Deployment. A Secret-change rollout mechanism or an explicit restart/rotation requirement would make this lifecycle behavior safe and discoverable.</violation>
</file>
<file name="charts/dagu/values.schema.json">
<violation number="1" location="charts/dagu/values.schema.json:73">
P3: OIDC `issuer` and `clientUrl` are URL values but the schema only constrains them as non-empty strings, so a misspelled or malformed URL (e.g. a missing scheme) passes `helm lint` and only fails at OIDC startup/callback. Consider adding `format: "uri"` (and `format: "email"` on `whitelist` items) so bogus provider settings are caught at values-validation time rather than at runtime. Note that enforcement depends on the validator's format support, so treat this as best-effort validation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
🟢 Ready to approve
The chart changes consistently validate inputs, keep secrets scoped to the UI deployment, and CI coverage was added for the new render paths.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="charts/dagu/templates/configmap.yaml">
<violation number="1" location="charts/dagu/templates/configmap.yaml:29">
P2: OIDC values with `whitelist` or `allowedDomains` but without the `email` scope pass chart rendering and then fail Dagu's `validateOIDCForBuiltin` at pod startup. Adding the same cross-field check to the chart would report this misconfiguration during Helm validation instead of leaving the deployment unhealthy.</violation>
</file>
<file name=".github/workflows/chart-ci.yaml">
<violation number="1" location=".github/workflows/chart-ci.yaml:150">
P3: These assertions pin the CI to the exact rendered indentation (6/8/10/12/18 spaces) of the ConfigMap/Deployment templates and only check line presence, not YAML structure. Any reformatting of configmap.yaml/ui-deployment.yaml or the dayu.yaml literal block (even a semantically identical change) breaks this step with a confusing 'missing' error, and a real regression that keeps the same lines but mis-nests them would pass silently. Consider validating structure instead of exact-indentation substrings — e.g. pipe the rendered ConfigMap through a YAML parse or use a Helm unittest assertion on the dayu.yaml block — or at least assert on the value lines without the leading-space-sensitive whole-line `-x` match.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
🟡 Not ready to approve
The README’s restart command guidance is incorrect for release names that don’t contain dagu, which can mislead operators.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
charts/dagu/README.md:228
- The Deployment name is based on
{{ include "dagu.fullname" . }}-ui; for release names that do not containdagu, this becomes<release-name>-dagu-ui(not<release-name>-ui). As written, the restart command guidance will be wrong for common release names likeprod.
The command assumes the release name is `dagu`; use `<release-name>-ui` when the release has another name.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/chart-ci.yaml">
<violation number="1" location=".github/workflows/chart-ci.yaml:134">
P2: The CI check does not enforce the documented UI-only Secret scoping: it can pass even if license or OIDC Secret references are added to scheduler, coordinator, or worker pods. Adding negative assertions for those rendered pod environments would make this security boundary regression-testable.</violation>
</file>
<file name="charts/dagu/values.schema.json">
<violation number="1" location="charts/dagu/values.schema.json:172">
P3: The chart's workspace grant schema only enforces `minLength: 1` for the workspace name, but the application's own validation is stricter: workspace names must match `^[A-Za-z0-9_-]+$` and must not be the reserved names `all`, `default`, or `global` (workspace.ValidateName in internal/workspace/workspace.go, mirrored in internal/cmn/schema/config.schema.json OIDCWorkspaceGrantDef). As a result, values like `workspace: all` or `workspace: my workspace` pass the chart's first-class schema validation but are rejected at runtime by dagu when it parses dagu.yaml. Tightening the chart schema to match the application would surface these errors at `helm` time instead of at pod startup.</violation>
</file>
<file name="charts/dagu/README.md">
<violation number="1" location="charts/dagu/README.md:228">
P3: The restart guidance tells users to target `<release-name>-ui` for any non-`dagu` release, but that does not match how this chart names the UI Deployment. The Deployment is named via `include "dagu.fullname" .` + `-ui` (ui-deployment.yaml line 4), and `dagu.fullname` resolves to `<release>-dagu` whenever the release name does not already contain "dagu". So `helm install foo ...` produces a `foo-dagu-ui` Deployment, not `foo-ui`, and the documented `kubectl rollout restart deployment/<release-name>-ui` would fail (Deployment not found). This also diverges from the updated NOTES.txt, which correctly uses `{{ include "dagu.fullname" . }}-ui`. Recommend correcting the doc to reflect the actual fullname-based name.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
charts/dagu/templates/configmap.yaml (2)
60-68: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRequire proxy-only network isolation.
ui.service.type: ClusterIPalone does not prevent in-cluster clients from reaching the UI Service. Proxy authentication trustsX-Auth-Request-Userand related headers, so any pod that can reach the UI directly can impersonate identities. KeepClusterIP, but require users to apply the NetworkPolicy for the correct proxy ingress controller before enablingauth.proxy.enabled; document or validate it as mandatory inPROXY_AUTH.md.🤖 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/dagu/templates/configmap.yaml` around lines 60 - 68, Update the auth.proxy.enabled validation in the configmap template to require the proxy-ingress NetworkPolicy configuration in addition to ClusterIP and disabled chart-managed ingress, using the chart’s existing configuration symbol if available. Document this NetworkPolicy as mandatory in PROXY_AUTH.md, including that it must restrict UI access to the approved proxy ingress controller before proxy authentication can be enabled.
229-233: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not hard-code insecure distributed transport.
Distributed worker communication uses the peer (gRPC) transport and the chart does not expose peer certificate/key/client-CA Secret references. Set
peer.insecure: falsewith protected TLS/mTLS configuration, or fail distributed mode when transport protection is absent.🤖 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/dagu/templates/configmap.yaml` around lines 229 - 233, Update the distributed-mode `peer` configuration in the Helm template to avoid hard-coding insecure transport: set `peer.insecure` to false and configure the required TLS/mTLS certificate, key, and client-CA Secret references, or make the template fail when those protected transport settings are absent.
🤖 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.
Outside diff comments:
In `@charts/dagu/templates/configmap.yaml`:
- Around line 60-68: Update the auth.proxy.enabled validation in the configmap
template to require the proxy-ingress NetworkPolicy configuration in addition to
ClusterIP and disabled chart-managed ingress, using the chart’s existing
configuration symbol if available. Document this NetworkPolicy as mandatory in
PROXY_AUTH.md, including that it must restrict UI access to the approved proxy
ingress controller before proxy authentication can be enabled.
- Around line 229-233: Update the distributed-mode `peer` configuration in the
Helm template to avoid hard-coding insecure transport: set `peer.insecure` to
false and configure the required TLS/mTLS certificate, key, and client-CA Secret
references, or make the template fail when those protected transport settings
are absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 092fbca9-3942-4502-9bdf-602007e46554
📒 Files selected for processing (2)
charts/dagu/Chart.yamlcharts/dagu/templates/configmap.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/dagu/Chart.yaml
There was a problem hiding this comment.
4 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="charts/dagu/templates/tests/test-connection.yaml">
<violation number="1" location="charts/dagu/templates/tests/test-connection.yaml:18">
P3: The new `test.image` config defaults `tag` to "1.36.1", but if a user overrides only `repository` (or clears the tag), the rendered image becomes `busybox:` with an empty tag, producing an invalid image reference and a failing `helm test`. The main Dagu image handles this pattern consistently via the `dagu.imageTag` helper (defaults empty tag to `Chart.appVersion`); reusing that pattern here would keep behavior consistent.</violation>
</file>
<file name="charts/dagu/templates/_helpers.tpl">
<violation number="1" location="charts/dagu/templates/_helpers.tpl:34">
P2: Distributed pods will not roll when omitted ConfigMap-backed settings such as `config.publicUrl` change, because this checksum only hashes a small subset of the rendered configuration. A changed public URL can therefore leave notification links using the previous value until the affected Deployments are restarted; hashing the rendered ConfigMap, as the UI Deployment does, would keep all distributed components synchronized.</violation>
</file>
<file name="charts/dagu/templates/ui-deployment.yaml">
<violation number="1" location="charts/dagu/templates/ui-deployment.yaml:18">
P1: Existing chart installations cannot be upgraded because this changes the UI Deployment's immutable selector from the previous `dagu.labels` set to `dagu.selectorLabels`. Retaining the legacy selector for this Deployment (or providing an explicit migration that recreates it) would avoid the failed `helm upgrade`.</violation>
</file>
<file name="charts/dagu/values.schema.json">
<violation number="1" location="charts/dagu/values.schema.json:517">
P3: Minor schema gap: when auth.oidc.enabled=true the schema applies minLength:1 to clientSecret.existingSecret/secretKey but doesn't make them required, so an empty client secret passes validation and is only caught later by the configmap template's fail. The basic branch already enforces this via required; add "existingSecret" (and "secretKey") to the oidc then-block's required list for parity so invalid combos fail at values-validation time.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| selector: | ||
| matchLabels: | ||
| {{- include "dagu.labels" . | nindent 6 }} | ||
| {{- include "dagu.selectorLabels" . | nindent 6 }} |
There was a problem hiding this comment.
P1: Existing chart installations cannot be upgraded because this changes the UI Deployment's immutable selector from the previous dagu.labels set to dagu.selectorLabels. Retaining the legacy selector for this Deployment (or providing an explicit migration that recreates it) would avoid the failed helm upgrade.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/templates/ui-deployment.yaml, line 18:
<comment>Existing chart installations cannot be upgraded because this changes the UI Deployment's immutable selector from the previous `dagu.labels` set to `dagu.selectorLabels`. Retaining the legacy selector for this Deployment (or providing an explicit migration that recreates it) would avoid the failed `helm upgrade`.</comment>
<file context>
@@ -1,24 +1,26 @@
selector:
matchLabels:
- {{- include "dagu.labels" . | nindent 6 }}
+ {{- include "dagu.selectorLabels" . | nindent 6 }}
app.kubernetes.io/component: ui
template:
</file context>
| {{- include "dagu.selectorLabels" . | nindent 6 }} | |
| {{- include "dagu.labels" . | nindent 6 }} |
| {{- end }} | ||
|
|
||
| {{- define "dagu.distributedConfigChecksum" -}} | ||
| {{- dict "deploymentMode" .Values.deploymentMode "envPassthrough" .Values.config.envPassthrough "envPassthroughPrefixes" .Values.config.envPassthroughPrefixes "coordinatorHealthPort" .Values.coordinator.healthPort "workerHealthPort" .Values.worker.healthPort | toJson | sha256sum -}} |
There was a problem hiding this comment.
P2: Distributed pods will not roll when omitted ConfigMap-backed settings such as config.publicUrl change, because this checksum only hashes a small subset of the rendered configuration. A changed public URL can therefore leave notification links using the previous value until the affected Deployments are restarted; hashing the rendered ConfigMap, as the UI Deployment does, would keep all distributed components synchronized.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/templates/_helpers.tpl, line 34:
<comment>Distributed pods will not roll when omitted ConfigMap-backed settings such as `config.publicUrl` change, because this checksum only hashes a small subset of the rendered configuration. A changed public URL can therefore leave notification links using the previous value until the affected Deployments are restarted; hashing the rendered ConfigMap, as the UI Deployment does, would keep all distributed components synchronized.</comment>
<file context>
@@ -15,10 +15,39 @@
+{{- end }}
+
+{{- define "dagu.distributedConfigChecksum" -}}
+{{- dict "deploymentMode" .Values.deploymentMode "envPassthrough" .Values.config.envPassthrough "envPassthroughPrefixes" .Values.config.envPassthroughPrefixes "coordinatorHealthPort" .Values.coordinator.healthPort "workerHealthPort" .Values.worker.healthPort | toJson | sha256sum -}}
+{{- end }}
+
</file context>
| {{- dict "deploymentMode" .Values.deploymentMode "envPassthrough" .Values.config.envPassthrough "envPassthroughPrefixes" .Values.config.envPassthroughPrefixes "coordinatorHealthPort" .Values.coordinator.healthPort "workerHealthPort" .Values.worker.healthPort | toJson | sha256sum -}} | |
| {{- include (print $.Template.BasePath "/configmap.yaml") . | sha256sum -}} |
| containers: | ||
| - name: wget | ||
| image: busybox | ||
| image: "{{ .Values.test.image.repository }}:{{ .Values.test.image.tag }}" |
There was a problem hiding this comment.
P3: The new test.image config defaults tag to "1.36.1", but if a user overrides only repository (or clears the tag), the rendered image becomes busybox: with an empty tag, producing an invalid image reference and a failing helm test. The main Dagu image handles this pattern consistently via the dagu.imageTag helper (defaults empty tag to Chart.appVersion); reusing that pattern here would keep behavior consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/templates/tests/test-connection.yaml, line 18:
<comment>The new `test.image` config defaults `tag` to "1.36.1", but if a user overrides only `repository` (or clears the tag), the rendered image becomes `busybox:` with an empty tag, producing an invalid image reference and a failing `helm test`. The main Dagu image handles this pattern consistently via the `dagu.imageTag` helper (defaults empty tag to `Chart.appVersion`); reusing that pattern here would keep behavior consistent.</comment>
<file context>
@@ -1,17 +1,22 @@
containers:
- name: wget
- image: busybox
+ image: "{{ .Values.test.image.repository }}:{{ .Values.test.image.tag }}"
+ imagePullPolicy: {{ .Values.test.image.pullPolicy }}
command: ['wget']
</file context>
| "minLength": 1, | ||
| "maxLength": 128 | ||
| }, | ||
| "clientSecret": { |
There was a problem hiding this comment.
P3: Minor schema gap: when auth.oidc.enabled=true the schema applies minLength:1 to clientSecret.existingSecret/secretKey but doesn't make them required, so an empty client secret passes validation and is only caught later by the configmap template's fail. The basic branch already enforces this via required; add "existingSecret" (and "secretKey") to the oidc then-block's required list for parity so invalid combos fail at values-validation time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/values.schema.json, line 517:
<comment>Minor schema gap: when auth.oidc.enabled=true the schema applies minLength:1 to clientSecret.existingSecret/secretKey but doesn't make them required, so an empty client secret passes validation and is only caught later by the configmap template's fail. The basic branch already enforces this via required; add "existingSecret" (and "secretKey") to the oidc then-block's required list for parity so invalid combos fail at values-validation time.</comment>
<file context>
@@ -19,26 +160,496 @@
+ "minLength": 1,
+ "maxLength": 128
+ },
+ "clientSecret": {
+ "type": "object",
+ "additionalProperties": false,
</file context>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
9 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="charts/dagu/templates/ui-deployment.yaml">
<violation number="1" location="charts/dagu/templates/ui-deployment.yaml:18">
P1: Upgrading an existing release will fail because this changes the UI Deployment's immutable selector: the old selector included version/chart/managed-by labels, while `dagu.selectorLabels` removes them. Preserving the existing selector for upgrades or providing an explicit delete/recreate migration path would prevent the release from becoming stuck.</violation>
</file>
<file name="charts/dagu/templates/scheduler-deployment.yaml">
<violation number="1" location="charts/dagu/templates/scheduler-deployment.yaml:15">
P1: Existing distributed installations cannot be upgraded in place because this removes `app.kubernetes.io/version` and `helm.sh/chart` from the scheduler Deployment selector, and Kubernetes treats `spec.selector` as immutable. Preserving the previous selector for this transition or providing an explicit delete/recreate migration would avoid the upgrade failure.</violation>
</file>
<file name="charts/dagu/templates/coordinator-deployment.yaml">
<violation number="1" location="charts/dagu/templates/coordinator-deployment.yaml:15">
P2: Upgrading an existing distributed release will fail for the coordinator because this change alters its immutable Deployment selector. A migration path that recreates the old Deployment while preserving the shared PVC, or a selector-preserving upgrade strategy, should accompany this chart revision.</violation>
</file>
<file name="charts/dagu/templates/ui-ingress.yaml">
<violation number="1" location="charts/dagu/templates/ui-ingress.yaml:9">
P1: Enabling the chart-managed ingress renders invalid YAML because the left-trimming template tags remove line breaks between adjacent YAML fields. Removing the left-trim markers (or using newline-aware indentation) keeps `metadata`, `spec`, TLS, and `rules` as separate YAML nodes.</violation>
</file>
<file name="charts/dagu/templates/NOTES.txt">
<violation number="1" location="charts/dagu/templates/NOTES.txt:5">
P2: Rotating a Secret or ConfigMap referenced by an arbitrary `extraEnv` entry will not be reflected in running pods, but the post-install notes do not mention a restart unless the variable is OIDC- or license-related. The reload detection could cover every `valueFrom` entry and the guidance should restart the Dagu deployments that consume it, not only the UI in distributed mode.</violation>
</file>
<file name=".github/workflows/chart-ci.yaml">
<violation number="1" location=".github/workflows/chart-ci.yaml:119">
P2: Central standalone defaults can regress while this CI job stays green: the render is only checked for `cors_allowed_origins`, so `start-all`, the ClusterIP Service, or retained PVC could disappear without failing this step. Assertions for those rendered invariants would make this smoke test cover the default behavior described by the chart.</violation>
<violation number="2" location=".github/workflows/chart-ci.yaml:192">
P3: A Helm upgrade can turn valid proxy-auth configurations into CI failures because these assertions match exact JSON-schema diagnostic wording while `azure/setup-helm` installs its default `latest` version. Checking a stable schema-path token or pinning the Helm version would avoid coupling this test to formatter wording.</violation>
</file>
<file name="charts/dagu/values.schema.json">
<violation number="1" location="charts/dagu/values.schema.json:83">
P3: test.image.tag uses minLength:1 while image.tag explicitly allows an empty value (which defaults to Chart.appVersion). This makes the empty-string contract inconsistent across the two image blocks; if intentional (test image has no appVersion default), a brief comment or the same empty-allowed pattern would keep the schema self-consistent and avoid confusion for extenders.</violation>
<violation number="2" location="charts/dagu/values.schema.json:355">
P3: ingress.host is required and must be a non-empty hostname even when ingress.enabled=false, so disabling ingress still forces a valid host. Other fields that are inert when disabled (e.g. tls.secretName, license.existingSecret) allow empty. Consider allowing an empty host (or moving it into a conditional that applies only when ingress.enabled=true) so users who disable ingress don't have to invent a fake hostname.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| selector: | ||
| matchLabels: | ||
| {{- include "dagu.labels" . | nindent 6 }} | ||
| {{- include "dagu.selectorLabels" . | nindent 6 }} |
There was a problem hiding this comment.
P1: Upgrading an existing release will fail because this changes the UI Deployment's immutable selector: the old selector included version/chart/managed-by labels, while dagu.selectorLabels removes them. Preserving the existing selector for upgrades or providing an explicit delete/recreate migration path would prevent the release from becoming stuck.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/templates/ui-deployment.yaml, line 18:
<comment>Upgrading an existing release will fail because this changes the UI Deployment's immutable selector: the old selector included version/chart/managed-by labels, while `dagu.selectorLabels` removes them. Preserving the existing selector for upgrades or providing an explicit delete/recreate migration path would prevent the release from becoming stuck.</comment>
<file context>
@@ -1,24 +1,26 @@
selector:
matchLabels:
- {{- include "dagu.labels" . | nindent 6 }}
+ {{- include "dagu.selectorLabels" . | nindent 6 }}
app.kubernetes.io/component: ui
template:
</file context>
| selector: | ||
| matchLabels: | ||
| {{- include "dagu.labels" . | nindent 6 }} | ||
| {{- include "dagu.selectorLabels" . | nindent 6 }} |
There was a problem hiding this comment.
P1: Existing distributed installations cannot be upgraded in place because this removes app.kubernetes.io/version and helm.sh/chart from the scheduler Deployment selector, and Kubernetes treats spec.selector as immutable. Preserving the previous selector for this transition or providing an explicit delete/recreate migration would avoid the upgrade failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/templates/scheduler-deployment.yaml, line 15:
<comment>Existing distributed installations cannot be upgraded in place because this removes `app.kubernetes.io/version` and `helm.sh/chart` from the scheduler Deployment selector, and Kubernetes treats `spec.selector` as immutable. Preserving the previous selector for this transition or providing an explicit delete/recreate migration would avoid the upgrade failure.</comment>
<file context>
@@ -1,18 +1,23 @@
selector:
matchLabels:
- {{- include "dagu.labels" . | nindent 6 }}
+ {{- include "dagu.selectorLabels" . | nindent 6 }}
app.kubernetes.io/component: scheduler
template:
</file context>
| labels: | ||
| {{- include "dagu.labels" . | nindent 4 }} | ||
| app.kubernetes.io/component: ui | ||
| {{- with .Values.ingress.annotations }} |
There was a problem hiding this comment.
P1: Enabling the chart-managed ingress renders invalid YAML because the left-trimming template tags remove line breaks between adjacent YAML fields. Removing the left-trim markers (or using newline-aware indentation) keeps metadata, spec, TLS, and rules as separate YAML nodes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/templates/ui-ingress.yaml, line 9:
<comment>Enabling the chart-managed ingress renders invalid YAML because the left-trimming template tags remove line breaks between adjacent YAML fields. Removing the left-trim markers (or using newline-aware indentation) keeps `metadata`, `spec`, TLS, and `rules` as separate YAML nodes.</comment>
<file context>
@@ -0,0 +1,36 @@
+ labels:
+ {{- include "dagu.labels" . | nindent 4 }}
+ app.kubernetes.io/component: ui
+{{- with .Values.ingress.annotations }}
+ annotations:
+{{ toYaml . | indent 4 }}
</file context>
| selector: | ||
| matchLabels: | ||
| {{- include "dagu.labels" . | nindent 6 }} | ||
| {{- include "dagu.selectorLabels" . | nindent 6 }} |
There was a problem hiding this comment.
P2: Upgrading an existing distributed release will fail for the coordinator because this change alters its immutable Deployment selector. A migration path that recreates the old Deployment while preserving the shared PVC, or a selector-preserving upgrade strategy, should accompany this chart revision.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/templates/coordinator-deployment.yaml, line 15:
<comment>Upgrading an existing distributed release will fail for the coordinator because this change alters its immutable Deployment selector. A migration path that recreates the old Deployment while preserving the shared PVC, or a selector-preserving upgrade strategy, should accompany this chart revision.</comment>
<file context>
@@ -1,18 +1,23 @@
selector:
matchLabels:
- {{- include "dagu.labels" . | nindent 6 }}
+ {{- include "dagu.selectorLabels" . | nindent 6 }}
app.kubernetes.io/component: coordinator
template:
</file context>
| {{- $legacyOIDC := false -}} | ||
| {{- $legacyReloadRequired := false -}} | ||
| {{- range .Values.extraEnv -}} | ||
| {{- if and .valueFrom (or (hasPrefix "DAGU_AUTH_OIDC_" .name) (has .name (list "DAGU_LICENSE" "DAGU_LICENSE_KEY" "DAGU_LICENSE_FILE"))) -}} |
There was a problem hiding this comment.
P2: Rotating a Secret or ConfigMap referenced by an arbitrary extraEnv entry will not be reflected in running pods, but the post-install notes do not mention a restart unless the variable is OIDC- or license-related. The reload detection could cover every valueFrom entry and the guidance should restart the Dagu deployments that consume it, not only the UI in distributed mode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/templates/NOTES.txt, line 5:
<comment>Rotating a Secret or ConfigMap referenced by an arbitrary `extraEnv` entry will not be reflected in running pods, but the post-install notes do not mention a restart unless the variable is OIDC- or license-related. The reload detection could cover every `valueFrom` entry and the guidance should restart the Dagu deployments that consume it, not only the UI in distributed mode.</comment>
<file context>
@@ -1,35 +1,99 @@
+{{- $legacyOIDC := false -}}
+{{- $legacyReloadRequired := false -}}
+{{- range .Values.extraEnv -}}
+{{- if and .valueFrom (or (hasPrefix "DAGU_AUTH_OIDC_" .name) (has .name (list "DAGU_LICENSE" "DAGU_LICENSE_KEY" "DAGU_LICENSE_FILE"))) -}}
+{{- $legacyReloadRequired = true -}}
+{{- end -}}
</file context>
| --namespace dagu \ | ||
| --kube-version 1.19.0 >/tmp/dagu-default-render.yaml | ||
|
|
||
| grep -Fq 'cors_allowed_origins: []' /tmp/dagu-default-render.yaml |
There was a problem hiding this comment.
P2: Central standalone defaults can regress while this CI job stays green: the render is only checked for cors_allowed_origins, so start-all, the ClusterIP Service, or retained PVC could disappear without failing this step. Assertions for those rendered invariants would make this smoke test cover the default behavior described by the chart.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/chart-ci.yaml, line 119:
<comment>Central standalone defaults can regress while this CI job stays green: the render is only checked for `cors_allowed_origins`, so `start-all`, the ClusterIP Service, or retained PVC could disappear without failing this step. Assertions for those rendered invariants would make this smoke test cover the default behavior described by the chart.</comment>
<file context>
@@ -102,10 +108,112 @@ jobs:
+ --namespace dagu \
+ --kube-version 1.19.0 >/tmp/dagu-default-render.yaml
+
+ grep -Fq 'cors_allowed_origins: []' /tmp/dagu-default-render.yaml
+
+ - name: Render distributed mode
</file context>
| grep -Fq 'cors_allowed_origins: []' /tmp/dagu-default-render.yaml | |
| grep -Fq 'cors_allowed_origins: []' /tmp/dagu-default-render.yaml | |
| grep -Fq -- '- start-all' /tmp/dagu-default-render.yaml | |
| grep -Fq 'type: ClusterIP' /tmp/dagu-default-render.yaml | |
| grep -Fq 'helm.sh/resource-policy: keep' /tmp/dagu-default-render.yaml |
| } | ||
|
|
||
| expect_proxy_render_failure \ | ||
| "at '/ui/service/type': value must be 'ClusterIP'" \ |
There was a problem hiding this comment.
P3: A Helm upgrade can turn valid proxy-auth configurations into CI failures because these assertions match exact JSON-schema diagnostic wording while azure/setup-helm installs its default latest version. Checking a stable schema-path token or pinning the Helm version would avoid coupling this test to formatter wording.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/chart-ci.yaml, line 192:
<comment>A Helm upgrade can turn valid proxy-auth configurations into CI failures because these assertions match exact JSON-schema diagnostic wording while `azure/setup-helm` installs its default `latest` version. Checking a stable schema-path token or pinning the Helm version would avoid coupling this test to formatter wording.</comment>
<file context>
@@ -102,10 +108,112 @@ jobs:
+ }
+
+ expect_proxy_render_failure \
+ "at '/ui/service/type': value must be 'ClusterIP'" \
+ --set ui.service.type=LoadBalancer
+ expect_proxy_render_failure \
</file context>
| "type": "string", | ||
| "minLength": 1 | ||
| }, | ||
| "tag": { |
There was a problem hiding this comment.
P3: test.image.tag uses minLength:1 while image.tag explicitly allows an empty value (which defaults to Chart.appVersion). This makes the empty-string contract inconsistent across the two image blocks; if intentional (test image has no appVersion default), a brief comment or the same empty-allowed pattern would keep the schema self-consistent and avoid confusion for extenders.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/values.schema.json, line 83:
<comment>test.image.tag uses minLength:1 while image.tag explicitly allows an empty value (which defaults to Chart.appVersion). This makes the empty-string contract inconsistent across the two image blocks; if intentional (test image has no appVersion default), a brief comment or the same empty-allowed pattern would keep the schema self-consistent and avoid confusion for extenders.</comment>
<file context>
@@ -1,12 +1,153 @@
+ "type": "string",
+ "minLength": 1
+ },
+ "tag": {
+ "type": "string",
+ "maxLength": 128,
</file context>
| "type": "string" | ||
| } | ||
| }, | ||
| "host": { |
There was a problem hiding this comment.
P3: ingress.host is required and must be a non-empty hostname even when ingress.enabled=false, so disabling ingress still forces a valid host. Other fields that are inert when disabled (e.g. tls.secretName, license.existingSecret) allow empty. Consider allowing an empty host (or moving it into a conditional that applies only when ingress.enabled=true) so users who disable ingress don't have to invent a fake hostname.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/values.schema.json, line 355:
<comment>ingress.host is required and must be a non-empty hostname even when ingress.enabled=false, so disabling ingress still forces a valid host. Other fields that are inert when disabled (e.g. tls.secretName, license.existingSecret) allow empty. Consider allowing an empty host (or moving it into a conditional that applies only when ingress.enabled=true) so users who disable ingress don't have to invent a fake hostname.</comment>
<file context>
@@ -19,26 +160,496 @@
+ "type": "string"
+ }
+ },
+ "host": {
+ "type": "string",
+ "minLength": 1,
</file context>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/dagu/templates/NOTES.txt (1)
94-95: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse the existing claim in storage troubleshooting commands.
When
persistence.existingClaimis set, these commands still renderdagu.pvcName. That generated name may not exist, so the instructions inspect the wrong PVC. Reuse the existing-claim fallback already used above.Proposed fix
- 1. Verify PVC status: kubectl --namespace {{ .Release.Namespace }} get pvc {{ include "dagu.pvcName" . }} - 2. Inspect events: kubectl --namespace {{ .Release.Namespace }} describe pvc {{ include "dagu.pvcName" . }} + 1. Verify PVC status: kubectl --namespace {{ .Release.Namespace }} get pvc {{ default (include "dagu.pvcName" .) .Values.persistence.existingClaim }} + 2. Inspect events: kubectl --namespace {{ .Release.Namespace }} describe pvc {{ default (include "dagu.pvcName" .) .Values.persistence.existingClaim }}🤖 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/dagu/templates/NOTES.txt` around lines 94 - 95, Update the PVC troubleshooting commands in NOTES.txt to reference the configured persistence.existingClaim when set, falling back to the existing dagu.pvcName expression otherwise. Apply the same existing-claim fallback used above to both the status and describe commands.
🤖 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.
Outside diff comments:
In `@charts/dagu/templates/NOTES.txt`:
- Around line 94-95: Update the PVC troubleshooting commands in NOTES.txt to
reference the configured persistence.existingClaim when set, falling back to the
existing dagu.pvcName expression otherwise. Apply the same existing-claim
fallback used above to both the status and describe commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf8b735e-2c62-4374-bac5-11fbcaa4d44f
📒 Files selected for processing (4)
.github/workflows/chart-ci.yamlcharts/dagu/templates/NOTES.txtcharts/dagu/values.schema.jsoncharts/dagu/values.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/chart-ci.yaml
- charts/dagu/values.yaml
- charts/dagu/values.schema.json
There was a problem hiding this comment.
5 issues found across 22 files
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/chart-ci.yaml">
<violation number="1" location=".github/workflows/chart-ci.yaml:129">
P2: Distributed-mode CI only checks that Helm exits successfully; it would still pass if the coordinator/scheduler/worker resources or RWX storage were omitted or rendered incorrectly. Capturing this output and asserting the distributed resources and access mode would validate the advertised distributed path.</violation>
</file>
<file name="charts/dagu/templates/NOTES.txt">
<violation number="1" location="charts/dagu/templates/NOTES.txt:21">
P2: The rendered post-install notes lose line breaks because the `{{- ... }}` control actions chomp the newline before them. This makes the installation and access instructions run together; using non-trimming control actions (or otherwise preserving the prose newlines) throughout this file would keep the notes readable.</violation>
<violation number="2" location="charts/dagu/templates/NOTES.txt:61">
P2: Rotating a basic-auth, license, or OIDC Secret causes this command to restart every distributed component, unnecessarily rolling workers and coordination services that do not consume those UI-only values and potentially disrupting running workflows. The notes could target the UI deployment for these cases and provide separate all-component guidance only when a referenced `extraEnv` value is used by every pod.</violation>
</file>
<file name="charts/dagu/values.schema.json">
<violation number="1" location="charts/dagu/values.schema.json:269">
P2: worker pool `labels` currently accepts number/boolean values, but these are rendered into the Deployment `spec.template.metadata.labels` where Kubernetes requires string values. A user setting e.g. `team: 2` would pass schema validation yet produce an invalid manifest that fails at apply time. Restrict label values to strings to match the other value maps (nodeSelector/podAnnotations).</violation>
<violation number="2" location="charts/dagu/values.schema.json:420">
P3: The basic-auth credential keys are required with minLength 1 unconditionally, while the `basic` block (and its `existingSecret`) are only meaningfully enforced for `mode: basic`. A user running `mode: none` or `mode: builtin` is still forced to provide non-empty `usernameKey`/`passwordKey`. Consider relaxing these to be non-empty only when `mode: basic` (e.g. move minLength enforcement into the existing basic-mode conditional), consistent with how `existingSecret` is handled.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| helm template dagu ./charts/dagu \ | ||
| --namespace dagu \ | ||
| --kube-version 1.35.0 \ | ||
| --set deploymentMode=distributed \ |
There was a problem hiding this comment.
P2: Distributed-mode CI only checks that Helm exits successfully; it would still pass if the coordinator/scheduler/worker resources or RWX storage were omitted or rendered incorrectly. Capturing this output and asserting the distributed resources and access mode would validate the advertised distributed path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/chart-ci.yaml, line 129:
<comment>Distributed-mode CI only checks that Helm exits successfully; it would still pass if the coordinator/scheduler/worker resources or RWX storage were omitted or rendered incorrectly. Capturing this output and asserting the distributed resources and access mode would validate the advertised distributed path.</comment>
<file context>
@@ -102,10 +108,115 @@ jobs:
+ helm template dagu ./charts/dagu \
+ --namespace dagu \
+ --kube-version 1.35.0 \
+ --set deploymentMode=distributed \
+ --set persistence.accessMode=ReadWriteMany \
+ --set persistence.storageClass=nfs-client >/dev/null
</file context>
| Dagu has been installed successfully 🎉 | ||
|
|
||
| Deployment mode: {{ .Values.deploymentMode }} | ||
| {{- if eq .Values.deploymentMode "distributed" }} |
There was a problem hiding this comment.
P2: The rendered post-install notes lose line breaks because the {{- ... }} control actions chomp the newline before them. This makes the installation and access instructions run together; using non-trimming control actions (or otherwise preserving the prose newlines) throughout this file would keep the notes readable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/templates/NOTES.txt, line 21:
<comment>The rendered post-install notes lose line breaks because the `{{- ... }}` control actions chomp the newline before them. This makes the installation and access instructions run together; using non-trimming control actions (or otherwise preserving the prose newlines) throughout this file would keep the notes readable.</comment>
<file context>
@@ -1,35 +1,99 @@
Dagu has been installed successfully 🎉
+Deployment mode: {{ .Values.deploymentMode }}
+{{- if eq .Values.deploymentMode "distributed" }}
IMPORTANT: Dagu requires a shared filesystem (ReadWriteMany) for state persistence.
Make sure your storage class supports RWX access mode (e.g., NFS, EFS, CephFS).
</file context>
| {{- if or (eq .Values.auth.mode "basic") .Values.license.existingSecret .Values.auth.oidc.enabled $legacyOIDC $referencedEnv }} | ||
| Referenced environment values are read when the pod starts. | ||
| After changing one of these values or its referenced Secret or ConfigMap, restart the Dagu deployments: | ||
| kubectl --namespace {{ .Release.Namespace }} rollout restart deployment --selector app.kubernetes.io/instance={{ .Release.Name }} |
There was a problem hiding this comment.
P2: Rotating a basic-auth, license, or OIDC Secret causes this command to restart every distributed component, unnecessarily rolling workers and coordination services that do not consume those UI-only values and potentially disrupting running workflows. The notes could target the UI deployment for these cases and provide separate all-component guidance only when a referenced extraEnv value is used by every pod.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/templates/NOTES.txt, line 61:
<comment>Rotating a basic-auth, license, or OIDC Secret causes this command to restart every distributed component, unnecessarily rolling workers and coordination services that do not consume those UI-only values and potentially disrupting running workflows. The notes could target the UI deployment for these cases and provide separate all-component guidance only when a referenced `extraEnv` value is used by every pod.</comment>
<file context>
@@ -1,35 +1,99 @@
+{{- if or (eq .Values.auth.mode "basic") .Values.license.existingSecret .Values.auth.oidc.enabled $legacyOIDC $referencedEnv }}
+Referenced environment values are read when the pod starts.
+ After changing one of these values or its referenced Secret or ConfigMap, restart the Dagu deployments:
+ kubectl --namespace {{ .Release.Namespace }} rollout restart deployment --selector app.kubernetes.io/instance={{ .Release.Name }}
-{{ if .Values.auth.proxy.enabled }}
</file context>
| "labels": { | ||
| "type": "object", | ||
| "additionalProperties": { | ||
| "type": ["string", "number", "boolean"] |
There was a problem hiding this comment.
P2: worker pool labels currently accepts number/boolean values, but these are rendered into the Deployment spec.template.metadata.labels where Kubernetes requires string values. A user setting e.g. team: 2 would pass schema validation yet produce an invalid manifest that fails at apply time. Restrict label values to strings to match the other value maps (nodeSelector/podAnnotations).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/values.schema.json, line 269:
<comment>worker pool `labels` currently accepts number/boolean values, but these are rendered into the Deployment `spec.template.metadata.labels` where Kubernetes requires string values. A user setting e.g. `team: 2` would pass schema validation yet produce an invalid manifest that fails at apply time. Restrict label values to strings to match the other value maps (nodeSelector/podAnnotations).</comment>
<file context>
@@ -19,26 +160,502 @@
+ "labels": {
+ "type": "object",
+ "additionalProperties": {
+ "type": ["string", "number", "boolean"]
+ }
+ },
</file context>
| "type": ["string", "number", "boolean"] | |
| "type": "string" |
| "maxLength": 253, | ||
| "pattern": "^$|^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$" | ||
| }, | ||
| "usernameKey": { |
There was a problem hiding this comment.
P3: The basic-auth credential keys are required with minLength 1 unconditionally, while the basic block (and its existingSecret) are only meaningfully enforced for mode: basic. A user running mode: none or mode: builtin is still forced to provide non-empty usernameKey/passwordKey. Consider relaxing these to be non-empty only when mode: basic (e.g. move minLength enforcement into the existing basic-mode conditional), consistent with how existingSecret is handled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At charts/dagu/values.schema.json, line 420:
<comment>The basic-auth credential keys are required with minLength 1 unconditionally, while the `basic` block (and its `existingSecret`) are only meaningfully enforced for `mode: basic`. A user running `mode: none` or `mode: builtin` is still forced to provide non-empty `usernameKey`/`passwordKey`. Consider relaxing these to be non-empty only when `mode: basic` (e.g. move minLength enforcement into the existing basic-mode conditional), consistent with how `existingSecret` is handled.</comment>
<file context>
@@ -19,26 +160,502 @@
+ "maxLength": 253,
+ "pattern": "^$|^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$"
+ },
+ "usernameKey": {
+ "type": "string",
+ "minLength": 1,
</file context>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/dagu/templates/NOTES.txt (1)
36-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse plural output for multiple replicas.
ui.replicasis configurable, but this message always printsreplica. Rendered NOTES output is incorrect when the value is greater than1.Proposed fix
- Dagu server, scheduler, and local executor: {{ .Values.ui.replicas }} replica + Dagu server, scheduler, and local executor: {{ .Values.ui.replicas }} {{ if eq .Values.ui.replicas 1 }}replica{{ else }}replicas{{ end }}🤖 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/dagu/templates/NOTES.txt` at line 36, Update the replica wording in the Dagu server, scheduler, and local executor message to use the plural form when .Values.ui.replicas is greater than 1, while retaining the singular form for exactly one replica.
🤖 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.
Outside diff comments:
In `@charts/dagu/templates/NOTES.txt`:
- Line 36: Update the replica wording in the Dagu server, scheduler, and local
executor message to use the plural form when .Values.ui.replicas is greater than
1, while retaining the singular form for exactly one replica.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fcedfbe7-6e87-423a-8103-bd8a1e9b6b50
📒 Files selected for processing (3)
.github/workflows/chart-ci.yamlcharts/dagu/templates/NOTES.txtcharts/dagu/values.schema.json
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/chart-ci.yaml
- charts/dagu/values.schema.json
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Default experience
A default install runs
dagu start-allbehind a ClusterIP service, stores data on a retained PVC, and prints a working port-forward command. The image tag defaults toChart.appVersionwithIfNotPresent; ingress remains opt-in and uses the root path so the UI and API work together without rewrite rules.Distributed mode remains available through
deploymentMode: distributedand requires ReadWriteMany persistence for shared control-plane state. Worker execution data remains ephemeral by default.This is a major chart revision. Existing installations should inspect rendered manifests before upgrading; migration compatibility is not a goal of this change.
Validation
Documentation
Companion docs PR: dagucloud/docs#28
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests