Skip to content

feat(version): warn when project charts drift from the talm binary version#214

Closed
myasnikovdaniil wants to merge 1 commit into
mainfrom
feat/chart-version-check
Closed

feat(version): warn when project charts drift from the talm binary version#214
myasnikovdaniil wants to merge 1 commit into
mainfrom
feat/chart-version-check

Conversation

@myasnikovdaniil

@myasnikovdaniil myasnikovdaniil commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

What

talm vendors its preset and library charts into the project directory at
talm init time. Render commands (template/apply/upgrade) load that
local copy — never the binary's embedded charts. So upgrading the talm
binary leaves charts/talm/ frozen at whatever version last ran init, and
that drift was invisible: operators had to manually eyeball talm version
against Chart.yaml.

This PR surfaces the drift.

How

  • Compare the binary version against the version: stamped in the project's
    Chart.yaml on every config-loading command.
  • Default: non-fatal WARN: on stderr pointing at talm init --update.
    Rendered output (stdout) and exit code are unchanged — pipelines/CI keep working.
  • Opt-in enforcement: strictVersion: true in Chart.yaml (committed, so a
    whole team/CI inherits it) or the --strict-version flag turns a mismatch
    into a hard error (exit 1).
  • Stays silent for dev builds, the 0.1.0 dev sentinel, empty/unparseable
    versions, and matching versions — never a false alarm.

Reuses the existing version: stamp written by init/init --update; no new
required field. strictVersion is optional and absent means today's behavior —
fully backward-compatible.

Docs

  • New README section "How talm vendors charts (keeping a project in sync)"
    explaining the embed→vendor→render model and the init --update workflow.
  • Extended manual-test-plan B5 with the warning + strict-mode assertions.

Testing

  • go test ./... green; go vet ./... clean.
  • New unit tests (version_check_test.go, 9 cases) cover every warn/skip path.
  • Verified end-to-end with real -ldflags builds: warn / strict-via-Chart.yaml
    / strict-via-flag → exit 1, dev-build → silent, matched → silent.

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features

    • Added chart version drift detection that warns when the talm binary and vendored chart versions diverge
    • Added --strict-version flag and configuration option to enforce strict version matching with hard errors
  • Documentation

    • Documented chart vendoring behavior and procedures for keeping projects synchronized after talm binary updates
    • Added testing documentation for drift warning and strict mode functionality

…rsion

talm vendors its preset and library charts into the project at `talm init`
time; render commands (template/apply/upgrade) read that local copy, never
the binary's embedded charts. Upgrading the talm binary therefore leaves
charts/talm/ frozen at the version that last ran init, and the drift was
invisible — operators had to eyeball `talm version` against Chart.yaml.

Compare the binary version against the `version:` stamped in the project's
Chart.yaml on every config-loading command. On mismatch, warn on stderr by
default (output and exit code unchanged) and point at `talm init --update`.
Teams can opt into enforcement via `strictVersion: true` in Chart.yaml or
the `--strict-version` flag, turning drift into a hard error.

The check stays silent for dev builds, the 0.1.0 dev sentinel, empty or
unparseable versions, and matching versions, so it never raises a false
alarm. The strictVersion field is optional; absent means today's behavior.

Documents the vendoring model in the README and extends manual-test-plan B5.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR introduces chart drift detection for talm. When a binary version differs from a project's vendored Chart.yaml version, the tool now emits a non-fatal warning to stderr. Enabling strict mode via Chart.yaml strictVersion: true or the --strict-version flag converts mismatches into fatal errors with a hint to run talm init --update.

Changes

Chart Drift Detection

Layer / File(s) Summary
Version check logic and configuration model
pkg/commands/root.go, pkg/commands/version_check.go, pkg/commands/version_check_test.go
Config struct gains ChartVersion and StrictVersion fields mapped from Chart.yaml. CheckChartVersion compares semver versions, suppressing output for dev/unstamped builds and returning a mismatch boolean with descriptive message only when both sides are real releases that differ. Test table validates version equality, binary newer/older scenarios, prefix normalization, and dev/empty-version skip logic.
CLI flag and version check wiring
main.go
New --strict-version persistent flag backed by strictVersionFlag variable. PersistentPreRunE now calls CheckChartVersion after loading Chart.yaml, emitting a warning to stderr on mismatch by default or failing with a talm init --update hint when strict mode is active.
Feature documentation
README.md, docs/manual-test-plan.md
README explains that talm init vendors charts, binary upgrades do not auto-update vendored copies, and introduces talm init --update re-vendoring flow with optional strict version enforcement. Manual test plan documents release-only drift warning behavior (stderr-only, exit code unchanged) and strict mode converting mismatches to fatal errors.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

