Skip to content

Conversation

lodinis
Copy link
Contributor

@lodinis lodinis commented Sep 17, 2025

Fixes #1433.

Description

Enabling logstream currently triggers the logstream-job template, but it incorrectly iterates over .Values.parseable.s3ModeSecret instead of .Values.parseable.s3ModeSecret.secrets.
This causes the template to fail when trying to access keys because they are nested under the secrets key.

The fix updates the helm/templates/logstream-job.yaml file to correctly iterate over .Values.parseable.s3ModeSecret.secrets.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed handling of S3 mode secrets in the Helm chart for the logstream job, ensuring secrets defined as an array under the S3 configuration are correctly read and injected as environment variables in non-local deployments.
    • Improves reliability of environment variable population from secrets. Users may need to align values to the updated array structure if using custom configurations.

Copy link
Contributor

coderabbitai bot commented Sep 17, 2025

Walkthrough

Adjusted Helm template for logstream job to iterate over .Values.parseable.s3ModeSecret.secrets instead of .Values.parseable.s3ModeSecret in the non-local branch. The inner loop over each secret’s keys and environment variable construction remains unchanged.

Changes

Cohort / File(s) Summary
Helm template loop source fix
helm/templates/logstream-job.yaml
Updated range to use .Values.parseable.s3ModeSecret.secrets for iterating S3 mode secrets; kept inner key loop and env var mapping unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Helm as Helm CLI
  participant Chart as parseable Helm Chart
  participant Tpl as logstream-job.yaml
  participant Values as .Values.parseable.s3ModeSecret.secrets
  participant K8s as Kubernetes

  User->>Helm: helm install/upgrade with values
  Helm->>Chart: Render templates
  Chart->>Tpl: Process non-local branch
  Tpl->>Values: range over secrets[]
  loop For each secret in secrets[]
    Tpl->>Tpl: range over secret.keys
    Tpl->>Tpl: emit env vars from key/value
  end
  Tpl->>Helm: Rendered Job manifest
  Helm->>K8s: Apply manifest
  K8s-->>User: Job created/updated
  note over Tpl,Values: Changed data source: s3ModeSecret.secrets
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I nibbled the charts with careful delight,
Found secrets nested just out of sight—
A hop to “secrets” fixed the trail,
Now keys align and upgrades sail.
Thump-thump! The job spins up anew,
Carrots for logs, and YAML that’s true. 🥕✨

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 48f97e7 and 7e44cf1.

📒 Files selected for processing (1)
  • helm/templates/logstream-job.yaml (1 hunks)
🔇 Additional comments (1)
helm/templates/logstream-job.yaml (1)

50-60: Fix confirmed — apply across templates and add required guard

Fix is correct: iterate over .Values.parseable.s3ModeSecret.secrets and add a required guard before the range in every occurrence.

Files to change:

  • helm/templates/logstream-job.yaml
  • helm/templates/standalone-deployment.yaml
  • helm/templates/querier-statefulset.yaml
  • helm/templates/ingestor-statefulset.yaml

Apply this guard (unchanged) before each range:

-            {{- range $secret := .Values.parseable.s3ModeSecret.secrets }}
+            {{- $secrets := required "parseable.s3ModeSecret.secrets must be set when parseable.logstream is enabled and local=false" .Values.parseable.s3ModeSecret.secrets }}
+            {{- range $secret := $secrets }}

I could not find parseable.s3ModeSecret.secrets defined in the values files — ensure values.yaml (or values*.yaml) includes the secrets array or rely on the guard to produce a clear error.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Pre-merge checks

✅ Passed checks (5 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Add missing key 'secret' to logstream-job helm template." is concise and clearly related to the change (fixing the logstream-job Helm template), but it uses the singular "secret" while the patch targets the nested "secrets" key, which is a minor inaccuracy that could confuse reviewers.
Linked Issues Check ✅ Passed The change adjusts the Helm template to iterate over the nested "secrets" field, which directly addresses the template-evaluation error described in the linked issue [#1433], so the code-level objective from that issue is satisfied.
Out of Scope Changes Check ✅ Passed The provided summary indicates only helm/templates/logstream-job.yaml was modified and the edit is directly related to fixing the secrets iteration, so there are no apparent out-of-scope or unrelated changes in this PR.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Description Check ✅ Passed The PR description includes the required "Fixes #1433" reference, a Description that clearly states the bug (iterating over the wrong Helm value), the chosen fix (iterate over .Values.parseable.s3ModeSecret.secrets), and the file changed (helm/templates/logstream-job.yaml), so reviewers can understand intent and scope. The write-up is concise and actionable for code review. It does not, however, document alternative solutions considered and the repository checklist items (testing, comments, documentation) are left unfilled.

Copy link
Contributor

github-actions bot commented Sep 17, 2025

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@lodinis
Copy link
Contributor Author

lodinis commented Sep 17, 2025

I have read the CLA Document and I hereby sign the CLA

nitisht added a commit to parseablehq/.github that referenced this pull request Sep 17, 2025
@nitisht
Copy link
Member

nitisht commented Sep 17, 2025

Thanks for the PR @lodinis we'll take a look and merge this today.

@nitisht nitisht merged commit 5edecaf into parseablehq:main Sep 18, 2025
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Helm template bug: missing 'secrets' key in s3ModeSecret range

2 participants