Skip to content

chore(base-cluster): add loki retention value#1774

Merged
SyeKlu merged 1 commit intomainfrom
chore-retention-period-variable
Oct 31, 2025
Merged

chore(base-cluster): add loki retention value#1774
SyeKlu merged 1 commit intomainfrom
chore-retention-period-variable

Conversation

@SyeKlu
Copy link
Copy Markdown
Contributor

@SyeKlu SyeKlu commented Oct 29, 2025

Summary by CodeRabbit

  • New Features
    • Loki log retention period is now configurable via chart values; default remains 45 days.
  • Validation
    • Added schema validation for the retention setting (pattern and examples) to ensure valid duration formats (e.g., 45d, 7d, 24h).

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Oct 29, 2025

Walkthrough

The change parameterizes Loki's retention_period by replacing the hard-coded "45d" in the template with a Helm value, adds monitoring.loki.retention_period to values.yaml, and validates it in values.schema.json.

Changes

Cohort / File(s) Summary
Loki template update
charts/base-cluster/templates/monitoring/logs/loki.yaml
Replaces hard-coded retention_period: 45d with {{ .Values.monitoring.loki.retention_period }}
Chart values schema
charts/base-cluster/values.schema.json
Adds monitoring.loki.retention_period property (string) with pattern `[0-9]+(ms
Default chart values
charts/base-cluster/values.yaml
Introduces monitoring.loki.retention_period: 45d as the default value

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Small, localized changes across three files.
  • Check template substitution, schema pattern correctness, and default alignment.

Possibly related PRs

Suggested reviewers

  • tasches
  • marvinWolff
  • teutonet-bot

Poem

🐰
I hopped through templates, values in hand,
Swapped stubborn "45d" for a flexible strand.
Now retention sings from a tidy key,
Logs kept just as long as you decree. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "chore(base-cluster): add loki retention value" directly aligns with the main objective of the changeset, which is to introduce configurable Loki log retention through Helm templating. The title accurately captures the essence of the changes across all three modified files: adding the schema definition, default value, and template reference for the retention period. It uses the conventional commit format appropriately for a configuration enhancement, is concise and descriptive, and clearly communicates the primary change without unnecessary detail. A teammate reviewing commit history would immediately understand that this PR adds Loki retention configuration capability to the base-cluster.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore-retention-period-variable

📜 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 966b083 and 97ea028.

📒 Files selected for processing (3)
  • charts/base-cluster/templates/monitoring/logs/loki.yaml (1 hunks)
  • charts/base-cluster/values.schema.json (1 hunks)
  • charts/base-cluster/values.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • charts/base-cluster/templates/monitoring/logs/loki.yaml
  • charts/base-cluster/values.schema.json
  • charts/base-cluster/values.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: check licenses
  • GitHub Check: lint helm chart (base-cluster)

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
charts/base-cluster/values.schema.json (1)

924-926: Add pattern validation for consistency.

The retention_period schema lacks format validation, unlike the similar retentionDuration fields for Prometheus (line 630-632) and AlertManager (line 778-781), which both enforce the pattern [0-9]+(ms|s|m|h|d|w|y).

Apply this diff to add pattern validation and improve schema consistency:

  "retention_period": {
-   "type": "string"
+   "type": "string",
+   "pattern": "[0-9]+(ms|s|m|h|d|w|y)",
+   "description": "Loki log retention period (e.g., 45d, 7d, 24h)",
+   "examples": ["45d", "7d", "24h"]
  },
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cfb4bce and 966b083.

📒 Files selected for processing (3)
  • charts/base-cluster/templates/monitoring/logs/loki.yaml (1 hunks)
  • charts/base-cluster/values.schema.json (1 hunks)
  • charts/base-cluster/values.yaml (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: check licenses
  • GitHub Check: lint helm chart (base-cluster)
🔇 Additional comments (2)
charts/base-cluster/values.yaml (1)

313-318: ✓ LGTM

The default value is correctly placed and maintains backwards compatibility with the previously hard-coded 45d retention period.

charts/base-cluster/templates/monitoring/logs/loki.yaml (1)

80-82: ✓ LGTM

The template correctly references the new parameterized retention period value in the Loki limits configuration.

@SyeKlu SyeKlu force-pushed the chore-retention-period-variable branch from 966b083 to 97ea028 Compare October 29, 2025 13:00
@SyeKlu SyeKlu added this pull request to the merge queue Oct 31, 2025
Merged via the queue into main with commit de6b582 Oct 31, 2025
31 of 32 checks passed
@SyeKlu SyeKlu deleted the chore-retention-period-variable branch October 31, 2025 09:29
github-merge-queue Bot pushed a commit that referenced this pull request Oct 31, 2025
🤖 I have created a release *beep* *boop*
---


##
[10.0.2](base-cluster-v10.0.1...base-cluster-v10.0.2)
(2025-10-31)


### Bug Fixes

* **base-cluster:** add missing value to template
([#1775](#1775))
([870e7d1](870e7d1))


### Miscellaneous Chores

* **base-cluster:** add loki retention value
([#1774](#1774))
([de6b582](de6b582))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants