feat!: remove Stack v1 support and all v1 components #294
Conversation
WalkthroughRemoved legacy multi‑deployment and v2 conditional logic plus related types/fields and Caddy assets; unified ledger install/migration flow to a single, version-agnostic path and narrowed embedded reindex assets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Operator
participant Reconciler
participant MigrateJob
participant LedgerDeployment
participant Database
Note over Operator,Reconciler: Reconcile invoked with stack, ledger, version
Operator->>Reconciler: Reconcile(stack, ledger, version)
Reconciler->>Reconciler: compare savedVersion vs currentVersion
alt versions differ
Reconciler->>MigrateJob: create migration job (getUpgradeContainer)
MigrateJob->>Database: run migrations
MigrateJob-->>Reconciler: job result (success/failure)
opt success
Reconciler->>LedgerDeployment: installLedger(...) (single unified path)
end
else versions equal
Reconciler->>LedgerDeployment: installLedger(...) (no v2 branching)
end
LedgerDeployment->>Database: configure and start containers
LedgerDeployment-->>Operator: ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
flemzord
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
api/formance.com/v1beta1/ledger_types.go (1)
20-20: Remove unused import.The
timepackage is imported but not used in the file, causing a compilation error.Apply this diff to fix the compilation error:
-import ( - "time" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +)internal/resources/ledgers/deployments.go (1)
70-71: Fix function signature to use image parameter.The function should receive
imageinstead ofversionto be consistent with other deployment functions and fix the compilation error.Apply this diff to fix the function signature:
func installLedgerStateless(ctx core.Context, stack *v1beta1.Stack, - ledger *v1beta1.Ledger, database *v1beta1.Database, version string) error { + ledger *v1beta1.Ledger, database *v1beta1.Database, image string) error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
config/crd/bases/formance.com_ledgers.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_accounts.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_all.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_transactions.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_volumes.yamlis excluded by!**/*.yaml
📒 Files selected for processing (6)
api/formance.com/v1beta1/ledger_types.go(1 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(0 hunks)internal/resources/ledgers/assets.go(0 hunks)internal/resources/ledgers/assets/Caddyfile.gotpl(0 hunks)internal/resources/ledgers/deployments.go(8 hunks)internal/resources/ledgers/init.go(3 hunks)
💤 Files with no reviewable changes (3)
- internal/resources/ledgers/assets/Caddyfile.gotpl
- internal/resources/ledgers/assets.go
- api/formance.com/v1beta1/zz_generated.deepcopy.go
🧰 Additional context used
🪛 GitHub Actions: Default
api/formance.com/v1beta1/ledger_types.go
[error] 20-20: GolangCI-Lint typecheck error: "time" imported and not used.
🔇 Additional comments (5)
api/formance.com/v1beta1/ledger_types.go (1)
40-41: LGTM!The simplified description accurately reflects the ledger's purpose after removing multiple deployment strategies.
internal/resources/ledgers/init.go (1)
86-106: LGTM!The simplified migration logic correctly handles version changes while maintaining backward compatibility for versions >= v2.2.0-alpha.
internal/resources/ledgers/deployments.go (3)
30-49: LGTM!The simplified deployment logic correctly handles different ledger versions while removing v1-specific code paths.
290-306: LGTM!The consistent removal of environment variable prefixes standardizes the configuration approach across all ledger deployments.
260-285: LGTM!The simplified deployment creation correctly removes v1-specific volume logic, making the code cleaner and more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/09-Configuration reference/02-Custom Resource Definitions.md (1)
744-745: Add an explicit migration note for removed v1-only fieldsThe new description is concise, but existing users upgrading from v1 will likely look here for guidance. A short, in-place note that
deploymentStrategyandlockingfields are no longer supported would prevent silent confusion.The ledger is a stateful application that manages financial transactions and maintains an immutable audit trail. + +> **Important:** Starting with Stack v2, the legacy `deploymentStrategy` +> and `locking` fields have been removed. Manifests that still declare +> these properties will be rejected by the operator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
config/crd/bases/formance.com_ledgers.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_ledgers.formance.com.yamlis excluded by!**/*.yaml
📒 Files selected for processing (3)
api/formance.com/v1beta1/ledger_types.go(1 hunks)docs/09-Configuration reference/02-Custom Resource Definitions.md(1 hunks)internal/resources/ledgers/init.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/formance.com/v1beta1/ledger_types.go
- internal/resources/ledgers/init.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
flemzord
changed the title
- Remove deprecated DeploymentStrategy and Locking properties from Ledger CRD - Remove all v1-specific logic and conditional code paths - Simplify ledger deployment to always use v2 behavior - Remove v1.0.0 reindex assets - Remove unused Caddyfile template for ledger - Update documentation to remove references to deployment strategies - Clean up imports and remove unused dependencies BREAKING CHANGE: This removes support for Ledger v1 deployments. All ledgers will now use v2 behavior by default.
- Simplified the description of the ledger as a stateful application managing financial transactions and maintaining an immutable audit trail. - Removed deprecated properties related to deployment strategies and locking from the Ledger CRD and documentation.
- Changed the embedded asset path to include versioning for reindex YAML files.
BREAKING CHANGE: Remove Stack v1 support and upgrade to version 3.0.0 This is a major version bump due to the removal of Stack v1 support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
flemzord
force-pushed
the
remove-stack-v1-support
branch
from
1b82b3c to
10bbb5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/resources/ledgers/assets.go (1)
7-8: Verify that v2.0.0 reindex assets exist and are populated.The embed directive now exclusively targets
assets/reindex/v2.0.0/*.yaml. Since this is a breaking change removing v1 support, ensure that:
- The directory
internal/resources/ledgers/assets/reindex/v2.0.0/exists and contains the required YAML files- These assets are sufficient for all reindex scenarios
Run the following script to verify the reindex assets exist:
#!/bin/bash # Description: Verify that v2.0.0 reindex assets exist # Check if the directory exists and list its contents fd -t f . internal/resources/ledgers/assets/reindex/v2.0.0/ --exec ls -lh # Count the number of YAML files echo "Number of YAML files:" fd -e yaml . internal/resources/ledgers/assets/reindex/v2.0.0/ | wc -l
🧹 Nitpick comments (2)
internal/resources/ledgers/init.go (1)
85-102: Add logging for migration failures before falling back to install
Before returninginstallLedgeron anApplicationError, emit an Info-level log of the originalmigrateerror (e.g.log.FromContext(ctx).Info("ledger migration failed, falling back to install", "error", err)) to improve observability. Confirm that falling back to installation for all
ApplicationErrorcases is the intended behavior.internal/resources/ledgers/deployments.go (1)
431-460: Ensure safe deletion of deployments and add migration status
- Deleting all ledger Deployments in the PreCreate hook will tear down every live pod until the migration job finishes—confirm this aligns with your availability SLAs or adopt a rolling-orchestrated approach to avoid full downtime.
- The controller never sets a “migration in progress” condition on the Ledger CR; consider adding a Status condition or annotation so users can observe pending migrations.
jobs.Handlecorrectly returns acore.NewPendingError()andinit.gopropagates it to requeue reconciliation until the Job completes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (9)
config/crd/bases/formance.com_ledgers.yamlis excluded by!**/*.yamlhelm/crds/Chart.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_ledgers.formance.com.yamlis excluded by!**/*.yamlhelm/operator/Chart.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_accounts.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_all.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_transactions.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_volumes.yamlis excluded by!**/*.yaml
📒 Files selected for processing (7)
api/formance.com/v1beta1/ledger_types.go(1 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(0 hunks)docs/09-Configuration reference/02-Custom Resource Definitions.md(1 hunks)internal/resources/ledgers/assets.go(1 hunks)internal/resources/ledgers/assets/Caddyfile.gotpl(0 hunks)internal/resources/ledgers/deployments.go(9 hunks)internal/resources/ledgers/init.go(3 hunks)
💤 Files with no reviewable changes (2)
- internal/resources/ledgers/assets/Caddyfile.gotpl
- api/formance.com/v1beta1/zz_generated.deepcopy.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/resources/ledgers/init.go (3)
internal/resources/benthosstreams/create.go (1)
LoadFromFileSystem(19-90)internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)internal/core/errors.go (1)
IsApplicationError(43-45)
internal/resources/ledgers/deployments.go (10)
internal/core/context.go (1)
Context(10-16)internal/core/version.go (1)
IsLower(54-62)internal/core/env.go (2)
Env(11-16)GetDevEnvVarsWithPrefix(75-84)internal/resources/settings/aws_role.go (1)
GetAWSServiceAccount(5-7)internal/resources/settings/opentelemetry.go (1)
GetOTELEnvVarsWithPrefix(23-39)internal/core/module.go (1)
LowerCamelCaseKind(8-14)internal/resources/databases/env.go (1)
PostgresEnvVarsWithPrefix(20-104)internal/resources/brokers/utils.go (2)
GetEnvVarsWithPrefix(20-72)GetPublisherEnvVars(74-92)internal/core/stacks.go (1)
HasDependency(69-80)internal/resources/jobs/job.go (3)
Handle(137-220)PreCreate(31-35)WithServiceAccount(43-48)
🔇 Additional comments (8)
docs/09-Configuration reference/02-Custom Resource Definitions.md (1)
744-745: LGTM! Documentation simplified to match API changes.The updated Ledger description is concise and accurately reflects the removal of deployment strategy complexity. This aligns well with the API changes that removed
DeploymentStrategyandLockingfields.api/formance.com/v1beta1/ledger_types.go (1)
38-39: LGTM! API documentation updated to reflect simplified architecture.The streamlined documentation aligns with the removal of
DeploymentStrategyandLockingfields fromLedgerSpec, making the API cleaner and easier to understand.internal/resources/ledgers/init.go (1)
85-104: Approve simplified migration and installation flow.The removal of version-based branching (isV2) and the direct invocation of
migrate()when versions differ streamlines the ledger initialization logic. The unconditional call toinstallLedgerat line 104 ensures the deployment is always in sync with the desired state.internal/resources/ledgers/deployments.go (5)
30-49: LGTM! Simplified installation flow with version-based branching.The
installLedgerfunction now cleanly branches between:
- v2.2.0+ with stateless + optional worker deployment
- Older versions using single-instance deployment
The removal of the
v2parameter simplifies the API and aligns with the unified v2 architecture.
51-68: LGTM! Single-instance deployment simplified.The function signature is cleaner without the
v2parameter, and the logic now unconditionally usescreateLedgerContainerFullwith the simplified API. The call tosetCommonAPIContainerConfigurationcorrectly passesimageConfiguration(fixing the issue flagged in past reviews).
328-350: Approve simplified environment variable configuration.The removal of the
v2parameter and the NUMARY_ prefix simplifies the environment variable setup. All services now use consistent naming with empty prefixes, which is cleaner and easier to maintain.
383-429: LGTM! Unified broker environment variable handling.The
createLedgerContainerFullfunction now consistently uses empty string prefixes for all environment variables, including:
GetEnvVarsWithPrefix(..., "")at line 403GetPublisherEnvVars(..., "")at line 409This aligns with the removal of v2-specific NUMARY_ prefixes and creates a consistent environment variable naming scheme.
259-270: Validate pre-RC6 upgrade command and RC6+ default migration
- Confirm
buckets upgrade-allis available and correct for all versions < v2.0.0-rc.6 (note the webhooks deployment comments indicate upgrade support beginning in v2.0.0-rc.5).- Confirm that for v2.0.0-rc.6 and later, the default migration container uses the
migratecommand (args := ["migrate"]ininternal/resources/databases/migrate.go).
3 participants
BREAKING CHANGE: This removes support for Ledger v1 deployments. All ledgers will now use v2 behavior by default.