Skip to content

Conversation

@JAORMX
Copy link
Collaborator

@JAORMX JAORMX commented Oct 24, 2025

Problem

Helm chart tests are failing for PRs that only modify the operator chart without bumping the operator-crds chart version (e.g., PR #2176).

Root Cause

By default, ct install only installs charts that have changed in a PR. The operator chart has an implicit dependency on the operator-crds chart:

  1. The operator image contains code that requires CRDs to exist at startup
  2. The operator tries to create field indexes for CRD fields (e.g., MCPServer.Spec.GroupRef in cmd/thv-operator/main.go:82-96)
  3. When a PR only modifies the operator chart, ct install detects only that chart as changed
  4. ct install installs ONLY the operator chart (not operator-crds)
  5. The operator pod crashes immediately with:
    unable to retrieve the complete list of server APIs:
    toolhive.stacklok.dev/v1alpha1: no matches for toolhive.stacklok.dev/v1alpha1
    

Why This Happens

The dependency between operator and operator-crds charts is not declared in Chart.yaml, so chart-testing doesn't know to install both charts together. This is a common pattern when charts need to be independently versioned but have runtime dependencies.

Solution

Configure ct install to use all: true, which tells chart-testing to install all charts in the chart directories regardless of which ones changed. This ensures:

  • CRDs are always installed before the operator
  • Chart tests work for PRs that only modify the operator chart
  • No need to manually bump both chart versions together

Changes

  • Added all: true to ct-install.yaml
  • Added comprehensive inline documentation explaining why this is needed

Testing

This will fix the failing Helm chart tests in PR #2176 and prevent similar issues in the future.

Alternative Solutions Considered

  1. Declare dependency in Chart.yaml - Would require setting up chart repositories and complicates local development
  2. Combine charts into one - Would require significant restructuring
  3. Manual version bumps (current approach) - Error-prone and not obvious to contributors

The all: true approach is the simplest and most maintainable solution for charts with implicit dependencies.


🤖 Generated with Claude Code

This fixes a critical issue where Helm chart tests fail when PRs only
modify the operator chart without bumping the operator-crds chart version.

Problem:
--------
By default, 'ct install' only installs charts that have changed in a PR
(detected by comparing to the target branch). The operator chart depends
on the operator-crds chart, but this dependency is not explicitly declared
in Chart.yaml. When a PR modifies only the operator chart:

1. ct detects only the operator chart as changed
2. ct installs ONLY the operator chart (not operator-crds)
3. The operator pod starts but immediately crashes with:
   "unable to retrieve the complete list of server APIs:
    toolhive.stacklok.dev/v1alpha1: no matches for toolhive.stacklok.dev/v1alpha1"
4. This happens because the operator tries to create field indexes for
   CRD fields at startup (e.g., MCPServer.Spec.GroupRef in main.go:82-96)

Root Cause:
-----------
The operator and operator-crds charts have an implicit dependency:
- The operator image includes code that requires CRDs to exist at startup
- The charts are separate and the dependency is not declared
- chart-testing's default behavior is to only test changed charts

Previous Workaround:
--------------------
PRs had to manually bump both chart versions together (see PR #2284),
but this is error-prone and not obvious to contributors.

Solution:
---------
Set 'all: true' in ct-install.yaml to ensure chart-testing always
installs all charts in the chart-dirs, regardless of which ones changed.
This ensures the CRDs are always present before the operator starts.

This is the recommended approach when charts have dependencies that
aren't explicitly declared in Chart.yaml dependencies.

Alternative Solutions Considered:
----------------------------------
1. Declare dependency in Chart.yaml - Would work but requires managing
   chart repositories and complicates local development
2. Combine charts - Would require significant restructuring
3. Manual version bumps - Current approach, error-prone

Testing:
--------
With this change, PRs that only modify the operator chart (like #2176)
will now pass Helm chart tests because both charts will be installed.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@codecov
Copy link

codecov bot commented Oct 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.09%. Comparing base (bb76806) to head (11cbaae).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2310      +/-   ##
==========================================
+ Coverage   53.61%   54.09%   +0.48%     
==========================================
  Files         239      239              
  Lines       30733    23265    -7468     
==========================================
- Hits        16477    12585    -3892     
+ Misses      13085     9509    -3576     
  Partials     1171     1171              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JAORMX JAORMX requested a review from ChrisJBurns October 24, 2025 08:15
@ChrisJBurns ChrisJBurns merged commit 5b8af18 into main Oct 24, 2025
27 checks passed
@ChrisJBurns ChrisJBurns deleted the fix/ct-install-all-charts branch October 24, 2025 13:21
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.

3 participants