Skip to content

fix(archivebox): align template standards#646

Merged
mberlofa merged 2 commits into
mainfrom
fix/archivebox-template-standards
Jul 6, 2026
Merged

fix(archivebox): align template standards#646
mberlofa merged 2 commits into
mainfrom
fix/archivebox-template-standards

Conversation

@mberlofa

@mberlofa mberlofa commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add the canonical archivebox.validate helper and validate.yaml gate
  • centralize fail-fast validation for backup, ingress hosts, and selector label overrides
  • require both S3 access key and secret key when backup uses inline credentials
  • add helm-unittest coverage for validation failures

Validation

  • make template-standards-check CHART=archivebox
  • helm unittest charts/archivebox
  • helm lint --strict charts/archivebox
  • make standards-check CHART=archivebox
  • make validate-chart CHART=archivebox TIMEOUT=1200: FULLY VALIDATED (13 layers)
  • make site-sync-check CHART=archivebox
  • make release-check REPO=charts
  • make attribution-check REPO=charts

Site PR: helmforgedev/site#326
Issue: #633

Summary by CodeRabbit

  • Bug Fixes
    • Added fail-fast chart validation for common misconfigurations, including enabling ingress without hosts, ingress hosts missing required host values, conflicting pod label overrides, and incomplete/absent backup settings.
    • Strengthened S3 backup credential checks: when not using an existing S3 secret, both S3 access key and secret key must be provided together (with a consolidated error message).
  • Tests
    • Expanded chart validation test coverage for the above failure scenarios, including incomplete S3 backup credentials.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f029f7ed-35af-4d4b-9bef-987e311b363b

📥 Commits

Reviewing files that changed from the base of the PR and between 63ea834 and 9b3851b.

📒 Files selected for processing (1)
  • charts/archivebox/templates/_helpers.tpl
🚧 Files skipped from review as they are similar to previous changes (1)
  • charts/archivebox/templates/_helpers.tpl

📝 Walkthrough

Walkthrough

Adds template-time validation for ingress, podLabels, and backup credentials, wires it into chart rendering, and extends test coverage for the new failure cases.

Changes

Chart value validation

Layer / File(s) Summary
Validation helper and backup credential check
charts/archivebox/templates/_helpers.tpl
Adds archivebox.validate for ingress host and podLabels validation, and tightens archivebox.backupEnabled to require both S3 access and secret keys when no existing secret is set.
Template wiring
charts/archivebox/templates/validate.yaml
Adds SPDX header and includes archivebox.validate with the current context to trigger validation at render time.
Validation and backup tests
charts/archivebox/tests/backup_test.yaml, charts/archivebox/tests/validation_test.yaml
Adds a backup test for incomplete S3 credentials and a Validation suite covering ingress, podLabels, backup failure cases, and the default pass-through case.

Estimated code review effort: 2 (Simple) | ~12 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Helm
  participant validate.yaml
  participant archivebox.validate
  participant archivebox.backupEnabled

  Helm->>validate.yaml: render chart
  validate.yaml->>archivebox.validate: include with context
  archivebox.validate->>archivebox.validate: check ingress.hosts and host values
  archivebox.validate->>archivebox.validate: check podLabels overrides
  archivebox.validate->>archivebox.backupEnabled: include for backup state
  archivebox.backupEnabled-->>archivebox.validate: require accessKey and secretKey when existingSecret is absent
  archivebox.validate-->>Helm: pass or fail render
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: updating the ArchiveBox chart to align with template standards and validation rules.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/archivebox-template-standards

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

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Standards Check (GR-079) — PASS

Every changed chart fully passes standards-check.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

🟢 Security Scan: archivebox

Framework Score
MITRE + NSA + SOC2 75.757576%

✅ Security posture acceptable.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
charts/archivebox/templates/_helpers.tpl (1)

42-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Clarify intentional side-effect of unused $backupEnabled.

The variable is assigned solely to force evaluation of archivebox.backupEnabled for its fail side-effects, but it's never read afterward. A short comment would help avoid future confusion or accidental removal as "dead code."

💬 Suggested clarifying comment
 {{- define "archivebox.validate" -}}
+{{/* Triggers backup.enabled validation failures as a side effect; the return value is unused. */}}
 {{- $backupEnabled := include "archivebox.backupEnabled" . -}}
🤖 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/archivebox/templates/_helpers.tpl` around lines 42 - 60, The unused
$backupEnabled assignment in archivebox.validate is intentional because it
forces archivebox.backupEnabled to run for its fail side-effects. Add a short
clarifying comment near that assignment so future readers do not mistake it for
dead code or remove it accidentally, and keep the reference tied to the
archivebox.validate helper and archivebox.backupEnabled include.
🤖 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.

Nitpick comments:
In `@charts/archivebox/templates/_helpers.tpl`:
- Around line 42-60: The unused $backupEnabled assignment in archivebox.validate
is intentional because it forces archivebox.backupEnabled to run for its fail
side-effects. Add a short clarifying comment near that assignment so future
readers do not mistake it for dead code or remove it accidentally, and keep the
reference tied to the archivebox.validate helper and archivebox.backupEnabled
include.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b176f1e0-6168-4d77-bb20-4cfb4025559d

📥 Commits

Reviewing files that changed from the base of the PR and between 0d8168f and 5c4d634.

📒 Files selected for processing (4)
  • charts/archivebox/templates/_helpers.tpl
  • charts/archivebox/templates/validate.yaml
  • charts/archivebox/tests/backup_test.yaml
  • charts/archivebox/tests/validation_test.yaml

@mberlofa mberlofa force-pushed the fix/archivebox-template-standards branch from 5c4d634 to 63ea834 Compare July 4, 2026 17:59
@mberlofa

mberlofa commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the CodeRabbit review-body nitpick.

Changes:

  • Added a short template comment explaining that the archivebox.backupEnabled include in archivebox.validate intentionally triggers backup validation failures and that the returned value is unused.

Validation:

  • make validate-chart CHART=archivebox passed
  • Result: archivebox: FULLY VALIDATED (13 layers), including all k3d GR-027 scenarios
  • make release-check REPO=charts passed with only the expected post-merge release confirmation warning
  • make attribution-check REPO=charts passed
  • git diff --check passed

Note: this CodeRabbit item was posted in the review summary/body, not as an active review thread, so there is no thread ID to reply to or resolve for it.

@mberlofa mberlofa merged commit 3bbe503 into main Jul 6, 2026
17 checks passed
@mberlofa mberlofa deleted the fix/archivebox-template-standards branch July 6, 2026 08:12
mberlofa added a commit to helmforgedev/site that referenced this pull request Jul 6, 2026
## Summary
- register archivebox in the site sync playground registry
- keep the playground/site sync gate aligned with the chart validation
PR

## Validation
- make site-sync-check CHART=archivebox
- npm run lint
- npm run format:check
- npm run build
- make release-check REPO=site
- make attribution-check REPO=site

Chart PR: helmforgedev/charts#646
Issue: helmforgedev/charts#633

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **New Features**
* Added support for the `archivebox` chart in the playground, enabling
it to be synced and used with the other available chart options.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:templates area:tests chart:archivebox Issues related to the archivebox chart size:M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant