Default storage version migrator to enabled in operator chart#5603
Open
ChrisJBurns wants to merge 2 commits into
Open
Default storage version migrator to enabled in operator chart#5603ChrisJBurns wants to merge 2 commits into
ChrisJBurns wants to merge 2 commits into
Conversation
The StorageVersionMigrator controller shipped opt-in (chart default false) so it could land without functional impact. It has soaked through a release, so make the operator helm chart enable it by default: clusters get their CRD status.storedVersions kept clean automatically, which is the precondition for a future release to drop deprecated API versions without orphaning etcd objects. - Flip operator.features.storageVersionMigrator to true in values.yaml - Update the default-install test to assert the env var renders "true" - Add explicit opt-out (false) coverage to the feature-flags suite - Refresh the chart README, reference docs, and upgrade-guide notes - Sync the now-stale opt-in comments in the operator's env-var gate (the binary still defaults off when the var is unset; the chart sets it) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ChrisJBurns
requested review from
JAORMX,
blkt,
jerm-dro,
jhrozek,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
reyortiz3
previously approved these changes
Jun 23, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5603 +/- ##
==========================================
- Coverage 69.93% 69.92% -0.02%
==========================================
Files 651 651
Lines 66405 66405
==========================================
- Hits 46438 46431 -7
- Misses 16616 16624 +8
+ Partials 3351 3350 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
Author
|
/retest |
2 similar comments
Collaborator
Author
|
/retest |
Collaborator
Author
|
/retest |
The StorageVersionMigrator watches cluster-scoped CustomResourceDefinitions and re-stores custom resources across all namespaces, so it requires cluster-scoped RBAC and a cluster-scoped manager cache. A namespace-scoped operator gets only per-namespace RoleBindings and a namespace-restricted cache, so the controller cannot sync its CRD informer and wedges the manager — which is what broke the multi-tenancy chainsaw E2E once the feature was defaulted on. Add a Helm validation that fails the render when storageVersionMigrator is true under operator.rbac.scope=namespace, with an actionable message. Fail loudly rather than silently dropping the feature: a silent drop would let a namespace-scoped admin believe storedVersions are being kept clean when they are not, leading to the skip-a-version upgrade trap later. - Add validateStorageVersionMigrator helper; invoke it from deployment.yaml - Opt the namespace-scoped multi-tenancy chainsaw setup out of the migrator - Pin the rejection and the supported namespace opt-out in helm unittests; disable the migrator in the namespace-scope suite's shared values - Document the cluster-scope requirement and the standalone-migrator alternative for namespace-scoped clusters Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
lgtm by visual inspection but conflicts |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
StorageVersionMigratorcontroller shipped opt-in (chart defaultfalse) so it could land without functional impact. Now that it has soaked through a release, this flips the operator Helm chart to enable it by default. Clusters then keep each CRD'sstatus.storedVersionstrimmed automatically — the precondition for a future release to drop deprecated API versions (e.g.v1alpha1) without orphaning etcd objects.The operator binary still defaults the feature off when
TOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATORis unset; only the chart default changes. Operators can still opt out with--set operator.features.storageVersionMigrator=false.Medium level
operator.features.storageVersionMigratorflippedfalse→true, with the comment updated to describe the new default and how to opt out.default_install_test.yamlnow asserts the rendered env var is"true";feature_flags_test.yamlgains an explicit opt-out case so the disabled path stays pinned (it was previously covered by the baseline suite).README.md(regenerated viahelm-docs),docs/operator/storage-version-migration.md, and thedocs/operator/upgrade-guide/README.mdwalkthrough note updated to say enabled-by-default.cmd/thv-operator/app/app.go) updated to reflect the binary-vs-chart distinction. No behavior change.Low level
deploy/charts/operator/values.yamlstorageVersionMigratortotrue; reword commentdeploy/charts/operator/README.mdhelm-docsdeploy/charts/operator/tests/default_install_test.yaml"true"deploy/charts/operator/tests/feature_flags_test.yamlfalse) coverage; scopesetper-testdocs/operator/storage-version-migration.mddocs/operator/upgrade-guide/README.mdcmd/thv-operator/app/app.goType of change
Test plan
helm unittest deploy/charts/operatorpasses (59/59)helm lint deploy/charts/operatorpasseshelm templaterendersTOOLHIVE_ENABLE_STORAGE_VERSION_MIGRATOR: "true"by default and"false"with--set ...=falsetask buildpasses (covers theapp.gocomment change)Does this introduce a user-facing change?
Yes. New chart installs/upgrades run the StorageVersionMigrator controller by default. The controller is dormant on CRDs whose
storedVersionsis already clean, so there is no functional impact in the common case. Operators who want the prior behavior can setoperator.features.storageVersionMigrator=false.Special notes for reviewers
The operator binary's default is intentionally unchanged (off when the env var is unset) — only the chart supplies the
truevalue. Theapp.gochange is comment-only to keep the source consistent with the new chart default per the repo's comment-sync convention.Generated with Claude Code