Skip to content

docs(helm): document supported values and add usage examples#280

Merged
thepagent merged 4 commits intoopenabdev:mainfrom
chihkang:docs/helm-values-reference
May 6, 2026
Merged

docs(helm): document supported values and add usage examples#280
thepagent merged 4 commits intoopenabdev:mainfrom
chihkang:docs/helm-values-reference

Conversation

@chihkang
Copy link
Copy Markdown
Contributor

Summary

This PR improves the Helm chart documentation by adding several supported but currently undocumented values to the values reference and README.

Users can already rely on these options in practice, but today they are difficult to discover unless they inspect the chart templates directly. This change makes the chart easier to use and reduces trial-and-error during deployment.

Closes #163.

Changes

  • add fullnameOverride to values.yaml
  • add nameOverride to values.yaml
  • document envFrom in values.yaml comments and chart docs
  • document agentsMd usage with --set-file
  • document the discord.allowedChannels --set-string requirement more prominently
  • add a chart-level Helm values reference with practical examples
  • link the main README Helm install section to the chart values reference

Why

The chart already supports these configuration paths, but they are not clearly documented. That creates unnecessary friction for users, especially for:

  • multi-instance deployments
  • secret-based environment injection
  • large AGENTS.md configurations
  • Discord channel ID configuration

Notes

This PR is documentation-focused and does not change chart behavior. It only makes existing supported options easier to discover and use.

Testing

  • reviewed chart templates to confirm the documented values are already supported
  • documentation change only; helm CLI was not available in the local environment for helm lint

@chihkang chihkang requested a review from thepagent as a code owner April 13, 2026 07:29
Copy link
Copy Markdown
Collaborator

@chaodu-agent chaodu-agent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Needs Update — Good intent, but stale + merge conflicts + incomplete coverage.

Baseline Check (Step 0)
Field Value
State OPEN
Mergeable CONFLICTING
Created 2026-04-13 (18 days ago)
Changes +78 / -0, docs only
Labels p3, closing-soon

Main branch status: No charts/openab/README.md exists — PR creates it fresh. ✅ However, values.yaml has changed significantly since April 13 with ~19 feature commits touching the chart.

Four-Question Framework

1. What problem does it solve?
The Helm chart supports many values only discoverable by reading templates directly. This PR adds a chart-level README with values reference table and practical usage examples.

2. How does it solve it?

  • Creates charts/openab/README.md with values table + 4 usage examples
  • Adds top-level nameOverride and fullnameOverride defaults to values.yaml
  • Adds inline comments for envFrom and agentsMd

3. What was considered?
Documentation-only, no behavior change. Contributor reviewed templates to confirm documented values are supported.

4. Is it the best approach?
Sound approach — chart-level README is standard Helm convention. But execution has gaps due to staleness.

Traffic Light

🟢 INFO

  • Correct identification of undocumented gap (nameOverride/fullnameOverride missing from values.yaml)
  • Practical examples (envFrom, --set-file agentsMd, Discord ID precision warning)
  • Standard Helm convention for chart docs

🟡 NIT

  1. Values table very incomplete — Documents only ~8 values. Actual chart has 27+ agent-level keys including slack.*, gateway.*, reactions.*, stt.*, cron.*, persistence.*, pool.*, extraInitContainers, extraContainers, allowBotMessages, trustedBotIds, allowDm, maxBotTurns, etc.
  2. Consider helm-docs for auto-generation from values.yaml comments.

🔴 SUGGESTED CHANGES

  1. Merge conflict must be resolvedvalues.yaml has changed significantly since April 13.
  2. Stale documentation risk — Since PR opened, main gained: gateway templates, maxBotTurns, allowDm, reactions.toolDisplay, cron/cronjobs, extraContainers, Slack adapter config, allowBotMessages/trustedBotIds. The "values reference" title is misleading at ~15% coverage.
  3. closing-soon label — Missing Discord Discussion URL. Will auto-close in 3 days unless addressed.

Reviewed by 超渡法師 🔃 chaodu Backlog triage

@chaodu-agent
Copy link
Copy Markdown
Collaborator

超渡法師 Review — PR #280

1. What problem does it solve?

The Helm chart supports several useful values (fullnameOverride, nameOverride, envFrom, agentsMd, --set-string for Discord IDs) but none were documented in a discoverable way. Users had to read _helpers.tpl and template source to find them.

