feat: Add new manifests for Operator #334
Conversation
WalkthroughAdds a VersionManifest CRD and a manifest-driven deployment system: a loader, engine, docs, and tests. Replaces many legacy per-component reconcilers and deployment builders with manifest-based reconcilers, removes large legacy deployment logic, initializes the manifest loader in main, and adds test/bootstrap helpers and multiple unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Reconciler
participant Loader as manifests.Loader
participant Engine as manifests.Engine
participant K8s as Kubernetes API
participant DB as Database Operator
participant Registry as Image Registry
Note over Reconciler,Loader: manifest resolution & inheritance
Reconciler->>Loader: Load(component, version)
Loader->>K8s: List VersionManifest objects (labels)
Loader-->>Reconciler: Resolved manifest (merged, cached)
Reconciler->>DB: Ensure/Create Database
DB-->>Reconciler: Ready or Pending
alt migration enabled & version mismatch
Reconciler->>Registry: Resolve image for component@version
Reconciler->>DB: Run migration (strategy-aware)
DB-->>Reconciler: Migration result
Reconciler->>DB: Save module version
end
Reconciler->>Engine: Apply(manifest, ManifestContext)
Engine->>K8s: Create/Update Deployments/Services/Secrets
K8s-->>Reconciler: Resources applied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (20)
internal/resources/registries/image_test.go (1)
174-204: Consider adding basic format validation.This documentation test ensures registry format examples exist but doesn't validate they are well-formed. Consider adding simple checks like verifying each example contains a registry domain and tag separator (
:) to catch typos or malformed examples.Example enhancement:
for _, format := range registryFormats { t.Run(format.description, func(t *testing.T) { t.Parallel() - // This test just documents the formats, no actual assertions needed require.NotEmpty(t, format.example) + require.Contains(t, format.example, ":", "image URL should contain tag separator") + require.NotContains(t, format.example, " ", "image URL should not contain spaces") }) }internal/resources/searches/helpers.go (1)
9-29: Logic looks correct; consider caching the module kind.The consumer creation flow is well-structured: dependency check → create → verify readiness. One minor optimization opportunity exists.
Line 18 calls
LowerCamelCaseKind(ctx, o)twice for the same module. Consider caching the result:for _, o := range []v1beta1.Module{ &v1beta1.Payments{}, &v1beta1.Ledger{}, &v1beta1.Gateway{}, } { if ok, err := HasDependency(ctx, search.Spec.Stack, o); err != nil { return err } else if ok { - consumer, err := brokerconsumers.Create(ctx, search, LowerCamelCaseKind(ctx, o), LowerCamelCaseKind(ctx, o)) + kind := LowerCamelCaseKind(ctx, o) + consumer, err := brokerconsumers.Create(ctx, search, kind, kind) if err != nil { return err }internal/core/names_test.go (2)
116-117: Remove redundant field assertions.Lines 116-117 redundantly check individual fields after line 115 already verified struct equality with
require.Equal(t, tt.expected, result).Apply this diff to streamline the test:
result := GetNamespacedResourceName(tt.namespace, tt.resName) require.Equal(t, tt.expected, result) - require.Equal(t, tt.expected.Namespace, result.Namespace) - require.Equal(t, tt.expected.Name, result.Name)
159-161: Remove redundant Name field assertion.Line 161 redundantly checks the Name field after line 159 already verified struct equality. Line 160's namespace check is useful for clarity, but the Name check adds no value.
Apply this diff:
result := GetResourceName(tt.resName) require.Equal(t, tt.expected, result) require.Empty(t, result.Namespace, "Namespace should always be empty") - require.Equal(t, tt.expected.Name, result.Name)internal/resources/brokers/utils_test.go (3)
12-18: Consider test helper naming convention.The helper uses
mustprefix which conventionally indicates a panic on error, which is appropriate for test setup. However, consider whether a more descriptive name likeparseURIOrFailwould better communicate intent in a test context.
131-190: Consider consolidating redundant test cases.The
TestGetPublisherEnvVarsModeFormattingtest duplicates coverage already provided by the mainTestGetPublisherEnvVarstable. The separator assertions (hyphen for OneStreamByService, dot for OneStreamByStack) are already validated by the expectedMappings in the main test.Consider removing this test function and relying on the comprehensive table-driven tests, or keep only if you want to emphasize the formatting contract explicitly.
192-259: Consider consolidating NATS auto-provision tests.The
TestGetPublisherEnvVarsNATSAutoProvisiontest cases are already covered by the mainTestGetPublisherEnvVarstable (lines 55-88 cover NATS with OneStreamByStack setting auto-provision to false, and line 33-42 covers OneStreamByService without it).You could remove this test function to reduce duplication, or retain it if the explicit separation improves documentation of the NATS-specific behavior.
internal/resources/gateways/url_test.go (1)
101-153: Remove redundant test functions.Both
TestURLSchemeSelectionandTestURLFallbackToInternalServiceduplicate coverage already provided by the mainTestURLtable:
- Lines 101-131 (
TestURLSchemeSelection): Scheme selection is already tested in the main table at lines 18-46 (HTTPS and HTTP cases).- Lines 133-153 (
TestURLFallbackToInternalService): Fallback behavior is already tested at lines 48-59 (nil ingress case).Remove these two functions and rely on the comprehensive table-driven test. If you want to emphasize these specific behaviors, consider adding comments to the relevant test cases in the main table instead.
internal/resources/auths/helpers.go (1)
21-26: Consider early exit for improved efficiency.The loop continues checking all clients even after finding the first non-ready client. The condition message doesn't change after the first failure, so continuing to iterate provides no additional benefit.
Consider breaking early:
for _, client := range authClients { if !client.Status.Ready { condition.SetMessage("OneOfAuthClientsNotReady") condition.SetStatus(v1.ConditionFalse) + break } }internal/resources/reconciliations/init_manifests.go (2)
58-82: Resolve duplicate image resolution.Image configuration is retrieved twice: once for migration (line 59) and again for deployment (line 85). When migration is skipped or disabled, the first call is unnecessary. When migration runs, the second call is redundant.
Refactor to resolve the image configuration once before the migration check:
+ // 4. Get image configuration (needed for migration and deployment) + imageConfiguration, err := registries.GetFormanceImage(ctx, stack, "reconciliation", version) + if err != nil { + return errors.Wrap(err, "resolving image") + } + - // 4. Handle migration + // 5. Handle migration if manifest.Spec.Migration.Enabled && databases.GetSavedModuleVersion(database) != version { - imageConfiguration, err := registries.GetFormanceImage(ctx, stack, "reconciliation", version) - if err != nil { - return errors.Wrap(err, "resolving image") - } - err = databases.Migrate(ctx, stack, reconciliation, imageConfiguration, database) if err != nil { if manifest.Spec.Migration.Strategy == "continue-on-error" { if core.IsApplicationError(err) { // Continue with deployment despite migration error // TODO: Add proper logging here } else { return err } } else { return err } } if err := databases.SaveModuleVersion(ctx, database, version); err != nil { return errors.Wrap(err, "saving module version in database object") } } - // 5. Get image configuration - imageConfiguration, err := registries.GetFormanceImage(ctx, stack, "reconciliation", version) - if err != nil { - return errors.Wrap(err, "resolving image") - } - // 6. Create deployment
68-73: Add logging for continue-on-error strategy.The TODO comment at line 70 indicates missing logging when migration errors are ignored under the "continue-on-error" strategy. This could make debugging production issues difficult.
Do you want me to generate a logging implementation or open a new issue to track this task?
internal/resources/wallets/init_manifests.go (1)
30-70: Loader initialization confirmed; add status updates
manifests.InitLoaderis invoked incmd/main.go(L123) and ininternal/tests/internal/bootstrap.go(L111), so the manifest loader is always initialized beforeReconcileruns.Reconcilecurrently doesn’t set anyWalletsstatus conditions; implement or delegate status updates (e.g., Ready/NotReady) in the parent reconciler.internal/resources/payments/helpers.go (1)
119-133: Consider more specific error messages for URI validation.The validation logic correctly checks the Temporal URI format. However, the error messages could be more specific to help users diagnose configuration issues.
func validateTemporalURI(temporalURI *v1beta1.URI) error { if temporalURI.Scheme != "temporal" { - return fmt.Errorf("invalid temporal uri: %s", temporalURI.String()) + return fmt.Errorf("invalid temporal uri scheme (expected 'temporal', got '%s'): %s", temporalURI.Scheme, temporalURI.String()) } if temporalURI.Path == "" { - return fmt.Errorf("invalid temporal uri: %s", temporalURI.String()) + return fmt.Errorf("invalid temporal uri: path (namespace) is required: %s", temporalURI.String()) } if !strings.HasPrefix(temporalURI.Path, "/") { - return fmt.Errorf("invalid temporal uri: %s", temporalURI.String()) + return fmt.Errorf("invalid temporal uri: path must start with '/': %s", temporalURI.String()) } return nil }internal/resources/orchestrations/init_manifests.go (1)
109-112: Document why orchestration uses custom deployment logic.The comment explains that orchestration has special requirements not yet supported by the generic manifest engine (auth client, broker consumer, temporal config). This is reasonable for a migration period.
Consider adding a TODO or issue reference to track when orchestration can migrate to the generic
manifests.Applyapproach:// 8. Create deployment (using custom logic due to orchestration's complex requirements) // Note: Orchestration has special needs (auth client, broker consumer, temporal config) // that aren't yet supported by the generic manifest engine +// TODO(#XXX): Migrate to manifests.Apply when engine supports orchestration-specific features return createDeployment(ctx, stack, orchestration, database, authClient, consumer, imageConfiguration)internal/resources/ledgers/init_manifests.go (1)
102-106: Document non-semver versions as v2 in migration logic
The semver check in internal/resources/ledgers/init_manifests.go treats any non-compliant version (e.g., “latest”) as v2, which may inadvertently trigger v2 migrations for v1 tags. Add a comment above this block or resolve the actual version tag before comparison.internal/resources/webhooks/init_manifests.go (1)
72-96: Clarify migration error handling logic for continue-on-error strategy.The migration error handling at lines 81-87 checks
manifest.Spec.Migration.Strategy == "continue-on-error"and then only proceeds ifIsApplicationError(err)returns true. This means:
- ApplicationErrors (like pending, stack-not-found) are silently ignored with a TODO for logging
- Non-ApplicationErrors (e.g., network failures, permission errors) still propagate and fail the reconciliation
This asymmetry may be intentional, but it's worth confirming whether all migration errors should be ignored under
continue-on-error, or only specific application-level errors. Consider documenting this behavior or validating that migration errors are consistently classified.Additionally, the TODO at line 84 should be addressed:
if core.IsApplicationError(err) { - // Continue with deployment despite migration error - // TODO: Add proper logging here + // Continue with deployment despite migration error + // Log the application error for observability + // Example: logger.Info("Migration failed with application error, continuing", "error", err) } else {Do you want me to open an issue to track adding proper logging for migration errors under the continue-on-error strategy?
internal/resources/payments/init_manifests.go (1)
143-159: Consider extracting broker environment setup to a helper.The broker environment variable setup involves nested conditions (topic existence, topic readiness, broker retrieval, broker readiness) and spans multiple lines. Extracting this logic to a helper function would improve readability and reusability.
Example refactor:
// In a helper file (e.g., internal/resources/brokers/env_helpers.go) func GetPaymentsEnvVarsIfReady(ctx core.Context, stack *v1beta1.Stack, envPrefix string) ([]corev1.EnvVar, error) { t, err := brokertopics.Find(ctx, stack, "payments") if err != nil { return nil, err } if t == nil || !t.Status.Ready { return nil, nil } broker := &v1beta1.Broker{} if err := ctx.GetClient().Get(ctx, types.NamespacedName{Name: stack.Name}, broker); err != nil { return nil, err } if !broker.Status.Ready { return nil, nil } envVars, err := brokers.GetEnvVarsWithPrefix(ctx, broker.Status.URI, stack.Name, "payments", envPrefix) if err != nil { return nil, err } envVars = append(envVars, brokers.GetPublisherEnvVars(stack, broker, "payments", envPrefix)...) return envVars, nil }Then replace lines 143-159 with:
-// Broker env vars -if t, err := brokertopics.Find(ctx, stack, "payments"); err != nil { - return err -} else if t != nil && t.Status.Ready { - broker := &v1beta1.Broker{} - if err := ctx.GetClient().Get(ctx, types.NamespacedName{Name: stack.Name}, broker); err != nil { - return err - } - if broker.Status.Ready { - brokerEnvVars, err := brokers.GetEnvVarsWithPrefix(ctx, broker.Status.URI, stack.Name, "payments", manifest.Spec.EnvVarPrefix) - if err != nil { - return err - } - additionalEnv = append(additionalEnv, brokerEnvVars...) - additionalEnv = append(additionalEnv, brokers.GetPublisherEnvVars(stack, broker, "payments", manifest.Spec.EnvVarPrefix)...) - } -} +brokerEnv, err := brokers.GetPaymentsEnvVarsIfReady(ctx, stack, manifest.Spec.EnvVarPrefix) +if err != nil { + return err +} +additionalEnv = append(additionalEnv, brokerEnv...)internal/manifests/loader.go (1)
37-65: Consider cache invalidation strategy.The cache stores resolved manifests indefinitely with no eviction or TTL. If VersionManifest CRDs are updated in the cluster during operator runtime, stale manifests may be returned. Consider whether cache invalidation (via watch events or TTL) is needed for your use case.
internal/manifests/engine.go (1)
249-286: Document condition expression limitations.The
evaluateConditionfunction implements a simplified expression parser (lines 253-256) that requires exactly three space-separated tokens. It does not support:
- Complex boolean logic (AND, OR, NOT)
- Parentheses or precedence
- Comparison operators other than
==and!=Ensure manifest authors are aware of these limitations or add validation to reject unsupported expressions early.
api/formance.com/v1beta1/versionmanifest_types.go (1)
98-111: Consider validation for Replicas field.The
Replicasfield is typed asstringto support"auto"or integer values. Consider adding a kubebuilder validation pattern or webhook to enforce valid values (e.g.,+kubebuilder:validation:Pattern="^(auto|[0-9]+)$").
📜 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 (25)
config/crd/bases/formance.com_versionmanifests.yamlis excluded by!**/*.yamlconfig/crd/kustomization.yamlis excluded by!**/*.yamlconfig/samples/manifests/auth-v1.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/auth-v2.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/ledger-v0.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/ledger-v2.0-v2.1.yamlis excluded by!**/*.yamlconfig/samples/manifests/ledger-v2.2.yamlis excluded by!**/*.yamlconfig/samples/manifests/ledger-v2.3.yamlis excluded by!**/*.yamlconfig/samples/manifests/orchestration-v1.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/orchestration-v2.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/payments-v0.9.yamlis excluded by!**/*.yamlconfig/samples/manifests/payments-v1.0-v2.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/payments-v3.0.yamlis excluded by!**/*.yamlconfig/samples/manifests/reconciliation-v1.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/reconciliation-v2.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/search-v1.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/wallets-v1.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/webhooks-v0.7-v1.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/webhooks-v0.x.yamlis excluded by!**/*.yamlconfig/samples/manifests/webhooks-v2.x.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_versionmanifests.formance.com.yamlis excluded by!**/*.yamlinternal/manifests/versions/ledger/v2.0-v2.1.yamlis excluded by!**/*.yamlinternal/manifests/versions/ledger/v2.2.yamlis excluded by!**/*.yamlinternal/manifests/versions/ledger/v2.3.yamlis excluded by!**/*.yaml
📒 Files selected for processing (45)
Justfile(1 hunks)api/formance.com/v1beta1/benthos_types.go(1 hunks)api/formance.com/v1beta1/versionmanifest_types.go(1 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(16 hunks)cmd/main.go(2 hunks)docs/09-Configuration reference/02-Custom Resource Definitions.md(2 hunks)internal/core/env_test.go(1 hunks)internal/core/errors_test.go(1 hunks)internal/core/names_test.go(1 hunks)internal/core/utils_test.go(1 hunks)internal/core/version_test.go(1 hunks)internal/core/volumes_test.go(1 hunks)internal/manifests/README.md(1 hunks)internal/manifests/engine.go(1 hunks)internal/manifests/loader.go(1 hunks)internal/manifests/loader_test.go(1 hunks)internal/resources/applications/liveness_test.go(1 hunks)internal/resources/auths/helpers.go(1 hunks)internal/resources/auths/init.go(0 hunks)internal/resources/auths/init_manifests.go(1 hunks)internal/resources/brokers/utils_test.go(1 hunks)internal/resources/gateways/url_test.go(1 hunks)internal/resources/ledgers/deployments.go(0 hunks)internal/resources/ledgers/init.go(0 hunks)internal/resources/ledgers/init_manifests.go(1 hunks)internal/resources/orchestrations/deployments.go(1 hunks)internal/resources/orchestrations/init.go(0 hunks)internal/resources/orchestrations/init_manifests.go(1 hunks)internal/resources/payments/deployments.go(0 hunks)internal/resources/payments/helpers.go(1 hunks)internal/resources/payments/helpers_test.go(1 hunks)internal/resources/payments/init.go(1 hunks)internal/resources/payments/init_manifests.go(1 hunks)internal/resources/reconciliations/init.go(0 hunks)internal/resources/reconciliations/init_manifests.go(1 hunks)internal/resources/registries/image_test.go(1 hunks)internal/resources/searches/helpers.go(1 hunks)internal/resources/searches/init.go(0 hunks)internal/resources/searches/init_manifests.go(1 hunks)internal/resources/wallets/init.go(0 hunks)internal/resources/wallets/init_manifests.go(1 hunks)internal/resources/webhooks/init.go(0 hunks)internal/resources/webhooks/init_manifests.go(1 hunks)internal/tests/internal/bootstrap.go(4 hunks)pkg/client/formance.com/v1beta1/databases.go(1 hunks)
💤 Files with no reviewable changes (9)
- internal/resources/wallets/init.go
- internal/resources/reconciliations/init.go
- internal/resources/searches/init.go
- internal/resources/orchestrations/init.go
- internal/resources/payments/deployments.go
- internal/resources/auths/init.go
- internal/resources/ledgers/init.go
- internal/resources/webhooks/init.go
- internal/resources/ledgers/deployments.go
🧰 Additional context used
🧬 Code graph analysis (28)
internal/resources/wallets/init_manifests.go (7)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)api/formance.com/v1beta1/wallets_types.go (1)
Wallets(45-51)internal/manifests/loader.go (1)
Load(30-35)internal/core/stacks.go (1)
HasDependency(69-80)api/formance.com/v1beta1/authclient_types.go (2)
AuthClient(90-96)AuthClientSpec(25-55)internal/resources/gatewayhttpapis/create.go (1)
WithHealthCheckEndpoint(46-50)
internal/resources/payments/helpers_test.go (1)
api/formance.com/v1beta1/shared.go (1)
URI(310-312)
internal/resources/brokers/utils_test.go (3)
api/formance.com/v1beta1/shared.go (2)
URI(310-312)Status(224-233)api/formance.com/v1beta1/broker_types.go (5)
Mode(28-28)ModeOneStreamByService(31-31)ModeOneStreamByStack(32-32)Broker(59-65)BrokerStatus(35-49)internal/resources/brokers/utils.go (1)
GetPublisherEnvVars(74-92)
internal/manifests/engine.go (10)
internal/resources/registries/registries.go (1)
ImageConfiguration(18-23)api/formance.com/v1beta1/versionmanifest_types.go (4)
EnvVar(147-155)DeploymentSpec(98-111)ContainerSpec(113-133)VolumeMount(169-174)internal/core/context.go (1)
Context(10-16)internal/core/utils.go (1)
DeleteIfExists(141-151)internal/resources/settings/aws_role.go (1)
GetAWSServiceAccount(5-7)internal/resources/settings/helpers.go (1)
GetStringOrEmpty(55-57)internal/core/env.go (2)
Env(11-16)GetDevEnvVarsWithPrefix(75-84)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/webhooks/init_manifests.go (10)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/webhooks_types.go (1)
Webhooks(44-50)internal/manifests/loader.go (1)
Load(30-35)internal/core/errors.go (2)
NewPendingError(35-37)IsApplicationError(43-45)internal/resources/brokerconsumers/create.go (1)
CreateOrUpdateOnAllServices(52-68)internal/resources/gatewayhttpapis/create.go (1)
WithHealthCheckEndpoint(46-50)internal/resources/databases/create.go (2)
GetSavedModuleVersion(69-71)SaveModuleVersion(60-67)internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/resources/databases/migrate.go (1)
Migrate(15-46)internal/core/version.go (1)
IsGreaterOrEqual(44-52)
internal/tests/internal/bootstrap.go (3)
internal/manifests/loader.go (1)
InitLoader(23-27)api/formance.com/v1beta1/versionmanifest_types.go (1)
VersionManifest(243-249)api/formance.com/v1beta1/groupversion_info.go (1)
GroupVersion(38-38)
internal/core/errors_test.go (1)
internal/core/errors.go (6)
NewApplicationError(27-29)NewStackNotFoundError(31-33)ApplicationError(9-11)NewPendingError(35-37)NewMissingSettingsError(39-41)IsApplicationError(43-45)
cmd/main.go (1)
internal/manifests/loader.go (1)
InitLoader(23-27)
internal/resources/reconciliations/init_manifests.go (10)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)api/formance.com/v1beta1/reconciliation_types.go (1)
Reconciliation(45-51)internal/manifests/loader.go (1)
Load(30-35)internal/core/errors.go (2)
NewPendingError(35-37)IsApplicationError(43-45)internal/resources/authclients/create.go (1)
WithScopes(40-44)internal/resources/databases/create.go (2)
GetSavedModuleVersion(69-71)SaveModuleVersion(60-67)internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/resources/databases/migrate.go (1)
Migrate(15-46)internal/resources/gatewayhttpapis/create.go (1)
WithHealthCheckEndpoint(46-50)
internal/resources/ledgers/init_manifests.go (14)
internal/manifests/loader.go (1)
Load(30-35)internal/resources/gatewayhttpapis/create.go (1)
WithHealthCheckEndpoint(46-50)internal/resources/benthosstreams/create.go (1)
LoadFromFileSystem(19-90)internal/core/stacks.go (1)
HasDependency(69-80)internal/resources/databases/create.go (2)
GetSavedModuleVersion(69-71)SaveModuleVersion(60-67)internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/resources/databases/migrate.go (1)
Migrate(15-46)internal/resources/jobs/job.go (2)
Mutator(37-41)PreCreate(31-35)internal/core/version.go (1)
IsLower(54-62)internal/core/env.go (1)
Env(11-16)internal/resources/auths/env.go (1)
ProtectedAPIEnvVarsWithPrefix(16-53)internal/resources/brokertopics/brokertopics.go (1)
Find(9-23)internal/resources/brokers/utils.go (2)
GetEnvVarsWithPrefix(20-72)GetPublisherEnvVars(74-92)internal/manifests/engine.go (2)
Apply(29-44)ManifestContext(19-26)
internal/resources/auths/helpers.go (5)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/auth_types.go (1)
Auth(80-86)api/formance.com/v1beta1/authclient_types.go (1)
AuthClient(90-96)api/formance.com/v1beta1/shared.go (4)
NewCondition(126-133)AndConditions(195-204)ConditionTypeMatch(206-210)Status(224-233)internal/core/stacks.go (1)
GetAllStackDependencies(17-48)
internal/core/volumes_test.go (2)
internal/core/volumes.go (2)
NewVolumeFromConfigMap(7-18)NewVolumeMount(20-26)api/formance.com/v1beta1/versionmanifest_types.go (1)
VolumeMount(169-174)
internal/resources/registries/image_test.go (1)
internal/resources/registries/image.go (1)
NormalizeVersion(17-22)
internal/resources/searches/init_manifests.go (11)
internal/manifests/loader.go (1)
Load(30-35)internal/resources/settings/helpers.go (2)
RequireURL(92-98)GetMapOrEmpty(374-384)internal/resources/settings/aws_role.go (1)
GetAWSServiceAccount(5-7)internal/core/env.go (2)
Env(11-16)EnvFromSecret(40-52)internal/core/module.go (1)
LowerCamelCaseKind(8-14)internal/resources/registries/image.go (1)
GetFormanceImage(24-30)api/formance.com/v1beta1/benthos_types.go (2)
Batching(25-30)Benthos(58-64)internal/core/utils.go (1)
WithController(72-81)internal/resources/applications/ports.go (1)
StandardHTTPPort(5-10)internal/resources/applications/liveness.go (1)
DefaultLiveness(9-11)internal/resources/gatewayhttpapis/create.go (1)
WithHealthCheckEndpoint(46-50)
internal/resources/payments/helpers.go (5)
api/formance.com/v1beta1/payments_types.go (1)
Payments(45-51)internal/resources/settings/helpers.go (3)
GetStringOrEmpty(55-57)RequireURL(92-98)GetIntOrDefault(186-196)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)internal/core/env.go (2)
Env(11-16)EnvFromSecret(40-52)api/formance.com/v1beta1/shared.go (1)
URI(310-312)
internal/resources/gateways/url_test.go (2)
api/formance.com/v1beta1/gateway_types.go (3)
Gateway(76-82)GatewaySpec(50-56)GatewayIngress(28-48)internal/resources/gateways/url.go (1)
URL(9-15)
internal/resources/applications/liveness_test.go (2)
internal/resources/applications/ports.go (1)
StandardHTTPPort(5-10)internal/resources/applications/liveness.go (4)
ProbeOpts(24-24)WithProbePath(52-57)WithHost(45-50)DefaultLiveness(9-11)
internal/core/version_test.go (1)
internal/core/version.go (2)
IsGreaterOrEqual(44-52)IsLower(54-62)
api/formance.com/v1beta1/zz_generated.deepcopy.go (1)
api/formance.com/v1beta1/versionmanifest_types.go (21)
ArchitectureConfig(87-96)DeploymentSpec(98-111)AuthorizationConfig(201-205)ScopeDefinition(207-226)CleanupConfig(185-192)ConditionalEnv(162-167)EnvVar(147-155)ContainerSpec(113-133)PortSpec(135-138)HealthCheckSpec(140-145)VolumeMount(169-174)ServiceSpec(176-183)EnvVarSource(157-160)GatewayConfig(194-199)MigrationCondition(82-85)MigrationConfig(70-80)StreamsConfig(63-68)VersionManifest(243-249)VersionManifestList(254-258)VersionManifestSpec(24-61)VersionManifestStatus(229-232)
internal/core/env_test.go (3)
internal/core/env.go (8)
Env(11-16)EnvFromBool(18-24)EnvFromConfig(26-38)EnvFromSecret(40-52)EnvVarPlaceholder(54-56)ComputeEnvVar(58-64)GetDevEnvVars(68-73)GetDevEnvVarsWithPrefix(75-84)api/formance.com/v1beta1/stack_types.go (2)
Stack(77-83)StackSpec(23-45)api/formance.com/v1beta1/shared.go (1)
DevProperties(21-31)
api/formance.com/v1beta1/versionmanifest_types.go (3)
api/formance.com/v1beta1/gateway_types.go (1)
Gateway(76-82)api/formance.com/v1beta1/shared.go (2)
Conditions(135-135)Status(224-233)api/formance.com/v1beta1/groupversion_info.go (1)
SchemeBuilder(41-41)
internal/resources/payments/init_manifests.go (12)
internal/manifests/loader.go (1)
Load(30-35)internal/resources/databases/create.go (2)
GetSavedModuleVersion(69-71)SaveModuleVersion(60-67)internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/resources/databases/migrate.go (1)
Migrate(15-46)internal/resources/jobs/job.go (1)
WithEnvVars(50-55)internal/core/env.go (1)
Env(11-16)internal/resources/benthosstreams/create.go (1)
LoadFromFileSystem(19-90)internal/resources/gatewayhttpapis/create.go (3)
WithHealthCheckEndpoint(46-50)WithRules(40-44)RuleSecured(52-54)internal/resources/auths/env.go (1)
ProtectedAPIEnvVarsWithPrefix(16-53)internal/resources/brokertopics/brokertopics.go (1)
Find(9-23)internal/resources/brokers/utils.go (2)
GetEnvVarsWithPrefix(20-72)GetPublisherEnvVars(74-92)internal/manifests/engine.go (2)
Apply(29-44)ManifestContext(19-26)
internal/resources/searches/helpers.go (9)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/search_types.go (1)
Search(52-58)api/formance.com/v1beta1/shared.go (2)
Module(251-257)Status(224-233)api/formance.com/v1beta1/payments_types.go (1)
Payments(45-51)api/formance.com/v1beta1/ledger_types.go (1)
Ledger(97-103)api/formance.com/v1beta1/gateway_types.go (1)
Gateway(76-82)internal/core/stacks.go (1)
HasDependency(69-80)internal/core/module.go (1)
LowerCamelCaseKind(8-14)internal/core/errors.go (1)
NewPendingError(35-37)
internal/manifests/loader_test.go (3)
internal/manifests/loader.go (2)
Loader(15-18)Load(30-35)api/formance.com/v1beta1/versionmanifest_types.go (5)
VersionManifest(243-249)VersionManifestSpec(24-61)ArchitectureConfig(87-96)DeploymentSpec(98-111)StreamsConfig(63-68)api/formance.com/v1beta1/groupversion_info.go (1)
AddToScheme(44-44)
internal/resources/auths/init_manifests.go (7)
internal/core/context.go (1)
Context(10-16)internal/manifests/loader.go (1)
Load(30-35)internal/core/errors.go (2)
NewPendingError(35-37)IsApplicationError(43-45)internal/resources/databases/create.go (2)
GetSavedModuleVersion(69-71)SaveModuleVersion(60-67)internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/resources/databases/migrate.go (1)
Migrate(15-46)internal/resources/gatewayhttpapis/create.go (3)
WithHealthCheckEndpoint(46-50)WithRules(40-44)RuleUnsecured(56-60)
internal/manifests/loader.go (2)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/versionmanifest_types.go (3)
VersionManifest(243-249)VersionManifestList(254-258)ScopeDefinition(207-226)
internal/core/utils_test.go (2)
internal/core/utils.go (2)
HashFromConfigMaps(27-35)HashFromResources(37-47)internal/core/shell.go (1)
ShellScript(5-12)
internal/resources/orchestrations/init_manifests.go (10)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)api/formance.com/v1beta1/orchestration_types.go (1)
Orchestration(47-53)internal/manifests/loader.go (1)
Load(30-35)internal/core/errors.go (2)
NewPendingError(35-37)IsApplicationError(43-45)internal/resources/brokerconsumers/create.go (1)
CreateOrUpdateOnAllServices(52-68)internal/resources/gatewayhttpapis/create.go (1)
WithHealthCheckEndpoint(46-50)internal/resources/databases/create.go (2)
GetSavedModuleVersion(69-71)SaveModuleVersion(60-67)internal/resources/registries/image.go (1)
GetFormanceImage(24-30)internal/resources/databases/migrate.go (1)
Migrate(15-46)
🪛 markdownlint-cli2 (0.18.1)
docs/09-Configuration reference/02-Custom Resource Definitions.md
2724-2724: Link fragments should be valid
(MD051, link-fragments)
2725-2725: Link fragments should be valid
(MD051, link-fragments)
| #### VersionManifest | ||
|
|
||
|
|
||
|
|
||
| VersionManifest is the Schema for version manifests. | ||
| It defines the deployment configuration for a specific version range of a component. | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `apiVersion` _string_ | `formance.com/v1beta1` | | | | ||
| | `kind` _string_ | `VersionManifest` | | | | ||
| | `metadata` _[ObjectMeta](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#objectmeta-v1-meta)_ | Refer to Kubernetes API documentation for fields of `metadata`. | | | | ||
| | `spec` _[VersionManifestSpec](#versionmanifestspec)_ | | | | | ||
| | `status` _[VersionManifestStatus](#versionmanifeststatus)_ | | | | | ||
|
|
||
|
|
||
|
|
||
| ##### VersionManifestSpec | ||
|
|
||
|
|
||
|
|
||
| VersionManifestSpec defines the desired state of VersionManifest | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `component` _string_ | Component name (e.g., "ledger", "payments") | | | | ||
| | `versionRange` _string_ | Version range using semver (e.g., ">=v2.2.0 <v2.3.0") | | | | ||
| | `extends` _string_ | Inherit from another manifest (optional) | | | | ||
| | `envVarPrefix` _string_ | Environment variable prefix (e.g., "NUMARY_" or "") | | | | ||
| | `streams` _[StreamsConfig](#streamsconfig)_ | Stream configurations | | | | ||
| | `migration` _[MigrationConfig](#migrationconfig)_ | Migration configuration | | | | ||
| | `architecture` _[ArchitectureConfig](#architectureconfig)_ | Deployment architecture | | | | ||
| | `features` _object (keys:string, values:boolean)_ | Feature flags | | | | ||
| | `gateway` _[GatewayConfig](#gatewayconfig)_ | Gateway configuration | | | | ||
| | `authorization` _[AuthorizationConfig](#authorizationconfig)_ | Authorization configuration (scopes, permissions) | | | | ||
|
|
||
| ###### StreamsConfig | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `ingestion` _string_ | | | | | ||
| | `reindex` _string_ | | | | | ||
|
|
||
| ###### MigrationConfig | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `enabled` _boolean_ | | | | | ||
| | `strategy` _string_ | Strategy: "strict", "continue-on-error", "skip" | strict | | | ||
| | `commands` _string array_ | | | | | ||
|
|
||
| ###### ArchitectureConfig | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `type` _string_ | Type: "stateless", "single-or-multi-writer", "sharded" | | Enum: [stateless single-or-multi-writer sharded] <br /> | | ||
| | `deployments` _[DeploymentSpec](#deploymentspec) array_ | | | | | ||
| | `cleanup` _[CleanupConfig](#cleanupconfig)_ | | | | | ||
|
|
||
| ###### DeploymentSpec | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `name` _string_ | | | | | ||
| | `replicas` _string_ | Replicas: "auto" or integer as string | | | | ||
| | `stateful` _boolean_ | | | | | ||
| | `containers` _[ContainerSpec](#containerspec) array_ | | | | | ||
| | `service` _[ServiceSpec](#servicespec)_ | | | | | ||
|
|
||
| ###### CleanupConfig | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `deployments` _string array_ | | | | | ||
| | `services` _string array_ | | | | | ||
| | `reason` _string_ | | | | | ||
|
|
||
| ###### GatewayConfig | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `enabled` _boolean_ | | | | | ||
| | `healthCheckEndpoint` _string_ | | _healthcheck | | | ||
|
|
||
| ###### AuthorizationConfig | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `scopes` _[ScopeDefinition](#scopedefinition) array_ | OAuth/OIDC scopes available for this version | | | | ||
|
|
||
| ###### ScopeDefinition | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `name` _string_ | Scope name (e.g., "ledger:read", "ledger:write") | | | | ||
| | `description` _string_ | Human-readable description | | | | ||
| | `deprecated` _boolean_ | Whether this scope is deprecated | | | | ||
| | `replacedBy` _string_ | Replacement scope name if deprecated | | | | ||
| | `since` _string_ | Version when this scope was introduced | | | | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| ##### VersionManifestStatus | ||
|
|
||
|
|
||
|
|
||
| VersionManifestStatus defines the observed state of VersionManifest | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| | Field | Description | Default | Validation | | ||
| | --- | --- | --- | --- | | ||
| | `lastApplied` _[Time](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.27/#time-v1-meta)_ | | | | | ||
|
|
There was a problem hiding this comment.
Fix broken link fragments for nested types.
The documentation structure for VersionManifest is comprehensive and well-organized. However, static analysis detected broken link fragments at lines 2724-2725 for ContainerSpec and ServiceSpec references in the DeploymentSpec table.
These types are referenced but not defined in the documentation. Either:
- Add documentation sections for
ContainerSpecandServiceSpecwith their field definitions, or - Update the links to point to existing Kubernetes API documentation if these are standard Kubernetes types, or
- Remove the links if the types are self-explanatory from context
Based on static analysis hints.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
2724-2724: Link fragments should be valid
(MD051, link-fragments)
2725-2725: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
docs/09-Configuration reference/02-Custom Resource Definitions.md lines
2724-2725: the DeploymentSpec table references ContainerSpec and ServiceSpec
link fragments that do not exist, causing broken links; fix by adding new
documentation subsections for ContainerSpec and ServiceSpec (positioned near
DeploymentSpec) that list each field, description, defaults and validation, or
alternatively replace the broken fragments with valid links to the corresponding
Kubernetes API docs if these are standard K8s types, or remove the fragment
links entirely if you prefer plain text; implement one of these three options so
the table links resolve.
| name: "empty stack", | ||
| stack: "", | ||
| objName: "ledger", | ||
| expected: "-ledger", | ||
| }, | ||
| { | ||
| name: "empty name", | ||
| stack: "production", | ||
| objName: "", | ||
| expected: "production-", | ||
| }, | ||
| { | ||
| name: "both empty", | ||
| stack: "", | ||
| objName: "", | ||
| expected: "-", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify handling of empty stack/name parameters.
The test cases document that GetObjectName produces invalid Kubernetes resource names when given empty strings (e.g., "-ledger", "production-", "-"). Kubernetes resource names must follow DNS subdomain rules and cannot have leading or trailing hyphens.
Consider whether GetObjectName should validate inputs and return an error, or if callers are expected to ensure non-empty parameters. Run the following script to check how GetObjectName is used across the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of GetObjectName to determine if validation is needed.
# Search for GetObjectName calls and show surrounding context
rg -nP -C3 '\bGetObjectName\s*\(' --type=go -g '!*_test.go'Length of output: 148
Use this script to search for all non-test usages of GetObjectName:
🏁 Script executed:
#!/bin/bash
# Find GetObjectName calls in Go files excluding *_test.go
rg -nP -C3 '\bGetObjectName\s*\(' --type=go -g '!**/*_test.go'Length of output: 148
🏁 Script executed:
#!/bin/bash
# Search for GetObjectName calls in all Go files, then filter out test files
rg -nC3 '\bGetObjectName\b' -g '**/*.go' | grep -v '_test.go'Length of output: 5201
Sanitize GetObjectName to avoid invalid Kubernetes names.
GetObjectName (internal/core/names.go:9) currently uses
fmt.Sprintf("%s-%s", stack, name)which, when stack or name is empty, produces names like "-ledger", "production-" or "-", all of which violate Kubernetes DNS subdomain rules. Since this function is used across resource creation (e.g. in internal/resources/**), update it to:
func GetObjectName(stack, name string) string {
switch {
case stack == "":
return name
case name == "":
return stack
default:
return fmt.Sprintf("%s-%s", stack, name)
}
}Then adjust the edge‐case expectations in internal/core/names_test.go to match—e.g. expect "ledger", "production", and ""—instead of the current invalid hyphenated outputs.
🤖 Prompt for AI Agents
In internal/core/names_test.go around lines 37 to 53, the tests expect invalid
Kubernetes names like "-ledger", "production-" and "-" because GetObjectName in
internal/core/names.go simply fmt.Sprintf("%s-%s", stack, name); update
GetObjectName to return name when stack is empty, stack when name is empty, and
only join with a hyphen otherwise; then change the test expectations for those
edge cases to "ledger", "production", and "" respectively so they reflect the
new sanitized behavior.
| // Health check | ||
| if spec.HealthCheck != nil { | ||
| switch spec.HealthCheck.Type { | ||
| case "http": | ||
| container.LivenessProbe = applications.DefaultLiveness("http") | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete health check implementation.
Only the "http" health check type is implemented (line 169). The manifest schema supports "tcp" and "exec" types (api/formance.com/v1beta1/versionmanifest_types.go line 143), but these are silently ignored here. Either implement the missing types or add explicit error handling.
Consider adding:
// Health check
if spec.HealthCheck != nil {
switch spec.HealthCheck.Type {
case "http":
container.LivenessProbe = applications.DefaultLiveness("http")
+ case "tcp", "exec":
+ return nil, fmt.Errorf("health check type %s not yet implemented", spec.HealthCheck.Type)
+ default:
+ return nil, fmt.Errorf("unknown health check type: %s", spec.HealthCheck.Type)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Health check | |
| if spec.HealthCheck != nil { | |
| switch spec.HealthCheck.Type { | |
| case "http": | |
| container.LivenessProbe = applications.DefaultLiveness("http") | |
| } | |
| } | |
| // Health check | |
| if spec.HealthCheck != nil { | |
| switch spec.HealthCheck.Type { | |
| case "http": | |
| container.LivenessProbe = applications.DefaultLiveness("http") | |
| case "tcp", "exec": | |
| return nil, fmt.Errorf("health check type %s not yet implemented", spec.HealthCheck.Type) | |
| default: | |
| return nil, fmt.Errorf("unknown health check type: %s", spec.HealthCheck.Type) | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/manifests/engine.go around lines 165 to 171, the health check switch
only handles "http" and silently ignores "tcp" and "exec" types defined in the
manifest schema; update the switch to explicitly handle "tcp" (set appropriate
container.LivenessProbe, e.g. a TCP socket probe) and "exec" (set a command exec
probe), and add a default branch that returns or logs an explicit error when an
unsupported/unknown health check type is specified so missing types are not
silently ignored.
| func TestManifestInheritance(t *testing.T) { | ||
| parent := &v1beta1.VersionManifest{ | ||
| Spec: v1beta1.VersionManifestSpec{ | ||
| EnvVarPrefix: "NUMARY_", | ||
| Features: map[string]bool{ | ||
| "feature1": true, | ||
| "feature2": false, | ||
| }, | ||
| Streams: v1beta1.StreamsConfig{ | ||
| Ingestion: "streams/ledger", | ||
| Reindex: "assets/reindex/v1.0.0", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| child := &v1beta1.VersionManifest{ | ||
| Spec: v1beta1.VersionManifestSpec{ | ||
| Extends: "parent", | ||
| Features: map[string]bool{ | ||
| "feature2": true, // Override | ||
| "feature3": true, // New | ||
| }, | ||
| Streams: v1beta1.StreamsConfig{ | ||
| Reindex: "assets/reindex/v2.0.0", // Override | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| loader := &Loader{} | ||
| merged := loader.mergeManifests(parent, child) | ||
|
|
||
| // Check inherited values | ||
| if merged.Spec.EnvVarPrefix != "" { | ||
| t.Errorf("EnvVarPrefix not inherited, got %q", merged.Spec.EnvVarPrefix) | ||
| } | ||
|
|
||
| if merged.Spec.Streams.Ingestion != "streams/ledger" { | ||
| t.Errorf("Streams.Ingestion not inherited, got %q", merged.Spec.Streams.Ingestion) | ||
| } | ||
|
|
||
| // Check overridden values | ||
| if merged.Spec.Streams.Reindex != "assets/reindex/v2.0.0" { | ||
| t.Errorf("Streams.Reindex not overridden, got %q", merged.Spec.Streams.Reindex) | ||
| } | ||
|
|
||
| // Check feature merging | ||
| if !merged.Spec.Features["feature1"] { | ||
| t.Error("feature1 not inherited") | ||
| } | ||
|
|
||
| if !merged.Spec.Features["feature2"] { | ||
| t.Error("feature2 not overridden to true") | ||
| } | ||
|
|
||
| if !merged.Spec.Features["feature3"] { | ||
| t.Error("feature3 not added") | ||
| } | ||
| } |
There was a problem hiding this comment.
Test assertion contradicts error message for EnvVarPrefix inheritance.
Lines 236-238 check that merged.Spec.EnvVarPrefix != "" (expecting empty string), but the error message states "EnvVarPrefix not inherited". This is contradictory—if the prefix should be inherited from the parent (which has "NUMARY_"), the test should verify it equals "NUMARY_", not that it's empty.
Clarify the intended inheritance behavior and fix the assertion:
- // Check inherited values
- if merged.Spec.EnvVarPrefix != "" {
- t.Errorf("EnvVarPrefix not inherited, got %q", merged.Spec.EnvVarPrefix)
+ // Check inherited values (child should inherit parent's prefix when not overridden)
+ if merged.Spec.EnvVarPrefix != "NUMARY_" {
+ t.Errorf("EnvVarPrefix not inherited, got %q, want 'NUMARY_'", merged.Spec.EnvVarPrefix)
}Or, if the child is intentionally clearing the prefix (overriding to empty), update the error message and add a comment explaining this behavior:
- // Check inherited values
- if merged.Spec.EnvVarPrefix != "" {
- t.Errorf("EnvVarPrefix not inherited, got %q", merged.Spec.EnvVarPrefix)
+ // Check that child can clear inherited values by setting to empty
+ if merged.Spec.EnvVarPrefix != "" {
+ t.Errorf("EnvVarPrefix not cleared by child, got %q, want ''", merged.Spec.EnvVarPrefix)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestManifestInheritance(t *testing.T) { | |
| parent := &v1beta1.VersionManifest{ | |
| Spec: v1beta1.VersionManifestSpec{ | |
| EnvVarPrefix: "NUMARY_", | |
| Features: map[string]bool{ | |
| "feature1": true, | |
| "feature2": false, | |
| }, | |
| Streams: v1beta1.StreamsConfig{ | |
| Ingestion: "streams/ledger", | |
| Reindex: "assets/reindex/v1.0.0", | |
| }, | |
| }, | |
| } | |
| child := &v1beta1.VersionManifest{ | |
| Spec: v1beta1.VersionManifestSpec{ | |
| Extends: "parent", | |
| Features: map[string]bool{ | |
| "feature2": true, // Override | |
| "feature3": true, // New | |
| }, | |
| Streams: v1beta1.StreamsConfig{ | |
| Reindex: "assets/reindex/v2.0.0", // Override | |
| }, | |
| }, | |
| } | |
| loader := &Loader{} | |
| merged := loader.mergeManifests(parent, child) | |
| // Check inherited values | |
| if merged.Spec.EnvVarPrefix != "" { | |
| t.Errorf("EnvVarPrefix not inherited, got %q", merged.Spec.EnvVarPrefix) | |
| } | |
| if merged.Spec.Streams.Ingestion != "streams/ledger" { | |
| t.Errorf("Streams.Ingestion not inherited, got %q", merged.Spec.Streams.Ingestion) | |
| } | |
| // Check overridden values | |
| if merged.Spec.Streams.Reindex != "assets/reindex/v2.0.0" { | |
| t.Errorf("Streams.Reindex not overridden, got %q", merged.Spec.Streams.Reindex) | |
| } | |
| // Check feature merging | |
| if !merged.Spec.Features["feature1"] { | |
| t.Error("feature1 not inherited") | |
| } | |
| if !merged.Spec.Features["feature2"] { | |
| t.Error("feature2 not overridden to true") | |
| } | |
| if !merged.Spec.Features["feature3"] { | |
| t.Error("feature3 not added") | |
| } | |
| } | |
| func TestManifestInheritance(t *testing.T) { | |
| parent := &v1beta1.VersionManifest{ | |
| Spec: v1beta1.VersionManifestSpec{ | |
| EnvVarPrefix: "NUMARY_", | |
| Features: map[string]bool{ | |
| "feature1": true, | |
| "feature2": false, | |
| }, | |
| Streams: v1beta1.StreamsConfig{ | |
| Ingestion: "streams/ledger", | |
| Reindex: "assets/reindex/v1.0.0", | |
| }, | |
| }, | |
| } | |
| child := &v1beta1.VersionManifest{ | |
| Spec: v1beta1.VersionManifestSpec{ | |
| Extends: "parent", | |
| Features: map[string]bool{ | |
| "feature2": true, // Override | |
| "feature3": true, // New | |
| }, | |
| Streams: v1beta1.StreamsConfig{ | |
| Reindex: "assets/reindex/v2.0.0", // Override | |
| }, | |
| }, | |
| } | |
| loader := &Loader{} | |
| merged := loader.mergeManifests(parent, child) | |
| // Check inherited values (child should inherit parent's prefix when not overridden) | |
| if merged.Spec.EnvVarPrefix != "NUMARY_" { | |
| t.Errorf("EnvVarPrefix not inherited, got %q, want 'NUMARY_'", merged.Spec.EnvVarPrefix) | |
| } | |
| if merged.Spec.Streams.Ingestion != "streams/ledger" { | |
| t.Errorf("Streams.Ingestion not inherited, got %q", merged.Spec.Streams.Ingestion) | |
| } | |
| // Check overridden values | |
| if merged.Spec.Streams.Reindex != "assets/reindex/v2.0.0" { | |
| t.Errorf("Streams.Reindex not overridden, got %q", merged.Spec.Streams.Reindex) | |
| } | |
| // Check feature merging | |
| if !merged.Spec.Features["feature1"] { | |
| t.Error("feature1 not inherited") | |
| } | |
| if !merged.Spec.Features["feature2"] { | |
| t.Error("feature2 not overridden to true") | |
| } | |
| if !merged.Spec.Features["feature3"] { | |
| t.Error("feature3 not added") | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/manifests/loader_test.go around lines 204-261 the test incorrectly
asserts EnvVarPrefix is empty while the parent defines "NUMARY_"; update the
assertion to expect merged.Spec.EnvVarPrefix == "NUMARY_" and change the error
message to something like "EnvVarPrefix not inherited, expected %q", "NUMARY_";
alternatively, if the child is meant to clear the prefix, explicitly set
child.Spec.EnvVarPrefix = "" in the test, update the assertion to expect an
empty string and change the error message to clarify that the child
intentionally clears the prefix, plus add a brief comment explaining the chosen
behavior.
| var globalLoader *Loader | ||
|
|
||
| // InitLoader initializes the global loader with a Kubernetes client | ||
| func InitLoader(c client.Client) { | ||
| globalLoader = &Loader{ | ||
| client: c, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify InitLoader is called exactly once during startup.
The global globalLoader variable and InitLoader function lack synchronization. If InitLoader is called concurrently or multiple times, it could lead to race conditions. Ensure InitLoader is invoked exactly once during application initialization (e.g., in main before starting the manager).
🏁 Script executed:
#!/bin/bash
# Description: Verify InitLoader is called exactly once and not concurrently
# Search for all InitLoader call sites
rg -n "manifests\.InitLoader" --type=goLength of output: 134
Make InitLoader concurrency-safe and add nil-check
The InitLoader function is invoked from cmd/main.go:123 and internal/tests/internal/bootstrap.go:111. Without synchronization, repeated calls can race to set the global globalLoader. Wrap the initialization in a sync.Once (or guard with a mutex) to ensure a single assignment, and validate that the provided client.Client is non-nil before using it.
🤖 Prompt for AI Agents
internal/manifests/loader.go lines 20-27: InitLoader currently assigns to the
package-global globalLoader without synchronization and without validating the
incoming client; make it concurrency-safe by guarding initialization with a
sync.Once (or a package-level mutex) so only the first successful call sets
globalLoader, and add a nil-check on the provided client.Client returning/doing
nothing (or logging/returning an error if your API allows) when c is nil to
avoid assigning an invalid loader; ensure any existing callers expecting
re-initialization still work by documenting that subsequent calls are no-ops.
| func getEncryptionKey(ctx core.Context, payments *v1beta1.Payments) (string, error) { | ||
| encryptionKey := payments.Spec.EncryptionKey | ||
| if encryptionKey == "" { | ||
| return settings.GetStringOrEmpty(ctx, payments.Spec.Stack, "payments", "encryption-key") | ||
| } | ||
| return "", nil | ||
| } |
There was a problem hiding this comment.
Critical logic error: returns empty string when encryption key exists.
Line 19 returns an empty string when encryptionKey is NOT empty (the else branch is missing). This means the function will never return the encryption key from the Payments spec, and will always return an empty string or fetch from settings.
Apply this fix:
func getEncryptionKey(ctx core.Context, payments *v1beta1.Payments) (string, error) {
encryptionKey := payments.Spec.EncryptionKey
if encryptionKey == "" {
return settings.GetStringOrEmpty(ctx, payments.Spec.Stack, "payments", "encryption-key")
}
- return "", nil
+ return encryptionKey, nil
}🤖 Prompt for AI Agents
In internal/resources/payments/helpers.go around lines 14 to 20, the function
returns an empty string when payments.Spec.EncryptionKey is present; change the
control flow so that if encryptionKey is non-empty the function returns that
value and nil error, otherwise fall back to calling
settings.GetStringOrEmpty(ctx, payments.Spec.Stack, "payments",
"encryption-key") and return its result. Ensure the function returns
(encryptionKey, nil) in the first branch and only queries settings in the else
branch.
| // Get encryption key | ||
| encryptionKey, err := getEncryptionKey(ctx, payments) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Run migration | ||
| err = databases.Migrate(ctx, stack, payments, imageConfiguration, database, | ||
| jobs.WithEnvVars(core.Env("CONFIG_ENCRYPTION_KEY", encryptionKey)), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify encryption key is not exposed in logs or events.
The encryption key is passed as a plain environment variable to the migration job (line 73) and the main deployment (line 173). If pod specifications, job logs, or Kubernetes events are accessible to unauthorized parties, the encryption key could be exposed.
Verify that:
- The encryption key is not logged by the migration job or main container.
- Kubernetes RBAC policies prevent unauthorized access to pod specs and logs.
- Consider using a Kubernetes Secret with appropriate access controls and referencing it via
EnvVarSourceinstead of plain text if the key is truly sensitive.
Also applies to: 168-173
🏁 Script executed:
#!/bin/bash
# Description: Check if encryption key is retrieved from a secret or plain value.
# Search for getEncryptionKey implementation to understand the source
ast-grep --pattern $'func getEncryptionKey($_, $_) ($_, $_) {
$$$
}'Length of output: 563
Avoid exposing encryption key as plain environment variable and fix retrieval implementation
getEncryptionKeyininternal/resources/payments/helpers.go(lines 14–20) incorrectly falls through and never returnspayments.Spec.EncryptionKeywhen set; change the finalreturn "", niltoreturn encryptionKey, nil.- In
init_manifests.go(lines 65–74 and 168–173), the key is injected viacore.Env("CONFIG_ENCRYPTION_KEY", encryptionKey), embedding plaintext in the PodSpec. Anyone withget pods(or similar RBAC) can view it. - Instead, store the key in a Kubernetes Secret and use an
EnvVarSourcewithSecretKeyRefto inject it securely. - Audit for any logging of
"CONFIG_ENCRYPTION_KEY"or the key variable, and ensure RBAC policies restrict access to PodSpecs, logs, and events.
🤖 Prompt for AI Agents
In internal/resources/payments/helpers.go around lines 14–20 and
internal/resources/payments/init_manifests.go around lines 65–74 and 168–173:
fix getEncryptionKey to return the resolved encryptionKey instead of always
returning "", then stop embedding the plaintext key directly into PodSpec env
vars. Modify the helper to return payments.Spec.EncryptionKey when present
(change final return to return encryptionKey, nil). In init_manifests.go create
or reference a Kubernetes Secret containing the encryption key and replace
core.Env("CONFIG_ENCRYPTION_KEY", encryptionKey) with an EnvVar that uses
EnvVarSource -> SecretKeyRef to read the key from that Secret; ensure manifests
include Secret creation (or a reference) and avoid logging the key anywhere;
audit code for any logging of CONFIG_ENCRYPTION_KEY or the variable and tighten
RBAC to limit access to PodSpecs, logs, and events.
| if err != nil { | ||
| if manifest.Spec.Migration.Strategy == "continue-on-error" { | ||
| if core.IsApplicationError(err) { | ||
| // Continue with deployment despite migration error | ||
| // TODO: Add proper logging here | ||
| } else { | ||
| return err | ||
| } | ||
| } else { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Add logging for suppressed migration errors.
The TODO comment on line 80 indicates that proper logging is missing when migration errors are suppressed due to the continue-on-error strategy. Without logging, operators will have no visibility into migration failures.
Consider adding structured logging here to record the suppressed error:
if manifest.Spec.Migration.Strategy == "continue-on-error" {
if core.IsApplicationError(err) {
// Continue with deployment despite migration error
- // TODO: Add proper logging here
+ ctx.Log().WithError(err).Warn("Migration failed but continuing due to continue-on-error strategy")
} else {
return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| if manifest.Spec.Migration.Strategy == "continue-on-error" { | |
| if core.IsApplicationError(err) { | |
| // Continue with deployment despite migration error | |
| // TODO: Add proper logging here | |
| } else { | |
| return err | |
| } | |
| } else { | |
| return err | |
| } | |
| } | |
| if err != nil { | |
| if manifest.Spec.Migration.Strategy == "continue-on-error" { | |
| if core.IsApplicationError(err) { | |
| // Continue with deployment despite migration error | |
| ctx.Log().WithError(err).Warn("Migration failed but continuing due to continue-on-error strategy") | |
| } else { | |
| return err | |
| } | |
| } else { | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/resources/payments/init_manifests.go around lines 76 to 87, the
branch that suppresses migration errors for the "continue-on-error" strategy
currently has a TODO instead of logging; add a structured log entry (e.g.,
logger.Warnf or logger.WithFields(...).Warn/Info) that records the suppressed
error and contextual metadata such as manifest name/ID, namespace, migration
name/version, and the strategy, and include the actual error message and
stack/inner error if available; ensure the log level reflects a non-fatal but
important condition (warning) and keep behavior unchanged (still continue
deployment).
| if core.IsGreaterOrEqual(version, "v0.7.1") { | ||
| // Single deployment with embedded worker | ||
| if err := createSingleDeployment(ctx, stack, webhooks, database, consumer, version); err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| // Dual deployment (separate API and worker) | ||
| if err := createDualDeployment(ctx, stack, webhooks, database, consumer, version); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify semver comparison for version-based deployment selection.
The version comparison at line 100 uses core.IsGreaterOrEqual(version, "v0.7.1") to select between single and dual deployment architectures. Based on learnings, IsGreaterOrEqual handles both semver and non-semver versions gracefully.
Confirm that:
v0.7.1is the correct threshold for the single deployment architecture.- The comparison correctly handles edge cases like
latest,v0.7.1-rc.1, and malformed versions.
Run the following script to check the implementation of IsGreaterOrEqual and validate its behavior with test cases:
🏁 Script executed:
#!/bin/bash
# Description: Verify IsGreaterOrEqual handles v0.7.1 threshold correctly
# Check the implementation
ast-grep --pattern $'func IsGreaterOrEqual($_, $_) $_ {
$$$
}'
# Find related test cases
rg -nP --type=go -C3 'IsGreaterOrEqual.*v0\.7'Length of output: 1185
Confirm threshold and add unit tests for version‐based branching.
- Verify that v0.7.1 is indeed the release where the single-deployment architecture was introduced (e.g. check CHANGELOG.md or release notes).
- Add unit tests for
core.IsGreaterOrEqualcovering:- semver below threshold (
v0.7.0→ expect false) - at threshold (
v0.7.1→ expect true) - pre-release (
v0.7.1-rc.1→ expect false) - non-semver inputs (
latest, malformed strings → expect fallback to true)
- semver below threshold (
| tests-integration args='': | ||
| KUBEBUILDER_ASSETS=$(setup-envtest use 1.28.0 -p path) ginkgo -p ./... |
There was a problem hiding this comment.
Unused args parameter in tests-integration.
The tests-integration target declares an args parameter on line 23, but line 24 does not reference it (e.g., {{args}}). If the intention is to allow users to pass custom arguments to ginkgo, the command should be updated.
Apply this diff if you intend to pass args to ginkgo:
tests-integration args='':
- KUBEBUILDER_ASSETS=$(setup-envtest use 1.28.0 -p path) ginkgo -p ./...
+ KUBEBUILDER_ASSETS=$(setup-envtest use 1.28.0 -p path) ginkgo -p {{args}} ./...Otherwise, remove the unused parameter:
-tests-integration args='':
+tests-integration:
KUBEBUILDER_ASSETS=$(setup-envtest use 1.28.0 -p path) ginkgo -p ./...🤖 Prompt for AI Agents
In Justfile around lines 23-24, the tests-integration target declares an unused
args parameter; either remove the `args=''` parameter if you don't need custom
ginkgo args, or update the command to pass the parameter into ginkgo (e.g.,
append the args variable to the ginkgo invocation) so user-supplied arguments
are actually forwarded to ginkgo; pick one and apply the change accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/core/errors_test.go (1)
129-144: Consider adding a nil error test case.The test correctly verifies the
Ismethod behavior forApplicationErrorinstances and standard errors. Consider adding a test case fornilerror to ensureIshandles it gracefully without panicking.// Test with standard error standardErr := errors.New("standard error") require.False(t, err1.Is(standardErr), "ApplicationError should not match standard error") + + // Test with nil error + require.False(t, err1.Is(nil), "ApplicationError should not match nil error")
📜 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 (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (1)
internal/core/errors_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/core/errors_test.go (1)
internal/core/errors.go (6)
NewApplicationError(27-29)NewStackNotFoundError(31-33)ApplicationError(9-11)NewPendingError(35-37)NewMissingSettingsError(39-41)IsApplicationError(43-45)
🔇 Additional comments (11)
internal/core/errors_test.go (11)
11-19: LGTM!The test correctly verifies the zero-state behavior of
NewApplicationError, ensuring both the internalmessagefield and theError()method return empty values.
21-66: LGTM!Comprehensive table-driven tests covering various message formatting scenarios. The test correctly verifies both the
Error()method output and the internalmessagefield, ensuring consistent behavior.
68-77: LGTM!Correctly verifies the fluent API pattern, ensuring
WithMessagereturns the same instance for method chaining while modifying the error message.
79-87: LGTM!Correctly verifies the
NewStackNotFoundErrorconstructor, ensuring proper message content and type.
89-97: LGTM!Correctly verifies the
NewPendingErrorconstructor with appropriate assertions.
99-127: LGTM!Well-structured table-driven test for
NewMissingSettingsError, verifying that custom messages are correctly preserved.
146-199: LGTM!Comprehensive test coverage for
IsApplicationError, including all constructor types and negative cases.
201-213: LGTM!Correctly verifies error wrapping compatibility with Go's standard
errors.Ismechanism, essential for proper error handling in Go 1.13+.
215-237: LGTM!Important test ensuring type consistency across all error constructors. The dual verification (type assertion +
IsApplicationError) provides robust coverage.
239-252: LGTM!Correctly verifies the mutability of error messages through multiple
WithMessagecalls, demonstrating proper fluent API behavior.
254-283: LGTM!Excellent contextual tests demonstrating real-world usage patterns in reconciliation scenarios. The tests effectively verify both error type identification and message content.
No description provided.