kind/feature, area/commands, area/docs

🐰 A drift has been caught with care,
Charts vendored true, versions to bear,
Strict warnings guide the operator's way,
Keep sync'd and happy, day after day! 🌱

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely summarizes the main feature: adding a version drift warning between the talm binary and project charts.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/chart-version-check

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a mechanism to detect and handle drift between the talm binary version and the project's vendored chart version. It adds a drift warning on stderr when they differ, and a strict mode (via the --strict-version flag or strictVersion: true in Chart.yaml) that turns this warning into a hard error. The implementation includes documentation, CLI flag integration, version comparison logic using semver, and unit tests. There are no review comments to address.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@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 (2)
docs/manual-test-plan.md (1)

225-238: 💤 Low value

Optional: Add language hint to fenced code block at line 231.

The test plan additions are comprehensive and clearly document the expected behavior. The fenced code block showing the warning message (line 231-233) could optionally use ```text for consistent markdown style, matching the static analysis hint.

🤖 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 `@docs/manual-test-plan.md` around lines 225 - 238, In the "B5. Render with
stale chart preset" section update the fenced code block that shows the warning
message so it includes a language hint (e.g., change ``` to ```text) to match
the repo's markdown style; locate the block under the "Drift warning." paragraph
in that section (the fenced block that contains the WARN: talm binary...
message) and add the language token.
README.md (1)

229-268: 💤 Low value

Consider adding language hints to fenced code blocks.

The documentation is clear and comprehensive. As a minor style improvement, consider adding language specifiers to the fenced code blocks:

  • Line 233: Directory tree could use ```text
  • Line 255: Warning message could use ```text

This improves rendering in some markdown viewers. As per coding guidelines, this is flagged by markdownlint but is optional.

🤖 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 `@README.md` around lines 229 - 268, Add language hints to the two fenced code
blocks in the README: the directory tree block (the block showing
"your-project/... templates/ charts/talm/") and the warning message block (the
block starting with "WARN: talm binary is ..."); update their opening backticks
to include a language specifier such as ```text so markdown linters/renderers
treat them as plain text (leave other blocks like ```bash and ```yaml
unchanged).
🤖 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 `@docs/manual-test-plan.md`:
- Around line 225-238: In the "B5. Render with stale chart preset" section
update the fenced code block that shows the warning message so it includes a
language hint (e.g., change ``` to ```text) to match the repo's markdown style;
locate the block under the "Drift warning." paragraph in that section (the
fenced block that contains the WARN: talm binary... message) and add the
language token.

In `@README.md`:
- Around line 229-268: Add language hints to the two fenced code blocks in the
README: the directory tree block (the block showing "your-project/... templates/
charts/talm/") and the warning message block (the block starting with "WARN:
talm binary is ..."); update their opening backticks to include a language
specifier such as ```text so markdown linters/renderers treat them as plain text
(leave other blocks like ```bash and ```yaml unchanged).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 62fa366b-b263-4f32-8e4b-95dbeef4bb13

📥 Commits

Reviewing files that changed from the base of the PR and between 62f344c and 34d6912.

📒 Files selected for processing (6)
  • README.md
  • docs/manual-test-plan.md
  • main.go
  • pkg/commands/root.go
  • pkg/commands/version_check.go
  • pkg/commands/version_check_test.go

@lexfrei

Copy link
Copy Markdown
Contributor

Thanks for digging into this — the underlying problem is real: talm renders from the project's vendored charts/talm/, so upgrading the binary leaves that copy frozen and the drift is invisible.

I'm closing this in favor of a different implementation, because comparing the binary version against the project's top-level Chart.yaml version: has two issues:

  1. Wrong file. What actually drifts is the vendored library (charts/talm/), not the project's own chart version — which an operator may bump on their own cadence, making the warning fire on every command indefinitely.
  2. Version number, not content. The library version: is stamped with the binary version at init regardless of whether any chart file changed, so a patch bump that left charts/talm/ byte-identical would still warn — a false positive.

The replacement detects drift by comparing the vendored charts/talm/ against the binary's embedded copy by content (normalizing the version stamp away), so a pure version bump is never flagged. It keeps the UX you proposed: a non-fatal stderr warning by default, opt-in strict mode via Chart.yaml and a --strict-charts flag. Replacement: #216.

Appreciate the contribution — it surfaced a genuine gap.

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.

2 participants