2. How does it solve it?

  • New charts/openab/README.md — 66-line values reference with tables and three practical examples (name override, envFrom for secrets, --set-file for AGENTS.md, Discord ID precision warning).
  • charts/openab/values.yaml — adds nameOverride: "" and fullnameOverride: "" as top-level defaults (consumed by _helpers.tpl but never declared).
  • README.md — cross-reference linking to the chart README from Quick Start.

3. What was considered?

Docs-only, no behavior changes. Contributor reviewed chart templates to confirm all documented values are already functional.

4. Is this the best approach?

Clean, well-scoped docs PR that directly addresses every item in issue #163. Surfacing existing values in values.yaml defaults is standard Helm convention.


Traffic Light

🟢 INFO — Well done

  • nameOverride/fullnameOverride added to values.yaml — verified that _helpers.tpl on main already consumes both. helm show values will now surface them.
  • The --set-string precision warning for Discord IDs is a real footgun; documenting it prominently is valuable.
  • envFrom example correctly shows both secretRef and configMapRef patterns.
  • Cross-reference from root README to chart README is a nice touch.

🟡 NIT

  1. Values table is incomplete — The table omits commonly-used agent values already in values.yaml: pool, reactions, persistence, stt, gateway, slack, allowBotMessages, trustedBotIds. The table doesn't need to be exhaustive, but a note like "For the full list, run helm show values openab/openab" would set expectations.
  2. Chart-level vs agent-level nameOverride — The existing commented-out agent blocks already have a per-agent nameOverride. A one-line comment like # Chart-level name override (for per-agent override, see agents.<name>.nameOverride) would prevent confusion.
  3. allowedUsers default — Shown as [] with description "Empty means allow all users." Consider making this clearer: "Empty = allow all (default)".

Verdict

🟢 Looks good — Clean docs-only PR. The NITs above are minor improvements. No blocking issues.

@shaun-agent
Copy link
Copy Markdown
Contributor

OpenAB PR Screening

This is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Click 👍 if you find this useful. Human review will be done within 24 hours. We appreciate your support and contribution 🙏

Screening report ## Intent

PR #280 is trying to make the OpenAB Helm chart easier to deploy by documenting supported configuration values that already work but are currently hard to discover.

The operator-visible problem is deployment friction: users need to inspect chart templates or guess Helm syntax for common cases like name overrides, secret-based env injection, large AGENTS.md configs, and Discord channel IDs.

Feat

This is a documentation improvement with a small chart values-reference cleanup.

Behavioral change: no runtime behavior is intended to change. The PR adds or documents Helm values and usage examples:

  • fullnameOverride
  • nameOverride
  • envFrom
  • agentsMd with --set-file
  • discord.allowedChannels with --set-string
  • chart-level Helm values reference
  • README link to the chart documentation

Who It Serves

Primary beneficiaries: deployers and maintainers operating OpenAB through Helm.

Secondary beneficiaries: reviewers and support maintainers, because clearer deployment docs reduce repeated clarification around supported chart values and Helm CLI edge cases.

Rewritten Prompt

Review and merge a documentation-focused Helm chart update for OpenAB.

Confirm that every documented value is already supported by the existing chart templates, especially fullnameOverride, nameOverride, envFrom, agentsMd, and discord.allowedChannels. Ensure examples use correct Helm syntax, including --set-file for large AGENTS.md content and --set-string for Discord channel IDs. Verify the new chart README is linked from the main README and that no chart behavior changes are introduced beyond documenting existing supported values.

If Helm is available, run helm lint charts/openab; otherwise, perform template-level review and note that Helm CLI validation remains a follow-up.

Merge Pitch

This PR is worth advancing because it improves deployability without changing runtime behavior. The risk profile is low: the affected files are documentation and values comments, with the only practical risk being inaccurate documentation if a listed value is not actually supported by the chart.

Likely reviewer concern: whether the new examples are guaranteed to match current chart behavior and whether adding nameOverride / fullnameOverride to values.yaml could be interpreted as a behavior change rather than surfacing existing Helm conventions.

Best-Practice Comparison

OpenClaw principles relevant here:

  • Explicit delivery routing is indirectly relevant: documenting discord.allowedChannels helps operators route bot behavior to the intended Discord channels.
  • Durable job persistence, isolated executions, retry/backoff, run logs, and gateway-owned scheduling are not directly relevant because this PR is Helm documentation, not runtime scheduling or execution behavior.

Hermes Agent principles relevant here:

  • Self-contained prompts for scheduled tasks are loosely analogous to documenting agentsMd clearly, especially the --set-file pattern for larger configuration.
  • Atomic writes, file locking, daemon tick models, and fresh sessions per scheduled run are not relevant to this PR.

The best-practice takeaway is that operational systems should make routing and configuration explicit. This PR fits that principle by turning implicit chart support into visible deployment guidance.

Implementation Options

Option 1: Conservative documentation-only merge
Accept the PR if template review confirms the documented values already exist. Optionally ask the author to add a note that no behavior changes are intended.

Option 2: Balanced docs plus validation
Merge the documentation, but also run or request helm lint charts/openab and possibly helm template examples for the documented paths. This gives reviewers stronger confidence without expanding scope much.

Option 3: Ambitious Helm documentation hardening
Use this PR as the start of a fuller Helm chart docs pass: generated values reference, tested example commands, CI linting for chart changes, and sample values files for common deployment modes such as Discord-only, Slack-only, and multi-instance installs.

Comparison Table

Option Speed to ship Complexity Reliability Maintainability User impact Fit for OpenAB right now
Conservative documentation-only merge High Low Medium Medium Medium Good
Balanced docs plus validation Medium Low-Medium High High Medium-High Best
Ambitious Helm docs hardening Low Medium-High High High High Good later, too large for this PR

Recommendation

Advance with Option 2: merge the documentation after validating that the documented values are already supported by the chart and that the examples render correctly.

This keeps the PR appropriately scoped while addressing the main reviewer risk: documentation accuracy. If Helm is unavailable in the review environment, Masami or Pahud should either run helm lint charts/openab locally or leave a clear follow-up asking for chart linting in CI. Broader Helm docs automation should be split into a separate follow-up rather than added to this PR.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

⚠️ This PR is missing a Discord Discussion URL in the body.

All PRs must reference a prior Discord discussion to ensure community alignment before implementation.

Please edit the PR description to include a link like:

Discord Discussion URL: https://discord.com/channels/...

This PR will be automatically closed in 3 days if the link is not added.

@chihkang
Copy link
Copy Markdown
Contributor Author

chihkang commented May 4, 2026

Updated this PR and pushed a refreshed branch (8c51547).

What changed:

  • Merged current main into docs/helm-values-reference and resolved the charts/openab/values.yaml conflict.
  • Kept the docs-focused scope while refreshing charts/openab/README.md against the current chart values.
  • Renamed the chart docs section from Values Reference to Common Values and added a pointer to helm show values openab/openab / values.yaml for the full option list.
  • Clarified chart-level vs per-agent nameOverride and the default meaning of discord.allowedUsers.
  • Covered the newly added value families called out in review, including Slack, gateway, reactions, STT, cron, persistence, and extra container/volume hooks.

Local validation with Helm v3.18.5:

  • helm lint charts/openab passed
  • helm template openab charts/openab passed
  • Smoke render with fullnameOverride, envFrom, --set-file agentsMd, and --set-string Discord channel ID passed
  • helm unittest charts/openab passed: 7 suites, 74 tests

GitHub now shows the PR Discussion URL check passing. I do not have permission to remove the remaining closing-soon label, so that may need maintainer/bot cleanup.

@masami-agent
Copy link
Copy Markdown
Contributor

PR Review: #280

Based on commit: 8c51547

Summary

  • Problem: Helm chart values (fullnameOverride, nameOverride, envFrom, agentsMd) are supported but undocumented, causing friction for users.
  • Approach: Add a chart-level README.md with a values reference table and usage examples, add top-level nameOverride/fullnameOverride to values.yaml, and link from the main README.
  • Risk level: Low (documentation-only, no behavior change)

Core Assessment

  1. Problem clearly stated: ✅ — Closes docs: add Helm values reference with undocumented options #163, clear motivation
  2. Approach appropriate: ✅ — Standard Helm chart documentation pattern
  3. Alternatives considered: N/A — straightforward docs addition
  4. Best approach for now: ✅ — Makes existing features discoverable without changing behavior

Findings

Review Summary (Traffic Light)

🟢 INFO

  • The documented values (fullnameOverride, nameOverride, envFrom, agentsMd) are confirmed to exist in _helpers.tpl and chart templates — documentation is accurate.
  • CI passes.
  • Commit messages follow Conventional Commits (docs(helm): ...).
  • The charts/openab/README.md is well-structured with a clear table format and practical examples.
  • Good call documenting the --set-string requirement for Discord IDs — this is a common footgun.

🟡 NIT

  1. Merge commit in history — The third commit (Merge main and refresh Helm docs) is a merge commit rather than a rebase. This is fine since the project uses squash merge, but for future PRs a rebase would keep the branch history linear and easier to review.

  2. charts/openab/README.md line 64 — The example uses --set agents.kiro.discord.botToken=... (without --set-literal). Since bot tokens contain . characters, this can cause Helm to interpret them as nested keys. Consider noting --set-literal as the safer option for bot tokens, or at minimum adding a warning similar to the Discord ID precision warning at the bottom.

  3. closing-soon label — This PR has a closing-soon label indicating it's missing a Discord Discussion URL. This should be resolved before merge to prevent auto-close.

🔴 SUGGESTED CHANGES

  • None. This is a clean, well-scoped documentation PR.

Verdict

APPROVE — This is a solid documentation improvement. The values are confirmed to exist in the chart templates, the examples are practical, and the scope is clean (docs only). Ready for maintainer review.

@masami-agent
Copy link
Copy Markdown
Contributor

Follow-up: Additional Findings

After a closer look, here are additional items that should be addressed:


🟡 Minor: Use --set-literal for bot tokens in examples

Where: charts/openab/README.md lines 64, 82, 87
What: All three examples use --set agents.kiro.discord.botToken="$DISCORD_BOT_TOKEN"
Why it matters: Bot tokens contain . characters. Helm interprets . as a nested key separator with --set, which silently corrupts the token and causes Discord authentication failure. This is a known footgun.
Fix: Replace --set with --set-literal for all botToken fields:

--set-literal agents.kiro.discord.botToken="$DISCORD_BOT_TOKEN"

🟡 Minor: stt section is incomplete — missing required sub-fields

Where: charts/openab/README.md Agent values table
What: Only stt.enabled is documented, but values.yaml also defines stt.apiKey, stt.model, and stt.baseUrl.
Why it matters: A user who sees stt.enabled: true without knowing about apiKey will get a runtime failure. This contradicts the PR's goal of reducing trial-and-error.
Fix: Add the missing STT fields to the table:
| stt.apiKey | API key for the speech-to-text provider. | "" |
| stt.model | STT model name. | "whisper-large-v3-turbo" |
| stt.baseUrl | STT API base URL. | "https://api.groq.com/openai/v1" |


🟡 Minor: reactions.removeAfterReply not documented

Where: charts/openab/README.md Agent values table
What: reactions.enabled and reactions.toolDisplay are listed, but reactions.removeAfterReply is missing.
Why it matters: Completeness — if you document a section, document all its user-facing fields.
Fix: Add:
| reactions.removeAfterReply | Remove status reactions after the agent replies. | false |


🟡 Minor: closing-soon label needs resolution

Where: PR metadata
What: The PR has a closing-soon label indicating a missing Discord Discussion URL.
Why it matters: The PR may be auto-closed if this is not resolved.
Fix: Add the required Discord Discussion URL or request label removal.


Summary

# Severity Item Blocking?
1 🟡 --set-literal for botToken Yes (NIT must be resolved)
2 🟡 Missing stt.* sub-fields Yes
3 🟡 Missing reactions.removeAfterReply Yes
4 🟡 closing-soon label Yes (process)

All items are minor but need to be addressed before merge. Happy to re-review once updated!

@masami-agent
Copy link
Copy Markdown
Contributor

I pushed a fix in commit abc7672 to address the remaining NITs:

  1. --set-literal for botToken — Updated both examples (lines 64 and 87) to use --set-literal instead of --set.
  2. Missing stt.* sub-fields — Added stt.apiKey, stt.model, and stt.baseUrl to the Agent values table.
  3. Missing reactions.removeAfterReply — Added between reactions.enabled and reactions.toolDisplay.

The closing-soon label still needs maintainer removal (contributor confirmed the Discussion URL check is now passing).

All review items are now resolved. Ready for maintainer review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p3 Low — nice to have

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs: add Helm values reference with undocumented options

6 participants