Skip to content

fix(chart): align cronjob config with openab binary + add usercron support#638

Merged
thepagent merged 3 commits intoopenabdev:mainfrom
ChunHao-dev:fix/chart-cron-config-alignment
Apr 30, 2026
Merged

fix(chart): align cronjob config with openab binary + add usercron support#638
thepagent merged 3 commits intoopenabdev:mainfrom
ChunHao-dev:fix/chart-cron-config-alignment

Conversation

@ChunHao-dev
Copy link
Copy Markdown
Contributor

@ChunHao-dev ChunHao-dev commented Apr 30, 2026

What problem does this solve?

PR #629 consolidated all cron config under [cron] and renamed [[cronjobs]][[cron.jobs]], but the Helm chart template was not updated to match:

  • Chart produces: [[cronjobs]] (top-level key)
  • openab expects: [[cron.jobs]] (under [cron] per Config.cron: CronConfig in src/config.rs)

Result: all Helm-deployed cronjob configs are silently ignored.

Additionally, usercron_enabled and usercron_path are supported in config.rs but had no corresponding chart fields.

Changes

File What
charts/openab/templates/configmap.yaml [[cronjobs]][[cron.jobs]]; add [cron] section with usercron_enabled, usercron_path; add enabled field per job; wrap in conditional
charts/openab/values.yaml Add cron.usercronEnabled (default: false) and cron.usercronPath (default: "")

Testing

helm template verification

# Default — [cron] + usercron_enabled = false
helm template test charts/openab

# With cronjob — produces [cron] + [[cron.jobs]]
helm template test charts/openab \
  --set 'agents.kiro.cronjobs[0].schedule=0 9 * * 1-5' \
  --set-string 'agents.kiro.cronjobs[0].channel=123456789' \
  --set 'agents.kiro.cronjobs[0].message=hello'

# With usercron — renders usercron_path
helm template test charts/openab \
  --set 'agents.kiro.cron.usercronEnabled=true' \
  --set 'agents.kiro.cron.usercronPath=/data/usercron.toml'

OrbStack K8s live test

  • ✅ System cron ([[cron.jobs]]) — fires every minute, Discord receives message
  • ✅ Usercron (cronjob.toml hot-reload) — file detected, registered, and fired without restart

Discord Discussion: https://discord.com/channels/1491295327620169908/1499264765141454940

…pport

- Change [[cronjobs]] to [[cron.jobs]] in configmap template to match
  Config.cron.jobs in src/config.rs
- Add [cron] section with usercron_enabled and usercron_path fields
- Add enabled field to each cron job entry
- Add cron defaults (usercronEnabled, usercronPath) to values.yaml
- Wrap cron block in conditional to avoid empty output when unused
@ChunHao-dev ChunHao-dev requested a review from thepagent as a code owner April 30, 2026 04:23
@github-actions github-actions Bot added the pending-screening PR awaiting automated screening label Apr 30, 2026
@ChunHao-dev ChunHao-dev changed the title fix(chart): 修正 Helm cronjob 設定格式錯誤 + 補上 usercron 支援 fix(chart): align cronjob config with openab binary + add usercron support Apr 30, 2026
超渡法師 added 2 commits April 30, 2026 05:40
The previous conditional `or ($cfg.cronjobs) ($cfg.cron)` was always
truthy because values.yaml provides a default `cron:` map. Check the
actual feature flags instead: usercronEnabled, usercronPath, or cronjobs.
@thepagent thepagent merged commit fc0f499 into openabdev:main Apr 30, 2026
3 checks passed
@chaodu-agent
Copy link
Copy Markdown
Collaborator

四法師 Review 結論

感謝 @ChunHao-dev 的 PR!方向完全正確,我們交叉比對了 main 上的 config.rs,TOML key 對齊無誤。

✅ 已確認

  • [[cronjobs]][[cron.jobs]] 對齊 Config.cron: CronConfig
  • usercron_enabled / usercron_path 支援完整 ✅
  • helm template 三種 case 驗證通過 ✅

🔴 需修:Go template default 布林陷阱(by 覺渡法師)

# 現在(有 bug):
enabled = {{ .enabled | default true }}

# 修正:
enabled = {{ if hasKey . "enabled" }}{{ .enabled }}{{ else }}true{{ end }}

原因:Go template 的 defaultfalse 視為 zero value,所以 enabled: false 會被吃掉變成 true,使用者無法停用單條 job。

🟡 建議:enabled 註解措辭微調(by 擺渡法師)

validate_cronjobs() 在啟動時會驗證所有 jobs(包含 disabled),所以 enabled: false 的語意是「不排程」而非「完全忽略」。建議註解改為:

#       enabled: true          # false = skip scheduling (job config is still validated at startup)

🟡 建議:conditional render 精確化

# 現在($cfg.cron 因 values.yaml default 永遠 truthy):
{{- if or ($cfg.cronjobs) ($cfg.cron) }}

# 建議:
{{- if or ($cfg.cronjobs) (($cfg.cron).usercronEnabled) (($cfg.cron).usercronPath) }}

法師 Verdict
超渡法師 🟢 修完可 merge
普渡法師 🟡 需修 hasKey
覺渡法師 🟡 需修 hasKey(發現者)
擺渡法師 🟡 註解措辭 + runtime validation gap

Blocking: hasKey fix(1 行)
Non-blocking: conditional 精確化 + 註解措辭 + runtime validate_cronjobs() 跳過 disabled jobs(建議另開 issue)

thepagent pushed a commit that referenced this pull request Apr 30, 2026
validate_cronjobs() now skips jobs with enabled=false, aligning
startup validation with runtime scheduling semantics. Previously a
disabled job with an invalid cron expression or timezone would cause
openab to fail at startup even though the job would never be scheduled.

Discovered during four-monk review of PR #638.

Co-authored-by: 超渡法師 <chaodu@openab.dev>
Co-authored-by: 擺渡法師 <codex@openab.dev>
chaodu-agent pushed a commit that referenced this pull request May 1, 2026
…e default trap

Go template's `default` treats `false` as a zero value, so
`{{ false | default true }}` returns `true`. This made it impossible
to disable cronjobs or reactions via `enabled: false`.

Fix both occurrences:
- reactions.enabled (L104)
- cron.jobs[].enabled (L142)

Also clarify the enabled field comment in values.yaml to reflect that
disabled jobs are still validated at startup.

Discovered during four-monk review of PR #638.
Co-authored-by: 覺渡法師 <gemini@openab.dev>
thepagent pushed a commit that referenced this pull request May 1, 2026
…lt trap (#639)

* fix(chart): use hasKey for boolean enabled fields to avoid Go template default trap

Go template's `default` treats `false` as a zero value, so
`{{ false | default true }}` returns `true`. This made it impossible
to disable cronjobs or reactions via `enabled: false`.

Fix both occurrences:
- reactions.enabled (L104)
- cron.jobs[].enabled (L142)

Also clarify the enabled field comment in values.yaml to reflect that
disabled jobs are still validated at startup.

Discovered during four-monk review of PR #638.
Co-authored-by: 覺渡法師 <gemini@openab.dev>

* fix(chart): apply hasKey to removeAfterReply and usercronEnabled

Extend the hasKey pattern to the remaining '| default false' boolean
fields for consistency. While the current default-false behavior is
correct today, using hasKey prevents a latent trap if defaults are
ever changed to true.

---------

Co-authored-by: 超渡法師 <chaodu@openab.dev>
Co-authored-by: 覺渡法師 <gemini@openab.dev>
Co-authored-by: chaodu-agent <chaodu-agent@openab.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pending-screening PR awaiting automated screening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants